diff --git a/api_schemas/event_signup_schemas.py b/api_schemas/event_signup_schemas.py index 640794c3..0db24523 100644 --- a/api_schemas/event_signup_schemas.py +++ b/api_schemas/event_signup_schemas.py @@ -21,6 +21,8 @@ class EventSignupRead(BaseSchema): class EventSignupUpdate(BaseSchema): + """This schema uses partial updates, any field not sent to backend will be ignored and not updated.""" + user_id: int | None = None priority: str | None = None group_name: str | None = None diff --git a/helpers/constants.py b/helpers/constants.py index f95b334e..278435e8 100644 --- a/helpers/constants.py +++ b/helpers/constants.py @@ -96,6 +96,8 @@ # Event User DEFAULT_USER_PRIORITY = "Övrigt" +# Priorities tied to a nollning group instead of a post +NOLLNING_PRIORITIES = ["Nolla", "Gruppfadder", "Uppdragsfadder"] # Guild Meeting MAX_GUILD_MEETING_DATE_DESC = 500 diff --git a/services/event_signup_service.py b/services/event_signup_service.py index f0d2363c..fe6dcdd4 100644 --- a/services/event_signup_service.py +++ b/services/event_signup_service.py @@ -8,10 +8,21 @@ from db_models.group_model import Group_DB from db_models.group_user_model import GroupUser_DB from api_schemas.event_signup_schemas import EventSignupCreate, EventSignupUpdate -from helpers.constants import DEFAULT_USER_PRIORITY +from helpers.constants import DEFAULT_USER_PRIORITY, NOLLNING_PRIORITIES from helpers.types import GROUP_TYPE +def user_matches_existing_event_post_priorities(user: User_DB, event: Event_DB): + """Only true if a user has a post which matches one of the event's priorities, + and that priority match is not a nollning priority (which are tied to groups instead of posts).""" + if not event.priorities: + return False + + event_post_priorities = {p.priority for p in event.priorities if p.priority not in NOLLNING_PRIORITIES} + + return any(post.name_sv in event_post_priorities for post in user.posts) + + def signup_to_event(event: Event_DB, user: User_DB, data: EventSignupCreate, manage_permission: bool, db: Session): now = datetime.now(UTC) @@ -83,18 +94,30 @@ def update_event_signup(event: Event_DB, data: EventSignupUpdate, user_id: int, if signup is None: raise HTTPException(status.HTTP_404_NOT_FOUND) - if manage_permission == False and not is_group_allowed( - event, db.query(User_DB).filter(User_DB.id == user_id).one(), data.group_name + # Only fields the client actually sent are considered. + updates = data.model_dump(exclude_unset=True, exclude={"user_id"}) + + if "group_name" in updates and not updates["group_name"]: # if falsy + updates["group_name"] = None + + # Only authorize the group when the client asked to change it, otherwise an update that leaves + # the group alone would be rejected on nollning events for not carrying a group at all. + if ( + manage_permission == False + and "group_name" in updates + and not is_group_allowed(event, db.query(User_DB).filter(User_DB.id == user_id).one(), updates["group_name"]) ): raise HTTPException(status.HTTP_403_FORBIDDEN, detail="User cannot sign up with this group") - for var, value in vars(data).items(): - if var == "priority" and not value: - setattr(signup, "priority", DEFAULT_USER_PRIORITY) - elif var == "group_name" and not value: - setattr(signup, "group_name", None) - else: - setattr(signup, var, value) if value else None + # priority and drinkPackage are not nullable in the database, so a null means "back to default" + if "priority" in updates and not updates["priority"]: # if falsy + updates["priority"] = DEFAULT_USER_PRIORITY + + if "drinkPackage" in updates and updates["drinkPackage"] is None: + del updates["drinkPackage"] # None is not a valid value ("None" is), so leave the old value in place + + for var, value in updates.items(): + setattr(signup, var, value) if not event.drink_package: signup.drinkPackage = "None" @@ -132,7 +155,9 @@ def get_allowed_groups(event: Event_DB, user: User_DB): def is_group_allowed(event: Event_DB, user: User_DB, group_name: str | None): if event.is_nollning_event: if group_name is None: - return False + # Without a group the user has to qualify through a post priority instead, + # since nollning priorities are tied to a group + return user_matches_existing_event_post_priorities(user, event) allowed_group_types = event.mentor_group_types or list(get_args(GROUP_TYPE)) is_event_allowed = False diff --git a/tests/test_event_signup.py b/tests/test_event_signup.py index 97be0916..d008e56e 100644 --- a/tests/test_event_signup.py +++ b/tests/test_event_signup.py @@ -1,5 +1,6 @@ # type: ignore import pytest +from helpers.constants import DEFAULT_USER_PRIORITY from .basic_factories import add_user_to_group, auth_headers, event_data_factory @@ -37,14 +38,47 @@ def test_signup_with_group_the_user_is_not_in(client, member_token, membered_use assert response.status_code == 403 -def test_signup_without_group(client, member_token, membered_user, nollning_event, mission_group): - """Signing up without picking a group is not restricted by the group types.""" +def test_nollning_event_signup_without_group(client, member_token, membered_user, nollning_event, mission_group): + """Signing up to nollning event without picking a group is not allowed.""" response = client.post( f"/event-signup/{nollning_event['id']}", json={"user_id": membered_user.id}, headers=auth_headers(member_token), ) + assert response.status_code == 403, response.text + + +def test_nollning_event_signup_without_group_with_post_priority( + client, admin_token, admin_council_id, member_token, membered_user, member_post +): + """A user whose post matches one of the event's priorities may sign up without a group.""" + data = event_data_factory( + council_id=admin_council_id, + is_nollning_event=True, + mentor_group_types=["Mentor"], + priorities=[member_post.name_sv, "Nolla"], + ) + event = client.post("/events/", json=data, headers=auth_headers(admin_token)).json() + + response = client.post( + f"/event-signup/{event['id']}", + json={"user_id": membered_user.id}, + headers=auth_headers(member_token), + ) + + assert response.status_code in (200, 201), response.text + assert response.json()["group_name"] is None + + +def test_non_nollning_event_signup_without_group(client, member_token, membered_user, event, mission_group): + """Signing up to non-nollning event without picking a group is not restricted by the group types.""" + response = client.post( + f"/event-signup/{event['id']}", + json={"user_id": membered_user.id}, + headers=auth_headers(member_token), + ) + assert response.status_code in (200, 201), response.text assert response.json()["group_name"] is None @@ -106,25 +140,119 @@ def test_non_nollning_event_ignores_group_types(client, member_token, membered_u assert response.status_code in (200, 201), response.text -def test_update_signup_without_group_name_is_allowed(client, member_token, membered_user, nollning_event, mentor_group): - """ - Regression: patching some other field of a signup on a nollning event used to be - rejected with 403, because an omitted group_name was checked as if it were a group - the user isn't in. - """ +def test_update_signup_without_changing_group_name_is_allowed( + client, member_token, membered_user, nollning_event, mentor_group +): + """Not sending a group name with the update body should make it stay as-is""" + signup = client.post( + f"/event-signup/{nollning_event['id']}", + json={"user_id": membered_user.id, "group_name": mentor_group.name}, + headers=auth_headers(member_token), + ) + assert signup.status_code in (200, 201), signup.text + assert signup.json()["group_name"] == mentor_group.name + + response = client.patch( + f"/event-signup/{nollning_event['id']}", + json={"priority": "Nolla"}, + headers=auth_headers(member_token), + ) + + assert response.status_code == 200, response.text + assert response.json()["priority"] == "Nolla" + assert response.json()["group_name"] == mentor_group.name + + +@pytest.mark.parametrize("sent_group_name", [None, ""]) +def test_nollning_event_update_signup_remove_group_name_is_disallowed( + client, member_token, membered_user, nollning_event, mentor_group, sent_group_name +): + """Sending a null (or empty) group name to remove the group is disallowed for nollning events""" signup = client.post( f"/event-signup/{nollning_event['id']}", json={"user_id": membered_user.id, "group_name": mentor_group.name}, headers=auth_headers(member_token), ) assert signup.status_code in (200, 201), signup.text + assert signup.json()["group_name"] == mentor_group.name + + response = client.patch( + f"/event-signup/{nollning_event['id']}", + json={"group_name": sent_group_name}, + headers=auth_headers(member_token), + ) + + assert response.status_code == 403, response.text + + +@pytest.mark.parametrize("sent_group_name", [None, ""]) +def test_non_nollning_event_update_signup_remove_group_name_is_allowed( + client, member_token, membered_user, event, mentor_group, sent_group_name +): + """Sending a null (or empty) group name to remove the group is allowed for non-nollning events""" + signup = client.post( + f"/event-signup/{event['id']}", + json={"user_id": membered_user.id, "group_name": mentor_group.name}, + headers=auth_headers(member_token), + ) + assert signup.status_code in (200, 201), signup.text + assert signup.json()["group_name"] == mentor_group.name + + response = client.patch( + f"/event-signup/{event['id']}", + json={"priority": "Nolla", "group_name": sent_group_name}, + headers=auth_headers(member_token), + ) + + assert response.status_code == 200, response.text + assert response.json()["priority"] == "Nolla" + assert response.json()["group_name"] is None + + +def test_update_signup_without_priority_keeps_priority( + client, member_token, membered_user, nollning_event, mentor_group +): + """Leaving priority out of the update body should make it stay as-is""" + signup = client.post( + f"/event-signup/{nollning_event['id']}", + json={"user_id": membered_user.id, "group_name": mentor_group.name, "priority": "Nolla"}, + headers=auth_headers(member_token), + ) + assert signup.status_code in (200, 201), signup.text + assert signup.json()["priority"] == "Nolla" response = client.patch( - f"/event-signup/{nollning_event['id']}", json={"priority": "Nolla"}, headers=auth_headers(member_token) + f"/event-signup/{nollning_event['id']}", + json={"drinkPackage": "Alcohol"}, + headers=auth_headers(member_token), ) assert response.status_code == 200, response.text assert response.json()["priority"] == "Nolla" + assert response.json()["group_name"] == mentor_group.name + + +def test_update_signup_with_null_priority_resets_to_default( + client, member_token, membered_user, nollning_event, mentor_group +): + """Sending a null priority should reset it to the default one""" + signup = client.post( + f"/event-signup/{nollning_event['id']}", + json={"user_id": membered_user.id, "group_name": mentor_group.name, "priority": "Nolla"}, + headers=auth_headers(member_token), + ) + assert signup.status_code in (200, 201), signup.text + assert signup.json()["priority"] == "Nolla" + + response = client.patch( + f"/event-signup/{nollning_event['id']}", + json={"priority": None}, + headers=auth_headers(member_token), + ) + + assert response.status_code == 200, response.text + assert response.json()["priority"] == DEFAULT_USER_PRIORITY + assert response.json()["group_name"] == mentor_group.name def test_update_signup_to_disallowed_group(