search

moqui / mantle-usl

Mantle Universal Service Library
http://www.moqui.org/mantle.html
Other
30 stars 61 forks source link

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

Open eigood opened 2 years ago

eigood commented 2 years ago

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.

jonesde commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.