feat(meetings): allow hosts to delete meetings for everyone [WPB-25513]#21671
feat(meetings): allow hosts to delete meetings for everyone [WPB-25513]#21671thisisamir98 wants to merge 2 commits into
Conversation
Wire the existing delete meeting API through the repository layer and show a confirmation modal before removing a meeting. Only meeting hosts see the action, and the list refreshes after a successful deletion.
thisisamir98
requested review from
arjita-mitra,
e-maad,
otto-the-bot,
screendriver and
zskhan
as code owners
thisisamir98
changed the title
|
| return result.err(deleteMeetingErrors.deleteFailed); | ||
| } | ||
|
|
||
| await fetchMeetings(); |
There was a problem hiding this comment.
tryDeleteMeeting returns a Result, but fetchMeetings() can still reject here. That means a successful delete followed by a failed refresh escapes the Result flow and the modal cannot show the mapped error state.
Can we make this behavior explicit? Either make the refresh best-effort, add a separate refresh error, or catch the refresh failure here and return a Result. A test for a rejecting fetchMeetings would also help.
| <div css={rightStyles}> | ||
| <MeetingStatus start_date={start_date} end_date={end_date} attending={attending} nowMs={timestamp} /> | ||
| <MeetingAction meeting={meeting} /> | ||
| <MeetingAction meeting={meeting} onMeetingDeleted={onMeetingDeleted ?? (async () => {})} /> |
There was a problem hiding this comment.
Can we avoid the silent no-op fallback here?
After deleting a meeting, refreshing the list is part of the expected success flow. If onMeetingDeleted is not wired, the delete action can still run but the UI silently skips the refresh. I would prefer making this callback required through the component chain, or not exposing the delete action when the callback is missing.
Additionally we would have noop-esm in place for situations like that.
| icon: () => <TrashIcon css={contextMenuDangerItemIconStyles} />, | ||
| label: translate('meetings.action.deleteMeetingForAll'), | ||
| }, | ||
| ...(canEditMeeting(meeting, selfUser, nowMs) ? [editEntry, deleteForAllEntry] : []), |
There was a problem hiding this comment.
Is delete-for-everyone intentionally using exactly the same eligibility rule as edit?
If yes, I would still prefer a more neutral predicate name, for example canManageUpcomingMeeting, or a dedicated canDeleteMeetingForAll. Reusing canEditMeeting for deletion makes the delete rule look accidental and couples future edit/delete behavior.
| import type {User} from 'Repositories/entity/User'; | ||
| import {matchQualifiedIds} from 'Util/qualifiedId'; | ||
|
|
||
| export const isMeetingHost = (meeting: Meeting, selfUser: User): boolean => |
There was a problem hiding this comment.
Nit: can we avoid the immediate-return arrow function and use a block body with an explicit return here?
export function isMeetingHost(meeting: Meeting, selfUser: User): boolean {
return matchQualifiedIds(meeting.qualified_creator, selfUser.qualifiedId);
};|
|
||
| const getEditEntryLabel = (entries: ReturnType<typeof getMeetingActionEntries>) => | ||
| entries.find(entry => entry.label === 'meetings.action.editMeeting'); | ||
| const getEntryByLabel = (entries: ReturnType<typeof getMeetingActionEntries>, label: string) => |
There was a problem hiding this comment.
Nit: same here. Can we use an explicit return statement for this helper?
function getEntryByLabel(entries: ReturnType<typeof getMeetingActionEntries>, label: string) {
return entries.find(entry => entry.label === label);
};| @@ -77,7 +79,12 @@ const MeetingListItemGroupComponent = ({ | |||
| <div> | |||
| {nonEmptyGroups.flatMap(([, items]) => | |||
| items.map(item => ( | |||
There was a problem hiding this comment.
Nit: this nested flatMap / map callback now renders multi-line JSX. Can we use block bodies with explicit return statements here? It is more verbose, but easier to read and debug.
2 participants
Wire the existing delete meeting API through the repository layer and show a confirmation modal before removing a meeting. Only meeting hosts see the action, and the list refreshes after a successful deletion.