Fix Account Holding validation and synchronization #1818

2025-02-07T04:48:50+08:00

Shpigford commented

2025-02-07 04:48:50 +08:00

(Migrated from github.com)

Add comprehensive validations for Account::Holding
Implement validation to ensure amount matches qty * price
Update Account::Syncer to include qty and price during synchronization
Add database constraints for holding attributes and calculations

Fixes #1781 - Add comprehensive validations for Account::Holding - Implement validation to ensure amount matches qty * price - Update Account::Syncer to include qty and price during synchronization - Add database constraints for holding attributes and calculations

Shpigford commented

2025-02-07 04:49:25 +08:00

(Migrated from github.com)

@zachgoll Is this what you had in mind for addressing #1781 and #1767?

zachgoll (Migrated from github.com) approved these changes 2025-02-07 05:42:43 +08:00

zachgoll (Migrated from github.com) left a comment

Yep, that's pretty much what I was thinking!

db/migrate/20250206204404_add_constraints_to_account_holdings.rb

						
				@@ -0,0 +1,40 @@

				class AddConstraintsToAccountHoldings < ActiveRecord::Migration[7.2]

zachgoll (Migrated from github.com) commented

2025-02-07 05:42:22 +08:00

Migration makes sense overall, but I think for these more complex validations (i.e. check_positive_values and check_amount_matches), we should keep those in ActiveRecord.

While the current codebase has some gaps still, I'm trying to move our validations towards the following convention. Here's a project-conventions.mdc format that can be pasted in for the AI (forgot this first time around!):

### Convention 7: Use ActiveRecord for complex validations, DB for simple ones, keep business logic out of DB

- Enforce `null` checks, unique indexes, and other simple validations in the DB
- ActiveRecord validations _may_ mirror the DB level ones, but not 100% necessary.  These are for convenience when error handling in forms.  Always prefer client-side form validation when possible.
- Complex validations and business logic should remain in ActiveRecord

Migration makes sense overall, but I think for these more complex validations (i.e. `check_positive_values` and `check_amount_matches`), we should keep those in `ActiveRecord`. While the current codebase has some gaps still, I'm trying to move our validations towards the following convention. Here's a `project-conventions.mdc` format that can be pasted in for the AI (forgot this first time around!): ``` ### Convention 7: Use ActiveRecord for complex validations, DB for simple ones, keep business logic out of DB - Enforce `null` checks, unique indexes, and other simple validations in the DB - ActiveRecord validations _may_ mirror the DB level ones, but not 100% necessary. These are for convenience when error handling in forms. Always prefer client-side form validation when possible. - Complex validations and business logic should remain in ActiveRecord ```

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#1818

				`@@ -0,0 +1,40 @@`
				`class AddConstraintsToAccountHoldings < ActiveRecord::Migration[7.2]`