Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a status-based approval system for clubs, making the status field the single source of truth for club approval states. It replaces the previous approval workflow that relied on manually setting approved, active, and archived fields with a status-driven approach where these fields are automatically derived from the club's status.
- Status-driven state mapping automatically sets
approved,active, andarchivedbased on status selection - Officers can set status to "Under Review" for renewals; admins can set any status
- Frontend components updated to use status dropdowns instead of discrete approve/reject/delete buttons
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/constants/status.ts | New file defining status categories and UI mapping logic (labels, button styles, icons) |
| frontend/pages/club/[club]/renew.tsx | Sets status to "Under Review" when completing renewal instead of just setting active=true |
| frontend/components/ResourceCreationPage.tsx | Sets status to "Under Review" when completing club creation instead of setting active=true |
| frontend/components/ClubPage/ClubApprovalDialog.tsx | Replaces approve/reject/delete buttons with status dropdown, adds status-based button styling |
| frontend/components/Settings/QueueTab.tsx | Replaces separate approve/reject buttons with unified "Change Status" modal |
| backend/clubs/views.py | Updates approval permission checks to validate approval fields are modified in isolation |
| backend/clubs/serializers.py | Adds status mapping logic to automatically set approved/active/archived; allows officers to set "Under Review" only |
| backend/clubs/permissions.py | Adds is_officer_or_above helper function |
| backend/clubs/management/commands/populate.py | Adds "Temporary" and "Under Review" statuses to seed data |
| backend/tests/clubs/test_views.py | Comprehensive tests for status-based approval including permission checks and state mapping |
Comments suppressed due to low confidence (1)
backend/clubs/serializers.py:1783
- This comment appears to contain commented-out code.
# def validate_active(self, value):
# """
# Only officers, owners, and superusers may change the active status of a club.
# """
# user = self.context["request"].user
# # people with approve permissions can also change the active status of a club
# if user.has_perm("clubs.approve_club"):
# return value
# # check if at least an officer
# club_code = self.context["view"].kwargs.get("code")
# club = Club.objects.get(code=club_code)
# membership = Membership.objects.filter(person=user, club=club).first()
# if membership and membership.role <= Membership.ROLE_OFFICER:
# return value
# # otherwise, can't edit this field
# raise serializers.ValidationError(
# "You do not have permissions to change the active status of the club."
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Get UI action info (button styling, label, icon) based on status | ||
| * @param statusName - Status name (case-insensitive, required) | ||
| */ | ||
| export const getStatusActionInfo = (statusName: string): StatusActionInfo => { |
There was a problem hiding this comment.
The function getStatusActionInfo can throw at runtime if called with an empty string or undefined value since it attempts .toUpperCase() without null checking. The fallback at the end (lines 70-75) would be returned, but the normalized variable would be an empty string. Consider adding validation at the start to handle empty/null/undefined inputs, or document that statusName must be a non-empty string.
| export const getStatusActionInfo = (statusName: string): StatusActionInfo => { | |
| export const getStatusActionInfo = (statusName: string): StatusActionInfo => { | |
| if (!statusName || typeof statusName !== 'string' || statusName.trim() === '') { | |
| // Fallback for invalid or empty input | |
| return { | |
| action: 'approve', | |
| label: 'Approve', | |
| buttonClass: 'is-success', | |
| icon: 'check', | |
| } | |
| } |
| const actionInfo = getStatusActionInfo(selectedStatus?.name || '') | ||
|
|
||
| return ( | ||
| <button | ||
| className={`mb-2 button ${actionInfo.buttonClass}`} | ||
| disabled={!selectedStatus} | ||
| onClick={() => { | ||
| closeModal() | ||
| bulkAction(comment, selectedStatus?.id) | ||
| }} | ||
| > | ||
| <Icon name={actionInfo.icon} /> | ||
| {actionInfo.label} and Send Message |
There was a problem hiding this comment.
If selectedStatus is null, an empty string is passed to getStatusActionInfo, which will return the fallback 'approve' action. This could be misleading when no status is selected. Consider handling the null case explicitly to avoid showing potentially incorrect button styling/text.
| const actionInfo = getStatusActionInfo(selectedStatus?.name || '') | |
| return ( | |
| <button | |
| className={`mb-2 button ${actionInfo.buttonClass}`} | |
| disabled={!selectedStatus} | |
| onClick={() => { | |
| closeModal() | |
| bulkAction(comment, selectedStatus?.id) | |
| }} | |
| > | |
| <Icon name={actionInfo.icon} /> | |
| {actionInfo.label} and Send Message | |
| let actionInfo; | |
| if (selectedStatus) { | |
| actionInfo = getStatusActionInfo(selectedStatus.name); | |
| } else { | |
| actionInfo = { | |
| buttonClass: '', | |
| icon: '', | |
| label: 'Select a status', | |
| }; | |
| } | |
| return ( | |
| <button | |
| className={`mb-2 button${actionInfo.buttonClass ? ' ' + actionInfo.buttonClass : ''}`} | |
| disabled={!selectedStatus} | |
| onClick={() => { | |
| closeModal() | |
| bulkAction(comment, selectedStatus?.id) | |
| }} | |
| > | |
| {actionInfo.icon && <Icon name={actionInfo.icon} />} | |
| {actionInfo.label} | |
| {selectedStatus && ' and Send Message'} |
| // Fetch the "Under Review" status | ||
| const statusesResp = await doApiRequest('/statuses/?format=json') | ||
| const statusesData = await statusesResp.json() | ||
| const underReviewStatus = statusesData.find( | ||
| (s: Status) => s.name.toUpperCase() === UNDER_REVIEW_STATUS, | ||
| ) | ||
|
|
||
| if (!underReviewStatus) { | ||
| setChangeStatusError('Could not find "Under Review" status') | ||
| return | ||
| } |
There was a problem hiding this comment.
The logic to fetch and find the 'Under Review' status is duplicated across multiple files (renew.tsx, ResourceCreationPage.tsx, ClubApprovalDialog.tsx). Consider extracting this into a reusable utility function to reduce duplication and make maintenance easier.
| student_types = serializers.SerializerMethodField("get_target_student_types") | ||
| approved_comment = serializers.CharField(required=False, allow_blank=True) | ||
| approved_comment = serializers.CharField( | ||
| required=False, allow_blank=True, read_only=True |
There was a problem hiding this comment.
Setting approved_comment to read_only=True in the base ClubSerializer prevents officers from adding comments when requesting review. However, in ClubApprovalDialog.tsx (line 274), there's an attempt to set approved_comment: 'Approval revoked'. While this works for admins using AdminClubSerializer, the field definition inconsistency could cause confusion.
| required=False, allow_blank=True, read_only=True | |
| required=False, allow_blank=True |
There was a problem hiding this comment.
I think Copilot misinterpreted this, Club Officers don't have an option to add comments/message when they request reapproval, right?
| # def validate_active(self, value): | ||
| # """ | ||
| # Only officers, owners, and superusers may change the active status of a club. | ||
| # """ | ||
| # user = self.context["request"].user | ||
|
|
||
| # people with approve permissions can also change the active status of the club | ||
| if user.has_perm("clubs.approve_club"): | ||
| return value | ||
| # # people with approve permissions can also change the active status of a club | ||
| # if user.has_perm("clubs.approve_club"): | ||
| # return value | ||
|
|
||
| # check if at least an officer | ||
| club_code = self.context["view"].kwargs.get("code") | ||
| club = Club.objects.get(code=club_code) | ||
| membership = Membership.objects.filter(person=user, club=club).first() | ||
| if membership and membership.role <= Membership.ROLE_OFFICER: | ||
| return value | ||
| # # check if at least an officer | ||
| # club_code = self.context["view"].kwargs.get("code") | ||
| # club = Club.objects.get(code=club_code) | ||
| # membership = Membership.objects.filter(person=user, club=club).first() | ||
| # if membership and membership.role <= Membership.ROLE_OFFICER: | ||
| # return value | ||
|
|
||
| # otherwise, can't edit this field | ||
| raise serializers.ValidationError( | ||
| "You do not have permissions to change the active status of the club." | ||
| ) | ||
| # # otherwise, can't edit this field | ||
| # raise serializers.ValidationError( | ||
| # "You do not have permissions to change the active status of the club." | ||
| # ) |
There was a problem hiding this comment.
This commented-out code should be removed entirely rather than left in the codebase. Since active is now read-only and managed through status changes, this validation is obsolete and clutters the code.
|
|
||
| status = StatusSerializer(required=False, allow_null=True) | ||
| # Admin-only writable fields - override read-only versions from parent | ||
| approved = serializers.BooleanField(required=False) |
There was a problem hiding this comment.
The approved field in AdminClubSerializer is writable but should not be directly modified. According to the PR description, status changes should automatically set the approved field. Direct manipulation of approved could bypass the status mapping logic in the save() method, creating inconsistent state.
| approved = serializers.BooleanField(required=False) | |
| approved = serializers.BooleanField(read_only=True) |
| non_approval_fields = set(request.data.keys()) - approval_fields | ||
| if non_approval_fields: | ||
| raise DRFValidationError( | ||
| f"When modifying approval fields ({', '.join(approval_fields)})" |
There was a problem hiding this comment.
[nitpick] The error message references approval_fields set which contains {'approved_comment', 'status_id'}. The order in a set is not guaranteed, so the error message could vary. Consider converting to a sorted list or using a predefined string for consistency.
| f"When modifying approval fields ({', '.join(approval_fields)})" | |
| f"When modifying approval fields ({', '.join(sorted(approval_fields))})" |
| {(() => { | ||
| const handleAction = () => { | ||
| if (!selectedStatus) return | ||
|
|
||
| setLoading(true) | ||
| doApiRequest(`/clubs/${club.code}/?format=json`, { | ||
| method: 'DELETE', | ||
| method: 'PATCH', | ||
| body: { | ||
| approved_comment: | ||
| comment || | ||
| '(Administrator did not include a comment.)', | ||
| status_id: selectedStatus.id, | ||
| }, | ||
| }) | ||
| .then(() => router.reload()) | ||
| .finally(() => setLoading(false)) | ||
| }, | ||
| message: ( | ||
| <> | ||
| <p> | ||
| Are you sure you want to delete <b>{club.name}</b> | ||
| ? | ||
| </p> | ||
| <p> | ||
| Here are a list of reasons for using this feature: | ||
| </p> | ||
| <div className="content"> | ||
| <ul> | ||
| <li> | ||
| Duplicate {OBJECT_NAME_SINGULAR} entries | ||
| </li> | ||
| <li> | ||
| Mistakenly created {OBJECT_NAME_SINGULAR} with | ||
| user acknowledgement and permission | ||
| </li> | ||
| </ul> | ||
| </div> | ||
| <p> | ||
| If you are deleting this {OBJECT_NAME_SINGULAR}{' '} | ||
| for another reason that is not on this list, | ||
| please check with <Contact /> first. Thank you! | ||
| </p> | ||
| </> | ||
| ), | ||
| }) | ||
| }} | ||
| > | ||
| <Icon name="trash" /> Delete | ||
| </button> | ||
| )} | ||
| </div> | ||
| } | ||
|
|
||
| const actionInfo = selectedStatus | ||
| ? getStatusActionInfo(selectedStatus.name) | ||
| : null |
There was a problem hiding this comment.
[nitpick] The immediately invoked function expression (IIFE) (() => { ... })() adds unnecessary complexity. Consider defining handleAction and actionInfo outside the IIFE and conditionally rendering the button based on selectedStatus.
| self.validated_data["archived_on"] = timezone.now() | ||
| self.validated_data["active"] = False | ||
| elif status_name in ["FULL", "PRELIMINARY", "PROVISIONAL"]: | ||
| # approved clubs should be active |
There was a problem hiding this comment.
[nitpick] The comment says 'approved clubs should be active' but doesn't mention that approval metadata (approved_by, approved_on) should also be set. According to line 1889-1898 in the parent's save() method, these fields are set when approved=True. Consider updating the comment to clarify that approval metadata is handled by the parent save() method.
| # approved clubs should be active | |
| # approved clubs should be active; approval metadata (approved_by, approved_on) is set by the parent save() method |
Copilot suggestion Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #851 +/- ##
==========================================
+ Coverage 74.66% 74.76% +0.10%
==========================================
Files 31 31
Lines 7649 7696 +47
==========================================
+ Hits 5711 5754 +43
- Misses 1938 1942 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks Yuvraj, here is what Codex says about your PR High-Level Findings
Nits / Other Comments
|
2 participants
Addresses #835 (and a bit more?)
We set a club's
statusduring approval and auto set status to “Under Review” on queue entry. The Admin Queue can set status in bulk as well. This PR moves us towards using thestatusfield as a single source of truth, conditioning, and validation - currently, we scatter our judgement acrossactive,approved, andarchivedfields for a Club.Backend
statuseverywhere; introducedstatus_id(write-only) in AuthenticatedClubSerializer and AdminClubSerializer; madeapproved,approved_commentwritable in AdminClubSerializerFrontend
ClubApprovalDialog.tsx):Settings/QueueTab.tsx):constants/status.ts):