From e507f13ac52b5a37bf81828fe540bb413b2c0424 Mon Sep 17 00:00:00 2001 From: Julian Weng Date: Mon, 4 Nov 2024 18:34:39 -0500 Subject: [PATCH 1/6] Add route for club approval history (in-progress) --- backend/clubs/views.py | 12 ++++++++++++ frontend/components/ClubPage/ClubApprovalDialog.tsx | 12 ++++++++++++ 2 files changed, 24 insertions(+) diff --git a/backend/clubs/views.py b/backend/clubs/views.py index d8a127c62..fb4c2bb10 100644 --- a/backend/clubs/views.py +++ b/backend/clubs/views.py @@ -1276,6 +1276,18 @@ def upload_file(self, request, *args, **kwargs): return file_upload_endpoint_helper(request, code=club.code) + @action(detail=True, methods=["get"]) + def history(self, request, *args, **kwargs): + """ + Return a simplified approval history for the club. + """ + club = self.get_object() + return Response( + club.history.order_by("approved_on").values( + "approved", "approved_on", "approved_by", "history_date" + ) + ) + @action(detail=True, methods=["get"]) def owned_badges(self, request, *args, **kwargs): """ diff --git a/frontend/components/ClubPage/ClubApprovalDialog.tsx b/frontend/components/ClubPage/ClubApprovalDialog.tsx index faa479759..facc2b159 100644 --- a/frontend/components/ClubPage/ClubApprovalDialog.tsx +++ b/frontend/components/ClubPage/ClubApprovalDialog.tsx @@ -30,9 +30,17 @@ type ConfirmParams = { message: ReactElement | string } +type HistoricItem = { + approved: boolean | null + approved_on: string | null + approved_by: string | null + history_date: string +} + const ClubApprovalDialog = ({ club }: Props): ReactElement | null => { const router = useRouter() const year = getCurrentSchoolYear() + const [history, setHistory] = useState([]) const [comment, setComment] = useState(club.approved_comment || '') const [loading, setLoading] = useState(false) const [confirmModal, setConfirmModal] = useState(null) @@ -62,6 +70,10 @@ const ClubApprovalDialog = ({ club }: Props): ReactElement | null => { doApiRequest('/templates/?format=json') .then((resp) => resp.json()) .then(setTemplates) + + doApiRequest(`/clubs/${club.code}/history/?format=json`) + .then((resp) => resp.json()) + .then(setHistory) } setComment( From 835355d3b386dbf1bedfe4a41e515f78117ca91f Mon Sep 17 00:00:00 2001 From: Julian Weng Date: Mon, 11 Nov 2024 02:00:25 -0500 Subject: [PATCH 2/6] Finish UI for club approval history + testcase --- backend/clubs/serializers.py | 28 +++++ backend/clubs/views.py | 11 +- backend/tests/clubs/test_views.py | 13 ++ .../ClubPage/ClubApprovalDialog.tsx | 112 +++++++++++++++++- frontend/components/DropdownFilter.tsx | 8 +- 5 files changed, 163 insertions(+), 9 deletions(-) diff --git a/backend/clubs/serializers.py b/backend/clubs/serializers.py index eb70bbb2c..0a21f11d6 100644 --- a/backend/clubs/serializers.py +++ b/backend/clubs/serializers.py @@ -2975,6 +2975,34 @@ class Meta: ) +class ApprovalHistorySerializer(serializers.ModelSerializer): + approved = serializers.BooleanField() + approved_on = serializers.DateTimeField() + approved_by = serializers.SerializerMethodField("get_approved_by") + approved_comment = serializers.CharField() + history_date = serializers.DateTimeField() + + def get_approved_by(self, obj): + user = self.context["request"].user + if not user.is_authenticated: + return None + if not user.has_perm("clubs.see_pending_clubs"): + return None + if obj.approved_by is None: + return "Unknown" + return obj.approved_by.get_full_name() + + class Meta: + model = Club + fields = ( + "approved", + "approved_on", + "approved_by", + "approved_comment", + "history_date", + ) + + class AdminNoteSerializer(ClubRouteMixin, serializers.ModelSerializer): creator = serializers.SerializerMethodField("get_creator") title = serializers.CharField(max_length=255, default="Note") diff --git a/backend/clubs/views.py b/backend/clubs/views.py index fb4c2bb10..461b36466 100644 --- a/backend/clubs/views.py +++ b/backend/clubs/views.py @@ -154,6 +154,7 @@ ApplicationSubmissionCSVSerializer, ApplicationSubmissionSerializer, ApplicationSubmissionUserSerializer, + ApprovalHistorySerializer, AssetSerializer, AuthenticatedClubSerializer, AuthenticatedMembershipSerializer, @@ -1283,9 +1284,11 @@ def history(self, request, *args, **kwargs): """ club = self.get_object() return Response( - club.history.order_by("approved_on").values( - "approved", "approved_on", "approved_by", "history_date" - ) + ApprovalHistorySerializer( + club.history.order_by("history_date"), + many=True, + context={"request": request}, + ).data ) @action(detail=True, methods=["get"]) @@ -2171,6 +2174,8 @@ def get_serializer_class(self): return ClubConstitutionSerializer if self.action == "notes_about": return NoteSerializer + if self.action == "history": + return ApprovalHistorySerializer if self.action in {"list", "fields"}: if self.request is not None and ( self.request.accepted_renderer.format == "xlsx" diff --git a/backend/tests/clubs/test_views.py b/backend/tests/clubs/test_views.py index f005efd00..646c88b4a 100644 --- a/backend/tests/clubs/test_views.py +++ b/backend/tests/clubs/test_views.py @@ -2182,6 +2182,12 @@ def test_club_sensitive_field_renew(self): club.refresh_from_db() self.assertTrue(club.approved) + # store result of approval history query + resp = self.client.get(reverse("clubs-history", args=(club.code,))) + self.assertIn(resp.status_code, [200], resp.content) + previous_history = json.loads(resp.content.decode("utf-8")) + self.assertTrue(previous_history[-1]["approved"]) + with patch("django.conf.settings.REAPPROVAL_QUEUE_OPEN", True): for field in {"name"}: # edit sensitive field @@ -2191,6 +2197,13 @@ def test_club_sensitive_field_renew(self): content_type="application/json", ) self.assertIn(resp.status_code, [200, 201], resp.content) + resp = self.client.get(reverse("clubs-history", args=(club.code,))) + # find the approval history + resp = self.client.get(reverse("clubs-history", args=(club.code,))) + self.assertIn(resp.status_code, [200], resp.content) + history = json.loads(resp.content.decode("utf-8")) + self.assertEqual(len(history), len(previous_history) + 1) + self.assertFalse(history[-1]["approved"]) # ensure club is marked as not approved club.refresh_from_db() diff --git a/frontend/components/ClubPage/ClubApprovalDialog.tsx b/frontend/components/ClubPage/ClubApprovalDialog.tsx index facc2b159..74eacd1df 100644 --- a/frontend/components/ClubPage/ClubApprovalDialog.tsx +++ b/frontend/components/ClubPage/ClubApprovalDialog.tsx @@ -1,3 +1,5 @@ +import { EmotionJSX } from '@emotion/react/types/jsx-namespace' +import moment from 'moment-timezone' import { useRouter } from 'next/router' import { ReactElement, useEffect, useState } from 'react' import Select from 'react-select' @@ -18,6 +20,7 @@ import { SITE_NAME, } from '../../utils/branding' import { Contact, Icon, Modal, Text, TextQuote } from '../common' +import { Chevron } from '../DropdownFilter' import { ModalContent } from './Actions' type Props = { @@ -34,9 +37,100 @@ type HistoricItem = { approved: boolean | null approved_on: string | null approved_by: string | null + approved_comment: string | null history_date: string } +const ClubHistoryDropdown = ({ history }: { history: HistoricItem[] }) => { + const [active, setActive] = useState(false) + const [reason, setReason] = useState(null) + const getReason = (item: HistoricItem): EmotionJSX.Element | string => { + return item.approved_comment ? ( + item.approved_comment.length > 100 ? ( + setReason(item.approved_comment)} + > + View Reason + + ) : ( + item.approved_comment + ) + ) : ( + 'No reason provided' + ) + } + return ( + <> +
setActive(!active)} + > + {active ? 'Hide' : 'Show'} History + +
+ setReason(null)} + marginBottom={false} + width="80%" + > + {reason} + + {active && ( +
+ {history.map((item, i) => ( +
+ {item.approved === true ? ( + + Approved by {item.approved_by} on{' '} + {moment(item.history_date) + .tz('America/New_York') + .format('LLL')}{' '} + - {getReason(item)} + + ) : item.approved === false ? ( + + Rejected by {item.approved_by} on{' '} + {moment(item.history_date) + .tz('America/New_York') + .format('LLL')}{' '} + - {getReason(item)} + + ) : ( + + Submitted for re-approval on{' '} + {moment(item.history_date) + .tz('America/New_York') + .format('LLL')} + + )} +
+ ))} +
+ )} + + ) +} + const ClubApprovalDialog = ({ club }: Props): ReactElement | null => { const router = useRouter() const year = getCurrentSchoolYear() @@ -73,9 +167,21 @@ const ClubApprovalDialog = ({ club }: Props): ReactElement | null => { doApiRequest(`/clubs/${club.code}/history/?format=json`) .then((resp) => resp.json()) - .then(setHistory) - } + .then((data) => { + // Get last version of club for each change in approved status + const lastVersions: HistoricItem[] = [] + for (let i = data.length - 1; i >= 0; i--) { + const item = data[i] + const lastItem = lastVersions[lastVersions.length - 1] + + if (item.approved !== lastItem?.approved || !lastItem) { + lastVersions.push(item) + } + } + setHistory(lastVersions) + }) + } setComment( selectedTemplates.map((template) => template.content).join('\n\n'), ) @@ -142,6 +248,7 @@ const ClubApprovalDialog = ({ club }: Props): ReactElement | null => { > Revoke Approval + )} {(club.active || canDeleteClub) && club.approved !== true ? ( @@ -378,6 +485,7 @@ const ClubApprovalDialog = ({ club }: Props): ReactElement | null => { )} + ) : null} {(seeFairStatus || isOfficer) && fairs.length > 0 && ( diff --git a/frontend/components/DropdownFilter.tsx b/frontend/components/DropdownFilter.tsx index 3805f3089..0ad45306b 100644 --- a/frontend/components/DropdownFilter.tsx +++ b/frontend/components/DropdownFilter.tsx @@ -73,17 +73,17 @@ const TableContainer = styled.div` } ` -const Chevron = styled(Icon)<{ open?: boolean }>` +export const Chevron = styled(Icon)<{ open?: boolean; color?: string }>` cursor: pointer; - color: ${CLUBS_GREY}; + color: ${({ color }) => color ?? CLUBS_GREY}; transform: rotate(0deg) translateY(0); transition: transform ${ANIMATION_DURATION}ms ease; - ${({ open }) => open && 'transform: rotate(180deg) translateY(-4px);'} + ${({ open }) => open && 'transform: rotate(180deg);'} ${mediaMaxWidth(MD)} { margin-top: 0.1em !important; margin-left: 0.1em !important; - color: ${LIGHT_GRAY}; + color: ${({ color }) => color ?? LIGHT_GRAY}; ${({ open }) => open && 'transform: rotate(180deg)'} } ` From c6861cc72d385011441fbda4e77a259cbe97e570 Mon Sep 17 00:00:00 2001 From: Julian Weng Date: Mon, 11 Nov 2024 02:17:50 -0500 Subject: [PATCH 3/6] Add docs to approval history endpoint --- backend/clubs/views.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/backend/clubs/views.py b/backend/clubs/views.py index 461b36466..598830918 100644 --- a/backend/clubs/views.py +++ b/backend/clubs/views.py @@ -1281,6 +1281,35 @@ def upload_file(self, request, *args, **kwargs): def history(self, request, *args, **kwargs): """ Return a simplified approval history for the club. + --- + responses: + "200": + content: + application/json: + schema: + type: array + items: + type: object + properties: + approved: + type: boolean + approved_on: + type: string + format: date-time + approved_by: + type: string + description: > + The full name of the user who approved + the club. + approved_comment: + type: string + history_date: + type: string + format: date-time + description: > + The time in which the specific version + of the club was saved at. + --- """ club = self.get_object() return Response( From 88674348293b93c2a11010c12c8cdda346a9ceef Mon Sep 17 00:00:00 2001 From: Julian Weng Date: Mon, 11 Nov 2024 02:45:32 -0500 Subject: [PATCH 4/6] Fix non-admin case --- backend/clubs/serializers.py | 5 ++++- frontend/components/ClubPage/ClubApprovalDialog.tsx | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/backend/clubs/serializers.py b/backend/clubs/serializers.py index 0a21f11d6..d7dd7071b 100644 --- a/backend/clubs/serializers.py +++ b/backend/clubs/serializers.py @@ -2987,7 +2987,10 @@ def get_approved_by(self, obj): if not user.is_authenticated: return None if not user.has_perm("clubs.see_pending_clubs"): - return None + club = Club.objects.get(code=obj.code) + membership = Membership.objects.filter(person=user, club=club).first() + if membership is None or membership.role < Membership.ROLE_OFFICER: + return None if obj.approved_by is None: return "Unknown" return obj.approved_by.get_full_name() diff --git a/frontend/components/ClubPage/ClubApprovalDialog.tsx b/frontend/components/ClubPage/ClubApprovalDialog.tsx index 74eacd1df..76d645bd1 100644 --- a/frontend/components/ClubPage/ClubApprovalDialog.tsx +++ b/frontend/components/ClubPage/ClubApprovalDialog.tsx @@ -164,7 +164,9 @@ const ClubApprovalDialog = ({ club }: Props): ReactElement | null => { doApiRequest('/templates/?format=json') .then((resp) => resp.json()) .then(setTemplates) + } + if (isOfficer || canApprove) { doApiRequest(`/clubs/${club.code}/history/?format=json`) .then((resp) => resp.json()) .then((data) => { @@ -182,6 +184,7 @@ const ClubApprovalDialog = ({ club }: Props): ReactElement | null => { setHistory(lastVersions) }) } + setComment( selectedTemplates.map((template) => template.content).join('\n\n'), ) From 0610295f11346a73e188970e2f2743946ba1adc2 Mon Sep 17 00:00:00 2001 From: Julian Weng Date: Tue, 12 Nov 2024 19:56:22 -0500 Subject: [PATCH 5/6] Add permissioning on route-level, change ordering convention for history --- backend/clubs/serializers.py | 8 -------- backend/clubs/views.py | 4 +++- backend/tests/clubs/test_views.py | 4 ++-- frontend/components/ClubPage/ClubApprovalDialog.tsx | 1 + 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/backend/clubs/serializers.py b/backend/clubs/serializers.py index d7dd7071b..fc9f1a095 100644 --- a/backend/clubs/serializers.py +++ b/backend/clubs/serializers.py @@ -2983,14 +2983,6 @@ class ApprovalHistorySerializer(serializers.ModelSerializer): history_date = serializers.DateTimeField() def get_approved_by(self, obj): - user = self.context["request"].user - if not user.is_authenticated: - return None - if not user.has_perm("clubs.see_pending_clubs"): - club = Club.objects.get(code=obj.code) - membership = Membership.objects.filter(person=user, club=club).first() - if membership is None or membership.role < Membership.ROLE_OFFICER: - return None if obj.approved_by is None: return "Unknown" return obj.approved_by.get_full_name() diff --git a/backend/clubs/views.py b/backend/clubs/views.py index 598830918..1b32cf83d 100644 --- a/backend/clubs/views.py +++ b/backend/clubs/views.py @@ -18,6 +18,7 @@ import pytz import qrcode import requests +import rest_framework from asgiref.sync import async_to_sync from channels.layers import get_channel_layer from CyberSource import ( @@ -1278,6 +1279,7 @@ def upload_file(self, request, *args, **kwargs): return file_upload_endpoint_helper(request, code=club.code) @action(detail=True, methods=["get"]) + @rest_framework.decorators.permission_classes([ClubSensitiveItemPermission]) def history(self, request, *args, **kwargs): """ Return a simplified approval history for the club. @@ -1314,7 +1316,7 @@ def history(self, request, *args, **kwargs): club = self.get_object() return Response( ApprovalHistorySerializer( - club.history.order_by("history_date"), + club.history.order_by("-history_date"), many=True, context={"request": request}, ).data diff --git a/backend/tests/clubs/test_views.py b/backend/tests/clubs/test_views.py index 646c88b4a..503ef08c5 100644 --- a/backend/tests/clubs/test_views.py +++ b/backend/tests/clubs/test_views.py @@ -2186,7 +2186,7 @@ def test_club_sensitive_field_renew(self): resp = self.client.get(reverse("clubs-history", args=(club.code,))) self.assertIn(resp.status_code, [200], resp.content) previous_history = json.loads(resp.content.decode("utf-8")) - self.assertTrue(previous_history[-1]["approved"]) + self.assertTrue(previous_history[0]["approved"]) with patch("django.conf.settings.REAPPROVAL_QUEUE_OPEN", True): for field in {"name"}: @@ -2203,7 +2203,7 @@ def test_club_sensitive_field_renew(self): self.assertIn(resp.status_code, [200], resp.content) history = json.loads(resp.content.decode("utf-8")) self.assertEqual(len(history), len(previous_history) + 1) - self.assertFalse(history[-1]["approved"]) + self.assertFalse(history[0]["approved"]) # ensure club is marked as not approved club.refresh_from_db() diff --git a/frontend/components/ClubPage/ClubApprovalDialog.tsx b/frontend/components/ClubPage/ClubApprovalDialog.tsx index 76d645bd1..cace58b99 100644 --- a/frontend/components/ClubPage/ClubApprovalDialog.tsx +++ b/frontend/components/ClubPage/ClubApprovalDialog.tsx @@ -181,6 +181,7 @@ const ClubApprovalDialog = ({ club }: Props): ReactElement | null => { lastVersions.push(item) } } + lastVersions.reverse() // Avoids O(n^2) of unshift() method setHistory(lastVersions) }) } From 3993a51f6c7b12c8251b4d99a9490d2df072f904 Mon Sep 17 00:00:00 2001 From: Julian Weng Date: Wed, 13 Nov 2024 12:44:41 -0500 Subject: [PATCH 6/6] Consolidate approval_history perms into ClubPermission --- backend/clubs/permissions.py | 8 +++++--- backend/clubs/views.py | 2 -- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/backend/clubs/permissions.py b/backend/clubs/permissions.py index daf103333..664c0775c 100644 --- a/backend/clubs/permissions.py +++ b/backend/clubs/permissions.py @@ -116,6 +116,8 @@ class ClubPermission(permissions.BasePermission): Anyone should be able to view, if the club is approved. Otherwise, only members or people with permission should be able to view. + + Actions default to owners/officers only unless specified below. """ def has_object_permission(self, request, view, obj): @@ -163,7 +165,7 @@ def has_object_permission(self, request, view, obj): if membership is None: return False # user has to be an owner to delete a club, an officer to edit it - if view.action in ["destroy"]: + if view.action in {"destroy"}: return membership.role <= Membership.ROLE_OWNER else: return membership.role <= Membership.ROLE_OFFICER @@ -171,7 +173,9 @@ def has_object_permission(self, request, view, obj): def has_permission(self, request, view): if view.action in { "children", + "create", "destroy", + "history", "parents", "partial_update", "update", @@ -179,8 +183,6 @@ def has_permission(self, request, view): "upload_file", }: return request.user.is_authenticated - elif view.action in {"create"}: - return request.user.is_authenticated else: return True diff --git a/backend/clubs/views.py b/backend/clubs/views.py index 1b32cf83d..7210e7a2e 100644 --- a/backend/clubs/views.py +++ b/backend/clubs/views.py @@ -18,7 +18,6 @@ import pytz import qrcode import requests -import rest_framework from asgiref.sync import async_to_sync from channels.layers import get_channel_layer from CyberSource import ( @@ -1279,7 +1278,6 @@ def upload_file(self, request, *args, **kwargs): return file_upload_endpoint_helper(request, code=club.code) @action(detail=True, methods=["get"]) - @rest_framework.decorators.permission_classes([ClubSensitiveItemPermission]) def history(self, request, *args, **kwargs): """ Return a simplified approval history for the club.