86b7kbjcf - Fix: item counter when deleting form item on second page #740
Conversation
niko-exo
force-pushed
the
fix/refresh-item-counter
branch
from
1a81b2a to
156ba5b
Compare
📝 WalkthroughWalkthroughAdds a unit test for sponsor form item deletion, makes the page refresh sponsor form items after a successful delete, and removes Changes
Sequence Diagram(s)sequenceDiagram
participant Page as SponsorFormItemListPage
participant Thunk as deleteSponsorFormItem (thunk)
participant API as openstack-uicore-foundation.deleteRequest
participant Action as getSponsorFormItems
participant Reducer as SponsorFormItemsListReducer
Page->>Thunk: dispatch(deleteSponsorFormItem(formId, itemId))
Thunk->>API: deleteRequest(..., token)
API-->>Thunk: 200 OK
Thunk->>Page: snackbarSuccessHandler()
Thunk->>Action: dispatch(getSponsorFormItems(pagination, sort, archive))
Action->>Reducer: dispatch(LOAD_SPONSOR_FORM_ITEMS_SUCCESS)
Reducer-->>Page: updated items state
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
Branch rebased on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/actions/__tests__/sponsor-forms-actions.test.js:
- Around line 25-64: Test has a typo in the expectation message and is missing
stronger assertions: fix the typo "acces" -> "access" in the comment/assertion,
remove the unused sponsorFormItemsListState from the mockedGetState return, and
add assertions inside the "execute" test to verify deleteRequest was called with
the correct URL and handlers and that the SPONSOR_FORM_ITEM_DELETED action (or
the action creator used by deleteSponsorFormItem) was dispatched with the
expected itemId; use the existing spies/mocks (spyOnGetAccessTokenSafely,
spyOnSnackbarSuccessHandler, OpenStackUiCoreActions.deleteRequest) and the
deleteSponsorFormItem invocation to assert the exact parameters and dispatched
action.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/actions/__tests__/sponsor-forms-actions.test.jssrc/actions/sponsor-forms-actions.jssrc/pages/sponsors/sponsor-form-item-list-page/index.jssrc/reducers/sponsors/__tests__/sponsor-form-items-list-reducer.test.jssrc/reducers/sponsors/sponsor-form-items-list-reducer.js
💤 Files with no reviewable changes (2)
- src/actions/sponsor-forms-actions.js
- src/reducers/sponsors/tests/sponsor-form-items-list-reducer.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/sponsors/sponsor-form-item-list-page/index.js (1)
src/actions/sponsor-forms-actions.js (4)
deleteSponsorFormItem(933-959)deleteSponsorFormItem(933-959)getSponsorFormItems(862-909)getSponsorFormItems(862-909)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (5)
src/actions/__tests__/sponsor-forms-actions.test.js (3)
1-5: LGTM!The imports are well-organized and include all necessary dependencies for testing the DeleteSponsorFormItem action flow.
6-16: LGTM!The mock correctly preserves the original module while stubbing
deleteRequestwith a triple-nested function that matches the thunk pattern used in the codebase:deleteRequest(...args)(params)(dispatch).
18-24: LGTM!The test suite structure is clean, with proper cleanup using
jest.restoreAllMocks()inafterEachto prevent test interference.src/pages/sponsors/sponsor-form-item-list-page/index.js (1)
104-115: LGTM! This correctly fixes the item counter refresh issue.The change properly addresses the root cause by refetching the list after deletion, ensuring
totalCountand the items array are synchronized with the backend state. This approach is more reliable than attempting to calculatetotalCountlocally in the reducer.The refresh preserves all current filters, sorting, and pagination parameters, maintaining the user's view state.
src/reducers/sponsors/sponsor-form-items-list-reducer.js (1)
131-136: LGTM! Simplifying the reducer is the right approach.Removing the local
totalCountrecalculation eliminates the source of the bug. The subsequent refetch (added in the page component) ensurestotalCountis updated accurately from the backend.This change follows the single-source-of-truth principle: instead of attempting to maintain synchronization locally, rely on the server response via
RECEIVE_SPONSOR_FORM_ITEMSto update bothitemsandtotalCountcorrectly.
| it("execute", async () => { | ||
| const mockedDispatch = jest.fn(); | ||
| const mockedGetState = jest.fn(() => ({ | ||
| currentSummitState: { | ||
| currentSummit: "SSS" | ||
| }, | ||
| sponsorFormItemsListState: { | ||
| currentPage: 1, | ||
| perPage: 10, | ||
| order: "asc", | ||
| orderDir: 1, | ||
| hideArchived: false | ||
| } | ||
| })); | ||
|
|
||
| const params = { | ||
| formId: "AAA", | ||
| itemId: "III" | ||
| }; | ||
|
|
||
| const spyOnGetAccessTokenSafely = jest | ||
| .spyOn(UtilsMethods, "getAccessTokenSafely") | ||
| .mockImplementation(() => "access _token"); | ||
| const spyOnSnackbarSuccessHandler = jest.spyOn( | ||
| BaseActions, | ||
| "snackbarSuccessHandler" | ||
| ); | ||
|
|
||
| await deleteSponsorFormItem(params.formId, params.itemId)( | ||
| mockedDispatch, | ||
| mockedGetState | ||
| ); | ||
|
|
||
| // gets acces token safely | ||
| expect(spyOnGetAccessTokenSafely).toHaveBeenCalled(); | ||
| // calls delete request | ||
| expect(OpenStackUiCoreActions.deleteRequest).toHaveBeenCalled(); | ||
| // shows snackbar | ||
| expect(spyOnSnackbarSuccessHandler).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Fix typo and consider more comprehensive assertions.
Minor typo on line 58: "acces" should be "access".
The test validates the observable side effects but could be more thorough. Consider adding assertions to verify:
- The correct parameters are passed to
deleteRequest(URL, action creator, error handler) - The
SPONSOR_FORM_ITEM_DELETEDaction is dispatched with the correct itemId
Additionally, sponsorFormItemsListState in the mocked state (lines 31-37) is not used by deleteSponsorFormItem and can be removed for clarity.
📝 Proposed fix for typo
- // gets acces token safely
+ // gets access token safely
expect(spyOnGetAccessTokenSafely).toHaveBeenCalled();♻️ Optional: More comprehensive assertions
const mockedGetState = jest.fn(() => ({
currentSummitState: {
currentSummit: "SSS"
- },
- sponsorFormItemsListState: {
- currentPage: 1,
- perPage: 10,
- order: "asc",
- orderDir: 1,
- hideArchived: false
}
}));And add more specific assertions:
// gets access token safely
expect(spyOnGetAccessTokenSafely).toHaveBeenCalled();
// calls delete request
expect(OpenStackUiCoreActions.deleteRequest).toHaveBeenCalled();
+ // verify deleteRequest was called with correct action creator
+ const deleteRequestCall = OpenStackUiCoreActions.deleteRequest.mock.calls[0];
+ expect(deleteRequestCall[1]).toBeDefined(); // action creator
+ expect(deleteRequestCall[2]).toContain('/show-forms/AAA/items/III'); // URL
// shows snackbar
expect(spyOnSnackbarSuccessHandler).toHaveBeenCalled();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("execute", async () => { | |
| const mockedDispatch = jest.fn(); | |
| const mockedGetState = jest.fn(() => ({ | |
| currentSummitState: { | |
| currentSummit: "SSS" | |
| }, | |
| sponsorFormItemsListState: { | |
| currentPage: 1, | |
| perPage: 10, | |
| order: "asc", | |
| orderDir: 1, | |
| hideArchived: false | |
| } | |
| })); | |
| const params = { | |
| formId: "AAA", | |
| itemId: "III" | |
| }; | |
| const spyOnGetAccessTokenSafely = jest | |
| .spyOn(UtilsMethods, "getAccessTokenSafely") | |
| .mockImplementation(() => "access _token"); | |
| const spyOnSnackbarSuccessHandler = jest.spyOn( | |
| BaseActions, | |
| "snackbarSuccessHandler" | |
| ); | |
| await deleteSponsorFormItem(params.formId, params.itemId)( | |
| mockedDispatch, | |
| mockedGetState | |
| ); | |
| // gets acces token safely | |
| expect(spyOnGetAccessTokenSafely).toHaveBeenCalled(); | |
| // calls delete request | |
| expect(OpenStackUiCoreActions.deleteRequest).toHaveBeenCalled(); | |
| // shows snackbar | |
| expect(spyOnSnackbarSuccessHandler).toHaveBeenCalled(); | |
| }); | |
| it("execute", async () => { | |
| const mockedDispatch = jest.fn(); | |
| const mockedGetState = jest.fn(() => ({ | |
| currentSummitState: { | |
| currentSummit: "SSS" | |
| } | |
| })); | |
| const params = { | |
| formId: "AAA", | |
| itemId: "III" | |
| }; | |
| const spyOnGetAccessTokenSafely = jest | |
| .spyOn(UtilsMethods, "getAccessTokenSafely") | |
| .mockImplementation(() => "access _token"); | |
| const spyOnSnackbarSuccessHandler = jest.spyOn( | |
| BaseActions, | |
| "snackbarSuccessHandler" | |
| ); | |
| await deleteSponsorFormItem(params.formId, params.itemId)( | |
| mockedDispatch, | |
| mockedGetState | |
| ); | |
| // gets access token safely | |
| expect(spyOnGetAccessTokenSafely).toHaveBeenCalled(); | |
| // calls delete request | |
| expect(OpenStackUiCoreActions.deleteRequest).toHaveBeenCalled(); | |
| // verify deleteRequest was called with correct action creator | |
| const deleteRequestCall = OpenStackUiCoreActions.deleteRequest.mock.calls[0]; | |
| expect(deleteRequestCall[1]).toBeDefined(); // action creator | |
| expect(deleteRequestCall[2]).toContain('/show-forms/AAA/items/III'); // URL | |
| // shows snackbar | |
| expect(spyOnSnackbarSuccessHandler).toHaveBeenCalled(); | |
| }); |
🤖 Prompt for AI Agents
In @src/actions/__tests__/sponsor-forms-actions.test.js around lines 25 - 64,
Test has a typo in the expectation message and is missing stronger assertions:
fix the typo "acces" -> "access" in the comment/assertion, remove the unused
sponsorFormItemsListState from the mockedGetState return, and add assertions
inside the "execute" test to verify deleteRequest was called with the correct
URL and handlers and that the SPONSOR_FORM_ITEM_DELETED action (or the action
creator used by deleteSponsorFormItem) was dispatched with the expected itemId;
use the existing spies/mocks (spyOnGetAccessTokenSafely,
spyOnSnackbarSuccessHandler, OpenStackUiCoreActions.deleteRequest) and the
deleteSponsorFormItem invocation to assert the exact parameters and dispatched
action.
|
Ready for review |
niko-exo
force-pushed
the
fix/refresh-item-counter
branch
from
156ba5b to
e44ae59
Compare
|
Branch rebased on master |
|
@smarcet Ready for review |
|
Parameter replaced with constant! My bad I understood something different. |
ref: https://app.clickup.com/t/86b7kbjcf
86b7kbjcf - Fix: item counter when deleting form item on second page
Changelog
Links
86b7kbjcf - Item counter does not refresh after deleting item until page refresh
Evidence
2025-12-30_13-34-24.mp4
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.