Add error handling for vehicle and property account creation #1179

2024-09-14T14:50:59+08:00

tonyvince commented

2024-09-14 14:50:59 +08:00

(Migrated from github.com)

Close #1178

zachgoll (Migrated from github.com) reviewed 2024-09-16 21:40:45 +08:00

zachgoll (Migrated from github.com) left a comment

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.

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 commented

2024-09-16 21:45:05 +08:00

(Migrated from github.com)

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.

> 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.

zachgoll (Migrated from github.com) reviewed 2024-09-17 00:34:42 +08:00

app/helpers/forms_helper.rb

						
				@@ -37,2 +37,3 @@

				      label:         options[:label] || "Amount"

				      label:         options[:label] || "Amount",

				      required:      options[:required] || false

				    }

zachgoll (Migrated from github.com) commented

2024-09-17 00:34:37 +08:00

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

fd40111264/app/views/accounts/_form.html.erb (L8)

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

We'll need to update this improper usage for this to flow through correctly: https://github.com/maybe-finance/maybe/blob/fd4011126422023228a9295b2d9b30bb1b9402cb/app/views/accounts/_form.html.erb#L8 See `required: "required"` should be -> `required: true`

test/controllers/properties_controller_test.rb

zachgoll (Migrated from github.com) commented

2024-09-17 00:25:10 +08:00

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:

fd40111264/app/controllers/accounts_controller.rb (L61-L62)

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: https://github.com/maybe-finance/maybe/blob/fd4011126422023228a9295b2d9b30bb1b9402cb/app/controllers/accounts_controller.rb#L61-L62

zachgoll (Migrated from github.com) approved these changes 2024-09-17 22:36:50 +08:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#1179