Conversation
…into feature/admin-mass-accept-bookings merge with remote
Shum-ster
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds bulk action functionality to the admin booking table, allowing admins to select multiple bookings via checkboxes and approve or reject them in bulk.
Changes:
- Added a new
admin-booking-base-tablecomponent (duplicating most ofbooking-base-table) with checkbox selection support - Created a new
admin-bulk-action-footercomponent that appears when bookings are selected, providing "Approve all" and "Reject all" actions - Updated
booking-admin-tableto use the new components and manage selection state
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/components/booking-admin-table/booking-admin-table.tsx | Updated to use new admin-booking-base-table, added state management for selected booking IDs, and integrated the bulk action footer |
| frontend/src/components/admin-bulk-action-footer/index.ts | Export file for the new bulk action footer component |
| frontend/src/components/admin-bulk-action-footer/admin-bulk-action-footer.tsx | New component implementing bulk approve/reject functionality with sequential API calls |
| frontend/src/components/admin-bulk-action-footer/admin-bulk-action-footer.module.scss | Styles for the fixed-position footer with slide-up animation |
| frontend/src/components/admin-booking-base-table/index.ts | Export file for the new admin booking base table |
| frontend/src/components/admin-booking-base-table/admin-booking-base-table.tsx | Duplicated booking-base-table with added checkbox column for row selection |
| frontend/src/components/admin-booking-base-table/admin-booking-base-table.module.scss | Copied styles from booking-base-table |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Column<BookingViewProps> | ||
| key="checkbox" // hmm change to a constant? | ||
| title="" | ||
| width={50} | ||
| align="center" | ||
| cellRenderer={CheckboxRenderer} | ||
| resizable={false} | ||
| frozen="left" | ||
| /> |
There was a problem hiding this comment.
The checkbox column lacks an accessible label or aria-label. Screen readers will not be able to announce what the checkbox is for. Consider adding an aria-label to the Column component or to the Checkbox itself to indicate its purpose (e.g., "Select booking for bulk action").
There was a problem hiding this comment.
I think it's intuitive, can ignore
|
|
||
| setProcessing(true); | ||
|
|
||
| // filter out clashes in booking |
There was a problem hiding this comment.
The comment "filter out clashes in booking" at line 51 is misleading. The code actually filters bookings based on their status to avoid redundant state transitions, not to handle booking clashes or conflicts. The comment should be updated to accurately describe what the filtering does, such as "filter out bookings that are already in the target state or are cancelled".
| // filter out clashes in booking | |
| // filter out bookings that are already in the target state or are cancelled |
| const allUpdatedBookings: BookingData[] = []; | ||
|
|
||
| for (const booking of validBookingsToUpdate) { | ||
| if (!booking.id) continue; | ||
| const result = await updateBookingStatus(booking.id, action); | ||
| allUpdatedBookings.push(...result); | ||
| } |
There was a problem hiding this comment.
Sequential API calls in a loop can cause performance issues, especially with a large number of selected bookings. Each booking status update waits for the previous one to complete before starting, which can lead to long wait times. Consider implementing batch API processing on the backend and updating the frontend to make a single API call with an array of booking IDs, or use Promise.all() to parallelize the requests if a batch endpoint isn't available.
| const allUpdatedBookings: BookingData[] = []; | |
| for (const booking of validBookingsToUpdate) { | |
| if (!booking.id) continue; | |
| const result = await updateBookingStatus(booking.id, action); | |
| allUpdatedBookings.push(...result); | |
| } | |
| const updatePromises = validBookingsToUpdate | |
| .filter((booking) => booking.id) | |
| .map((booking) => updateBookingStatus(booking.id as number, action)); | |
| const updateResults = await Promise.all(updatePromises); | |
| const allUpdatedBookings: BookingData[] = updateResults.flat(); |
| import { useCallback, useState, useRef, useEffect } from "react"; | ||
| import { AutoResizer, Column, ColumnShape, RowKey } from "react-base-table"; | ||
| import { Segment, Checkbox } from "semantic-ui-react"; | ||
|
|
||
| import { | ||
| ACTION, | ||
| CREATED_AT_STRING, | ||
| EVENT_TIME_RANGE, | ||
| ID, | ||
| START_DATE_TIME_STRING, | ||
| START_TIME_MINS, | ||
| STATUS, | ||
| } from "../../constants"; | ||
| import { useGetSingleBooking } from "../../custom-hooks/api/bookings-api"; | ||
| import { useAppDispatch } from "../../redux/hooks"; | ||
| import { updateBookingsAction } from "../../redux/slices/bookings-slice"; | ||
| import { BookingData } from "../../types/bookings"; | ||
| import BookingDetailsView from "../booking-details-view"; | ||
| import BookingStatusButton from "../booking-status-button"; | ||
| import Table, { TableProps } from "../table"; | ||
| import styles from "./admin-booking-base-table.module.scss"; | ||
|
|
||
| export type BookingViewProps = BookingData & { | ||
| [START_TIME_MINS]: number; | ||
| [START_DATE_TIME_STRING]: string; | ||
| [EVENT_TIME_RANGE]: string; | ||
| [CREATED_AT_STRING]: string; | ||
| booking?: BookingData; | ||
| children: { [ID]: string; booking: BookingData }[]; | ||
| }; | ||
|
|
||
| type Props = Partial<TableProps<BookingViewProps>> & { | ||
| adminView?: boolean; | ||
| defaultStatusColumnWidth?: number; | ||
| defaultActionColumnWidth?: number; | ||
| selectedBookingIds?: Set<number>; | ||
| onSelectionChange?: (selectedIds: Set<number>) => void; | ||
| }; | ||
|
|
||
| const RowRenderer: TableProps<BookingViewProps>["rowRenderer"] = ({ | ||
| rowData: { booking }, | ||
| cells, | ||
| columns, | ||
| }: { | ||
| rowData: BookingViewProps; | ||
| cells: React.ReactNode[]; | ||
| columns: ColumnShape<BookingViewProps>; | ||
| }) => | ||
| // Only render details if there are booking details | ||
| // and the column is not the frozen column | ||
| booking && columns.length > 1 ? ( | ||
| <Segment className={styles.extraContentContainer} basic> | ||
| <BookingDetailsView | ||
| className={styles.detailsContainer} | ||
| booking={booking} | ||
| /> | ||
| </Segment> | ||
| ) : ( | ||
| cells | ||
| ); | ||
|
|
||
| function BookingBaseTable({ | ||
| adminView = false, | ||
| defaultStatusColumnWidth = 100, | ||
| defaultActionColumnWidth = 100, | ||
| selectedBookingIds = new Set(), | ||
| onSelectionChange, | ||
|
|
||
| children, | ||
| ...props | ||
| }: Props) { | ||
| const { getSingleBooking } = useGetSingleBooking(); | ||
| const dispatch = useAppDispatch(); | ||
|
|
||
| const StatusButtonRenderer: ColumnShape<BookingViewProps>["cellRenderer"] = | ||
| useCallback( | ||
| ({ rowData: { status, id } }: { rowData: BookingViewProps }) => | ||
| status && | ||
| id !== undefined && ( | ||
| <BookingStatusButton | ||
| bookingId={id} | ||
| status={status} | ||
| adminView={adminView} | ||
| /> | ||
| ), | ||
| [adminView], | ||
| ); | ||
|
|
||
| const selectedIdsRef = useRef(selectedBookingIds); | ||
|
|
||
| useEffect(() => { | ||
| selectedIdsRef.current = selectedBookingIds; | ||
| }, [selectedBookingIds]); | ||
|
|
||
| const CheckboxRenderer: ColumnShape<BookingViewProps>["cellRenderer"] = | ||
| useCallback( | ||
| ({ rowData: { id } }: { rowData: BookingViewProps }) => { | ||
| if (id === undefined) return null; | ||
| const isChecked = selectedBookingIds.has(id); | ||
| return ( | ||
| <div onClick={(e) => e.stopPropagation()}> | ||
| <Checkbox | ||
| checked={isChecked} | ||
| onChange={() => { | ||
| const currentSet = selectedIdsRef.current; | ||
| const newSelected = new Set(currentSet); | ||
| if (currentSet.has(id)) { | ||
| newSelected.delete(id); | ||
| } else { | ||
| newSelected.add(id); | ||
| } | ||
| if (onSelectionChange) { | ||
| onSelectionChange(newSelected); | ||
| } | ||
| }} | ||
| /> | ||
| </div> | ||
| ); | ||
| }, | ||
| [selectedBookingIds, onSelectionChange], | ||
| ); | ||
|
|
||
| const [expandedRowKeys, setExpandedRowKeys] = useState<RowKey[]>([]); | ||
| const onRowExpand: TableProps<BookingViewProps>["onRowExpand"] = ({ | ||
| expanded, | ||
| rowData: { id, formResponseData }, | ||
| }) => { | ||
| if (!expanded || formResponseData) { | ||
| return; | ||
| } | ||
| getSingleBooking(id) | ||
| .then((booking) => { | ||
| if (booking) { | ||
| dispatch(updateBookingsAction({ bookings: [booking] })); | ||
| } else { | ||
| console.error("Failed to update booking details for booking ID", id); | ||
| } | ||
| }) | ||
| .catch((error) => console.error(error)); | ||
| }; | ||
|
|
||
| return ( | ||
| <Segment className={styles.bookingBaseTable}> | ||
| <AutoResizer> | ||
| {({ width, height }) => ( | ||
| <Table<BookingViewProps> | ||
| width={width} | ||
| height={height} | ||
| ignoreFunctionInColumnCompare={false} | ||
| rowRenderer={RowRenderer} | ||
| estimatedRowHeight={50} | ||
| fixed | ||
| expandColumnKey={ACTION} | ||
| onRowExpand={onRowExpand} | ||
| expandedRowKeys={expandedRowKeys} | ||
| rowEventHandlers={{ | ||
| onClick: ({ rowData, rowKey, rowIndex }) => { | ||
| if (!rowData.children || rowData.children.length === 0) return; | ||
| if (expandedRowKeys.includes(rowKey)) | ||
| setExpandedRowKeys( | ||
| expandedRowKeys.filter((x) => x !== rowKey), | ||
| ); | ||
| else { | ||
| setExpandedRowKeys([...expandedRowKeys, rowKey]); | ||
| onRowExpand({ expanded: true, rowData, rowIndex, rowKey }); | ||
| } | ||
| }, | ||
| }} | ||
| {...props} | ||
| > | ||
| <Column<BookingViewProps> | ||
| key="checkbox" // hmm change to a constant? | ||
| title="" | ||
| width={50} | ||
| align="center" | ||
| cellRenderer={CheckboxRenderer} | ||
| resizable={false} | ||
| frozen="left" | ||
| /> | ||
| {children} | ||
| <Column<BookingViewProps> | ||
| key={STATUS} | ||
| title="Status" | ||
| width={defaultStatusColumnWidth} | ||
| sortable | ||
| align="center" | ||
| cellRenderer={StatusButtonRenderer} | ||
| resizable | ||
| /> | ||
| <Column<BookingViewProps> | ||
| key={ACTION} | ||
| title="" | ||
| width={defaultActionColumnWidth} | ||
| align="center" | ||
| frozen="right" | ||
| /> | ||
| </Table> | ||
| )} | ||
| </AutoResizer> | ||
| </Segment> | ||
| ); | ||
| } | ||
|
|
||
| export default BookingBaseTable; No newline at end of file |
There was a problem hiding this comment.
The entire admin-booking-base-table component appears to be a near-complete duplication of the booking-base-table component, with only the addition of checkbox functionality and the ignoreFunctionInColumnCompare prop. This creates significant code duplication and maintenance burden. Consider refactoring the original booking-base-table to support the checkbox feature through props, or extract common logic into a shared component to avoid maintaining two nearly identical implementations.
| } | ||
| }; | ||
|
|
||
| if (selectedIds.size === 0) return null; |
There was a problem hiding this comment.
The footer component only becomes visible when items are selected (returns null at line 94), which means the "Select all" button is not accessible when no items are selected. Users must manually select at least one item before they can access the "Select all" functionality. Consider making the footer always visible with a "Select all" button, or add a checkbox in the table header row that allows selecting all visible items.
| } | ||
|
|
||
| @keyframes slideUp { | ||
| from { transform: translate(-50%, 100%); opacity: 0; } |
There was a problem hiding this comment.
The slideUp animation uses translate(-50%, 100%) which moves the element down by 100% before animating up. However, the footer is positioned at bottom: 40px, which means the element might not be fully off-screen at the start of the animation, potentially causing a visual glitch. Consider using translateY(calc(100% + 40px)) to ensure the element starts completely off-screen.
| from { transform: translate(-50%, 100%); opacity: 0; } | |
| from { transform: translate(-50%, calc(100% + 40px)); opacity: 0; } |
| <div onClick={(e) => e.stopPropagation()}> | ||
| <Checkbox | ||
| checked={isChecked} | ||
| onChange={() => { | ||
| const currentSet = selectedIdsRef.current; | ||
| const newSelected = new Set(currentSet); | ||
| if (currentSet.has(id)) { | ||
| newSelected.delete(id); | ||
| } else { | ||
| newSelected.add(id); | ||
| } | ||
| if (onSelectionChange) { | ||
| onSelectionChange(newSelected); | ||
| } | ||
| }} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
The checkbox is wrapped in a div with onClick stopPropagation, which prevents the row click event from firing when the checkbox is clicked. However, this only stops mouse click propagation. Keyboard users using Space or Enter to toggle the checkbox may still trigger the row expansion. Consider also adding onKeyDown handler to stop keyboard event propagation, or ensure the Checkbox component itself handles keyboard events appropriately.
| {...props} | ||
| > | ||
| <Column<BookingViewProps> | ||
| key="checkbox" // hmm change to a constant? |
There was a problem hiding this comment.
The inline comment "hmm change to a constant?" suggests unfinished work. The checkbox column key should be defined as a constant (similar to other column keys like ACTION, STATUS, ID, etc. from the constants file) to maintain consistency with the rest of the codebase.
| @@ -0,0 +1 @@ | |||
| export { default } from "./admin-bulk-action-footer"; | |||
There was a problem hiding this comment.
The PR description is incomplete - it ends with "Added" without specifying what was added. This makes it difficult for reviewers to understand the full scope of changes.
| onSelectionChange(new Set()); | ||
|
|
||
| } catch (error) { | ||
| resolveApiError(error as ApiResponseError); |
There was a problem hiding this comment.
If an error occurs during bulk processing after some bookings have already been updated, the user is shown a generic error message but the partial updates are already persisted in Redux state. This can lead to inconsistent UI state where some bookings show as updated but the error message suggests the operation failed. Consider either rolling back partial updates on error, or providing a more informative error message that indicates which bookings were successfully updated and which failed.
| resolveApiError(error as ApiResponseError); | |
| resolveApiError(error as ApiResponseError); | |
| toast.error( | |
| `An error occurred while trying to ${actionLabel} ${selectedIds.size} bookings. Some bookings may have been updated before the error occurred. Please review the bookings list to confirm which ones were changed.` | |
| ); |
| master_status_mapping = {} | ||
|
|
||
| for booking in bookings_to_update: | ||
| # reuse single update_booking_status |
| class PatchSingleBookingSerializer(serializers.Serializer): | ||
| action = serializers.ChoiceField(choices=BookingStatusAction.choices) | ||
|
|
||
| class PatchBulkBookingSerializer(serializers.Serializer): |
| booking_ids=booking_ids, action=action, user=requester | ||
| ) | ||
|
|
||
| send_updated_booking_emails( |
There was a problem hiding this comment.
@Shum-ster help to test this after deployed to beta, thanks. LGTM.
zachchong
left a comment
zachchong
left a comment
There was a problem hiding this comment.
LGTM, good work @Shum-ster
Uh oh!
There was an error while loading. Please reload this page.