Feature updates: DataTable, user management, events, and UI fixes#16
Feature updates: DataTable, user management, events, and UI fixes#16
Conversation
… fixes - Migrate tables to TanStack Table with reusable DataTable component - Add PATCH /users/:id endpoint for admins to edit user information - Add priorityListDeadline field to events - Add remove role functionality - Fix sugar cubes visibility for registered participants and admins - Add Terms & Conditions, Code of Conduct, Photo Consent links to event edit modal - Fix frontend API domain for SSR in Docker - Set organization legalName on initialise
| @@ -0,0 +1,15 @@ | |||
| import { MigrationInterface, QueryRunner } from "typeorm"; | |||
|
|
|||
| export class EventPriorityListDeadline1769500000000 implements MigrationInterface { | |||
There was a problem hiding this comment.
How was this migrations created?
Also this migration is missing the updated enum for role permissions.
Use Nestjs migration:generate by pnpm migration:generate ./database/migration/<migration_name>
There was a problem hiding this comment.
alright I will use pnpm migration:generate
compose.yml
Outdated
| @@ -88,6 +89,7 @@ services: | |||
| environment: | |||
| - NEXT_PUBLIC_WEB_DOMAIN=${WEB_DOMAIN:-localhost:3000} | |||
| - NEXT_PUBLIC_API_DOMAIN=${API_DOMAIN:-localhost:4000} | |||
| - API_DOMAIN=http://backend:4000 | |||
There was a problem hiding this comment.
Why adding API_DOMAIN to frontend, when NEXT_PUBLIC_API_DOMAIN is available?
This value also should be configurable by .env not directly set inside docker compose.
There was a problem hiding this comment.
I had some trouble when building the docker image adding this to be able to run it
| @@ -1 +1 @@ | |||
| name: ers | |||
There was a problem hiding this comment.
Why was compose.yml renamed to docker-compose.yml?
There was a problem hiding this comment.
I will keep the same
frontend/src/proxy.ts
Outdated
| ]; | ||
|
|
||
| const apiUrl = process.env.NEXT_PUBLIC_API_DOMAIN; | ||
| const apiUrl = process.env.API_DOMAIN || process.env.NEXT_PUBLIC_API_DOMAIN; |
There was a problem hiding this comment.
NEXT_PUBLIC_API_DOMAIN and API_DOMAIN are technically the same value.
More in docker-compose.yml
There was a problem hiding this comment.
I had trouble while run the docker image
| onSuccess: () => { | ||
| queryClient.invalidateQueries({ queryKey: [getGetAllUsersQueryKey()] }); | ||
| handleOnSuccess(); | ||
| form.reset(); |
|
|
||
| export function DataTable<TData>({ | ||
| columns, | ||
| data, |
There was a problem hiding this comment.
Is this data prop set as "Stable" reference according to the TanStack docs?
https://tanstack.com/table/latest/docs/guide/data#give-data-a-stable-reference
There was a problem hiding this comment.
Yes memoize them (data and columns)
| sinceSince: now, | ||
| }); | ||
|
|
||
| const ongoingEvents = useMemo(() => { |
There was a problem hiding this comment.
Ongoing events will be visible only on the home page.
Send application is just review of all the previous registrations send by user
| deleteOrganizationMutation, | ||
| ); | ||
|
|
||
| // const rows = organisationsList?.map((organization, index) => ( |
There was a problem hiding this comment.
This extractText is used as extractor from Short description in JSON format for RichText.
Use RichTextRenderer to render this format, which already includes this functionality. Also possible to add prop textOnly for the component or create separate component for SimpleTextRendered based on the RichTextRenderer JSON structure
|
@Alpha871 |
- Add organization join request entity, controller, and migration - Add spot notification response DTO and email template - Add browse-organisations, my-join-requests, and organisation-join-requests pages - Add OrganisationJoinRequests component - Add application management columns for DataTable - Add event priority list deadline migration - Add pnpmrc config
7fe50a9 to
fa9e00a
Compare
…on, UI improvements - Add organization join requests (entity, controller, service methods, migration) - Add spot notification emails for assigned participants - Migrate all DataTable columns to TanStack ColumnDef[] (remove createColumns wrapper) - Memoize data/columns for stable TanStack Table references - Add RichTextRenderer textOnly prop, remove extractText helper - Add image processing: resize + webp conversion via sharp - Add user organisations column to export sheet - Add browse-organisations and my-join-requests pages - Add manage requests button on MyOrganisation page - Simplify NEXT_PUBLIC_API_DOMAIN usage - Fix tsconfig baseUrl deprecation warnings - Block priority list deadline modification after event ends - Add skipGlobalNotifications support for MutationCache - Clean up unused imports and dead code
- Use ApplicationManagementColumns with DataTable instead of raw Mantine Table - Memoize columns and data for stable TanStack Table references - Remove unused Table/ScrollArea imports
- Refactor DataTable columns (events, organisations, people, members, applications) - Simplify manage-events, manage-organisations and sent-applications pages - Update events controller and roles permissions - Replace event-priority-list-deadline migration with new timestamp - Replace docker-compose.yml with compose.yml (modern format) - Minor tweaks to proxy, customInstance, table-helpers and EditUserModal
frontend/src/utils/routes.ts
Outdated
|
|
||
| // Organization | ||
| ORGANISATION_MEMBERS: (props: { id: string }) => `/organisation-members/${props.id}`, | ||
| ORGANISATION_JOIN_REQUESTS: (props: { id: string }) => `/organisation-join-requests/${props.id}`, |
There was a problem hiding this comment.
Why are there 3 new routes?
It should be possible to create the workflow on existing organisation page.
| [OrganizationJoinRequestStatus.rejected]: "red", | ||
| }; | ||
|
|
||
| const MyJoinRequestsPage = () => { |
There was a problem hiding this comment.
Unnecessary page.
Usually people send request only to one organisation.
They can request it on the already existing page before this comming not to create more confusing user interface
| params: Promise<{ id: string }>; | ||
| } | ||
|
|
||
| const OrganisationJoinRequestsPage = ({ params }: Props) => { |
There was a problem hiding this comment.
Unnecessary page. Why cannot this be on the page of the organisation for easier overview?
| const handleDuplicateEvent = (event: EventSimple) => { | ||
| duplicateEventMutation.mutate({ id: event.id }); | ||
| }; | ||
| const handleDuplicateEvent = useCallback( |
There was a problem hiding this comment.
Why do the handleDuplicateEvent and handleDeleteEvent use useCallback?
There was a problem hiding this comment.
Columns must maintain stable references and track their dependencies; otherwise, updates will reset their internal state and trigger column re-creation.
tanstack.com/table/latest/docs/guide/data
https://tanstack.com/table/latest/docs/faq
if we don't want to use usecallback we can just to do this : const columns = useMemo(() => eventManagementColumns(
(event) => duplicateEventMutation.mutate({ id: event.id }),
(event) => deleteEventMutation.mutate({ eventId: event.id }),
), [duplicateEventMutation.mutate, deleteEventMutation.mutate]);
Or keep the structure of the code
| /** | ||
| * Update user data by admin | ||
| */ | ||
| @ApiBadRequestResponse({ description: "Username is already taken" }) |
There was a problem hiding this comment.
I dont think this is supposed to be the description for this EP
| id: application.id, | ||
| }); | ||
| }; | ||
| const handleDeleteApplication = useCallback( |
There was a problem hiding this comment.
Why the use of useCallback?
| const exportToSheet = useGenerateSheetEventApplication(eventId, { | ||
| request: { | ||
| responseType: "blob", | ||
| const handleEditApplication = useCallback( |
There was a problem hiding this comment.
Why the use of useCallback?
| }, | ||
| }); | ||
| }; | ||
| const handleChangeApplicationSpot = useCallback( |
There was a problem hiding this comment.
Why the use of useCallback?
| id: "legalName", | ||
| accessorKey: "legalName", | ||
| header: ({ column }) => <DataTableColumnHeader column={column} title="Legal Name" />, | ||
| enableSorting: false, |
| if (!event) throw new NotFoundException("Event not found"); | ||
|
|
||
| if ( | ||
| event.until < new Date() && |
There was a problem hiding this comment.
dayjs comparison can be used
Summary
Test plan