From b280dc017bc9b313c271b67a95201fc12f918659 Mon Sep 17 00:00:00 2001 From: Julian Weng Date: Wed, 13 Nov 2024 10:06:12 -0800 Subject: [PATCH] Approval history UI (#746) * Add route for club approval history (in-progress) * Finish UI for club approval history + testcase * Add docs to approval history endpoint * Fix non-admin case * Add permissioning on route-level, change ordering convention for history * Consolidate approval_history perms into ClubPermission --- backend/clubs/permissions.py | 8 +- backend/clubs/serializers.py | 23 ++++ backend/clubs/views.py | 46 +++++++ backend/tests/clubs/test_views.py | 13 ++ .../ClubPage/ClubApprovalDialog.tsx | 124 ++++++++++++++++++ frontend/components/DropdownFilter.tsx | 8 +- 6 files changed, 215 insertions(+), 7 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/serializers.py b/backend/clubs/serializers.py index eb70bbb2c..fc9f1a095 100644 --- a/backend/clubs/serializers.py +++ b/backend/clubs/serializers.py @@ -2975,6 +2975,29 @@ 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): + 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 d645a24ad..b425af8f1 100644 --- a/backend/clubs/views.py +++ b/backend/clubs/views.py @@ -154,6 +154,7 @@ ApplicationSubmissionCSVSerializer, ApplicationSubmissionSerializer, ApplicationSubmissionUserSerializer, + ApprovalHistorySerializer, AssetSerializer, AuthenticatedClubSerializer, AuthenticatedMembershipSerializer, @@ -1276,6 +1277,49 @@ 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. + --- + 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( + ApprovalHistorySerializer( + club.history.order_by("-history_date"), + many=True, + context={"request": request}, + ).data + ) + @action(detail=True, methods=["get"]) def owned_badges(self, request, *args, **kwargs): """ @@ -2159,6 +2203,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 150bbc8fa..b4931d737 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[0]["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[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 faa479759..cace58b99 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 = { @@ -30,9 +33,108 @@ type ConfirmParams = { message: ReactElement | string } +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() + const [history, setHistory] = useState([]) const [comment, setComment] = useState(club.approved_comment || '') const [loading, setLoading] = useState(false) const [confirmModal, setConfirmModal] = useState(null) @@ -64,6 +166,26 @@ const ClubApprovalDialog = ({ club }: Props): ReactElement | null => { .then(setTemplates) } + if (isOfficer || canApprove) { + doApiRequest(`/clubs/${club.code}/history/?format=json`) + .then((resp) => resp.json()) + .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) + } + } + lastVersions.reverse() // Avoids O(n^2) of unshift() method + setHistory(lastVersions) + }) + } + setComment( selectedTemplates.map((template) => template.content).join('\n\n'), ) @@ -130,6 +252,7 @@ const ClubApprovalDialog = ({ club }: Props): ReactElement | null => { > Revoke Approval + )} {(club.active || canDeleteClub) && club.approved !== true ? ( @@ -366,6 +489,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)'} } `