Used global and score breakdowns for avg score in hacker summary (#691)

* Added function to include breakdown fields in summary

* Changed function temporarily for route

* Fixed unit tests

* Removed redundant unit test

Author: IanWearsHat

apps/api/src/admin/applicant_review_processor.py

@@ -12,6 +12,8 @@
 }
 
 
+# TODO: Make this function only used for old applicant summary
+# meaning remove the resume_reviewed
 def include_hacker_app_fields(
     applicant_record: dict[str, Any], accept_threshold: float, waitlist_threshold: float
 ) -> None:
@@ -23,6 +25,18 @@ def include_hacker_app_fields(
     _include_resume_reviewed(applicant_record)
 
 
+def include_hacker_app_fields_with_global_and_breakdown(
+    applicant_record: dict[str, Any], accept_threshold: float, waitlist_threshold: float
+) -> None:
+    """For applicant summary where global and breakdown scores are used"""
+    _include_decision_based_on_threshold_and_score_breakdown(
+        applicant_record, accept_threshold, waitlist_threshold
+    )
+    _include_reviewers(applicant_record)
+    _include_avg_score_with_global_and_breakdown(applicant_record)
+    _include_resume_reviewed(applicant_record)
+
+
 def include_review_decision(applicant_record: dict[str, Any]) -> None:
     """Sets the applicant's decision as the last submitted review decision or None."""
     reviews = applicant_record["application_data"]["reviews"]
@@ -43,6 +57,8 @@ def _get_last_score(reviewer: str, reviews: list[tuple[str, str, float]]) -> flo
     return NOT_FULLY_REVIEWED
 
 
+# TODO: Make this function only used for old applicant summary
+# meaning remove the global_field_scores usage
 def _get_avg_score(
     reviews: list[tuple[str, str, float]], global_field_scores: dict[str, Any]
 ) -> float:
@@ -62,6 +78,31 @@ def _get_avg_score(
     return (last_score + last_score2) / 2
 
 
+def _get_avg_score_with_globals_and_breakdown(
+    review_breakdowns: dict[str, dict[str, int]], global_field_scores: dict[str, int]
+) -> float:
+    # Check global_field_scores first - if any value is less than 0,
+    # return OVERQUALIFIED
+    if global_field_scores and any(score < 0 for score in global_field_scores.values()):
+        return OVERQUALIFIED
+
+    if len(review_breakdowns) < 2:
+        return NOT_FULLY_REVIEWED
+
+    # Review breakdowns should be the most recent scores
+    total_score = 2 * sum(global_field_scores.values())
+    for breakdown in review_breakdowns.values():
+        for field, score in breakdown.items():
+            # TODO: Fields from global_field_scores should not be in breakdowns
+            # This check should be removed once breakdown models remove global fields
+            if field in global_field_scores:
+                continue
+
+            total_score += score
+
+    return total_score / 2
+
+
 def _include_decision_based_on_threshold(
     applicant_record: dict[str, Any], accept: float, waitlist: float
 ) -> None:
@@ -77,6 +118,21 @@ def _include_decision_based_on_threshold(
         applicant_record["decision"] = Decision.REJECTED
 
 
+def _include_decision_based_on_threshold_and_score_breakdown(
+    applicant_record: dict[str, Any], accept: float, waitlist: float
+) -> None:
+    avg_score = _get_avg_score_with_globals_and_breakdown(
+        applicant_record["application_data"]["review_breakdown"],
+        applicant_record["application_data"].get("global_field_scores", {}),
+    )
+    if avg_score >= accept:
+        applicant_record["decision"] = Decision.ACCEPTED
+    elif avg_score >= waitlist:
+        applicant_record["decision"] = Decision.WAITLISTED
+    else:
+        applicant_record["decision"] = Decision.REJECTED
+
+
 def _include_reviewers(applicant_record: dict[str, Any]) -> None:
     unique_reviewers = get_unique_reviewers(applicant_record)
     applicant_record["reviewers"] = sorted(list(unique_reviewers))
@@ -89,6 +145,15 @@ def _include_avg_score(applicant_record: dict[str, Any]) -> None:
     )
 
 
+def _include_avg_score_with_global_and_breakdown(
+    applicant_record: dict[str, Any]
+) -> None:
+    applicant_record["avg_score"] = _get_avg_score_with_globals_and_breakdown(
+        applicant_record["application_data"]["review_breakdown"],
+        applicant_record["application_data"].get("global_field_scores", {}),
+    )
+
+
 def _include_resume_reviewed(applicant_record: dict[str, Any]) -> None:
     global_field_scores = applicant_record["application_data"].get(
         "global_field_scores", {}

apps/api/src/routers/admin.py

@@ -174,7 +174,9 @@ async def hacker_applicants(
         raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR)
 
     for record in records:
-        applicant_review_processor.include_hacker_app_fields(
+        # TODO: If we change back to old avg score for summary, change this function
+        # applicant_review_processor.include_hacker_app_fields
+        applicant_review_processor.include_hacker_app_fields_with_global_and_breakdown(
             record, thresholds["accept"], thresholds["waitlist"]
         )
 

apps/api/tests/test_admin.py

@@ -73,58 +73,6 @@ def test_restricted_admin_route_is_forbidden(
     assert res.status_code == 403
 
 
-@patch("services.mongodb_handler.retrieve", autospec=True)
-@patch("services.mongodb_handler.retrieve_one", autospec=True)
-def test_can_retrieve_applicants(
-    mock_mongodb_handler_retrieve_one: AsyncMock,
-    mock_mongodb_handler_retrieve: AsyncMock,
-) -> None:
-    """Test that the applicants summary can be processed."""
-
-    mock_mongodb_handler_retrieve_one.side_effect = [
-        HACKER_REVIEWER_IDENTITY,
-        {"accept": 8, "waitlist": 5},
-    ]
-    mock_mongodb_handler_retrieve.return_value = [
-        {
-            "_id": "edu.uci.petr",
-            "first_name": "Peter",
-            "last_name": "Anteater",
-            "status": "REVIEWED",
-            "application_data": {
-                "school": "UC Irvine",
-                "submission_time": datetime(2023, 1, 12, 9, 0, 0),
-                "reviews": [
-                    [datetime(2023, 1, 18), "edu.uci.alicia", 8],
-                    [datetime(2023, 1, 18), "edu.uci.albert", 9],
-                ],
-            },
-        },
-    ]
-
-    res = reviewer_client.get("/applicants/hackers")
-
-    assert res.status_code == 200
-    mock_mongodb_handler_retrieve.assert_awaited_once()
-    data = res.json()
-    assert data == [
-        {
-            "_id": "edu.uci.petr",
-            "first_name": "Peter",
-            "last_name": "Anteater",
-            "resume_reviewed": False,
-            "avg_score": 8.5,
-            "reviewers": ["edu.uci.albert", "edu.uci.alicia"],
-            "status": "REVIEWED",
-            "decision": "ACCEPTED",
-            "application_data": {
-                "school": "UC Irvine",
-                "submission_time": "2023-01-12T09:00:00",
-            },
-        },
-    ]
-
-
 @patch("services.mongodb_handler.retrieve", autospec=True)
 @patch("services.mongodb_handler.retrieve_one", autospec=True)
 def test_cannot_retrieve_applicants_without_role(
@@ -334,6 +282,64 @@ def test_non_waitlisted_applicant_cannot_be_released(
     mock_mongodb_handler_update_one.assert_not_awaited()
 
 
+# TODO: Should uncomment once new applicant summary is created at different route
+# @patch("services.mongodb_handler.retrieve_one", autospec=True)
+# @patch("services.mongodb_handler.retrieve", autospec=True)
+# def test_hacker_applicants_returns_correct_applicants(
+#     mock_mongodb_handler_retrieve: AsyncMock,
+#     mock_mongodb_handler_retrieve_one: AsyncMock,
+# ) -> None:
+#     """Test that the /applicants/hackers route returns correctly"""
+#     returned_records: list[dict[str, object]] = [
+#         {
+#             "_id": "edu.uci.sydnee",
+#             "first_name": "sydnee",
+#             "last_name": "unknown",
+#             "status": "REVIEWED",
+#             "application_data": {
+#                 "school": "Hamburger University",
+#                 "submission_time": datetime(2023, 1, 12, 9, 0, 0),
+#                 "reviews": [
+#                     [datetime(2023, 1, 19), "edu.uci.alicia", 100],
+#                     [datetime(2023, 1, 19), "edu.uci.alicia2", 200],
+#                 ],
+#             },
+#         }
+#     ]
+
+#     expected_records = [
+#         {
+#             "_id": "edu.uci.sydnee",
+#             "first_name": "sydnee",
+#             "last_name": "unknown",
+#             "resume_reviewed": False,
+#             "status": "REVIEWED",
+#             "decision": "ACCEPTED",
+#             "avg_score": 150.0,
+#             "reviewers": ["edu.uci.alicia", "edu.uci.alicia2"],
+#             "application_data": {
+#                 "school": "Hamburger University",
+#                 "submission_time": "2023-01-12T09:00:00",
+#             },
+#         },
+#     ]
+
+#     returned_thresholds: dict[str, object] = {"accept": 12, "waitlist": 5}
+
+#     mock_mongodb_handler_retrieve.return_value = returned_records
+#     mock_mongodb_handler_retrieve_one.side_effect = [
+#         HACKER_REVIEWER_IDENTITY,
+#         returned_thresholds,
+#     ]
+
+#     res = reviewer_client.get("/applicants/hackers")
+
+#     assert res.status_code == 200
+#     mock_mongodb_handler_retrieve.assert_awaited_once()
+#     data = res.json()
+#     assert data == expected_records
+
+
 @patch("services.mongodb_handler.retrieve_one", autospec=True)
 @patch("services.mongodb_handler.retrieve", autospec=True)
 def test_hacker_applicants_returns_correct_applicants(
@@ -351,9 +357,28 @@ def test_hacker_applicants_returns_correct_applicants(
                 "school": "Hamburger University",
                 "submission_time": datetime(2023, 1, 12, 9, 0, 0),
                 "reviews": [
-                    [datetime(2023, 1, 19), "edu.uci.alicia", 100],
-                    [datetime(2023, 1, 19), "edu.uci.alicia2", 200],
+                    [datetime(2023, 1, 19), "edu.uci.alicia", 56],
+                    [datetime(2023, 1, 19), "edu.uci.alicia2", 60],
                 ],
+                "review_breakdown": {
+                    "alicia": {
+                        "resume": 15,
+                        "elevator_pitch_saq": 6,
+                        "tech_experience_saq": 6,
+                        "learn_about_self_saq": 8,
+                        "pixel_art_saq": 16,
+                        "hackathon_experience": -1000,
+                    },
+                    "alicia2": {
+                        "resume": 15,
+                        "elevator_pitch_saq": 10,
+                        "tech_experience_saq": 10,
+                        "learn_about_self_saq": 10,
+                        "pixel_art_saq": 10,
+                        "hackathon_experience": 5,
+                    },
+                },
+                "global_field_scores": {"resume": 15, "hackathon_experience": 5},
             },
         }
     ]
@@ -363,10 +388,10 @@ def test_hacker_applicants_returns_correct_applicants(
             "_id": "edu.uci.sydnee",
             "first_name": "sydnee",
             "last_name": "unknown",
-            "resume_reviewed": False,
+            "resume_reviewed": True,
             "status": "REVIEWED",
             "decision": "ACCEPTED",
-            "avg_score": 150.0,
+            "avg_score": 58.0,
             "reviewers": ["edu.uci.alicia", "edu.uci.alicia2"],
             "application_data": {
                 "school": "Hamburger University",