From 6e704c9e4e60d94cb25d2597e5d187d7dbb8d269 Mon Sep 17 00:00:00 2001 From: Ian Dai Date: Mon, 23 Dec 2024 21:16:32 +0800 Subject: [PATCH 01/10] - Added admin/review/hacker route for submitting hacker review - Added admin/release/hackers route for releasing hacker applicant decisions - Added test for /review/hacker and /release/hackers --- .../src/admin/applicant_review_processor.py | 14 ++-- apps/api/src/routers/admin.py | 84 +++++++++++++++++-- apps/api/tests/test_admin.py | 83 +++++++++++++++++- 3 files changed, 166 insertions(+), 15 deletions(-) diff --git a/apps/api/src/admin/applicant_review_processor.py b/apps/api/src/admin/applicant_review_processor.py index f445fa00..fee3a2ed 100644 --- a/apps/api/src/admin/applicant_review_processor.py +++ b/apps/api/src/admin/applicant_review_processor.py @@ -19,6 +19,12 @@ def include_review_decision(applicant_record: dict[str, Any]) -> None: applicant_record["decision"] = reviews[-1][2] if reviews else None +def get_num_unique_reviewers(applicant_record: dict[str, Any]) -> int: + reviews = applicant_record["application_data"]["reviews"] + unique_reviewers = {t[1] for t in reviews} + return len(unique_reviewers) + + def _get_last_score(reviewer: str, reviews: list[tuple[str, str, float]]) -> float: for review in reversed(reviews): if review[1] == reviewer: @@ -48,14 +54,8 @@ def _include_decision_based_on_threshold( applicant_record["decision"] = Decision.REJECTED -def _get_num_unique_reviewers(applicant_record: dict[str, Any]) -> int: - reviews = applicant_record["application_data"]["reviews"] - unique_reviewers = {t[1] for t in reviews} - return len(unique_reviewers) - - def _include_num_reviewers(applicant_record: dict[str, Any]) -> None: - applicant_record["num_reviewers"] = _get_num_unique_reviewers(applicant_record) + applicant_record["num_reviewers"] = get_num_unique_reviewers(applicant_record) def _include_avg_score(applicant_record: dict[str, Any]) -> None: diff --git a/apps/api/src/routers/admin.py b/apps/api/src/routers/admin.py index 239b38eb..9f404986 100644 --- a/apps/api/src/routers/admin.py +++ b/apps/api/src/routers/admin.py @@ -11,7 +11,7 @@ from admin.participant_manager import Participant from auth.authorization import require_role from auth.user_identity import User, utc_now -from models.ApplicationData import Decision, OtherReview +from models.ApplicationData import Decision, HackerReview, OtherReview from models.user_record import Applicant, ApplicantStatus, Role, Status from services import mongodb_handler, sendgrid_handler from services.mongodb_handler import BaseRecord, Collection @@ -172,6 +172,47 @@ async def submit_review( raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) +@router.post("/review/hacker") +async def submit_hacker_review( + applicant: str = Body(), + score: float = Body(), + reviewer: User = Depends(require_role({Role.REVIEWER})), +) -> None: + """Submit a review decision from the reviewer for the given hacker applicant.""" + log.info("%s reviewed hacker %s", reviewer, applicant) + + review: HackerReview = (utc_now(), reviewer.uid, score) + + try: + await mongodb_handler.raw_update_one( + Collection.USERS, + {"_id": applicant}, + {"$push": {"application_data.reviews": review}}, + ) + except RuntimeError: + log.error("Could not submit review for %s", applicant) + raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) + + applicant_record = await mongodb_handler.retrieve_one( + Collection.USERS, + {"_id": applicant}, + ["_id", "application_data.reviews"], + ) + if not applicant_record: + log.error("Could not retrieve applicant after submitting review") + raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) + + num_reviewers = applicant_review_processor.get_num_unique_reviewers( + applicant_record + ) + if num_reviewers > 1: + await mongodb_handler.raw_update_one( + Collection.USERS, + {"_id": applicant}, + {"$set": {"status": "REVIEWED"}}, + ) + + @router.post("/release", dependencies=[Depends(require_director)]) async def release_decisions() -> None: """Update applicant status based on decision and send decision emails.""" @@ -184,14 +225,33 @@ async def release_decisions() -> None: for record in records: applicant_review_processor.include_review_decision(record) - for decision in (Decision.ACCEPTED, Decision.WAITLISTED, Decision.REJECTED): - group = [record for record in records if record["decision"] == decision] - if not group: - continue - await asyncio.gather( - *(_process_batch(batch, decision) for batch in batched(group, 100)) + await _process_records_in_batches(records) + + +@router.post("/release/hackers", dependencies=[Depends(require_director)]) +async def release_hacker_decisions() -> None: + """Update hacker applicant status based on decision and send decision emails.""" + records = await mongodb_handler.retrieve( + Collection.USERS, + {"status": Status.REVIEWED}, + ["_id", "application_data.reviews", "first_name"], + ) + + thresholds = await mongodb_handler.retrieve_one( + Collection.SETTINGS, {"_id": "hacker_score_thresholds"}, ["accept", "waitlist"] + ) + + if not thresholds: + log.error("Could not retrieve thresholds") + raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) + + for record in records: + applicant_review_processor.include_hacker_app_fields( + record, thresholds["accept"], thresholds["waitlist"] ) + await _process_records_in_batches(records) + @router.post("/rsvp-reminder", dependencies=[Depends(require_director)]) async def rsvp_reminder() -> None: @@ -345,6 +405,16 @@ async def subevent_checkin( await participant_manager.subevent_checkin(event, uid, organizer) +async def _process_records_in_batches(records: list[dict[str, object]]) -> None: + for decision in (Decision.ACCEPTED, Decision.WAITLISTED, Decision.REJECTED): + group = [record for record in records if record["decision"] == decision] + if not group: + continue + await asyncio.gather( + *(_process_batch(batch, decision) for batch in batched(group, 100)) + ) + + async def _process_status(uids: Sequence[str], status: Status) -> None: ok = await mongodb_handler.update( Collection.USERS, {"_id": {"$in": uids}}, {"status": status} diff --git a/apps/api/tests/test_admin.py b/apps/api/tests/test_admin.py index cc15d497..07370f47 100644 --- a/apps/api/tests/test_admin.py +++ b/apps/api/tests/test_admin.py @@ -1,4 +1,5 @@ from datetime import datetime +from typing import Any from unittest.mock import ANY, AsyncMock, call, patch from fastapi import FastAPI @@ -251,7 +252,7 @@ def test_hacker_applicants_returns_correct_applicants( mock_mongodb_handler_retrieve: AsyncMock, mock_mongodb_handler_retrieve_one: AsyncMock, ) -> None: - """Test that the /hackerApplicants route returns correctly""" + """Test that the /applicants/hackers route returns correctly""" returned_records: list[dict[str, object]] = [ { "_id": "edu.uci.sydnee", @@ -297,3 +298,83 @@ def test_hacker_applicants_returns_correct_applicants( }, }, ] + + +@patch("services.mongodb_handler.raw_update_one", autospec=True) +@patch("services.mongodb_handler.retrieve_one", autospec=True) +def test_submit_hacker_review_works( + mock_mongodb_handler_retrieve_one: AsyncMock, + mock_mongodb_handler_raw_update_one: AsyncMock, +) -> None: + """Test that the /review/hacker route works""" + returned_record: dict[str, Any] = { + "_id": "edu.uci.sydnee", + "application_data": { + "reviews": [ + [datetime(2023, 1, 19), "edu.uci.alicia", 100], + ] + }, + } + + mock_mongodb_handler_retrieve_one.side_effect = [ + REVIEWER_IDENTITY, + returned_record, + REVIEWER_IDENTITY, + returned_record, + ] + mock_mongodb_handler_raw_update_one.side_effect = [True, True, True] + + res = reviewer_client.post( + "/review/hacker", json={"applicant": "edu.uci.sydnee", "score": 0} + ) + + assert res.status_code == 200 + assert mock_mongodb_handler_raw_update_one.await_count == 1 + + # Simulate 2 reviewers + returned_record["application_data"]["reviews"].append( + [datetime(2023, 1, 19), "edu.uci.alicia2", 200] + ) + res = reviewer_client.post( + "/review/hacker", json={"applicant": "edu.uci.sydnee", "score": 0} + ) + + assert res.status_code == 200 + assert mock_mongodb_handler_raw_update_one.await_count == 3 + + +@patch("routers.admin._process_records_in_batches", autospec=True) +@patch("services.mongodb_handler.retrieve", autospec=True) +@patch("services.mongodb_handler.retrieve_one", autospec=True) +def test_release_hacker_decisions_works( + mock_mongodb_handler_retrieve_one: AsyncMock, + mock_mongodb_handler_retrieve: AsyncMock, + mock_admin_process_records_in_batches: AsyncMock, +) -> None: + """Test that the /release/hackers route works""" + returned_records: list[dict[str, Any]] = [ + { + "_id": "edu.uci.sydnee", + "first_name": "sydnee", + "application_data": { + "reviews": [ + [datetime(2023, 1, 19), "edu.uci.alicia", 100], + [datetime(2023, 1, 19), "edu.uci.alicia2", 300], + ] + }, + } + ] + + threshold_record: dict[str, Any] = {"accept": 10, "waitlist": 5} + + mock_mongodb_handler_retrieve_one.side_effect = [ + DIRECTOR_IDENTITY, + threshold_record, + ] + mock_mongodb_handler_retrieve.return_value = returned_records + mock_admin_process_records_in_batches.return_value = None + + res = reviewer_client.post("/release/hackers") + + assert res.status_code == 200 + assert returned_records[0]["decision"] == Decision.ACCEPTED From 51c47bfb77498e05ff7e501e7baaaa2ca60bcc3b Mon Sep 17 00:00:00 2001 From: Ian Dai Date: Mon, 23 Dec 2024 21:52:45 +0800 Subject: [PATCH 02/10] - Added Hacker role to ParticipantRole in userRecord.ts - Added hacker review logs in ApplicantOverview - Added reviewer indicator in HackerApplicants.tsx --- .../app/admin/applicants/[uid]/Applicant.tsx | 20 +++++-- .../[uid]/components/ApplicantOverview.tsx | 15 ++++- .../components/HackerApplicantActions.tsx | 47 +++++++++++++++ .../components/HackerApplicationReviews.tsx | 40 +++++++++++++ .../components/ApplicantReviewerIndicator.tsx | 58 +++++++++++++++++++ .../applicants/hackers/HackerApplicants.tsx | 8 ++- apps/site/src/lib/admin/useApplicant.ts | 29 +++++++++- apps/site/src/lib/userRecord.ts | 4 ++ 8 files changed, 210 insertions(+), 11 deletions(-) create mode 100644 apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx create mode 100644 apps/site/src/app/admin/applicants/[uid]/components/HackerApplicationReviews.tsx create mode 100644 apps/site/src/app/admin/applicants/components/ApplicantReviewerIndicator.tsx diff --git a/apps/site/src/app/admin/applicants/[uid]/Applicant.tsx b/apps/site/src/app/admin/applicants/[uid]/Applicant.tsx index 866ed7ee..d5fcf2b5 100644 --- a/apps/site/src/app/admin/applicants/[uid]/Applicant.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/Applicant.tsx @@ -10,6 +10,8 @@ import useApplicant from "@/lib/admin/useApplicant"; import ApplicantActions from "./components/ApplicantActions"; import ApplicantOverview from "./components/ApplicantOverview"; import Application from "./components/Application"; +import HackerApplicantActions from "./components/HackerApplicantActions"; +import { ParticipantRole } from "@/lib/userRecord"; interface ApplicantProps { params: { uid: string }; @@ -18,7 +20,8 @@ interface ApplicantProps { function Applicant({ params }: ApplicantProps) { const { uid } = params; - const { applicant, loading, submitReview } = useApplicant(uid); + const { applicant, loading, submitReview, submitHackerReview } = + useApplicant(uid); if (loading || !applicant) { return ( @@ -37,10 +40,17 @@ function Applicant({ params }: ApplicantProps) { variant="h1" description="Applicant" actions={ - + applicant.roles.includes(ParticipantRole.Hacker) ? ( + + ) : ( + + ) } > {first_name} {last_name} diff --git a/apps/site/src/app/admin/applicants/[uid]/components/ApplicantOverview.tsx b/apps/site/src/app/admin/applicants/[uid]/components/ApplicantOverview.tsx index 94aa7d64..c5fd7a74 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/ApplicantOverview.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/ApplicantOverview.tsx @@ -4,9 +4,16 @@ import Container from "@cloudscape-design/components/container"; import Header from "@cloudscape-design/components/header"; import ApplicantStatus from "@/app/admin/applicants/components/ApplicantStatus"; -import { Applicant } from "@/lib/admin/useApplicant"; +import { Applicant, HackerReview, Review } from "@/lib/admin/useApplicant"; import ApplicationReviews from "./ApplicationReviews"; +import HackerApplicationReviews from "./HackerApplicationReviews"; + +function isHackerReviewArray( + arr: Review[] | HackerReview[], +): arr is HackerReview[] { + return arr.every((tuple) => typeof tuple[2] === "number"); +} interface ApplicantOverviewProps { applicant: Applicant; @@ -31,7 +38,11 @@ function ApplicantOverview({ applicant }: ApplicantOverviewProps) {
Reviews - + {isHackerReviewArray(reviews) ? ( + + ) : ( + + )}
diff --git a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx new file mode 100644 index 00000000..6450041d --- /dev/null +++ b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx @@ -0,0 +1,47 @@ +import { useContext, useState } from "react"; + +import { Button, Input, SpaceBetween } from "@cloudscape-design/components"; +import { submitHackerReview } from "@/lib/admin/useApplicant"; +import { Uid } from "@/lib/userRecord"; +import UserContext from "@/lib/admin/UserContext"; +import { isReviewer } from "@/lib/admin/authorization"; + +interface ApplicantActionsProps { + applicant: Uid; + submitHackerReview: submitHackerReview; +} + +function HackerApplicantActions({ + applicant, + submitHackerReview, +}: ApplicantActionsProps) { + const { roles } = useContext(UserContext); + const [value, setValue] = useState(""); + + if (!isReviewer(roles)) { + return null; + } + + return ( + + setValue(detail.value)} + value={value} + type="number" + inputMode="decimal" + placeholder="Applicant score" + step={0.1} + /> + + + ); +} + +export default HackerApplicantActions; diff --git a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicationReviews.tsx b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicationReviews.tsx new file mode 100644 index 00000000..0a608677 --- /dev/null +++ b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicationReviews.tsx @@ -0,0 +1,40 @@ +import { useContext } from "react"; + +import { HackerReview } from "@/lib/admin/useApplicant"; +import { Uid } from "@/lib/userRecord"; +import UserContext from "@/lib/admin/UserContext"; + +interface HackerApplicationReviewsProps { + reviews: HackerReview[]; +} + +function HackerApplicationReviews({ reviews }: HackerApplicationReviewsProps) { + const { uid } = useContext(UserContext); + + if (reviews.length === 0) { + return

-

; + } + + const formatUid = (uid: Uid) => uid.split(".").at(-1); + const formatDate = (timestamp: string) => + new Date(timestamp).toLocaleDateString(); + + return ( +
    + {reviews.map(([date, reviewer, score]) => + reviewer === uid ? ( +
  • + You scored this applicant a {score} on {formatDate(date)} +
    • + ) : ( +
  • + {formatUid(reviewer)} reviewed this application on{" "} + {formatDate(date)} +
    • + ), + )} +
+ ); +} + +export default HackerApplicationReviews; diff --git a/apps/site/src/app/admin/applicants/components/ApplicantReviewerIndicator.tsx b/apps/site/src/app/admin/applicants/components/ApplicantReviewerIndicator.tsx new file mode 100644 index 00000000..4034b675 --- /dev/null +++ b/apps/site/src/app/admin/applicants/components/ApplicantReviewerIndicator.tsx @@ -0,0 +1,58 @@ +import { + Box, + SpaceBetween, + StatusIndicator, +} from "@cloudscape-design/components"; + +interface IndicatorContainerProps { + reviewerNumber: string; + type: string; +} + +const IndicatorContainer = ({ + reviewerNumber, + type, +}: IndicatorContainerProps) => { + return ( + <> + Reviewer {reviewerNumber} + {type === "notReviewed" ? ( + Not Reviewed + ) : ( + Reviewed + )} + + ); +}; +interface ApplicantReviewerIndicatorProps { + num_reviewers: number; +} + +function ApplicantReviewerIndicator({ + num_reviewers, +}: ApplicantReviewerIndicatorProps) { + return ( + + {num_reviewers === 0 ? ( + <> + + + + ) : num_reviewers === 1 ? ( + <> + + + + ) : num_reviewers === 2 ? ( + <> + + + + ) : ( + <> + )} + + ); +} + +export default ApplicantReviewerIndicator; diff --git a/apps/site/src/app/admin/applicants/hackers/HackerApplicants.tsx b/apps/site/src/app/admin/applicants/hackers/HackerApplicants.tsx index fb11748a..0d909485 100644 --- a/apps/site/src/app/admin/applicants/hackers/HackerApplicants.tsx +++ b/apps/site/src/app/admin/applicants/hackers/HackerApplicants.tsx @@ -19,6 +19,7 @@ import ApplicantStatus from "../components/ApplicantStatus"; import UserContext from "@/lib/admin/UserContext"; import { isApplicationManager } from "@/lib/admin/authorization"; +import ApplicantReviewerIndicator from "../components/ApplicantReviewerIndicator"; function HackerApplicants() { const router = useRouter(); @@ -77,6 +78,11 @@ function HackerApplicants() { header: "Status", content: ApplicantStatus, }, + { + id: "reviewers", + header: "", + content: ApplicantReviewerIndicator, + }, { id: "submission_time", header: "Applied", @@ -90,7 +96,7 @@ function HackerApplicants() { }, { id: "decision", - header: "Decision (based on average score)", + header: "Decision", content: DecisionStatus, }, ], diff --git a/apps/site/src/lib/admin/useApplicant.ts b/apps/site/src/lib/admin/useApplicant.ts index 8e4fd460..2c727d67 100644 --- a/apps/site/src/lib/admin/useApplicant.ts +++ b/apps/site/src/lib/admin/useApplicant.ts @@ -1,9 +1,16 @@ import axios from "axios"; import useSWR from "swr"; -import { Decision, ParticipantRole, Status, Uid } from "@/lib/userRecord"; +import { + Decision, + ParticipantRole, + Status, + Uid, + Score, +} from "@/lib/userRecord"; export type Review = [string, Uid, Decision]; +export type HackerReview = [string, Uid, Score]; // The application responses submitted by an applicant export interface ApplicationData { @@ -21,7 +28,7 @@ export interface ApplicationData { frq_dream_job: string; resume_url: string; submission_time: string; - reviews: Review[]; + reviews: Review[] | HackerReview[]; } export type ApplicationQuestion = Exclude; @@ -56,9 +63,25 @@ function useApplicant(uid: Uid) { mutate(); } - return { applicant: data, loading: isLoading, error, submitReview }; + async function submitHackerReview(uid: Uid, score: string) { + await axios.post("/api/admin/review/hacker", { + applicant: uid, + score: score, + }); + // TODO: provide success status to display in alert + mutate(); + } + + return { + applicant: data, + loading: isLoading, + error, + submitReview, + submitHackerReview, + }; } export type submitReview = (uid: Uid, review: Decision) => Promise; +export type submitHackerReview = (uid: Uid, score: string) => Promise; export default useApplicant; diff --git a/apps/site/src/lib/userRecord.ts b/apps/site/src/lib/userRecord.ts index bd1420ca..fa7c8686 100644 --- a/apps/site/src/lib/userRecord.ts +++ b/apps/site/src/lib/userRecord.ts @@ -1,11 +1,15 @@ /** Represents a UID of a user record, just an alias for string. */ export type Uid = string; +/** Represents score for a hacker applicant, just an alias for number */ +export type Score = number; + // Note: role labels should match `user_record.Role` in the API /** The possible roles of general participants. */ export enum ParticipantRole { Applicant = "Applicant", + Hacker = "Hacker", Mentor = "Mentor", Volunteer = "Volunteer", Sponsor = "Sponsor", From 33e8f4a79f2be690428f93f999c7e3b103b788b9 Mon Sep 17 00:00:00 2001 From: Ian Dai Date: Tue, 24 Dec 2024 16:18:10 +0800 Subject: [PATCH 03/10] - Simplified duplication in ApplicantReviewerIndicator - Made StatusIndicator pending type - Imported cloudscape components directly - Moved useApplicant.ts to admin/applicants/hackers - Made _retrieve_threshold function in admin.py - Added comment for /admin/review/hacker route - Moved review fields into a body model for /review/hacker and /review --- apps/api/src/routers/admin.py | 49 +++++++++++------- apps/api/tests/test_admin.py | 5 +- .../app/admin/applicants/[uid]/Applicant.tsx | 2 +- .../[uid]/components/ApplicantActions.tsx | 2 +- .../[uid]/components/ApplicantOverview.tsx | 6 ++- .../[uid]/components/Application.tsx | 5 +- .../[uid]/components/ApplicationReviews.tsx | 2 +- .../[uid]/components/ApplicationSection.tsx | 5 +- .../components/HackerApplicantActions.tsx | 2 +- .../components/HackerApplicationReviews.tsx | 2 +- .../components/ApplicantReviewerIndicator.tsx | 50 +++++++------------ .../applicants/hackers/HackerApplicants.tsx | 2 +- .../admin/applicants/hackers}/useApplicant.ts | 0 13 files changed, 72 insertions(+), 60 deletions(-) rename apps/site/src/{lib/admin => app/admin/applicants/hackers}/useApplicant.ts (100%) diff --git a/apps/api/src/routers/admin.py b/apps/api/src/routers/admin.py index 9f404986..c93b60f5 100644 --- a/apps/api/src/routers/admin.py +++ b/apps/api/src/routers/admin.py @@ -53,6 +53,16 @@ class HackerApplicantSummary(BaseRecord): application_data: ApplicationDataSummary +class HackerReviewModel(BaseModel): + applicant: str + score: float + + +class NonHackerReviewModel(BaseModel): + applicant: str + decision: Decision + + @router.get("/applicants") async def applicants( user: Annotated[User, Depends(require_manager)] @@ -104,9 +114,7 @@ async def hacker_applicants( ], ) - thresholds: Optional[dict[str, float]] = await mongodb_handler.retrieve_one( - Collection.SETTINGS, {"_id": "hacker_score_thresholds"}, ["accept", "waitlist"] - ) + thresholds: Optional[dict[str, float]] = await _retrieve_thresholds() if not thresholds: log.error("Could not retrieve thresholds") @@ -149,39 +157,37 @@ async def applicant_summary() -> dict[ApplicantStatus, int]: @router.post("/review") async def submit_review( - applicant: str = Body(), - decision: Decision = Body(), + applicant_review: NonHackerReviewModel, reviewer: User = Depends(require_role({Role.REVIEWER})), ) -> None: """Submit a review decision from the reviewer for the given applicant.""" - log.info("%s reviewed applicant %s", reviewer, applicant) + log.info("%s reviewed applicant %s", reviewer, applicant_review.applicant) - review: OtherReview = (utc_now(), reviewer.uid, decision) + review: OtherReview = (utc_now(), reviewer.uid, applicant_review.decision) try: await mongodb_handler.raw_update_one( Collection.USERS, - {"_id": applicant}, + {"_id": applicant_review.applicant}, { "$push": {"application_data.reviews": review}, "$set": {"status": "REVIEWED"}, }, ) except RuntimeError: - log.error("Could not submit review for %s", applicant) + log.error("Could not submit review for %s", applicant_review.applicant) raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) @router.post("/review/hacker") async def submit_hacker_review( - applicant: str = Body(), - score: float = Body(), + hacker_review: HackerReviewModel, reviewer: User = Depends(require_role({Role.REVIEWER})), ) -> None: """Submit a review decision from the reviewer for the given hacker applicant.""" log.info("%s reviewed hacker %s", reviewer, applicant) - review: HackerReview = (utc_now(), reviewer.uid, score) + review: HackerReview = (utc_now(), reviewer.uid, hacker_review.score) try: await mongodb_handler.raw_update_one( @@ -190,7 +196,9 @@ async def submit_hacker_review( {"$push": {"application_data.reviews": review}}, ) except RuntimeError: - log.error("Could not submit review for %s", applicant) + log.error( + "%s could not submit review for %s", reviewer, hacker_review.applicant + ) raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) applicant_record = await mongodb_handler.retrieve_one( @@ -205,7 +213,10 @@ async def submit_hacker_review( num_reviewers = applicant_review_processor.get_num_unique_reviewers( applicant_record ) - if num_reviewers > 1: + + # Because reviewing a hacker requires 2 reviewers, only set the + # applicant's status to REVIEWED if there are at least 2 reviewers + if num_reviewers >= 2: await mongodb_handler.raw_update_one( Collection.USERS, {"_id": applicant}, @@ -237,9 +248,7 @@ async def release_hacker_decisions() -> None: ["_id", "application_data.reviews", "first_name"], ) - thresholds = await mongodb_handler.retrieve_one( - Collection.SETTINGS, {"_id": "hacker_score_thresholds"}, ["accept", "waitlist"] - ) + thresholds: Optional[dict[str, float]] = await _retrieve_thresholds() if not thresholds: log.error("Could not retrieve thresholds") @@ -452,3 +461,9 @@ def _recover_email_from_uid(uid: str) -> str: local = local.replace("\n", ".") domain = ".".join(reversed(reversed_domain)) return f"{local}@{domain}" + + +async def _retrieve_thresholds() -> Optional[dict[str, Any]]: + return await mongodb_handler.retrieve_one( + Collection.SETTINGS, {"_id": "hacker_score_thresholds"}, ["accept", "waitlist"] + ) diff --git a/apps/api/tests/test_admin.py b/apps/api/tests/test_admin.py index 07370f47..4692cd43 100644 --- a/apps/api/tests/test_admin.py +++ b/apps/api/tests/test_admin.py @@ -125,6 +125,7 @@ def test_can_submit_review( json={"applicant": "edu.uci.applicant", "decision": Decision.ACCEPTED}, ) + assert res.status_code == 200 mock_mongodb_handler_retrieve_one.assert_awaited_once() mock_mongodb_handler_raw_update_one.assert_awaited_once_with( Collection.USERS, @@ -136,7 +137,6 @@ def test_can_submit_review( "$set": {"status": "REVIEWED"}, }, ) - assert res.status_code == 200 @patch("services.mongodb_handler.update", autospec=True) @@ -175,6 +175,7 @@ def test_confirm_attendance_route( res = director_client.post("/confirm-attendance") + assert res.status_code == 200 mock_mongodb_handler_retrieve.assert_awaited_once() mock_mognodb_handler_update.assert_has_calls( [ @@ -196,8 +197,6 @@ def test_confirm_attendance_route( ] ) - assert res.status_code == 200 - @patch("services.sendgrid_handler.send_email", autospec=True) @patch("services.mongodb_handler.update_one", autospec=True) diff --git a/apps/site/src/app/admin/applicants/[uid]/Applicant.tsx b/apps/site/src/app/admin/applicants/[uid]/Applicant.tsx index d5fcf2b5..c7e92973 100644 --- a/apps/site/src/app/admin/applicants/[uid]/Applicant.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/Applicant.tsx @@ -5,7 +5,7 @@ import Header from "@cloudscape-design/components/header"; import SpaceBetween from "@cloudscape-design/components/space-between"; import Spinner from "@cloudscape-design/components/spinner"; -import useApplicant from "@/lib/admin/useApplicant"; +import useApplicant from "@/app/admin/applicants/hackers/useApplicant"; import ApplicantActions from "./components/ApplicantActions"; import ApplicantOverview from "./components/ApplicantOverview"; diff --git a/apps/site/src/app/admin/applicants/[uid]/components/ApplicantActions.tsx b/apps/site/src/app/admin/applicants/[uid]/components/ApplicantActions.tsx index 08d62e01..50e3acf8 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/ApplicantActions.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/ApplicantActions.tsx @@ -5,7 +5,7 @@ import ButtonDropdown, { } from "@cloudscape-design/components/button-dropdown"; import { isReviewer } from "@/lib/admin/authorization"; -import { submitReview } from "@/lib/admin/useApplicant"; +import { submitReview } from "@/app/admin/applicants/hackers/useApplicant"; import UserContext from "@/lib/admin/UserContext"; import { Decision, Uid } from "@/lib/userRecord"; diff --git a/apps/site/src/app/admin/applicants/[uid]/components/ApplicantOverview.tsx b/apps/site/src/app/admin/applicants/[uid]/components/ApplicantOverview.tsx index c5fd7a74..d67c0ece 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/ApplicantOverview.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/ApplicantOverview.tsx @@ -4,7 +4,11 @@ import Container from "@cloudscape-design/components/container"; import Header from "@cloudscape-design/components/header"; import ApplicantStatus from "@/app/admin/applicants/components/ApplicantStatus"; -import { Applicant, HackerReview, Review } from "@/lib/admin/useApplicant"; +import { + Applicant, + HackerReview, + Review, +} from "@/app/admin/applicants/hackers/useApplicant"; import ApplicationReviews from "./ApplicationReviews"; import HackerApplicationReviews from "./HackerApplicationReviews"; diff --git a/apps/site/src/app/admin/applicants/[uid]/components/Application.tsx b/apps/site/src/app/admin/applicants/[uid]/components/Application.tsx index 15b9a797..e01f6ccd 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/Application.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/Application.tsx @@ -2,7 +2,10 @@ import Container from "@cloudscape-design/components/container"; import Header from "@cloudscape-design/components/header"; import SpaceBetween from "@cloudscape-design/components/space-between"; -import { Applicant, ApplicationQuestion } from "@/lib/admin/useApplicant"; +import { + Applicant, + ApplicationQuestion, +} from "@/app/admin/applicants/hackers/useApplicant"; import ApplicationSection from "./ApplicationSection"; diff --git a/apps/site/src/app/admin/applicants/[uid]/components/ApplicationReviews.tsx b/apps/site/src/app/admin/applicants/[uid]/components/ApplicationReviews.tsx index 820ac909..66373399 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/ApplicationReviews.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/ApplicationReviews.tsx @@ -1,7 +1,7 @@ import { useContext } from "react"; import ApplicantStatus from "@/app/admin/applicants/components/ApplicantStatus"; -import { Review } from "@/lib/admin/useApplicant"; +import { Review } from "@/app/admin/applicants/hackers/useApplicant"; import UserContext from "@/lib/admin/UserContext"; import { Uid } from "@/lib/userRecord"; diff --git a/apps/site/src/app/admin/applicants/[uid]/components/ApplicationSection.tsx b/apps/site/src/app/admin/applicants/[uid]/components/ApplicationSection.tsx index f4b52845..094417d8 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/ApplicationSection.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/ApplicationSection.tsx @@ -1,7 +1,10 @@ import ColumnLayout from "@cloudscape-design/components/column-layout"; import TextContent from "@cloudscape-design/components/text-content"; -import { ApplicationData, ApplicationQuestion } from "@/lib/admin/useApplicant"; +import { + ApplicationData, + ApplicationQuestion, +} from "@/app/admin/applicants/hackers/useApplicant"; interface ApplicationResponseProps { value: string | boolean | string[] | null; diff --git a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx index 6450041d..b5f5fb5c 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx @@ -1,7 +1,7 @@ import { useContext, useState } from "react"; import { Button, Input, SpaceBetween } from "@cloudscape-design/components"; -import { submitHackerReview } from "@/lib/admin/useApplicant"; +import { submitHackerReview } from "@/app/admin/applicants/hackers/useApplicant"; import { Uid } from "@/lib/userRecord"; import UserContext from "@/lib/admin/UserContext"; import { isReviewer } from "@/lib/admin/authorization"; diff --git a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicationReviews.tsx b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicationReviews.tsx index 0a608677..8ae2aad7 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicationReviews.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicationReviews.tsx @@ -1,6 +1,6 @@ import { useContext } from "react"; -import { HackerReview } from "@/lib/admin/useApplicant"; +import { HackerReview } from "@/app/admin/applicants/hackers/useApplicant"; import { Uid } from "@/lib/userRecord"; import UserContext from "@/lib/admin/UserContext"; diff --git a/apps/site/src/app/admin/applicants/components/ApplicantReviewerIndicator.tsx b/apps/site/src/app/admin/applicants/components/ApplicantReviewerIndicator.tsx index 4034b675..c34eacb5 100644 --- a/apps/site/src/app/admin/applicants/components/ApplicantReviewerIndicator.tsx +++ b/apps/site/src/app/admin/applicants/components/ApplicantReviewerIndicator.tsx @@ -1,29 +1,28 @@ -import { - Box, - SpaceBetween, - StatusIndicator, -} from "@cloudscape-design/components"; +import Box from "@cloudscape-design/components/box"; +import SpaceBetween from "@cloudscape-design/components/space-between"; +import StatusIndicator from "@cloudscape-design/components/status-indicator"; interface IndicatorContainerProps { - reviewerNumber: string; - type: string; + number: number; + hasReviewed: boolean; } const IndicatorContainer = ({ - reviewerNumber, - type, + number, + hasReviewed, }: IndicatorContainerProps) => { return ( <> - Reviewer {reviewerNumber} - {type === "notReviewed" ? ( - Not Reviewed - ) : ( + Reviewer {number} + {hasReviewed ? ( Reviewed + ) : ( + Not Reviewed )} ); }; + interface ApplicantReviewerIndicatorProps { num_reviewers: number; } @@ -33,24 +32,13 @@ function ApplicantReviewerIndicator({ }: ApplicantReviewerIndicatorProps) { return ( - {num_reviewers === 0 ? ( - <> - - - - ) : num_reviewers === 1 ? ( - <> - - - - ) : num_reviewers === 2 ? ( - <> - - - - ) : ( - <> - )} + {[1, 2].map((n) => ( + = num_reviewers} + /> + ))} ); } diff --git a/apps/site/src/app/admin/applicants/hackers/HackerApplicants.tsx b/apps/site/src/app/admin/applicants/hackers/HackerApplicants.tsx index 0d909485..035dcaec 100644 --- a/apps/site/src/app/admin/applicants/hackers/HackerApplicants.tsx +++ b/apps/site/src/app/admin/applicants/hackers/HackerApplicants.tsx @@ -80,7 +80,7 @@ function HackerApplicants() { }, { id: "reviewers", - header: "", + header: "Reviewer Status", content: ApplicantReviewerIndicator, }, { diff --git a/apps/site/src/lib/admin/useApplicant.ts b/apps/site/src/app/admin/applicants/hackers/useApplicant.ts similarity index 100% rename from apps/site/src/lib/admin/useApplicant.ts rename to apps/site/src/app/admin/applicants/hackers/useApplicant.ts From ca74ae8abc08691f42bf33dabfdcb2d427c6b2d3 Mon Sep 17 00:00:00 2001 From: Ian Dai Date: Tue, 24 Dec 2024 18:05:21 +0800 Subject: [PATCH 04/10] - Combined reviews into one route, now all reviews use a score, but UI is labelled differently based on app type - Updated test_admin.py - Fixed ApplicantReviewerIndicator - Combined HackerApplicationReviews and ApplicationReviews --- apps/api/src/models/ApplicationData.py | 9 +- apps/api/src/routers/admin.py | 106 +++++++-------- apps/api/tests/test_admin.py | 124 +++++++++--------- .../app/admin/applicants/[uid]/Applicant.tsx | 5 +- .../[uid]/components/ApplicantActions.tsx | 5 +- .../[uid]/components/ApplicantOverview.tsx | 23 +--- .../[uid]/components/ApplicationReviews.tsx | 23 +++- .../components/HackerApplicantActions.tsx | 8 +- .../components/HackerApplicationReviews.tsx | 40 ------ .../components/ApplicantReviewerIndicator.tsx | 2 +- .../applicants/hackers/HackerApplicants.tsx | 2 +- .../admin/applicants/hackers/useApplicant.ts | 30 +---- apps/site/src/lib/decisionScores.ts | 18 +++ 13 files changed, 174 insertions(+), 221 deletions(-) delete mode 100644 apps/site/src/app/admin/applicants/[uid]/components/HackerApplicationReviews.tsx create mode 100644 apps/site/src/lib/decisionScores.ts diff --git a/apps/api/src/models/ApplicationData.py b/apps/api/src/models/ApplicationData.py index a9c54120..051c1cc0 100644 --- a/apps/api/src/models/ApplicationData.py +++ b/apps/api/src/models/ApplicationData.py @@ -22,8 +22,7 @@ class Decision(str, Enum): REJECTED = "REJECTED" -HackerReview = tuple[datetime, str, float] -OtherReview = tuple[datetime, str, Decision] +Review = tuple[datetime, str, float] def make_empty_none(val: Union[str, None]) -> Union[str, None]: @@ -140,7 +139,7 @@ class ProcessedHackerApplicationData(BaseApplicationData): email: EmailStr resume_url: Union[HttpUrl, None] = None submission_time: datetime - reviews: list[HackerReview] = [] + reviews: list[Review] = [] @field_serializer("linkedin", "portfolio", "resume_url") def url2str(self, val: Union[HttpUrl, None]) -> Union[str, None]: @@ -153,7 +152,7 @@ class ProcessedMentorApplicationData(BaseMentorApplicationData): email: EmailStr resume_url: Union[HttpUrl, None] = None submission_time: datetime - reviews: list[OtherReview] = [] + reviews: list[Review] = [] @field_serializer("linkedin", "github", "portfolio", "resume_url") def url2str(self, val: Union[HttpUrl, None]) -> Union[str, None]: @@ -166,7 +165,7 @@ class ProcessedVolunteerApplication(BaseVolunteerApplicationData): # TODO: specify common attributes in mixin email: EmailStr submission_time: datetime - reviews: list[OtherReview] = [] + reviews: list[Review] = [] # To add more discriminating values, add a string diff --git a/apps/api/src/routers/admin.py b/apps/api/src/routers/admin.py index c93b60f5..c3a56b32 100644 --- a/apps/api/src/routers/admin.py +++ b/apps/api/src/routers/admin.py @@ -1,7 +1,7 @@ import asyncio from datetime import datetime from logging import getLogger -from typing import Annotated, Any, Optional, Sequence +from typing import Annotated, Any, Mapping, Optional, Sequence from fastapi import APIRouter, Body, Depends, HTTPException, status from pydantic import BaseModel, EmailStr, TypeAdapter, ValidationError @@ -11,7 +11,7 @@ from admin.participant_manager import Participant from auth.authorization import require_role from auth.user_identity import User, utc_now -from models.ApplicationData import Decision, HackerReview, OtherReview +from models.ApplicationData import Decision, Review from models.user_record import Applicant, ApplicantStatus, Role, Status from services import mongodb_handler, sendgrid_handler from services.mongodb_handler import BaseRecord, Collection @@ -53,16 +53,11 @@ class HackerApplicantSummary(BaseRecord): application_data: ApplicationDataSummary -class HackerReviewModel(BaseModel): +class ReviewRequestModel(BaseModel): applicant: str score: float -class NonHackerReviewModel(BaseModel): - applicant: str - decision: Decision - - @router.get("/applicants") async def applicants( user: Annotated[User, Depends(require_manager)] @@ -157,70 +152,47 @@ async def applicant_summary() -> dict[ApplicantStatus, int]: @router.post("/review") async def submit_review( - applicant_review: NonHackerReviewModel, - reviewer: User = Depends(require_role({Role.REVIEWER})), -) -> None: - """Submit a review decision from the reviewer for the given applicant.""" - log.info("%s reviewed applicant %s", reviewer, applicant_review.applicant) - - review: OtherReview = (utc_now(), reviewer.uid, applicant_review.decision) - - try: - await mongodb_handler.raw_update_one( - Collection.USERS, - {"_id": applicant_review.applicant}, - { - "$push": {"application_data.reviews": review}, - "$set": {"status": "REVIEWED"}, - }, - ) - except RuntimeError: - log.error("Could not submit review for %s", applicant_review.applicant) - raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) - - -@router.post("/review/hacker") -async def submit_hacker_review( - hacker_review: HackerReviewModel, + applicant_review: ReviewRequestModel, reviewer: User = Depends(require_role({Role.REVIEWER})), ) -> None: """Submit a review decision from the reviewer for the given hacker applicant.""" - log.info("%s reviewed hacker %s", reviewer, applicant) + log.info("%s reviewed hacker %s", reviewer, applicant_review.applicant) - review: HackerReview = (utc_now(), reviewer.uid, hacker_review.score) + review: Review = (utc_now(), reviewer.uid, applicant_review.score) - try: - await mongodb_handler.raw_update_one( - Collection.USERS, - {"_id": applicant}, - {"$push": {"application_data.reviews": review}}, - ) - except RuntimeError: - log.error( - "%s could not submit review for %s", reviewer, hacker_review.applicant - ) - raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) + await _try_update_applicant_with_query( + applicant_review, + query={"$push": {"application_data.reviews": review}}, + err_msg=f"{reviewer} could not submit review for {applicant_review.applicant}", + ) applicant_record = await mongodb_handler.retrieve_one( Collection.USERS, - {"_id": applicant}, - ["_id", "application_data.reviews"], + {"_id": applicant_review.applicant}, + ["_id", "application_data.reviews", "roles"], ) if not applicant_record: log.error("Could not retrieve applicant after submitting review") raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) - num_reviewers = applicant_review_processor.get_num_unique_reviewers( - applicant_record - ) + if Role.HACKER in applicant_record["roles"]: + num_reviewers = applicant_review_processor.get_num_unique_reviewers( + applicant_record + ) - # Because reviewing a hacker requires 2 reviewers, only set the - # applicant's status to REVIEWED if there are at least 2 reviewers - if num_reviewers >= 2: - await mongodb_handler.raw_update_one( - Collection.USERS, - {"_id": applicant}, - {"$set": {"status": "REVIEWED"}}, + # Because reviewing a hacker requires 2 reviewers, only set the + # applicant's status to REVIEWED if there are at least 2 reviewers + if num_reviewers >= 2: + await _try_update_applicant_with_query( + applicant_review, + query={"$set": {"status": "REVIEWED"}}, + err_msg=f"Could not update status for {applicant_review.applicant}", + ) + else: + await _try_update_applicant_with_query( + applicant_review, + query={"$set": {"status": "REVIEWED"}}, + err_msg=f"Could not update status for {applicant_review.applicant}", ) @@ -239,6 +211,7 @@ async def release_decisions() -> None: await _process_records_in_batches(records) +# TODO: need to make release hackers check roles as part of query @router.post("/release/hackers", dependencies=[Depends(require_director)]) async def release_hacker_decisions() -> None: """Update hacker applicant status based on decision and send decision emails.""" @@ -467,3 +440,20 @@ async def _retrieve_thresholds() -> Optional[dict[str, Any]]: return await mongodb_handler.retrieve_one( Collection.SETTINGS, {"_id": "hacker_score_thresholds"}, ["accept", "waitlist"] ) + + +async def _try_update_applicant_with_query( + applicant_review: ReviewRequestModel, + *, + query: Mapping[str, object], + err_msg: str = "", +) -> None: + try: + await mongodb_handler.raw_update_one( + Collection.USERS, + {"_id": applicant_review.applicant}, + query, + ) + except RuntimeError: + log.error(err_msg) + raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) diff --git a/apps/api/tests/test_admin.py b/apps/api/tests/test_admin.py index 4692cd43..1759e8b8 100644 --- a/apps/api/tests/test_admin.py +++ b/apps/api/tests/test_admin.py @@ -112,32 +112,81 @@ def test_can_retrieve_applicants( @patch("services.mongodb_handler.raw_update_one", autospec=True) @patch("services.mongodb_handler.retrieve_one", autospec=True) -def test_can_submit_review( +def test_can_submit_nonhacker_review( mock_mongodb_handler_retrieve_one: AsyncMock, mock_mongodb_handler_raw_update_one: AsyncMock, ) -> None: - """Test that a user can properly submit an applicant review.""" + """Test that a user can properly submit a nonhacker applicant review.""" + post_data = {"applicant": "edu.uci.sydnee", "score": 0} - mock_mongodb_handler_retrieve_one.return_value = REVIEWER_IDENTITY + returned_record: dict[str, Any] = { + "_id": "edu.uci.sydnee", + "roles": ["Applicant", "Mentor"], + "application_data": { + "reviews": [ + [datetime(2023, 1, 19), "edu.uci.alicia", 100], + ] + }, + } - res = reviewer_client.post( - "/review", - json={"applicant": "edu.uci.applicant", "decision": Decision.ACCEPTED}, - ) + mock_mongodb_handler_retrieve_one.side_effect = [ + REVIEWER_IDENTITY, + returned_record, + REVIEWER_IDENTITY, + returned_record, + ] + mock_mongodb_handler_raw_update_one.side_effect = [True, True, True] + + res = reviewer_client.post("/review", json=post_data) assert res.status_code == 200 - mock_mongodb_handler_retrieve_one.assert_awaited_once() - mock_mongodb_handler_raw_update_one.assert_awaited_once_with( - Collection.USERS, - {"_id": "edu.uci.applicant"}, - { - "$push": { - "application_data.reviews": (ANY, "edu.uci.alicia", Decision.ACCEPTED) - }, - "$set": {"status": "REVIEWED"}, + assert mock_mongodb_handler_raw_update_one.await_count == 2 + + +@patch("services.mongodb_handler.raw_update_one", autospec=True) +@patch("services.mongodb_handler.retrieve_one", autospec=True) +def test_submit_hacker_review_works( + mock_mongodb_handler_retrieve_one: AsyncMock, + mock_mongodb_handler_raw_update_one: AsyncMock, +) -> None: + """Test that a user can properly submit a hacker applicant review.""" + post_data = {"applicant": "edu.uci.sydnee", "score": 0} + + returned_record: dict[str, Any] = { + "_id": "edu.uci.sydnee", + "roles": ["Applicant", "Hacker"], + "application_data": { + "reviews": [ + [datetime(2023, 1, 19), "edu.uci.alicia", 100], + ] }, + } + + mock_mongodb_handler_retrieve_one.side_effect = [ + REVIEWER_IDENTITY, + returned_record, + REVIEWER_IDENTITY, + returned_record, + ] + mock_mongodb_handler_raw_update_one.side_effect = [True, True, True] + + res = reviewer_client.post("/review", json=post_data) + + assert res.status_code == 200 + assert mock_mongodb_handler_raw_update_one.await_count == 1 + + # Simulate 2 reviewers + # Should make raw_update_one run at very end of /review function + returned_record["application_data"]["reviews"].append( + [datetime(2023, 1, 19), "edu.uci.alicia2", 200] + ) + res = reviewer_client.post( + "/review", json={"applicant": "edu.uci.sydnee", "score": 0} ) + assert res.status_code == 200 + assert mock_mongodb_handler_raw_update_one.await_count == 3 + @patch("services.mongodb_handler.update", autospec=True) @patch("services.mongodb_handler.retrieve_one", autospec=True) @@ -299,49 +348,6 @@ def test_hacker_applicants_returns_correct_applicants( ] -@patch("services.mongodb_handler.raw_update_one", autospec=True) -@patch("services.mongodb_handler.retrieve_one", autospec=True) -def test_submit_hacker_review_works( - mock_mongodb_handler_retrieve_one: AsyncMock, - mock_mongodb_handler_raw_update_one: AsyncMock, -) -> None: - """Test that the /review/hacker route works""" - returned_record: dict[str, Any] = { - "_id": "edu.uci.sydnee", - "application_data": { - "reviews": [ - [datetime(2023, 1, 19), "edu.uci.alicia", 100], - ] - }, - } - - mock_mongodb_handler_retrieve_one.side_effect = [ - REVIEWER_IDENTITY, - returned_record, - REVIEWER_IDENTITY, - returned_record, - ] - mock_mongodb_handler_raw_update_one.side_effect = [True, True, True] - - res = reviewer_client.post( - "/review/hacker", json={"applicant": "edu.uci.sydnee", "score": 0} - ) - - assert res.status_code == 200 - assert mock_mongodb_handler_raw_update_one.await_count == 1 - - # Simulate 2 reviewers - returned_record["application_data"]["reviews"].append( - [datetime(2023, 1, 19), "edu.uci.alicia2", 200] - ) - res = reviewer_client.post( - "/review/hacker", json={"applicant": "edu.uci.sydnee", "score": 0} - ) - - assert res.status_code == 200 - assert mock_mongodb_handler_raw_update_one.await_count == 3 - - @patch("routers.admin._process_records_in_batches", autospec=True) @patch("services.mongodb_handler.retrieve", autospec=True) @patch("services.mongodb_handler.retrieve_one", autospec=True) diff --git a/apps/site/src/app/admin/applicants/[uid]/Applicant.tsx b/apps/site/src/app/admin/applicants/[uid]/Applicant.tsx index c7e92973..b3959aed 100644 --- a/apps/site/src/app/admin/applicants/[uid]/Applicant.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/Applicant.tsx @@ -20,8 +20,7 @@ interface ApplicantProps { function Applicant({ params }: ApplicantProps) { const { uid } = params; - const { applicant, loading, submitReview, submitHackerReview } = - useApplicant(uid); + const { applicant, loading, submitReview } = useApplicant(uid); if (loading || !applicant) { return ( @@ -43,7 +42,7 @@ function Applicant({ params }: ApplicantProps) { applicant.roles.includes(ParticipantRole.Hacker) ? ( ) : ( , ) => { - const review = event.detail.id; - submitReview(applicant, review as Decision); + const review = event.detail.id as Decision; + submitReview(applicant, decisionsToScores[review]); }; const dropdownItems: ReviewButtonItems = [ diff --git a/apps/site/src/app/admin/applicants/[uid]/components/ApplicantOverview.tsx b/apps/site/src/app/admin/applicants/[uid]/components/ApplicantOverview.tsx index d67c0ece..97eae362 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/ApplicantOverview.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/ApplicantOverview.tsx @@ -4,20 +4,10 @@ import Container from "@cloudscape-design/components/container"; import Header from "@cloudscape-design/components/header"; import ApplicantStatus from "@/app/admin/applicants/components/ApplicantStatus"; -import { - Applicant, - HackerReview, - Review, -} from "@/app/admin/applicants/hackers/useApplicant"; +import { Applicant } from "@/app/admin/applicants/hackers/useApplicant"; import ApplicationReviews from "./ApplicationReviews"; -import HackerApplicationReviews from "./HackerApplicationReviews"; - -function isHackerReviewArray( - arr: Review[] | HackerReview[], -): arr is HackerReview[] { - return arr.every((tuple) => typeof tuple[2] === "number"); -} +import { ParticipantRole } from "@/lib/userRecord"; interface ApplicantOverviewProps { applicant: Applicant; @@ -42,11 +32,10 @@ function ApplicantOverview({ applicant }: ApplicantOverviewProps) {
Reviews - {isHackerReviewArray(reviews) ? ( - - ) : ( - - )} +
diff --git a/apps/site/src/app/admin/applicants/[uid]/components/ApplicationReviews.tsx b/apps/site/src/app/admin/applicants/[uid]/components/ApplicationReviews.tsx index 66373399..e9488971 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/ApplicationReviews.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/ApplicationReviews.tsx @@ -3,13 +3,15 @@ import { useContext } from "react"; import ApplicantStatus from "@/app/admin/applicants/components/ApplicantStatus"; import { Review } from "@/app/admin/applicants/hackers/useApplicant"; import UserContext from "@/lib/admin/UserContext"; -import { Uid } from "@/lib/userRecord"; +import { Status, Uid } from "@/lib/userRecord"; +import { scoresToDecisions } from "@/lib/decisionScores"; interface ApplicationReviewsProps { reviews: Review[]; + isHacker: boolean; } -function ApplicationReviews({ reviews }: ApplicationReviewsProps) { +function ApplicationReviews({ reviews, isHacker }: ApplicationReviewsProps) { const { uid } = useContext(UserContext); if (reviews.length === 0) { @@ -22,13 +24,20 @@ function ApplicationReviews({ reviews }: ApplicationReviewsProps) { return (
    - {reviews.map(([date, reviewer, decision]) => + {reviews.map(([date, reviewer, score]) => reviewer === uid ? (
  • - <> - You reviewed as on{" "} - {formatDate(date)} - + {isHacker ? ( + <> + You scored this applicant a {score} on {formatDate(date)} + + ) : ( + <> + You reviewed as{" "} + {" "} + on {formatDate(date)} + + )}
    • ) : (
  • diff --git a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx index b5f5fb5c..a7ac7c74 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx @@ -1,19 +1,19 @@ import { useContext, useState } from "react"; import { Button, Input, SpaceBetween } from "@cloudscape-design/components"; -import { submitHackerReview } from "@/app/admin/applicants/hackers/useApplicant"; +import { submitReview } from "@/app/admin/applicants/hackers/useApplicant"; import { Uid } from "@/lib/userRecord"; import UserContext from "@/lib/admin/UserContext"; import { isReviewer } from "@/lib/admin/authorization"; interface ApplicantActionsProps { applicant: Uid; - submitHackerReview: submitHackerReview; + submitReview: submitReview; } function HackerApplicantActions({ applicant, - submitHackerReview, + submitReview, }: ApplicantActionsProps) { const { roles } = useContext(UserContext); const [value, setValue] = useState(""); @@ -34,7 +34,7 @@ function HackerApplicantActions({ /> + ) : ( + + Two reviewers have already submitted reviews. Log in as one of them to + submit a review. + ); } From 17869b88543813b8f25006be990879d71305623f Mon Sep 17 00:00:00 2001 From: Ian Dai Date: Wed, 25 Dec 2024 15:04:29 +0800 Subject: [PATCH 08/10] -Added uids to review error message -Moved comment to different line --- .../components/HackerApplicantActions.tsx | 34 +++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx index e54ea377..9b70ce15 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx @@ -14,6 +14,18 @@ import { Uid } from "@/lib/userRecord"; import UserContext from "@/lib/admin/UserContext"; import { isReviewer } from "@/lib/admin/authorization"; +interface ColoredTextBoxProps { + text: string | undefined; +} + +const ColoredTextBox = ({ text }: ColoredTextBoxProps) => { + return ( + + {text} + + ); +}; + interface ApplicantActionsProps { applicant: Uid; reviews: Review[]; @@ -27,10 +39,14 @@ function HackerApplicantActions({ }: ApplicantActionsProps) { const { uid, roles } = useContext(UserContext); const [value, setValue] = useState(""); + + const uniqueReviewers = Array.from( + new Set(reviews.map((review) => review[1])), + ); + // const canReview = either there are less than 2 reviewers or uid is in current reviews - const uniqueReviewers = new Set(reviews.map((review) => review[1])); const canReview = uid - ? uniqueReviewers.size < 2 || uniqueReviewers.has(uid) + ? uniqueReviewers.length < 2 || uniqueReviewers.includes(uid) : false; if (!isReviewer(roles)) { @@ -59,10 +75,16 @@ function HackerApplicantActions({ ) : ( - - Two reviewers have already submitted reviews. Log in as one of them to - submit a review. - + + + and{" "} + already + submitted reviews. + + + Log in as one of them to submit a review. + + ); } From 98394f4bc7856ee82f8bf19c1f47301bc586d96f Mon Sep 17 00:00:00 2001 From: Ian Dai Date: Sat, 28 Dec 2024 14:14:37 +0800 Subject: [PATCH 09/10] - Added backend check for correct num of reviewers - Returned full uid in _include_reviewers - Changed query parameter name to update_query - Changed decisionScores.ts to use numbers - Used direct importing for HackerApplicantActions - Removed hasReviewed prop - Changed more than 2 reviewer error msg - Renamed ReviewRequestModel --- .../src/admin/applicant_review_processor.py | 7 +- apps/api/src/routers/admin.py | 50 ++++++---- apps/api/tests/test_admin.py | 91 +++++++++++++++++-- .../tests/test_applicant_review_processor.py | 2 +- .../components/HackerApplicantActions.tsx | 13 ++- .../components/ApplicantReviewerIndicator.tsx | 12 +-- .../admin/applicants/hackers/useApplicant.ts | 4 +- apps/site/src/lib/decisionScores.ts | 8 +- 8 files changed, 136 insertions(+), 51 deletions(-) diff --git a/apps/api/src/admin/applicant_review_processor.py b/apps/api/src/admin/applicant_review_processor.py index 32ce745c..fff5293a 100644 --- a/apps/api/src/admin/applicant_review_processor.py +++ b/apps/api/src/admin/applicant_review_processor.py @@ -19,10 +19,10 @@ def include_review_decision(applicant_record: dict[str, Any]) -> None: applicant_record["decision"] = reviews[-1][2] if reviews else None -def get_num_unique_reviewers(applicant_record: dict[str, Any]) -> int: +def get_unique_reviewers(applicant_record: dict[str, Any]) -> set[str]: reviews = applicant_record["application_data"]["reviews"] unique_reviewers = {t[1] for t in reviews} - return len(unique_reviewers) + return unique_reviewers def _get_last_score(reviewer: str, reviews: list[tuple[str, str, float]]) -> float: @@ -55,8 +55,7 @@ def _include_decision_based_on_threshold( def _include_reviewers(applicant_record: dict[str, Any]) -> None: - reviews = applicant_record["application_data"]["reviews"] - unique_reviewers = {t[1].split(".")[-1] for t in reviews} + unique_reviewers = get_unique_reviewers(applicant_record) applicant_record["reviewers"] = sorted(list(unique_reviewers)) diff --git a/apps/api/src/routers/admin.py b/apps/api/src/routers/admin.py index 4d7fe571..0f91b751 100644 --- a/apps/api/src/routers/admin.py +++ b/apps/api/src/routers/admin.py @@ -53,7 +53,7 @@ class HackerApplicantSummary(BaseRecord): application_data: ApplicationDataSummary -class ReviewRequestModel(BaseModel): +class ReviewRequest(BaseModel): applicant: str score: float @@ -152,19 +152,14 @@ async def applicant_summary() -> dict[ApplicantStatus, int]: @router.post("/review") async def submit_review( - applicant_review: ReviewRequestModel, + applicant_review: ReviewRequest, reviewer: User = Depends(require_role({Role.REVIEWER})), ) -> None: """Submit a review decision from the reviewer for the given hacker applicant.""" log.info("%s reviewed hacker %s", reviewer, applicant_review.applicant) review: Review = (utc_now(), reviewer.uid, applicant_review.score) - - await _try_update_applicant_with_query( - applicant_review, - query={"$push": {"application_data.reviews": review}}, - err_msg=f"{reviewer} could not submit review for {applicant_review.applicant}", - ) + app = applicant_review.applicant applicant_record = await mongodb_handler.retrieve_one( Collection.USERS, @@ -176,23 +171,44 @@ async def submit_review( raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) if Role.HACKER in applicant_record["roles"]: - num_reviewers = applicant_review_processor.get_num_unique_reviewers( + unique_reviewers = applicant_review_processor.get_unique_reviewers( applicant_record ) + update_query: dict[str, object] = {} + + # Only add a review if there are either less than 2 reviewers + # or reviewer is one of the reviewers + if len(unique_reviewers) < 2 or str(reviewer) in unique_reviewers: + update_query.update({"$push": {"application_data.reviews": review}}) + else: + log.error( + "%s tried submitting a review, but %s already has two reviewers", + reviewer, + app, + ) + raise HTTPException(status.HTTP_403_FORBIDDEN) + # Because reviewing a hacker requires 2 reviewers, only set the # applicant's status to REVIEWED if there are at least 2 reviewers - if num_reviewers >= 2: + if len(unique_reviewers) >= 2: + update_query.update({"$set": {"status": "REVIEWED"}}) + + if update_query: await _try_update_applicant_with_query( applicant_review, - query={"$set": {"status": "REVIEWED"}}, - err_msg=f"Could not update status for {applicant_review.applicant}", + update_query=update_query, + err_msg=f"{reviewer} could not submit review for {app}", ) + else: await _try_update_applicant_with_query( applicant_review, - query={"$set": {"status": "REVIEWED"}}, - err_msg=f"Could not update status for {applicant_review.applicant}", + update_query={ + "$push": {"application_data.reviews": review}, + "$set": {"status": "REVIEWED"}, + }, + err_msg=f"{reviewer} could not submit review for {app}", ) @@ -443,16 +459,16 @@ async def _retrieve_thresholds() -> Optional[dict[str, Any]]: async def _try_update_applicant_with_query( - applicant_review: ReviewRequestModel, + applicant_review: ReviewRequest, *, - query: Mapping[str, object], + update_query: Mapping[str, object], err_msg: str = "", ) -> None: try: await mongodb_handler.raw_update_one( Collection.USERS, {"_id": applicant_review.applicant}, - query, + update_query, ) except RuntimeError: log.error(err_msg) diff --git a/apps/api/tests/test_admin.py b/apps/api/tests/test_admin.py index 62c0f71a..99e99371 100644 --- a/apps/api/tests/test_admin.py +++ b/apps/api/tests/test_admin.py @@ -140,12 +140,19 @@ def test_can_submit_nonhacker_review( res = reviewer_client.post("/review", json=post_data) assert res.status_code == 200 - assert mock_mongodb_handler_raw_update_one.await_count == 2 + mock_mongodb_handler_raw_update_one.assert_awaited_once_with( + Collection.USERS, + {"_id": "edu.uci.sydnee"}, + { + "$push": {"application_data.reviews": (ANY, "edu.uci.alicia", 0)}, + "$set": {"status": "REVIEWED"}, + }, + ) @patch("services.mongodb_handler.raw_update_one", autospec=True) @patch("services.mongodb_handler.retrieve_one", autospec=True) -def test_submit_hacker_review_works( +def test_submit_hacker_review_with_one_reviewer_works( mock_mongodb_handler_retrieve_one: AsyncMock, mock_mongodb_handler_raw_update_one: AsyncMock, ) -> None: @@ -173,19 +180,83 @@ def test_submit_hacker_review_works( res = reviewer_client.post("/review", json=post_data) assert res.status_code == 200 - assert mock_mongodb_handler_raw_update_one.await_count == 1 - - # Simulate 2 reviewers - # Should make raw_update_one run at very end of /review function - returned_record["application_data"]["reviews"].append( - [datetime(2023, 1, 19), "edu.uci.alicia2", 200] + mock_mongodb_handler_raw_update_one.assert_awaited_once_with( + Collection.USERS, + {"_id": "edu.uci.sydnee"}, + { + "$push": {"application_data.reviews": (ANY, "edu.uci.alicia", 0)}, + }, ) + + +@patch("services.mongodb_handler.raw_update_one", autospec=True) +@patch("services.mongodb_handler.retrieve_one", autospec=True) +def test_submit_hacker_review_with_two_reviewers_works( + mock_mongodb_handler_retrieve_one: AsyncMock, + mock_mongodb_handler_raw_update_one: AsyncMock, +) -> None: + + returned_record: dict[str, Any] = { + "_id": "edu.uci.sydnee", + "roles": ["Applicant", "Hacker"], + "application_data": { + "reviews": [ + [datetime(2023, 1, 19), "edu.uci.alicia", 100], + [datetime(2023, 1, 19), "edu.uci.alicia2", 100], + ] + }, + } + + mock_mongodb_handler_retrieve_one.side_effect = [ + REVIEWER_IDENTITY, + returned_record, + ] + mock_mongodb_handler_raw_update_one.return_value = True + res = reviewer_client.post( "/review", json={"applicant": "edu.uci.sydnee", "score": 0} ) assert res.status_code == 200 - assert mock_mongodb_handler_raw_update_one.await_count == 3 + mock_mongodb_handler_raw_update_one.assert_awaited_once_with( + Collection.USERS, + {"_id": "edu.uci.sydnee"}, + { + "$push": {"application_data.reviews": (ANY, "edu.uci.alicia", 0)}, + "$set": {"status": "REVIEWED"}, + }, + ) + + +@patch("services.mongodb_handler.raw_update_one", autospec=True) +@patch("services.mongodb_handler.retrieve_one", autospec=True) +def test_submit_hacker_review_with_three_reviewers_fails( + mock_mongodb_handler_retrieve_one: AsyncMock, + mock_mongodb_handler_raw_update_one: AsyncMock, +) -> None: + + returned_record: dict[str, Any] = { + "_id": "edu.uci.sydnee", + "roles": ["Applicant", "Hacker"], + "application_data": { + "reviews": [ + [datetime(2023, 1, 19), "edu.uci.alicia3", 100], + [datetime(2023, 1, 19), "edu.uci.alicia2", 100], + ] + }, + } + + mock_mongodb_handler_retrieve_one.side_effect = [ + REVIEWER_IDENTITY, + returned_record, + ] + mock_mongodb_handler_raw_update_one.return_value = True + + res = reviewer_client.post( + "/review", json={"applicant": "edu.uci.sydnee", "score": 0} + ) + + assert res.status_code == 403 @patch("services.mongodb_handler.update", autospec=True) @@ -326,7 +397,7 @@ def test_hacker_applicants_returns_correct_applicants( "status": "REVIEWED", "decision": "ACCEPTED", "avg_score": 150, - "reviewers": ["alicia", "alicia2"], + "reviewers": ["edu.uci.alicia", "edu.uci.alicia2"], "application_data": { "school": "Hamburger University", "submission_time": "2023-01-12T09:00:00", diff --git a/apps/api/tests/test_applicant_review_processor.py b/apps/api/tests/test_applicant_review_processor.py index 156e137c..c8eea849 100644 --- a/apps/api/tests/test_applicant_review_processor.py +++ b/apps/api/tests/test_applicant_review_processor.py @@ -46,7 +46,7 @@ def test_can_include_reviewers_from_reviews() -> None: }, } - expected_reviewers = ["alicia", "alicia2"] + expected_reviewers = ["edu.uci.alicia", "edu.uci.alicia2"] applicant_review_processor._include_reviewers(record) assert record["reviewers"] == expected_reviewers diff --git a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx index 9b70ce15..2a1615a1 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx @@ -1,11 +1,10 @@ import { useContext, useState } from "react"; -import { - Box, - Button, - Input, - SpaceBetween, -} from "@cloudscape-design/components"; +import Box from "@cloudscape-design/components/box"; +import Button from "@cloudscape-design/components/button"; +import Input from "@cloudscape-design/components/input"; +import SpaceBetween from "@cloudscape-design/components/space-between"; + import { Review, submitReview, @@ -82,7 +81,7 @@ function HackerApplicantActions({ submitted reviews. - Log in as one of them to submit a review. + Contact Rosalind, Nicole, or Albert if you think this is an error. ); diff --git a/apps/site/src/app/admin/applicants/components/ApplicantReviewerIndicator.tsx b/apps/site/src/app/admin/applicants/components/ApplicantReviewerIndicator.tsx index e6a60745..f6d35130 100644 --- a/apps/site/src/app/admin/applicants/components/ApplicantReviewerIndicator.tsx +++ b/apps/site/src/app/admin/applicants/components/ApplicantReviewerIndicator.tsx @@ -1,24 +1,23 @@ +import { Uid } from "@/lib/userRecord"; import Box from "@cloudscape-design/components/box"; import SpaceBetween from "@cloudscape-design/components/space-between"; import StatusIndicator from "@cloudscape-design/components/status-indicator"; interface IndicatorContainerProps { displayNumber: number; - reviewer: string; - hasReviewed: boolean; + reviewer: string | undefined; } const IndicatorContainer = ({ displayNumber, reviewer, - hasReviewed, }: IndicatorContainerProps) => { return ( <> Reviewer {displayNumber}: {reviewer} - {hasReviewed ? ( + {reviewer ? ( Reviewed ) : ( Not Reviewed @@ -34,14 +33,15 @@ interface ApplicantReviewerIndicatorProps { function ApplicantReviewerIndicator({ reviewers, }: ApplicantReviewerIndicatorProps) { + const formatUid = (uid: Uid) => uid.split(".").at(-1); + return ( {[0, 1].map((n) => ( = n + 1} + reviewer={reviewers[n] ? formatUid(reviewers[n]) : ""} /> ))} diff --git a/apps/site/src/app/admin/applicants/hackers/useApplicant.ts b/apps/site/src/app/admin/applicants/hackers/useApplicant.ts index 3b6ef668..9acc3fd1 100644 --- a/apps/site/src/app/admin/applicants/hackers/useApplicant.ts +++ b/apps/site/src/app/admin/applicants/hackers/useApplicant.ts @@ -50,7 +50,7 @@ function useApplicant(uid: Uid) { [string, Uid] >(["/api/admin/applicant/", uid], fetcher); - async function submitReview(uid: Uid, score: string) { + async function submitReview(uid: Uid, score: string | number) { await axios.post("/api/admin/review", { applicant: uid, score: score }); // TODO: provide success status to display in alert mutate(); @@ -64,6 +64,6 @@ function useApplicant(uid: Uid) { }; } -export type submitReview = (uid: Uid, score: string) => Promise; +export type submitReview = (uid: Uid, score: string | number) => Promise; export default useApplicant; diff --git a/apps/site/src/lib/decisionScores.ts b/apps/site/src/lib/decisionScores.ts index c9993c74..a2b03f2e 100644 --- a/apps/site/src/lib/decisionScores.ts +++ b/apps/site/src/lib/decisionScores.ts @@ -1,11 +1,11 @@ import { Decision } from "./userRecord"; /** For use in nonhacker applicant reviews.*/ -const acceptScore = "100"; -const waitlistScore = "-1"; -const rejectScore = "0"; +const acceptScore = 100; +const waitlistScore = -2; +const rejectScore = 0; -export const decisionsToScores: Record = { +export const decisionsToScores: Record = { [Decision.accepted]: acceptScore, [Decision.waitlisted]: waitlistScore, [Decision.rejected]: rejectScore, From 9e55cfb1e9fc46fae1528ac9567e3825c19b8c36 Mon Sep 17 00:00:00 2001 From: Ian Dai Date: Sun, 29 Dec 2024 10:46:21 +0800 Subject: [PATCH 10/10] - Inverted error logic in submit_review - Changed submitReview to use number parameter - Fixed update applicant status check --- apps/api/src/routers/admin.py | 24 +++++++++---------- apps/api/tests/test_admin.py | 20 ++++++---------- .../components/HackerApplicantActions.tsx | 14 +++++------ .../admin/applicants/hackers/useApplicant.ts | 4 ++-- 4 files changed, 27 insertions(+), 35 deletions(-) diff --git a/apps/api/src/routers/admin.py b/apps/api/src/routers/admin.py index 0f91b751..0625e160 100644 --- a/apps/api/src/routers/admin.py +++ b/apps/api/src/routers/admin.py @@ -175,31 +175,29 @@ async def submit_review( applicant_record ) - update_query: dict[str, object] = {} - # Only add a review if there are either less than 2 reviewers # or reviewer is one of the reviewers - if len(unique_reviewers) < 2 or str(reviewer) in unique_reviewers: - update_query.update({"$push": {"application_data.reviews": review}}) - else: + if len(unique_reviewers) >= 2 and reviewer.uid not in unique_reviewers: log.error( - "%s tried submitting a review, but %s already has two reviewers", + "%s tried to submit a review, but %s already has two reviewers", reviewer, app, ) raise HTTPException(status.HTTP_403_FORBIDDEN) + update_query: dict[str, object] = { + "$push": {"application_data.reviews": review} + } # Because reviewing a hacker requires 2 reviewers, only set the # applicant's status to REVIEWED if there are at least 2 reviewers - if len(unique_reviewers) >= 2: + if len(unique_reviewers | {reviewer.uid}) >= 2: update_query.update({"$set": {"status": "REVIEWED"}}) - if update_query: - await _try_update_applicant_with_query( - applicant_review, - update_query=update_query, - err_msg=f"{reviewer} could not submit review for {app}", - ) + await _try_update_applicant_with_query( + applicant_review, + update_query=update_query, + err_msg=f"{reviewer} could not submit review for {app}", + ) else: await _try_update_applicant_with_query( diff --git a/apps/api/tests/test_admin.py b/apps/api/tests/test_admin.py index 99e99371..b6c219fe 100644 --- a/apps/api/tests/test_admin.py +++ b/apps/api/tests/test_admin.py @@ -132,10 +132,8 @@ def test_can_submit_nonhacker_review( mock_mongodb_handler_retrieve_one.side_effect = [ REVIEWER_IDENTITY, returned_record, - REVIEWER_IDENTITY, - returned_record, ] - mock_mongodb_handler_raw_update_one.side_effect = [True, True, True] + mock_mongodb_handler_raw_update_one.return_value = True res = reviewer_client.post("/review", json=post_data) @@ -164,18 +162,13 @@ def test_submit_hacker_review_with_one_reviewer_works( "roles": ["Applicant", "Hacker"], "application_data": { "reviews": [ - [datetime(2023, 1, 19), "edu.uci.alicia", 100], + [datetime(2023, 1, 19), "edu.uci.alicia2", 100], ] }, } - mock_mongodb_handler_retrieve_one.side_effect = [ - REVIEWER_IDENTITY, - returned_record, - REVIEWER_IDENTITY, - returned_record, - ] - mock_mongodb_handler_raw_update_one.side_effect = [True, True, True] + mock_mongodb_handler_retrieve_one.side_effect = [REVIEWER_IDENTITY, returned_record] + mock_mongodb_handler_raw_update_one.return_value = True res = reviewer_client.post("/review", json=post_data) @@ -185,6 +178,7 @@ def test_submit_hacker_review_with_one_reviewer_works( {"_id": "edu.uci.sydnee"}, { "$push": {"application_data.reviews": (ANY, "edu.uci.alicia", 0)}, + "$set": {"status": "REVIEWED"}, }, ) @@ -195,7 +189,7 @@ def test_submit_hacker_review_with_two_reviewers_works( mock_mongodb_handler_retrieve_one: AsyncMock, mock_mongodb_handler_raw_update_one: AsyncMock, ) -> None: - + """Test that a user can submit a hacker applicant review with 2 reviewers.""" returned_record: dict[str, Any] = { "_id": "edu.uci.sydnee", "roles": ["Applicant", "Hacker"], @@ -234,7 +228,7 @@ def test_submit_hacker_review_with_three_reviewers_fails( mock_mongodb_handler_retrieve_one: AsyncMock, mock_mongodb_handler_raw_update_one: AsyncMock, ) -> None: - + """Test that a hacker applicant review with 3 reviewers fails.""" returned_record: dict[str, Any] = { "_id": "edu.uci.sydnee", "roles": ["Applicant", "Hacker"], diff --git a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx index 2a1615a1..8c3accec 100644 --- a/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx +++ b/apps/site/src/app/admin/applicants/[uid]/components/HackerApplicantActions.tsx @@ -52,6 +52,12 @@ function HackerApplicantActions({ return null; } + const handleClick = () => { + // TODO: use flashbar or modal for submit status + submitReview(applicant, parseFloat(value)); + setValue(""); + }; + return canReview ? ( - diff --git a/apps/site/src/app/admin/applicants/hackers/useApplicant.ts b/apps/site/src/app/admin/applicants/hackers/useApplicant.ts index 9acc3fd1..4ee1646b 100644 --- a/apps/site/src/app/admin/applicants/hackers/useApplicant.ts +++ b/apps/site/src/app/admin/applicants/hackers/useApplicant.ts @@ -50,7 +50,7 @@ function useApplicant(uid: Uid) { [string, Uid] >(["/api/admin/applicant/", uid], fetcher); - async function submitReview(uid: Uid, score: string | number) { + async function submitReview(uid: Uid, score: number) { await axios.post("/api/admin/review", { applicant: uid, score: score }); // TODO: provide success status to display in alert mutate(); @@ -64,6 +64,6 @@ function useApplicant(uid: Uid) { }; } -export type submitReview = (uid: Uid, score: string | number) => Promise; +export type submitReview = (uid: Uid, score: number) => Promise; export default useApplicant;