Add partial account sync support #653

2024-04-19T16:27:00+08:00

jakubkottnauer commented

2024-04-19 16:27:00 +08:00

(Migrated from github.com)

Closes #584

🎉 1

jakubkottnauer commented

2024-04-24 21:18:23 +08:00

(Migrated from github.com)

@zachgoll Ready for review

zachgoll (Migrated from github.com) reviewed 2024-04-24 21:31:53 +08:00

zachgoll (Migrated from github.com) left a comment

Thanks for tackling this one! Definitely a tricky flow.

I think the code looks good overall, just added a few comments around the location and content of the tests.

Thanks for tackling this one! Definitely a tricky flow. I think the code looks good overall, just added a few comments around the location and content of the tests.

test/controllers/valuations_controller_test.rb

						
				@@ -19,1 +63,4 @@

				      delete valuation_url(first)

				    end

				  end

				end

zachgoll (Migrated from github.com) commented

2024-04-24 21:25:10 +08:00

So looking through the tests outlined in valuations_controller_test and transactions_controller_test, I think the test cases themselves are useful to have, but they probably don't belong in a controller test. It looks like the questions we're asking here are:

Did the job happen?
Did the job happen with correct start date?
Did running the job result in the correct end state?

I think the first and the second assertions are correct to be here in the controllers, but the third question should probably be tested in syncable_test as it asserts against the logic that is handled by the Account model.

We could probably consolidate all the controller tests so that for each CRUD controller action, we assert that:

The resource was edited/created/deleted
The sync job was enqueued with the correct params (we can use assert_enqueued_with for this).

That way, we're only testing the responsibility of the controller, which is to ensure the job is successfully enqueued with the correct args.

So looking through the tests outlined in `valuations_controller_test` and `transactions_controller_test`, I think the test cases themselves are useful to have, but they probably don't belong in a controller test. It looks like the questions we're asking here are: 1. Did the job happen? 2. Did the job happen with correct start date? 3. Did running the job result in the correct end state? I think the first and the second assertions are correct to be here in the controllers, but the third question should probably be tested in `syncable_test` as it asserts against the logic that is handled by the `Account` model. We could probably consolidate all the controller tests so that for each CRUD controller action, we assert that: 1. The resource was edited/created/deleted 2. The sync job was enqueued with the correct params (we can use [assert_enqueued_with](https://guides.rubyonrails.org/testing.html#testing-jobs-in-context) for this). That way, we're only testing the responsibility of the controller, which is to ensure the job is successfully enqueued with the correct args.

test/models/account/syncable_test.rb

zachgoll (Migrated from github.com) commented

2024-04-24 21:29:10 +08:00

In reference to my comment above about the controller tests, I think what you have here is good and covers the core logic that we're trying to validate.

jakubkottnauer (Migrated from github.com) reviewed 2024-04-25 02:00:49 +08:00

test/controllers/valuations_controller_test.rb

						
				@@ -19,1 +63,4 @@

				      delete valuation_url(first)

				    end

				  end

				end

jakubkottnauer (Migrated from github.com) commented

2024-04-25 02:00:49 +08:00

Thanks for the great feedback, I agree!

jakubkottnauer (Migrated from github.com) reviewed 2024-04-25 02:57:00 +08:00

test/controllers/valuations_controller_test.rb

						
				@@ -19,1 +63,4 @@

				      delete valuation_url(first)

				    end

				  end

				end

jakubkottnauer (Migrated from github.com) commented

2024-04-25 02:57:00 +08:00

I've updated the tests

zachgoll (Migrated from github.com) approved these changes 2024-04-25 03:55:44 +08:00

zachgoll (Migrated from github.com) left a comment

Awesome, thanks for the updates.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#653