fix: add delete on sponsor form managed table - #1035
Open
tomrndom wants to merge 3 commits into
Open
Conversation
… and tests Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
📝 WalkthroughWalkthroughChangesThe sponsor forms flow now tracks managed-form assignment types. Explicitly assigned managed forms can be deleted through the API. Redux removes deleted forms, and the tab refreshes both form lists after successful deletion. Sponsor-managed form deletion
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SponsorFormsTab
participant deleteSponsorManagedForm
participant SponsorFormsAPI
participant ReduxReducer
SponsorFormsTab->>deleteSponsorManagedForm: Confirm managed-form deletion
deleteSponsorManagedForm->>SponsorFormsAPI: Send DELETE request
SponsorFormsAPI-->>deleteSponsorManagedForm: Return deletion result
deleteSponsorManagedForm->>ReduxReducer: Dispatch SPONSOR_MANAGED_FORM_DELETED
SponsorFormsTab->>SponsorFormsAPI: Refresh managed and customized lists
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/__tests__/sponsor-forms-tab.test.js (3)
235-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect form 2 by row, not by icon index.
getAllByTestId("DeleteIcon")[1]assumes the table order and action count. If either changes, the test can click a different row while still appearing to validate form 2. Query the row containing"Managed Form 2"and then find its delete button.Proposed test selection
- const deleteButtons = screen.getAllByTestId("DeleteIcon"); - const secondDeleteButton = deleteButtons[1].closest("button"); + const targetRow = screen.getByText("Managed Form 2").closest("tr"); + const deleteButton = within(targetRow).getByTestId("DeleteIcon").closest("button"); await act(async () => { - await userEvent.click(secondDeleteButton); + await userEvent.click(deleteButton); });Based on learnings,
MuiTable.onDeletepasses the primitive row identifier, so keep the existing ID assertion while making the target-row lookup explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/__tests__/sponsor-forms-tab.test.js` around lines 235 - 239, Update the test around the delete-button interaction to locate the table row containing “Managed Form 2,” then query that row’s delete button instead of selecting the second DeleteIcon globally. Keep the existing click flow and primitive row-ID assertion unchanged.Source: Learnings
313-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAwait the rejected request instead of using a fixed delay.
The 50 ms timeout is not tied to the request lifecycle. Store the rejected promise and await it before asserting the unchanged state. This removes arbitrary timing from the test.
Proposed deterministic wait
+ let requestPromise; deleteRequest.mockImplementation( - () => () => () => Promise.reject(new Error("delete failed")) + () => () => () => { + requestPromise = Promise.reject(new Error("delete failed")); + return requestPromise; + } ); ... - await act(async () => { - await new Promise((resolve) => { - setTimeout(resolve, 50); - }); - }); + await waitFor(() => expect(requestPromise).toBeDefined()); + await act(async () => { + await expect(requestPromise).rejects.toThrow("delete failed"); + });Also applies to: 342-350
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/__tests__/sponsor-forms-tab.test.js` around lines 313 - 315, Update the test’s delete-request setup and assertions around deleteRequest.mockImplementation so the rejected request promise is stored and explicitly awaited before checking the unchanged state. Apply the same deterministic waiting pattern to the additional occurrence, and remove the fixed 50 ms delay.
301-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck both list refresh actions after cancellation.
The cancellation test checks only
getSponsorManagedForms. Add the customized-list assertion so an accidental refresh ofgetSponsorCustomizedFormsalso fails the test.Proposed assertion
expect(deleteRequest).not.toHaveBeenCalled(); expect(getSponsorManagedForms).toHaveBeenCalledTimes(1); // mount only + expect(getSponsorCustomizedForms).toHaveBeenCalledTimes(1); // mount only expect(screen.getByText("Managed Form 1")).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/__tests__/sponsor-forms-tab.test.js` around lines 301 - 309, Add an assertion in the cancellation test after the cancel action verifying getSponsorCustomizedForms was called only once for the initial mount, alongside the existing getSponsorManagedForms assertion. Keep the existing no-delete and rendered-form checks unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/__tests__/sponsor-forms-tab.test.js`:
- Around line 235-239: Update the test around the delete-button interaction to
locate the table row containing “Managed Form 2,” then query that row’s delete
button instead of selecting the second DeleteIcon globally. Keep the existing
click flow and primitive row-ID assertion unchanged.
- Around line 313-315: Update the test’s delete-request setup and assertions
around deleteRequest.mockImplementation so the rejected request promise is
stored and explicitly awaited before checking the unchanged state. Apply the
same deterministic waiting pattern to the additional occurrence, and remove the
fixed 50 ms delay.
- Around line 301-309: Add an assertion in the cancellation test after the
cancel action verifying getSponsorCustomizedForms was called only once for the
initial mount, alongside the existing getSponsorManagedForms assertion. Keep the
existing no-delete and rendered-form checks unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 285c21ee-5996-4420-be53-b128da7676ff
📒 Files selected for processing (1)
src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/__tests__/sponsor-forms-tab.test.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/9014802374/86bb8wb20
Summary by CodeRabbit
New Features
Bug Fixes
Tests