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

Stripe subscription updates (amount/schedule) #80

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VSydor
Copy link
Contributor

@VSydor VSydor commented Sep 10, 2022

No description provided.

@VSydor VSydor requested a review from brmeyer September 10, 2022 19:39
@VSydor VSydor force-pushed the feature/stripe-subscritpion-update branch 3 times, most recently from 3a0c1ff to ab108a3 Compare September 11, 2022 07:52
Copy link
Contributor

@brmeyer brmeyer left a comment

Choose a reason for hiding this comment

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

Realized my comment never posted until now!

@@ -158,6 +158,7 @@ default Optional<CrmRecurringDonation> getRecurringDonation(PaymentGatewayEvent
return getRecurringDonationBySubscriptionId(paymentGatewayEvent.getSubscriptionId());
}
String insertRecurringDonation(PaymentGatewayEvent paymentGatewayEvent) throws Exception;
void updateRecurringDonation(PaymentGatewayEvent paymentGatewayEvent) throws Exception;
Copy link
Contributor

Choose a reason for hiding this comment

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

@VSydor Hey! @Levi-Lehman and I were talking on #75 (comment) about something similar. With what we're doing on that other PR, we'll wind up with a set of methods like this:

updateRecurringDonation(CrmRecurringDonation)
updateRecurringDonation(PaymentGatewayEvent)
updateRecurringDonation(ManageDonationEvent)

In the CrmService impls, all 3 of them wind up with similar code (sometimes minor differences). Starting to think through refactors that will square all that up. Ex: rather than all the separate fields in PaymentGatewayEvent, nest CrmRecurringDonation as a field inside that event, then persist directly from a default method in CrmService? That's exactly what we do with CrmAccount and CrmConact in PaymentGatewayEvent currently...

Any other ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense - let's have it this way ("what we do with CrmAccount and CrmConact in PaymentGatewayEvent currently")

@VSydor VSydor force-pushed the feature/stripe-subscritpion-update branch from ab108a3 to b2b1483 Compare November 27, 2023 14:27
@VSydor VSydor requested a review from brmeyer November 27, 2023 14:30
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.

2 participants