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

Track modified donations via transactions #62

Merged
merged 3 commits into from
Jan 28, 2025
Merged

Conversation

ivanb777
Copy link
Contributor

@ivanb777 ivanb777 commented Oct 3, 2024

Summary

This PR changes the approach to tracking modified donations to ensure consistency and accuracy. Instead of relying on the donation's updated_at, we now track donation modifications via transactions updates.

Key Changes

  1. Modified Donations: Updates to donations are now driven by changes in related transactions, eliminating inconsistencies with the previous updated_at approach.
  2. Refund Handling: Refunded transactions are now recorded as separate negative donations. Additionally, the original donation is marked with a refunded_at timestamp to keep record of the refund status.
  3. Test Coverage: We’ve added the necessary tests for fetch_donation_updates to validate the donations sync behaviour.

For more info see the Asana issue.

@ivanb777 ivanb777 force-pushed the fix-donation-sync branch 9 times, most recently from c9122a5 to 2f5dc47 Compare October 14, 2024 07:01
@ivanb777 ivanb777 force-pushed the fix-donation-sync branch 2 times, most recently from 9e94ee2 to ccb8fd7 Compare October 24, 2024 00:32
@ivanb777 ivanb777 changed the title Fix donation sync Track modified donations via transactions Oct 24, 2024
Copy link
Contributor

@michael-gratton michael-gratton left a comment

Choose a reason for hiding this comment

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

Hey, this is a much smaller change than I expected! Thanks heaps for adding those donation tests.

I am a little surprised that no changes except removing that one line from Donation.import was needed - will that actually pick up all the related things fine? Like ensure that refunds in Id are correctly related?

@ivanb777
Copy link
Contributor Author

ivanb777 commented Nov 1, 2024

Hey @michael-gratton, thanks heaps for the feedback. I've taken them onboard, but went slightly differently re: import approach. The signature for instance import is changed, but the signature for the class method is the same – I've updated the logic to be compatible with what the instance method expects.

I am a little surprised that no changes except removing that one line from Donation.import was needed - will that actually pick up all the related things fine? Like ensure that refunds in Id are correctly related?

Yes, it's simple as that. The previous implementation didn't record the refunds, it just marked the corresponding refund_of_id donation as refunded. Yes, removing that line was necessary for refunds to also be recorded in addition to marking/timestamping the corresponding transaction with refunded_at. Although the implementation now has slightly changed.

Copy link
Contributor

@michael-gratton michael-gratton left a comment

Choose a reason for hiding this comment

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

Think I'm pretty happy with this. Let's get this merged once the current batch of Id changes gets promoted to prod.

@ivanb777 ivanb777 merged commit b46cff7 into main Jan 28, 2025
1 check 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.

3 participants