Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

Better usage handling #78

Merged
merged 4 commits into from
Jun 17, 2021
Merged

Better usage handling #78

merged 4 commits into from
Jun 17, 2021

Conversation

bpuig
Copy link
Owner

@bpuig bpuig commented Jun 15, 2021

Right now you can record feature usage without issue and it gets renewed automatically if it hasExpired. But if you check it beforehand with canUseFeature, you will be denied usage because it hasExpired.

New logic should:

  • Should not care about expiration in canUseFeature since it's the subscription period that rules if you can use features.
  • Validate canUseFeature before recordFeatureUsage recording feature usage. Because, why should you record a feature you cannot use?

@bpuig bpuig added bug Something isn't working major Major change work in progress This work is in progress. labels Jun 15, 2021
@bpuig bpuig added this to the v5.0.0 milestone Jun 15, 2021
@bpuig bpuig linked an issue Jun 15, 2021 that may be closed by this pull request
@bpuig
Copy link
Owner Author

bpuig commented Jun 15, 2021

Why are we able to?

$plan->features()->saveMany([
    new PlanFeature(['tag' => 'posts_per_social_profile', 'name' => 'Scheduled posts per profile', 'value' => 30, 'sort_order' => 10, 'resettable_period' => 1, 'resettable_interval' => 'month'])
]);

And set custom 'resettable_period' => 1, 'resettable_interval' => 'month'. Why is not inherited from plan?

Looks weird to me to have a plan with different period that it's features. Like, you pay monthly but your features are renewed every couple months? Right now I think it won't affect this PR. But does not look OK.

@boryn
Copy link
Contributor

boryn commented Jun 17, 2021

Paying subscription monthly and resetting features quaterly could be possible in some cases. I'd leave it as it is, this gives more flexibility for the features definition & usage.

@boryn
Copy link
Contributor

boryn commented Jun 17, 2021

And the other real life scenario is that someone pays for the year (so we have invoice_period set to '1' and invoice_interval set to 'year' at plans) but they consume the features on the monthly basis (so resettable_period set to '1' and resettable_interval set to 'month' in the plan_features).

@boryn
Copy link
Contributor

boryn commented Jun 17, 2021

Just as a side note. While it does not make sense to record feature when the user is not allowed to use it, it's good to be able to recordFeatureUsage() when feature is set to 'true' (not integer). For example we charge per "file conversion", the user has this permission and we count the real monthly usage.

@bpuig
Copy link
Owner Author

bpuig commented Jun 17, 2021

Just as a side note. While it does not make sense to record feature when the user is not allowed to use it, it's good to be able to recordFeatureUsage() when feature is set to 'true' (not integer). For example we charge per "file conversion", the user has this permission and we count the real monthly usage.

I don't think that is currently the way the package it's suposed to work. It does not support mettered use. You have to specify a "usage limit". One reason, among others, is that, when period has expired, it restarts the counter. You would not know how much did the subscriber use because it would be wiped and replaced with the new "1" used value.

@boryn
Copy link
Contributor

boryn commented Jun 17, 2021

Yes, that's true. It counts the usage, but only for the current period and when new appears, this information is lost...

@bpuig bpuig merged commit f9f653e into v5-develop Jun 17, 2021
@bpuig bpuig deleted the better_usage_handling branch June 17, 2021 12:39
bpuig added a commit that referenced this pull request Jun 17, 2021
* Refactor function
* Do not check if usage has expired
* Deny record if cannot use
* Update docs
@bpuig bpuig removed the work in progress This work is in progress. label Oct 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working major Major change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants