-
Notifications
You must be signed in to change notification settings - Fork 9
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
fix: filters for only active policies for spend_limit check #494
Conversation
enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py
Outdated
Show resolved
Hide resolved
2ddbba2
to
3177fbe
Compare
if self.active: | ||
policy_balances.append(self.spend_limit or 0) |
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.
Nested this logic in the conditional to check to see if the policy we are modifying is not active, we do not want to include it in the policy balances. This is in parity with the changes to the filtering that only looks for active sibling policies.
3177fbe
to
16a9ab5
Compare
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.
Non-blocking idea:
FWIW, there's probably a way in pure ORM to this entire function all in one line and one SQL query without having to exclude self.uuid or loop over the results, e.g.:
return SubsidyAccessPolicy.objects.filter(
enterprise_customer_uuid=self.enterprise_customer_uuid,
subsidy_uuid=self.subsidy_uuid,
active=True,
).aggregate(Sum("spend_limit", default=0))["spend_limit__sum"]
Theoretically when this encounters a NULL spend_limit, The default=0
takes over. Just an Idea that could make this function less complex and easier to understand/debug.
Based on the standup discussion, it sounds like you want your clean function to say this instead: if self.is_spend_limit_updated and not self.active: |
@@ -350,7 +358,7 @@ def clean(self): | |||
""" | |||
Used to help validate field values before saving this model instance. | |||
""" | |||
if self.is_spend_limit_updated: | |||
if self.active and (self.is_active_updated or self.is_spend_limit_updated): |
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.
Modified the conditional to avoid the path of a user disabling the policy. Setting an out of bounds spend_limit, then re-enabling the policy.
Before conditional added:
https://github.com/openedx/enterprise-access/assets/82611798/16a80f8d-de14-43ac-8ba3-efd71f9e5751
Screen.Recording.2024-06-24.at.2.10.02.PM.mov
79562b9
to
5edc117
Compare
5edc117
to
9121365
Compare
9121365
to
1cabea0
Compare
Updates the
spend_limit
constraint check to only check on policies that are active and to determine the sum of thespend_limit
's on only active policies.Also updates how we sum sibling policies in the
total_spend_limit_for_all_policies_associated_to_subsidy
function by using the ORM to determine the sum of siblings and adding together thespend_limit
in memory.Description:
Add a description of your changes here.
Jira:
ENT-XXXX
Merge checklist:
./manage.py makemigrations
has been runPost merge: