-
Notifications
You must be signed in to change notification settings - Fork 195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Prevent trashing a campaign default form through bulk actions #7765
base: epic/campaigns
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR prevents deletion of a campaign’s default form through bulk actions by introducing an isIdSelectable method. Key changes include:
- Adding the isIdSelectable property to the BulkActionsConfig.
- Filtering checkbox selections based on whether their ID is selectable.
- Implementing the isIdSelectable method in the donation forms bulk actions to exclude the default form.
Reviewed Changes
File | Description |
---|---|
src/Views/Components/ListTable/ListTablePage/index.tsx | Enhances bulk action processing to filter out non-selectable form IDs. |
src/DonationForms/V2/resources/components/DonationFormsListTable.tsx | Implements isIdSelectable to prevent deletion of the default form through bulk actions. |
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/DonationForms/V2/resources/components/DonationFormsListTable.tsx:243
- [nitpick] Consider using explicit conversion (e.g. Number(id)) and strict equality (===) for comparing data.defaultForm to id to avoid potential type coercion issues.
isIdSelectable: (id, data) => data.defaultForm == null || data.defaultForm !== +id,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This pull request prevents unintended deletion of a campaign’s default form when performing bulk actions. It introduces a new isIdSelectable callback in the bulk actions configuration and implements it in the donation forms list to filter out the default form ID.
- Added an optional isIdSelectable method in the ListTablePage bulk actions configuration.
- Implemented custom logic in DonationFormsListTable.tsx to prevent selection of the default form for the trash action.
Reviewed Changes
File | Description |
---|---|
src/Views/Components/ListTable/ListTablePage/index.tsx | Added isIdSelectable property to BulkActionsConfig and updated the code to filter checkboxes using it. |
src/DonationForms/V2/resources/components/DonationFormsListTable.tsx | Implemented isIdSelectable for the trash action to filter out the default form based on its numeric ID. |
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/Views/Components/ListTable/ListTablePage/index.tsx:195
- Consider adding unit tests to verify that checkboxes corresponding to non-selectable IDs (such as the default form ID) are correctly filtered out during bulk actions.
const isSelectable = bulkAction?.isIdSelectable?.(checkbox.dataset.id, data) ?? true;
src/DonationForms/V2/resources/components/DonationFormsListTable.tsx:243
- [nitpick] Review the expression to ensure it handles cases where data.defaultForm might not be a number; consider using explicit parenthesis for clarity if necessary.
isIdSelectable: (id, data) => !(typeof data?.defaultForm === 'number') || data.defaultForm !== Number(id),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this check should be added on the backend. Regarding the frontend, I'd expect that the checkbox for the default form shouldn't be selectable or visible, but that's just cosmetics and it's up to you. Bulk actions are handled through API so if we really want to prevent someone from deleting the default form, we should do that in the API endpoint.
@alaca I agree with you. However, this PR is a follow-up to #7739, where we just removed the row action while keeping it in the bulk actions. It’s important to keep in mind that all these List Tables might be revisited in the near future for a refactoring and are very likely to have their API endpoints refactored as well, following up on Glauber’s work to establish the first truly RESTful API. |
Resolves GIVE-2280
Description
Although we have removed the inline option to delete a campaign’s default form, users are still able to do so through bulk actions. This pull request prevents that by:
isIdSelectable
method, allowing each action to determine whether an ID should be included in the list of IDs to be processed.isIdSelectable
method in thetrash
action to filter out the default form ID if it has been selected.If only the default form is selected, the behavior remains unchanged—it will not open the modal, as there are no valid selections. Ideally, we should provide more context to users in the future when this occurs. However, for now, this solution is sufficient, as the primary goal is to prevent users from deleting their campaign’s default form.
Affects
List Table's bulk actions but specifically for Campaign Forms
Visuals
Testing Instructions
Pre-review Checklist
@unreleased
tags included in DocBlocks