-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat: pick the most recently start plan as auto-applicable #745
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -237,7 +237,7 @@ def auto_applicable_subscription(self): | |||
is_active=True, | |||
start_date__lte=now, | |||
expiration_date__gte=now | |||
).first() | |||
).order_by('-start_date').first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[sanity check] Just to confirm / for posterity, this change wouldn't incorrectly impact any other existing customers that might not have the same business rules as VSF, right (e.g., where they might want the plan with the earliest start_date
), correct? Given that the query is technically non-deterministic today, where we previously assumed any active/current plan was suitable, making it deterministic now really isn't changing any prior expectations as a result. That said, there could be a future use case where the ordering might depend on customer-specific rules (though, probably a bridge to cross if/when that happens, not now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, exactly, although its probably deterministic today based on the row id in the database.
c2863a8
to
4f7e6da
Compare
" will automatically enable the \"Should auto apply licenses\" field on the subscription plan" | ||
help_text=( | ||
""" | ||
The subscription plan from which licenses will be auto-applied, if any. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice update/clarification on the help_text
.
Description
Ensures that we pick the most recently started plan as the auto-applicable plan for an agreement.
Link to the associated ticket: https://openedx.atlassian.net/browse/ENT-9734
Testing considerations
Post-review
Squash commits into discrete sets of changes