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

feat: new "retired" field on policy models #386

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Conversation

pwnage101
Copy link
Contributor

@pwnage101 pwnage101 commented Jan 12, 2024

Implements ADR 0016-policy-retirement, and also updates it to reflect
the decision to rename the field to "retired" and change it to a
boolean.

Note this also adds extra visibility into the historical values of
various policy fields via the django admin record history interface.
Previously, changes were only noted (e.g. "active field changed"), but
now the actual historical value of the field is visible.

ENT-8206

Screenshots

What the checkboxes look like in django admin edit page for a policy.
image

Example of the history page for a policy that I changed to retired:
Screenshot 2024-01-17 at 15 53 35

@pwnage101 pwnage101 force-pushed the pwnage101/ENT-8206-2 branch 2 times, most recently from f9fd781 to a403ebd Compare January 16, 2024 05:26
@pwnage101 pwnage101 marked this pull request as ready for review January 16, 2024 05:26
Comment on lines 1300 to 1304
# The following policy should never be returned as it has redeemability disabled.
PerLearnerEnrollmentCapLearnerCreditAccessPolicyFactory(
enterprise_customer_uuid=self.enterprise_uuid,
redeemability_disabled_at=datetime.utcnow() - timedelta(days=1),
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed at standup on Friday, when redeemability is disabled (was: policy is "retired"), we should make the policy effectively disappear from a learner's perspective. I.e. it should not be returned by the credits_available endpoint, tested here.

Copy link
Member

@brobro10000 brobro10000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I don't see any usages in the frontend for policy_is_active so it looks like its safe to change it without any FE regressions.

@pwnage101 pwnage101 force-pushed the pwnage101/ENT-8206-2 branch 2 times, most recently from ee2f5a0 to e0bd1e3 Compare January 17, 2024 23:56
Implements ADR 0016-policy-retirement, and also updates it to reflect
the decision to rename the field to "retired" and change it to a
boolean.

Note this also adds extra visibility into the historical values of
various policy fields via the django admin record history interface.
Previously, changes were only noted (e.g. "active field changed"), but
now the actual historical value of the field is visible.

ENT-8206
@pwnage101
Copy link
Contributor Author

Forced pushed changes:

  • renamed field from "redeemability_disabled_at" to "retired"
  • changed field type to a boolean
  • Made sure the field changes were clearly visible from the django admin history page for a policy record.
  • updated ADR to match field changes.

Example of the history page for a policy that I changed to retired:
Screenshot 2024-01-17 at 15 53 35

@pwnage101 pwnage101 changed the title feat: new redeemability_disabled_at field on policy models feat: new "retired" field on policy models Jan 18, 2024
with the customer uuid requested by the client.
"""
return SubsidyAccessPolicy.objects.filter(
return SubsidyAccessPolicy.policies_with_redemption_enabled().filter(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 nice

@pwnage101 pwnage101 merged commit 4dfe92d into main Jan 18, 2024
4 checks passed
@pwnage101 pwnage101 deleted the pwnage101/ENT-8206-2 branch January 18, 2024 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants