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 for Network Manager exceed program's total budget. #479

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

hemant10yadav
Copy link
Contributor

@hemant10yadav hemant10yadav commented Jan 24, 2025

Product Description

In the "Add Budget" page, for managed opportunities, the program budget will be validated. If the additional visits cause the total budget to exceed the program budget, the user will see the error message:
Additional visits exceed the program budget.
This message will appear below the "Additional Visits" field.

Technical Summary

The logic checks whether the opportunity's total budget exceeds the program budget after adding additional visits. If the total budget exceeds the program budget, the form submission is blocked, and a form error is displayed. CCCT-639

Safety Assurance

Safety story

This functionality has been tested on the staging environment.

Automated test coverage

Added test scenarios to validate both cases:

  • When the total budget exceeds the program budget for a managed opportunity.
  • When the total budget remains within the program budget.

Labels & Review

  • The set of people pinged as reviewers is appropriate for the level of risk of the change

Copy link
Collaborator

@calellowitz calellowitz left a comment

Choose a reason for hiding this comment

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

One question about the logic, but this feels like form validation, can it not be handled at that level? it feels wrong that form.is_valid would return True in this case, and that handles the error messaging automatically, without the special handling you added here

@@ -455,16 +455,43 @@ def add_budget_existing_users(request, org_slug=None, pk=None):
if form.is_valid():
selected_users = form.cleaned_data["selected_users"]
additional_visits = form.cleaned_data["additional_visits"]
if form.cleaned_data["end_date"]:
OpportunityClaim.objects.filter(pk__in=selected_users).update(end_date=form.cleaned_data["end_date"])
end_date = form.cleaned_data["end_date"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this isn't an optional field?

Copy link
Contributor Author

@hemant10yadav hemant10yadav Feb 10, 2025

Choose a reason for hiding this comment

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

Yes

.aggregate(total=Sum("total_budget"))["total"]
or 0
)
if total_budget_sum + opportunity.total_budget > program.budget:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the opportunity have a predefined budget limit separate from the program? I would have thought we don't need to check the program total, because it's limited at the opportunity level (which is guaranteed to be within program limits at the time of opportunity creation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This view allows us to increase the opportunity's total budget by adding additional visits to the user, which the creator fixed during opportunity creation, as you mentioned. That's why I checked the program budget, as the opportunity budget will increase if we try to add additional visits. Let me know if I missed anything here.

commcare_connect/opportunity/views.py Outdated Show resolved Hide resolved
commcare_connect/opportunity/views.py Outdated Show resolved Hide resolved
@hemant10yadav
Copy link
Contributor Author

One question about the logic, but this feels like form validation, can it not be handled at that level? it feels wrong that form.is_valid would return True in this case, and that handles the error messaging automatically, without the special handling you added here

I will move the validation logic in the form itself.

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