Refactor :Button option on triggers page #3643
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR introduces a View action and navigation handler to TriggerList, updates tests to mock react-router and assert navigation, and adds restrictedAction configuration to List. FormLayout gains new props (isView, errorButtonState, buttonState.show) and removes skipCancel; several containers now pass show/header/errorButtonState and adjust button visibility or headerHelp usage. Trigger and TriggerType now support view/edit behavior and disabled radios. Multiple tests and i18n added "Go Back" and updated many UI text expectations. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
priyanshu6238
changed the title
|
🚀 Deployed on https://deploy-preview-3643--glific-frontend.netlify.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/containers/Trigger/TriggerList/TriggerList.tsx (2)
89-91: Misleading function name for View action.The function
handleEditis used for the View action, but its name suggests it performs an edit operation. This creates confusion about the intent.Apply this diff to rename the function for clarity:
- const handleEdit = (id: any) => { + const handleView = (id: any) => { navigate(`/trigger/${id}/edit`); };Then update the usage on Line 110:
parameter: 'id', - dialog: handleEdit, + dialog: handleView, },
174-176: Document the purpose of restrictedAction.The
restrictedActionprop always returns{ delete: false }, which disables the List component's built-in delete functionality. While this makes sense since you're implementing custom delete logic with a confirmation dialog, it would benefit from a comment explaining this decision.Add a brief comment:
+ // Disable List's built-in delete; we handle deletion with custom dialog restrictedAction={(item: any) => ({ delete: false, })}src/containers/Trigger/TriggerList/TriggerList.test.tsx (2)
30-40: Test lacks assertion of expected behavior after clicking.The test clicks the action button but doesn't verify what happens afterward. It should assert that the expected navigation or dialog appears, making it a more meaningful test.
Consider adding assertions to verify the expected behavior:
test('click on Make a copy', async () => { - const { getByText, findAllByTestId } = render(wrapper); + const { getByText, findAllByTestId, findByText } = render(wrapper); const copyIcons = await findAllByTestId('copy-icon'); expect(copyIcons.length).toBeGreaterThan(0); expect(getByText('Triggers')).toBeInTheDocument(); const actionButtons = await findAllByTestId('additionalButton'); expect(actionButtons.length).toBeGreaterThan(0); fireEvent.click(actionButtons[0]); + + // Verify expected behavior after click + // For example, if it opens a menu: + // await waitFor(() => { + // expect(findByText('Copy')).toBeInTheDocument(); + // }); });
30-40: Consider adding tests for the new Delete functionality.The PR introduces delete confirmation dialog functionality, but there are no tests verifying this critical workflow. Tests should cover:
- Opening the delete dialog
- Confirming deletion
- Canceling deletion
Add a new test case:
test('delete trigger with confirmation', async () => { const { getByText, findAllByTestId, findByText } = render(wrapper); await waitFor(() => { expect(getByText('Triggers')).toBeInTheDocument(); }); const actionButtons = await findAllByTestId('additionalButton'); fireEvent.click(actionButtons[0]); // Click Delete action from the menu const deleteButton = await findByText('Delete'); fireEvent.click(deleteButton); // Verify delete confirmation dialog appears await waitFor(() => { expect(getByText('Do you want to delete this trigger?')).toBeInTheDocument(); }); // Optionally test cancel and confirm paths });Note: You'll need to add a mock for the DELETE_TRIGGER mutation to the mocks array.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/containers/Trigger/TriggerList/TriggerList.test.tsx(1 hunks)src/containers/Trigger/TriggerList/TriggerList.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
src/containers/Trigger/TriggerList/TriggerList.test.tsxsrc/containers/Trigger/TriggerList/TriggerList.tsx
🧬 Code graph analysis (1)
src/containers/Trigger/TriggerList/TriggerList.tsx (3)
src/components/UI/DialogBox/DialogBox.tsx (1)
DialogBox(32-149)src/containers/List/List.tsx (1)
List(168-836)src/common/HelpData.tsx (1)
triggerInfo(19-24)
⏰ 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). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: glific (1.18.3-otp-27, 27.3.3)
- GitHub Check: CI
- GitHub Check: build
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## bug_bash_1125 #3643 +/- ##
================================================
Coverage ? 82.56%
================================================
Files ? 348
Lines ? 11721
Branches ? 2474
================================================
Hits ? 9678
Misses ? 1297
Partials ? 746 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…cement/list-component
Glific
|
||||||||||||||||||||||||||||
| Project |
Glific
|
| Branch Review |
refactor/trigger_page
|
| Run status |
|
| Run duration | 28m 32s |
| Commit |
|
| Committer | Priyanshu singh |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
3
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
182
|
| View all changes introduced in this branch ↗︎ | |
akanshaaa19
left a comment
akanshaaa19
left a comment
There was a problem hiding this comment.
@priyanshu6238 can you take this as the base branch and then make changes accordingly? because otherwise it will be inconsistent. then you just have to add a view button and pass editSupport as false
… refactor/trigger_page
priyanshu6238
changed the base branch from
bug_bash_1125
to
enhancement/list-component
akanshaaa19
left a comment
akanshaaa19
left a comment
There was a problem hiding this comment.
check the unit test cases too
akanshaaa19
left a comment
akanshaaa19
left a comment
There was a problem hiding this comment.
on create also it shows edit
@akanshaaa19 Done |
|
@CodeRabbit review pls |
|
@priyanshu6238: I'll review the changes in this pull request. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/containers/HSM/HSM.tsx (1)
892-892: Simplify boolean expression.The ternary
isEditing ? true : falsecan be simplified to justisEditingsince it already returns a boolean value.Apply this diff:
- isView={isEditing ? true : false} + isView={isEditing}src/containers/Trigger/Trigger.tsx (1)
453-453: Simplify boolean expression.The ternary
isEditing ? true : falsecan be simplified to justisEditingsince it already returns a boolean value.Apply this diff:
- isView={isEditing ? true : false} + isView={isEditing}src/containers/Form/FormLayout.tsx (1)
667-667: Consider making headerHelp conditional on view mode.Currently,
headerHelpis initialized with a default value on line 667, which means it will be rendered in both edit and view modes. Based on the PR objectives (view mode refactoring) and the Heading.tsx changes (removedshowHeaderHelpprop), it appearsheaderHelpshould only be set when in view mode.Consider this refactoring:
- let headerHelp: string | undefined = `Please enter below details.`; + let headerHelp: string | undefined; // set title if there is a title if (title) { formTitle = title; } else if (type === 'copy') { formTitle = `Copy ${listItemName}`; // case when copying an item } else if (itemId) { formTitle = isView ? `${listItemName}` : `Edit ${listItemName}`; // case when editing a item } else { formTitle = `Create a new ${listItemName}`; // case when adding a new item } if (isView) { headerHelp = `Please view below details.`; } let heading = <Heading backLink={backLinkButton} formTitle={formTitle} headerHelp={headerHelp} />;This ensures the subtitle is only displayed in view mode, aligning with the UI requirements mentioned in the PR comments.
Also applies to: 679-682
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/components/UI/Heading/Heading.tsx(2 hunks)src/containers/Consulting/Consulting.test.tsx(1 hunks)src/containers/ContactManagement/ContactManagement.tsx(1 hunks)src/containers/Flow/Flow.tsx(1 hunks)src/containers/Form/FormLayout.tsx(8 hunks)src/containers/HSM/HSM.test.tsx(4 hunks)src/containers/HSM/HSM.tsx(3 hunks)src/containers/InteractiveMessage/InteractiveMessage.tsx(1 hunks)src/containers/MyAccount/MyAccount.tsx(1 hunks)src/containers/Profile/Contact/ContactProfile.tsx(1 hunks)src/containers/Search/Search.tsx(1 hunks)src/containers/SettingList/OrganizationFlows/OrganisationFLows.test.tsx(1 hunks)src/containers/SettingList/Providers/Providers.test.tsx(1 hunks)src/containers/SettingList/Providers/Providers.tsx(1 hunks)src/containers/SpeedSend/SpeedSend.tsx(1 hunks)src/containers/TemplateOptions/TemplateOptions.test.tsx(3 hunks)src/containers/Trigger/Trigger.test.tsx(14 hunks)src/containers/Trigger/Trigger.tsx(2 hunks)src/containers/Trigger/TriggerList/TriggerList.test.tsx(2 hunks)src/containers/Trigger/TriggerList/TriggerList.tsx(3 hunks)src/containers/Trigger/TriggerType/TriggerType.tsx(3 hunks)src/containers/WaGroups/GroupDetails.tsx/GroupDetails.tsx(1 hunks)src/containers/WhatsAppForms/WhatsAppForms.tsx(1 hunks)src/i18n/en/en.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/containers/SettingList/Providers/Providers.test.tsx
- src/containers/HSM/HSM.test.tsx
- src/containers/Consulting/Consulting.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/containers/Trigger/TriggerList/TriggerList.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
src/containers/SettingList/OrganizationFlows/OrganisationFLows.test.tsxsrc/containers/SettingList/Providers/Providers.tsxsrc/containers/WhatsAppForms/WhatsAppForms.tsxsrc/containers/TemplateOptions/TemplateOptions.test.tsxsrc/containers/InteractiveMessage/InteractiveMessage.tsxsrc/containers/WaGroups/GroupDetails.tsx/GroupDetails.tsxsrc/containers/Trigger/TriggerList/TriggerList.test.tsxsrc/containers/Profile/Contact/ContactProfile.tsxsrc/containers/Trigger/TriggerType/TriggerType.tsxsrc/containers/Search/Search.tsxsrc/containers/SpeedSend/SpeedSend.tsxsrc/containers/Flow/Flow.tsxsrc/components/UI/Heading/Heading.tsxsrc/containers/Trigger/Trigger.tsxsrc/containers/Form/FormLayout.tsxsrc/containers/Trigger/Trigger.test.tsxsrc/containers/MyAccount/MyAccount.tsxsrc/containers/ContactManagement/ContactManagement.tsxsrc/containers/HSM/HSM.tsx
🧬 Code graph analysis (6)
src/containers/WaGroups/GroupDetails.tsx/GroupDetails.tsx (1)
src/components/UI/Heading/Heading.tsx (1)
Heading(22-64)
src/containers/Profile/Contact/ContactProfile.tsx (1)
src/components/UI/Heading/Heading.tsx (1)
Heading(22-64)
src/containers/Form/FormLayout.tsx (2)
src/components/UI/Form/Button/Button.tsx (1)
Button(12-29)src/components/UI/Heading/Heading.tsx (1)
Heading(22-64)
src/containers/MyAccount/MyAccount.tsx (1)
src/components/UI/Heading/Heading.tsx (1)
Heading(22-64)
src/containers/ContactManagement/ContactManagement.tsx (2)
src/components/UI/Heading/Heading.tsx (1)
Heading(22-64)src/common/HelpData.tsx (1)
contactVariablesInfo(80-84)
src/containers/HSM/HSM.tsx (1)
src/graphql/queries/Template.ts (1)
GET_SHORTCODES(86-92)
⏰ 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). (1)
- GitHub Check: glific (1.18.3-otp-27, 27.3.3)
🔇 Additional comments (29)
src/containers/SettingList/OrganizationFlows/OrganisationFLows.test.tsx (1)
27-27: LGTM! Minor test description improvement.The capitalization and clarity enhancement to the test description is appropriate.
src/i18n/en/en.json (1)
34-34: LGTM! Translation entry added for view mode navigation.The "Go Back" translation supports the new view mode functionality introduced in this PR.
src/containers/WhatsAppForms/WhatsAppForms.tsx (1)
269-273: LGTM! Explicit button visibility control added.The
show: trueflag provides explicit control over button visibility, aligning with the FormLayout API enhancement introduced in this PR.src/containers/SettingList/Providers/Providers.tsx (1)
276-280: LGTM! Button visibility flag added.The
show: trueflag is consistent with the FormLayout API enhancement pattern used throughout this PR.src/containers/SpeedSend/SpeedSend.tsx (1)
499-499: LGTM! Button visibility control added.The
show: trueflag maintains consistency with the FormLayout API enhancement pattern.src/containers/TemplateOptions/TemplateOptions.test.tsx (1)
20-20: LGTM! Test assertions updated to match UI text changes.The test expectations correctly reflect the UI text change from "Add a new HSM Template" to "Create a new HSM Template".
Also applies to: 34-34, 61-61
src/containers/Flow/Flow.tsx (1)
354-354: LGTM! Button visibility flag added.The
show: trueflag is consistent with the FormLayout API enhancement pattern applied throughout this PR.src/containers/Profile/Contact/ContactProfile.tsx (1)
182-182: LGTM! Cleaned up unnecessary prop.Removing
showHeaderHelp={false}is appropriate since theHeadingcomponent's default behavior handles missingheaderHelpprops correctly.src/containers/ContactManagement/ContactManagement.tsx (1)
48-48: LGTM!The removal of
showHeaderHelp={false}is correct. The Heading component no longer accepts this prop, and header help rendering is now controlled by whetherheaderHelpis provided.src/containers/WaGroups/GroupDetails.tsx/GroupDetails.tsx (1)
226-226: LGTM!Consistent removal of
showHeaderHelp={false}following the Heading component API update.src/containers/Trigger/TriggerList/TriggerList.test.tsx (2)
20-28: LGTM!The react-router mock setup is well-structured and follows best practices. Using
vi.importActualensures that only the necessary functions are mocked while preserving other exports.
60-71: LGTM!The new test case properly validates the view button navigation behavior. Good use of
waitForfor async operations and clear assertion of the navigation call.src/containers/MyAccount/MyAccount.tsx (1)
306-306: LGTM!Consistent removal of
showHeaderHelp={false}following the Heading component API update.src/containers/InteractiveMessage/InteractiveMessage.tsx (1)
1044-1044: LGTM!Adding the explicit
show: trueflag tobuttonStatemakes the button visibility intent clear and prevents potential undefined field issues, as discussed in previous review comments.src/containers/HSM/HSM.tsx (3)
107-117: LGTM!Formatting improvement to the query destructuring. No functional change.
904-904: LGTM!The
errorButtonStatecorrectly switches between "Go Back" (when editing/viewing) and "Cancel" (when creating), providing appropriate context to users.
908-908: LGTM!Setting
show: !isEditingappropriately hides the action button in view mode, which aligns with HSM templates being non-editable after approval.src/containers/Search/Search.tsx (2)
544-544: LGTM!The migration from
skipCancel={chatFilters}toerrorButtonState={{ text: t('Cancel'), show: !chatFilters }}correctly preserves the logic (inverted as expected) while adopting the new, more flexible API.
545-552: LGTM!Adding explicit
show: truetobuttonStatewhenchatFiltersis active makes the button visibility intent clear and follows the established pattern.src/containers/Trigger/Trigger.tsx (4)
375-375: LGTM!Adding
disabled: isEditingto the TriggerType component appropriately makes the field read-only in view mode, consistent with other form fields.
447-447: LGTM!Capitalizing
listItemNameto "Trigger" improves UI text consistency.
454-454: LGTM!The
errorButtonStatecorrectly adapts the button text based on context ("Go Back" for viewing, "Cancel" for creating), improving user experience.
455-455: LGTM!Setting
buttonState={{ show: !isEditing }}appropriately hides the action button in view mode, aligning with the read-only nature of the trigger view.src/containers/Trigger/Trigger.test.tsx (1)
43-43: LGTM! Test assertions updated to match UI text capitalization.All test expectations have been consistently updated to capitalize "Trigger" as a proper noun, aligning with improved UI text standards.
Also applies to: 142-142, 210-210, 247-247, 271-271, 323-323, 351-351, 379-379, 409-409, 439-439, 445-445, 487-487, 518-518, 545-545, 575-575
src/containers/Trigger/TriggerType/TriggerType.tsx (1)
9-9: LGTM! Disabled prop correctly implemented.The
disabledprop has been properly added with appropriate typing, default value, and is correctly propagated to both Radio controls. This enables the view mode functionality where radio buttons become non-interactive during editing.Also applies to: 16-16, 45-45, 55-55
src/components/UI/Heading/Heading.tsx (1)
22-28: LGTM! API simplified by removing redundant prop.Removing
showHeaderHelpand relying solely on the truthiness ofheaderHelpis a cleaner approach that follows React conventions. The component now renders header help only whenheaderHelpis provided, eliminating the need for an additional control flag.Also applies to: 45-45
src/containers/Form/FormLayout.tsx (3)
103-103: LGTM! Sensible defaults maintain backward compatibility.The default values ensure that existing usage patterns continue to work without modification, while enabling new view mode behavior when explicitly requested.
Also applies to: 107-110, 131-131
596-613: LGTM! Button rendering correctly implements view mode behavior.The conditional rendering of submit and cancel buttons based on
buttonState.showanderrorButtonState.showproperly enables hiding/showing buttons in different modes. The use of optional chaining forerrorButtonState?.textis appropriate.Also applies to: 632-636
675-675: LGTM! Title logic correctly implements view mode.The conditional logic properly removes the "Edit" prefix when
isViewis true, showing just the item name (e.g., "Trigger"). This aligns with the test expectations and provides a cleaner UI in view mode.
akanshaaa19
left a comment
akanshaaa19
left a comment
There was a problem hiding this comment.
looks good one small change.
also shouldn't we remove this isActive button from HSM page if we are not going to give the save button now?
yes |
* refactor: improve actions visibility in List component * remove unused 'insideMore' property from action items in multiple lists * refactor: update test cases to use data-testid for action buttons * fix: trigger listing page * fix: trigger page * refactor: update data-testid attributes for copy and edit icons in FlowList component * fix: deep scan * chore: checkout bug_bash_1126 branch in cypress testing setup * refactor: remove unused 'screen' import from TriggerList test file * refactor: trigger listing page * fix: deep scan * fix: add test case * fix: naming * fix: route * fix: label * fix: naming * fix: header naming * fix: description * refactor: add back button * fix: test case * FIx: add props to trigger * fix: back button * fix: Remove save when it is in editing mode * fix: remove save button from hsm page * fix: trigger page * fix: add global header for format layout * refactor: add global header for hsm and add test case * fix: test case * fix: add new props * fix: add new props for cancel button * fix: add new props for header * fix: view props * fix: header * fix: button logic * fix: cancel button * fix: props globally * refactor: format layout * refactor: rename props * fix: deep scan * fix: props * fix: format layout * fix: isView condition * refactor: remove console * fix: header help function * fix: test case * reafactor: isView props condition in formLayout * fix: remove undefined type from headerHelp function * fix: hsm page --------- Co-authored-by: Akansha Sakhre <asakhre2002@gmail.com>
Summary by CodeRabbit
New Features
UI Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.