replace AbstractRecurringUserPlan.has_automatic_renewal with AbstractRecurringUserPlan.renewal_triggered_by #188
Conversation
radekholy24
force-pushed
the
automatic_renewal_trigger
branch
6 times, most recently
from
eac8cbb to
4777fb8
Compare
radekholy24
force-pushed
the
automatic_renewal_trigger
branch
from
4777fb8 to
f937837
Compare
plans/base/models.py
Outdated
| @@ -533,11 +585,28 @@ class AbstractRecurringUserPlan(BaseMixin, models.Model): | |||
| _("tax"), max_digits=4, decimal_places=2, db_index=True, null=True, blank=True | |||
| ) # Tax=None is when tax is not applicable | |||
| currency = models.CharField(_("currency"), max_length=3) | |||
| renewal_trigger = models.IntegerField( | |||
| _("renewal trigger"), | |||
There was a problem hiding this comment.
Maybe renewal triggered by or renewal triggered from?
There was a problem hiding this comment.
Replaced with renewal triggered by
plans/base/models.py
Outdated
| "TASK = autorenew_account-task-initiated renewal, OTHER = renewal is triggered using another mechanism, " | ||
| "None = use has_automatic_renewal value (deprecated)). Overrides has_automatic_renewal, if not None." | ||
| ), | ||
| # TODO: Nullable deprecated. Set to False in the next major release. |
There was a problem hiding this comment.
I am not sure about this. In order to be backwards compatible we would have to populate the value at some point anyway. Will we make data migration in the next major release?
Or wouldn't it be better to populate the value now and add a mechanism that will guess it's value for legacy usages?
There was a problem hiding this comment.
I am not sure about this. In order to be backwards compatible we would have to populate the value at some point anyway. Will we make data migration in the next major release?
Hm, I think that if it is a major release, we may ask users to populate the value on their own.
Or wouldn't it be better to populate the value now and add a mechanism that will guess it's value for legacy usages?
I am fine with that but is it possible to make a data migration for all subclasses of AbstractRecurringUserPlan? Can you point me to such mechanism please?
There was a problem hiding this comment.
Hm... But... If we made the data migration, what would we do with users that won't migrate to renewal_triggerrenewal_triggered_by but keep changing only has_automatic_renewal? At some point in the past, I had a __getattribute__ mechanism that synchronized these two values. Would that work for you? (That ofc assumes that users do not manipulate the data in the DB directly.)
There was a problem hiding this comment.
@PetrDlouhy, I made the changes discussed in person.
edit: OMG, I forgot to write the data migration 🤦 mmnt...
edit: Migration added
plans/tasks.py
Outdated
| userplan__expire__lt=datetime.date.today() | ||
| + datetime.timedelta( | ||
| days=PLANS_AUTORENEW_BEFORE_DAYS, hours=PLANS_AUTORENEW_BEFORE_HOURS | ||
| ( |
There was a problem hiding this comment.
Two subsequent filter() calls instead of & might make this a bit clearer.
There was a problem hiding this comment.
Replaced with double-filter
radekholy24
force-pushed
the
automatic_renewal_trigger
branch
from
f937837 to
e01a4ca
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #188 +/- ##
==========================================
+ Coverage 86.73% 86.81% +0.07%
==========================================
Files 64 65 +1
Lines 2375 2404 +29
==========================================
+ Hits 2060 2087 +27
- Misses 315 317 +2 ☔ View full report in Codecov by Sentry. |
radekholy24
force-pushed
the
automatic_renewal_trigger
branch
from
e01a4ca to
0cb8a54
Compare
radekholy24
changed the title
radekholy24
force-pushed
the
automatic_renewal_trigger
branch
from
0cb8a54 to
9f5b1bd
Compare
radekholy24
changed the title
radekholy24
force-pushed
the
automatic_renewal_trigger
branch
2 times, most recently
from
c0dbb98 to
f1d1ee3
Compare
…RecurringUserPlan.renewal_triggered_by
radekholy24
force-pushed
the
automatic_renewal_trigger
branch
from
f1d1ee3 to
33ffece
Compare
|
@radekholy24 Thank you very much. |
has_automatic_renewalcan indicate whether the renewal is initiated by this app or by the user. However, there is no way to indicate that the renewal is initiated by another (3rd party) mechanism such as by the payment provider's server (PayPal). This new field provides such functionality while preservinghas_automatic_renewalproperty for backward compatibility.