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

Program Management Tests #414

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

Conversation

pxwxnvermx
Copy link
Contributor

@pxwxnvermx pxwxnvermx commented Oct 14, 2024

This PR adds missing tests for the Enterprise User Visit Review functionality in the Program Management features.

@pxwxnvermx pxwxnvermx self-assigned this Oct 14, 2024
@pxwxnvermx pxwxnvermx marked this pull request as ready for review October 17, 2024 12:48
@pxwxnvermx pxwxnvermx requested review from hemant10yadav, sravfeyn and calellowitz and removed request for sravfeyn October 17, 2024 12:48
for visit in visits:
assert visit.review_status == VisitReviewStatus.pending

def test_user_visit_review_program_manager_agree(self):
Copy link
Member

Choose a reason for hiding this comment

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

You might be able to use @pytest.mark.parametrize to combine the agree and reject test cases into a single test. And similarly program_manager_pending and network_manager get request tests.

assert visit.review_status == VisitReviewStatus.pending


def get_form_json_for_payment_unit(payment_unit):
Copy link
Member

@sravfeyn sravfeyn Oct 18, 2024

Choose a reason for hiding this comment

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

(Not in scope for this PR) We have too many get-form-json methods, wonder if they can be combined.

Copy link
Contributor Author

@pxwxnvermx pxwxnvermx Oct 18, 2024

Choose a reason for hiding this comment

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

Yeah, I was thinking the same. I am in favor of keeping the latest methods that were created including the fixture that you created for mobile user. That will simplify these tests.

@@ -596,6 +587,44 @@ def test_reciever_verification_flags_catchment_areas(
assert ["catchment", "Visit outside worker catchment areas"] in visit.flag_reason.get("flags", [])


@pytest.mark.parametrize("opportunity", [{"managed": True}], indirect=True)
def test_receiver_auto_agree_approved_visit(
Copy link
Member

Choose a reason for hiding this comment

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

This could potentially be combined with test_receiver_flagged_visit_review_pending using parametrize

mobile_user_with_connect_link: User, api_client: APIClient, opportunity: Opportunity
):
assert opportunity.managed
form_json = get_form_json_for_payment_unit(opportunity.paymentunit_set.all()[0])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
form_json = get_form_json_for_payment_unit(opportunity.paymentunit_set.all()[0])
form_json = get_form_json_for_payment_unit(opportunity.paymentunit_set.first())



@pytest.mark.parametrize("opportunity", [{"managed": True}], indirect=True)
def test_network_manager_approve_flagged_visit(mobile_user: User, opportunity: Opportunity):
Copy link
Member

Choose a reason for hiding this comment

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

The two tests could potentially be combined

@pytest.mark.parametrize("visit_status, review_status, expect_review_created", [
    (VisitValidationStatus.approved.value, VisitReviewStatus.pending, True),  # Approved case
    (VisitValidationStatus.rejected.value, None, False),  # Rejected case
])
@pytest.mark.parametrize("opportunity", [{"managed": True}], indirect=True)
def test_network_manager_update_flagged_visit(mobile_user: User, opportunity: Opportunity, visit_status, review_status, expect_review_created):
    assert opportunity.managed
    access = OpportunityAccess.objects.get(user=mobile_user, opportunity=opportunity)

    # Create pending visits
    visits = UserVisitFactory.create_batch(
        5, opportunity=opportunity, status=VisitValidationStatus.pending, user=mobile_user, opportunity_access=access
    )

    # Prepare dataset based on status parameter
    dataset = Dataset(headers=["visit id", "status", "rejected reason", "justification"])
    dataset.extend([[visit.xform_id, visit_status, "", "justification"] for visit in visits])

    before_update = now()
    import_status = _bulk_update_visit_status(opportunity, dataset)
    after_update = now()

    # Ensure visits were found and updated
    assert not import_status.missing_visits
    updated_visits = UserVisit.objects.filter(opportunity=opportunity)

    for visit in updated_visits:
        assert visit.status == visit_status
        assert visit.status_modified_date is not None
        assert before_update <= visit.status_modified_date <= after_update
        
        # For approved visits, check review created on date
        if expect_review_created:
            assert before_update <= visit.review_created_on <= after_update
        else:
            assert visit.review_created_on is None

        # Check review status and justification
        assert visit.review_status == review_status
        assert visit.justification == "justification"

@@ -1119,7 +1120,7 @@ def user_visit_review(request, org_slug, opp_id):
review_status = request.POST.get("review_status").lower()
updated_reviews = request.POST.getlist("pk")
user_visits = UserVisit.objects.filter(pk__in=updated_reviews)
if review_status in ["agree", "disagree"]:
if review_status in [VisitReviewStatus.agree.value, VisitReviewStatus.disagree.value]:
Copy link
Member

Choose a reason for hiding this comment

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

This might be out of scope for this PR, but why are we using Agree/Disagree terms as opposed to existing Approved/Rejected from VisiValidationStatus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was done to reduce the confusion that came due to those choices. These Agree/Disagree choices were distinct enough from the Approved/Rejected choices.

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