-
Notifications
You must be signed in to change notification settings - Fork 1
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
Network manager funnel performance #391
base: main
Are you sure you want to change the base?
Conversation
…rk-manager-funnel-performance
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.
generally looks good. a few questions and one small nit
) | ||
.exclude(status__in=[VisitValidationStatus.over_limit, VisitValidationStatus.trial]) | ||
.order_by("visit_date") | ||
.values("visit_date")[:1] |
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.
nit: You can use .first()
here instead of the slice
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.
I tried .first() but subquery here expects a queryset.
from commcare_connect.program.models import ManagedOpportunity, Program | ||
|
||
|
||
def calculate_safe_percentage(numerator, denominator): |
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.
It's not worth for this to be a function, may be a private function inside the caller would be better
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.
I have used this function in PR-402 too.
assert annotated_opp.percentage_conversion == 60.0 | ||
|
||
def test_empty_scenario(self): | ||
opps = get_annotated_managed_opportunity(self.program) |
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.
You could consider using parametrize, to combine some of the tests like below (not tested). Allows to be more DRY and concise.
@pytest.mark.parametrize(
"visit_statuses, passing_assessments, expected_invited, expected_passing, expected_delivery, expected_conversion",
[
([VisitValidationStatus.pending, VisitValidationStatus.pending, VisitValidationStatus.trial],
[True, True, True], 5, 5, 3, 60.0), # Basic scenario
([], [], 0, 0, 0, 0.0), # Empty scenario
([VisitValidationStatus.pending] * 3, [True] * 3, 1, 1, 1, 100.0), # Multiple visits scenario
([VisitValidationStatus.over_limit, VisitValidationStatus.trial],
[True, True], 2, 2, 0, 0.0), # Excluded statuses
([VisitValidationStatus.pending, VisitValidationStatus.pending],
[False, True], 2, 1, 2, 100.0), # Failed assessments
]
)
def test_scenarios(self, visit_statuses, passing_assessments, expected_invited, expected_passing, expected_delivery, expected_conversion):
for i, visit_status in enumerate(visit_statuses):
self.create_user_with_access(visit_status=visit_status, passed_assessment=passing_assessments[i])
opps = get_annotated_managed_opportunity(self.program)
assert len(opps) == 1
annotated_opp = opps[0]
assert annotated_opp.workers_invited == expected_invited
assert annotated_opp.workers_passing_assessment == expected_passing
assert annotated_opp.workers_starting_delivery == expected_delivery
assert annotated_opp.percentage_conversion == expected_conversion
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.
Added f87aa02
…rk-manager-funnel-performance
…nnel-performance merge main branch into current branch
…rk-manager-funnel-performance
…mance' into hy/Network-manager-funnel-performance
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. Thanks 🙌🏽
https://dimagi.atlassian.net/browse/CCCT-479