fix:widget name validation #34963
fix:widget name validation #34963
Conversation
saicharan-zemoso
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe changes introduce validation to ensure widget names do not start with a digit, alongside new test cases to verify this functionality. A test suite for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Cypress Test
participant PageSagas
participant PropertyPane
User->>Cypress Test: Run CheckboxGroup renaming tests
Cypress Test->>PageSagas: Request to rename with new name
PageSagas->>PageSagas: Validate new name
alt Name starts with digit
PageSagas->>Cypress Test: Dispatch error
else Name is valid
PageSagas->>Cypress Test: Rename success
end
Cypress Test->>User: Display test results
User->>PropertyPane: Edit title
PropertyPane->>PropertyPane: Check if title starts with digit
alt Title starts with digit
PropertyPane->>User: Reset title to original
else Title is valid
PropertyPane->>User: Update title successfully
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
saicharan-zemoso
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBoxGroupTestWithInvalidName.ts (1)
20-32: Review: Cypress test for renaming a widget with a valid name.The test case for renaming a widget with a valid name is well-implemented. It checks both the existence and the correct renaming of the widget. Additionally, the test waits for 3000 milliseconds after the renaming, which might be necessary for some asynchronous operations to complete.
However, using a hardcoded delay (
cy.wait(3000)) is generally discouraged as it can lead to flaky tests if the required action takes longer than expected. It's better to use more deterministic waits, such as waiting for a specific API call to complete or an element to become visible.- cy.wait(3000); + cy.wait('@someAlias'); // Replace 'someAlias' with the actual alias of a network request or a more deterministic condition.app/client/src/ce/sagas/PageSagas.tsx (1)
997-1008: Review: Validation logic to prevent widget names starting with a number.The implementation uses a regular expression to check if the new widget name starts with a digit. If it does, an error is dispatched, which is in line with the PR's objectives.
However, the error message could be made more user-friendly by suggesting a valid naming format. Additionally, it would be beneficial to ensure that this validation logic does not interfere with other naming conventions or validations that might be in place.
- message: `Entity name: ${action.payload.newName} cannot start with a number.`, + message: `Entity name '${action.payload.newName}' is invalid because it starts with a number. Please use a name that starts with a letter.`,
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBoxGroupTestWithInvalidName.ts (1 hunks)
- app/client/src/ce/sagas/PageSagas.tsx (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBoxGroupTestWithInvalidName.ts (1)
Pattern
app/client/cypress/**/**.*:
Additional comments not posted (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBoxGroupTestWithInvalidName.ts (1)
6-19: Review: Cypress test for invalid widget name starting with a digit.This test case correctly simulates the scenario where a user attempts to rename a widget to a name that starts with a digit, which should trigger a validation error. The test checks for the existence of a warning toast, which is appropriate.
However, it's important to ensure that the selector used for the warning toast (
.Toastify__toast-body) is specific and reliable for this particular error message. Using more specific selectors or data attributes would enhance the robustness of this test.Verification successful
Verified: The CSS selector
.Toastify__toast-bodyis used consistently in the codebase for detecting toast notifications.The selector
.Toastify__toast-bodyis used in multiple Cypress test files to check for the existence of toast notifications. This indicates that it is a reliable and consistent selector for this purpose.
app/client/cypress/e2e/Regression/ClientSide/Widgets/ListV2/Listv2_Copy_Paste_Delete_spec.jsapp/client/cypress/e2e/Regression/ClientSide/Widgets/TreeSelect/Single_Select_Tree_spec.tsapp/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBoxGroupTestWithInvalidName.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the specific CSS selector for the error toast in the application's components. # Test: Search for the CSS selector usage. Expect: At least one occurrence in the component files. rg --type css ".Toastify__toast-body"Length of output: 37
Script:
#!/bin/bash # Description: Verify the existence of the specific CSS selector for the error toast in the application's components. # Test: Search for the CSS selector usage in JavaScript or TypeScript files. Expect: At least one occurrence in the component files. rg --type js,ts ".Toastify__toast-body"Length of output: 69
Script:
#!/bin/bash # Description: Verify the existence of the specific CSS selector for the error toast in the application's components. # Test: Search for the CSS selector usage in JavaScript files. rg --type js ".Toastify__toast-body"Length of output: 203
Script:
#!/bin/bash # Description: Verify the existence of the specific CSS selector for the error toast in the application's components. # Test: Search for the CSS selector usage in TypeScript files. rg --type ts ".Toastify__toast-body"Length of output: 480
|
@saicharan-zemoso , I have seen there is change in src with business logic. This logic can be tested with Jest unit tests or React testing library. Cypress is redundant here. |
| const updateCanvasWithDSLMock = jest.fn(); | ||
| const updateWidgetNameSuccessMock = jest.fn(); | ||
| import { ReduxActionTypes } from "ce/constants/ReduxActionConstants"; | ||
| import { updateWidgetNameSaga } from "./PageSagas"; | ||
|
|
||
| jest.mock("redux-saga/effects", () => { | ||
| const originalModule = jest.requireActual("redux-saga/effects"); | ||
|
|
||
| return { | ||
| __esModule: true, | ||
| ...originalModule, | ||
| select: jest.fn(), | ||
| call: jest.fn(), | ||
| put: jest.fn(), | ||
| }; | ||
| }); | ||
| jest.mock("actions/pageActions", () => { | ||
| const originalModule = jest.requireActual("actions/pageActions"); | ||
|
|
||
| return { | ||
| __esModule: true, | ||
| ...originalModule, | ||
| updateWidgetNameSuccess: updateWidgetNameSuccessMock, | ||
| }; | ||
| }); | ||
| jest.mock("./PageSagas", () => { | ||
| const originalModule = jest.requireActual("./PageSagas"); | ||
| return { | ||
| __esModule: true, | ||
| ...originalModule, | ||
| updateCanvasWithDSL: updateCanvasWithDSLMock, | ||
| }; | ||
| }); | ||
| jest.mock("sagas/ErrorSagas", () => { | ||
| const originalModule = jest.requireActual("sagas/ErrorSagas"); | ||
|
|
||
| return { | ||
| __esModule: true, | ||
| ...originalModule, | ||
| validateResponse: jest.fn(), | ||
| }; | ||
| }); | ||
| jest.mock("sagas/helper", () => { | ||
| const originalModule = jest.requireActual("sagas/helper"); | ||
|
|
||
| return { | ||
| __esModule: true, | ||
| ...originalModule, | ||
| checkAndLogErrorsIfCyclicDependency: jest.fn(), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Review of mock setups
The mock setups are extensive and cover various dependencies like Redux actions and saga effects. This is crucial for isolating the unit under test, which in this case is the updateWidgetNameSaga. However, ensure that each mock is necessary and correctly implements the intended behavior, especially the jest.fn() which should ideally simulate realistic scenarios or return values.
- Potential Over-mocking: Over-mocking can lead to tests that pass despite changes that should cause failures. Ensure that mocks like
updateCanvasWithDSLMockandupdateWidgetNameSuccessMockare used appropriately within the tests to reflect realistic use cases. - Consistency in Mocking Style: The consistent use of
jest.requireActualhelps maintain behavior from the original modules while allowing specific overrides, which is good practice in maintaining a balance between realistic testing and necessary isolation.
| describe("PageSagas", () => { | ||
| it("test updateWidgetNameSaga with name that starts with a number", () => { | ||
| const gen = updateWidgetNameSaga({ | ||
| type: ReduxActionTypes.UPDATE_WIDGET_NAME_INIT, | ||
| payload: { id: "checkBox1", newName: "1checkbox" }, | ||
| }); | ||
| gen.next(); | ||
| gen.next(mockCanvasWidgets.checkBox1 as never); | ||
| gen.next("layoutId1" as never); | ||
| gen.next("pageId1" as never); | ||
| gen.next(mockUsedNames as never); | ||
| gen.next(undefined as never); | ||
| expect(gen.next().done).toBeTruthy(); | ||
| }); | ||
| it("test updateWidgetNameSaga with name that is valid", () => { | ||
| const gen = updateWidgetNameSaga({ | ||
| type: ReduxActionTypes.UPDATE_WIDGET_NAME_INIT, | ||
| payload: { id: "checkBox1", newName: "checkBox10" }, | ||
| }); | ||
| gen.next(); | ||
| gen.next(mockCanvasWidgets.checkBox1 as never); | ||
| gen.next("layoutId1" as never); | ||
| gen.next("pageId1" as never); | ||
| gen.next(mockUsedNames as never); | ||
| gen.next(undefined as never); | ||
| gen.next({ data: "mock" } as never); | ||
| gen.next(true as never); | ||
| gen.next(); | ||
| gen.next(); | ||
| expect(gen.next().done).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
Review of test cases in PageSagas
-
Test Case: "test updateWidgetNameSaga with name that starts with a number"
- This test correctly sets up a scenario where the widget name starts with a number. The test flow appears to follow the saga's logic, but the assertions are minimal. It only checks if the saga completes without checking the behavior or outcome (e.g., error handling or correct function calls).
-
Test Case: "test updateWidgetNameSaga with name that is valid"
- Similar to the first test, this scenario tests a valid name update. However, the assertions are still minimal, and there's no check to ensure that
updateWidgetNameSuccessMockwas called, which is crucial for verifying that the saga handles valid names correctly.
- Similar to the first test, this scenario tests a valid name update. However, the assertions are still minimal, and there's no check to ensure that
- Improving Assertions: Both tests could benefit from more detailed assertions. For instance, checking that the correct actions are dispatched or that certain functions are called with expected arguments would make these tests more robust and meaningful.
+ // Add assertions to check for specific behaviors or outcomes
+ expect(updateWidgetNameSuccessMock).toHaveBeenCalledWith(expectedArguments);
+ expect(updateCanvasWithDSLMock).toHaveBeenCalledTimes(expectedCalls);- Handling Asynchronous Behavior: Consider using
async/awaitwithredux-saga-test-planto handle asynchronous behavior more predictably in your tests.
Committable suggestion was skipped due to low confidence.
|
Hello @sagar-qa007 if everything is ok can you merge this pr. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/ce/sagas/PageSagas.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/sagas/PageSagas.test.ts
|
Hello @sagar-qa007 @ApekshaBhosale @rajatagrawal , |
|
Hello @sagar-qa007 @ApekshaBhosale @rajatagrawal , |
|
@saicharan-zemoso request you to go through this important information before raising PRs |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10144322089. |
|
Deploy-Preview-URL: https://ce-34963.dp.appsmith.com |
|
widget name should revert back to it's previous valid name after numeric validation @saicharan-zemoso |
| const numPattern = /^\d/i; | ||
| //check if the name starts with a number | ||
| if (numPattern.test(action.payload.newName)) { |
There was a problem hiding this comment.
Why let it reach this stage? Can we not do this validation in the widget name input itself?
There was a problem hiding this comment.
Yes we can do it at input.
But the option to change the widget name was provided in multiple places (ie from property pane and entity explorer list) and that was the common point for the widget name to update and In the same file the other validations were added and the error is being dispatched from that saga.
So wrote the code in the saga file so that all the validations are together and error message can be dispatched.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/pages/Editor/PropertyPane/PropertyPaneTitle.tsx (4 hunks)
Additional comments not posted (5)
app/client/src/pages/Editor/PropertyPane/PropertyPaneTitle.tsx (5)
74-74: LGTM!The addition of the
forceUpdatestate variable is appropriate for managing the component's reactivity based on specific title input conditions.
117-121: LGTM!The
useEffecthook for resettingforceUpdateensures that the component can react to subsequent title changes correctly.
196-196: LGTM!The addition of the
forceDefaultprop is necessary to control theEditableTextcomponent's behavior based on theforceUpdatestate.
Line range hint
74-196:
LGTM!The overall integration of the new state management and logic enhancements is seamless and aligns with the objective of handling invalid widget names.
Line range hint
94-112:
LGTM! But verify the error message display logic.The use of a regular expression to validate the widget name is a good approach. Ensure that the error message is displayed to the user when the name is invalid.
|
Hello @ramsaptami @rahulbarwal , |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10163821989. |
|
Deploy-Preview-URL: https://ce-34963.dp.appsmith.com |
|
Hello @rahulbarwal , |
| newTitle.dblclick(); | ||
| newTitle.type("CheckboxGroup{enter}"); | ||
| newTitle.should("have.text", "CheckboxGroup"); | ||
| cy.wait(3000); |
There was a problem hiding this comment.
@saicharan-zemoso Please remove cy.wait
There was a problem hiding this comment.
removed that line
| }); | ||
| it("CheckboxGroup renamed with the name which start with a digit", () => { | ||
| EditorNavigation.SelectEntityByName("CheckboxGroup1", EntityType.Widget); | ||
| const title = cy.get(".t--property-pane-title"); |
There was a problem hiding this comment.
Kindly check for existing locator or create locator variable and use it.
There was a problem hiding this comment.
replaced with propPane._paneTitle
| EntityType, | ||
| } from "../../../../../support/Pages/EditorNavigation"; | ||
|
|
||
| describe("Checkbox Tests", { tags: ["@tag.Widget", "@tag.Checkbox"] }, () => { |
There was a problem hiding this comment.
Please give explanation of functionality that needs to be test here.
There was a problem hiding this comment.
changed the title and added a comment
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBoxGroupTestWithInvalidName.ts (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBoxGroupTestWithInvalidName.ts (1)
Pattern
app/client/cypress/**/**.*:
Learnings (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBoxGroupTestWithInvalidName.ts (1)
Learnt from: abhishek-bandameedi PR: appsmithorg/appsmith#35133 File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBox2_spec.js:0-0 Timestamp: 2024-07-24T08:29:41.208Z Learning: In Cypress tests for the Appsmith project, ensure the use of locator variables for selectors and include multiple assertions for comprehensive testing.
Additional comments not posted (6)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBoxGroupTestWithInvalidName.ts (6)
1-7: LGTM!The import statements are appropriate and necessary for the test file.
The code changes are approved.
9-9: LGTM!The comment clearly describes the purpose of the test.
The code changes are approved.
10-40: LGTM!The describe block is well-structured and the tags are appropriate.
The code changes are approved.
14-16: LGTM!The before hook is correctly setting up the initial state for the tests.
The code changes are approved.
17-26: LGTM!The first test case is well-written and correctly verifies that the widget shows an error when renamed with a name starting with a digit.
The code changes are approved.
27-37: LGTM!The second test case is well-written and correctly verifies that the widget can be renamed with a valid name.
The code changes are approved.
|
Hello @sagar-qa007 , If every thing looks good can you merge this PR. |
| } from "../../../../../support/Pages/EditorNavigation"; | ||
|
|
||
| //The widget should throw an error when the widget is renamed with a name that starts with a digit or a number | ||
| describe( |
There was a problem hiding this comment.
Can you please confirm that we need this cypress test case for this change and why?
Please also check unit test are sufficient.
There was a problem hiding this comment.
Here the cypress test was just added to visually conform that the changes are working later I have added the unit tests.
if the cypress test is not required we can remove it.
There was a problem hiding this comment.
@sagar-qa007 , is this ok or should I remove this cypress test. Please let me know.
saicharan-zemoso
changed the title
|
Hello @sagar-qa007 , can you review this PR if you have some bandwidth available. I will work on the comments if any. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
issue: 22259
added the validation to check if the widget name start with a number
when the widget name starts with a digit then this error is thrown:
and when the error is thrown the name of the widget is not updated.
if the widget name is valid:
similarly this condition is checked while updating any widget name.
cypress test video:
checkbox.validations.mp4
Summary by CodeRabbit
New Features
Tests