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

Fix re-enabling of a subscription after a long period of it being off. #183

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

eigood
Copy link
Contributor

@eigood eigood commented Mar 7, 2022

If a user has a monthly auto-renew, but then their card had been denied(because they had closed their card account at the bank), then after several months they decided to re-enable their account, the previous code would find the long-ago Subscription record, and add a subscription on the end of that. But since we are 6 or more months past that, the new subscription was totally 100% in the past; this makes no sense.

This adds a date filter, which then excludes those long-ago records. Now, we can see that there is no previous subscription that is active, and create a new entry.

exclude previous subscriptions that have already timed out(by doing a
date filter).
@jonesde
Copy link
Member

jonesde commented Mar 8, 2022

For the thruDate constraint I think you're right that we should have an option for this... but I don't know if it should always be the behavior. There are two main scenarios I'm thinking of, and we may need a setting on SubscriptionResource (I think that's the best spot) to specify which:

  1. extend previously expired Subscription (as it does before this change) because the SubscriptionResource was still available the whole time and the customer effectively has back-charges due that should be "paid" before they can continue using the resource2.

  2. leave a gap and create a new Subscription (as it does with the changes in this PR) because the resource was not available (auto disabled or something), or the vendor does not wish to collect back-charges for the time used but not paid for.

For anyone relying on the current behavior because their business policy is #1 we need some sort of option. As I think about this, perhaps the option should be a time limit... ie if the time since the most recent thruDate is greater than the time limit create a new record (ie more than 1 month or whatever), otherwise extend from the prior thruDate. I don't know if that's helpful... maybe a simple boolean (text-indicator) would be better.

I wonder about the fromDate constraint, and it would be odd to have a subscription active in the future but not one active in the present... but if that were to happen would we want one starting now? As I think through that it seems odd... but I'm not sure what the best behavior should be... perhaps it should be as you have it now where it would create a record for the present (that may or may not overlap with a future fromDate record), or perhaps it would be better if there was one in the future it should extend that and continue to not have an active Subscription in the present. I don't know, probably weird either way and I'm thinking how you have it is best.

@eigood
Copy link
Contributor Author

eigood commented Mar 8, 2022

#179 seems to be related to this.

Let's say a user bought a single product, 1 year of monthly subscriptions. The fromDate/thruDate of the Subscription is set to expire in 1 year. They are not auto-renewing.

After that year has past, they do not purchase another one.

Then, 5 years down the road, when they are in a better place financially, they come back and purchase another year of monthly subscriptions. The current code(un-patched) will set the subscription to start and end in the past. I am having a hard time understanding how that could ever be valid.

@eigood
Copy link
Contributor Author

eigood commented Mar 9, 2022

Ok, this is worse then I thought. My original local commit had a "fromDate <= nowTimestamp", that if valid, would then set fromDate = nowTimestamp. This is what we had deployed to production.

However, locally I had merged in master, and adjusted that path. And it showcased a new problem.

fulfill#OrderPartSubscription, line 87, calls fulfill#ProductSubscriptionResource, and sets fromDate to the value of the ProductSubscriptionResource row; this is set to 2010-02-03 00:00:00(the row was generated from seed data). This is actually very wrong. Creating the subscription start date based on the Product->SubscriptionResource mapping can't possibly be correct; I'm now leaning towards removing that fromDate setting(line 91, same file).

@eigood
Copy link
Contributor Author

eigood commented Mar 9, 2022

Ok, so yeah, this is tricky. There is currently no way that I can see that a user can purchase a Subscription to be turned on in the future. This would have to be attached somehow via the Order entities, which then get passed down through the various ECAs. The path is: SubscriptionServices:fulfill#OrderSubscriptions(eca) -> iterate OrderPart -> fulfill#OrderPartSubscription -> iterate ProductSubscriptionResource -> fulfill#ProductSubscriptionResource(using fromDate of the joining table ProductSubscriptionResource, can't be overridden from the Order entities). In this final service, since there was a fromDate passed in, any previous subscriptions do not apply at all, and in fact, repeated purchases will always create a subscription against that ancient fromDate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants