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

fix: filters for only active policies for spend_limit check #494

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

brobro10000
Copy link
Contributor

@brobro10000 brobro10000 commented Jun 18, 2024

Updates the spend_limit constraint check to only check on policies that are active and to determine the sum of the spend_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 the spend_limit in memory.

Description:
Add a description of your changes here.

Jira:
ENT-XXXX

Merge checklist:

  • ./manage.py makemigrations has been run
    • Note: This must be run if you modified any models.
      • It may or may not make a migration depending on exactly what you modified, but it should still be run.

Post merge:

  • Ensure that your changes went out to the stage instance
  • Deploy to prod instance

Comment on lines 321 to 322
if self.active:
policy_balances.append(self.spend_limit or 0)
Copy link
Contributor Author

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.

Copy link
Contributor

@pwnage101 pwnage101 left a 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.

@pwnage101
Copy link
Contributor

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):
Copy link
Contributor Author

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

@brobro10000 brobro10000 merged commit 1c20396 into main Jun 25, 2024
3 checks passed
@brobro10000 brobro10000 deleted the hu/ent-inactive-policies branch June 25, 2024 13:21
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.

2 participants