Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add error handling for vehicle and property account creation #1179

Merged
merged 3 commits into from
Sep 17, 2024

Conversation

tonyvince
Copy link
Contributor

Close #1178

Copy link
Collaborator

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be solved easier if we just apply client-side form validations?

I think we can add a required attribute on these fields and let the browser do all of this for us.

@Shpigford
Copy link
Member

I think we can add a required attribute on these fields and let the browser do all of this for us.

I'm in favor of this for these types of low-stakes validations.

@tonyvince tonyvince requested a review from zachgoll September 16, 2024 15:53
assert_redirected_to accounts_path
assert_equal "Balance can't be blank and Entries is invalid", flash[:alert]
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can still remove all of these tests alongside the rescue handlers in the controller.

The form validation should handle all the main requirements directly in the browser. If a user somehow finds a way to side-step these, I'd rather the system raise an error so that we can capture it in observability systems like Sentry (rather than the user seeing an error message and the system acting as if nothing went wrong).

Following this logic, I think our existing create action in accounts_controller should probably not have the rescue block either (as I'm sure that was the pattern you followed here). In other words, we could probably remove this as well:

rescue ActiveRecord::RecordInvalid => e
redirect_back_or_to accounts_path, alert: e.record.errors.full_messages.to_sentence

@@ -34,7 +34,8 @@ def money_with_currency_field(form, money_method, options = {})
default_currency: options[:default_currency] || "USD",
disable_currency: options[:disable_currency] || false,
hide_currency: options[:hide_currency] || false,
label: options[:label] || "Amount"
label: options[:label] || "Amount",
required: options[:required] || false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to update this improper usage for this to flow through correctly:

<%= money_with_currency_field f, :balance_money, label: t(".balance"), required: "required", default_currency: Current.family.currency %>

See required: "required" should be -> required: true

@zachgoll zachgoll merged commit e06f0c7 into maybe-finance:main Sep 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Blank Current Balance in New Property or Vehicle Causing Crashes
3 participants