appsmith icon indicating copy to clipboard operation
appsmith copied to clipboard
verified publisher icon appsmithorg

fix: allow currentRow for onClick in menuButton item in Table widget

Open SaiCharanChetpelly31 opened this issue 1 year ago ā€¢ 7 comments

Fixes #26052

Summary by CodeRabbit

  • New Features

    • Introduced a new property, customJSControl, for enhanced configuration in the Table Widget, allowing for dynamic table data handling.
    • Added a new test case to ensure the menu button's onClick property correctly accesses the currentRow context, improving the robustness of the component.

  • Bug Fixes

    • Enhanced testing coverage for the table menu button component to ensure correct behavior of dynamic properties.

SaiCharanChetpelly31 avatar Aug 02 '24 12:08 SaiCharanChetpelly31

Walkthrough

The recent changes enhance the functionality of a table widget by enabling the use of the currentRow context in menu button actions. Additionally, a new property, customJSControl, has been introduced to support more dynamic configurations within the widget. These updates collectively improve the component's usability and flexibility, addressing specific user-reported issues.

Changes

Files Change Summary
app/client/cypress/e2e/.../menubutton_spec.js Added a test case to verify that menu item actions can reference currentRow, enhancing coverage for dynamic behavior.
app/client/src/widgets/TableWidgetV2/widget/.../Basic.ts Introduced a new property customJSControl with the value "TABLE_COMPUTE_VALUE", allowing for custom JavaScript logic.

Assessment against linked issues

Objective Addressed Explanation
Use currentRow within menu items of a Menu Button (26052) āœ…
Enhance widget flexibility with custom controls (26052) āœ…

Possibly related PRs

  • #36061: This PR enhances the test specifications for the TableV2 component, which is directly related to the main PR's focus on testing the behavior of table components, specifically regarding the onClick property and its context.
  • #36375: This PR introduces validation for required select columns in the table widget, which aligns with the main PR's focus on testing the functionality of menu items in a table context, ensuring that dynamic properties are correctly linked to the current row data.
  • #36393: This PR addresses the issue of incorrect updates to the selectedRowIndex when the primary column is set, which is relevant to the main PR's focus on ensuring that the onClick property of menu items correctly accesses the currentRow context.

Suggested labels

Bug, Enhancement, Table Widget, Widgets Product, ok-to-test, Widgets & Accelerators Pod

Suggested reviewers

  • sagar-qa007
  • ApekshaBhosale
  • rahulbarwal

Poem

In the realm of tables bright,
Menu buttons take to flight.
With currentRow, actions thrive,
Custom controls help us drive.
A widget's dance, so rich and bold,
In code we trust, new tales unfold! šŸŽ‰

šŸ“œ Recent review details

Configuration used: .coderabbit.yaml Review profile: CHILL

šŸ“„ Commits

Files that changed from the base of the PR and between 29251b42bdf3099456a9cc1aaaadc897c33f19f6 and baadf6b0a824c3259f9f65a281dc533a7f735d42.

šŸ“’ Files selected for processing (1)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/ColumnTypeMenubutton_spec.js (1 hunks)
šŸ§° Additional context used
šŸ““ Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/ColumnTypeMenubutton_spec.js (1)

Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:

  • Follow best practices for Cypress code and e2e automation.
  • Avoid using cy.wait in code.
  • Avoid using cy.pause in code.
  • Avoid using agHelper.sleep().
  • Use locator variables for locators and do not use plain strings.
  • Use data-* attributes for selectors.
  • Avoid Xpaths, Attributes and CSS path.
  • Avoid selectors like .btn.submit or button[type=submit].
  • Perform logins via API with LoginFromAPI.
  • Perform logout via API with LogOutviaAPI.
  • Perform signup via API with SignupFromAPI.
  • Avoid using it.only.
  • Avoid using after and aftereach in test cases.
  • Use multiple assertions for expect statements.
  • Avoid using strings for assertions.
  • Do not use duplicate filenames even with different paths.
  • Avoid using agHelper.Sleep, this.Sleep in any file in code.
šŸ”‡ Additional comments (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/ColumnTypeMenubutton_spec.js (1)

166-175: Good job on adding a new test case, class! Let's review it together.

The new test case follows the structure of previous tests and adheres to many of our coding guidelines. Well done on avoiding cy.wait, using locator variables, and implementing multiple assertions!

However, I have a few suggestions to make it even better:

  1. Consider adding more descriptive assertions to clearly state what you're testing. For example:

    _.agHelper.GetNAssertElementText(
  _.locators._hints,
  "currentRow",
  "contain.text",
  "The autocomplete hints should include 'currentRow'"
);

  2. It might be helpful to add a comment explaining the purpose of typing "{{currentR" - this is to trigger the autocomplete suggestions, right?

  3. Consider checking if the entire "currentRow" object is accessible, not just its existence in the hints.

Keep up the good work, and remember to always strive for clarity in your tests!

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?

ā¤ļø Share
šŸŖ§ Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar Aug 02 '24 12:08 coderabbitai[bot]

@SaiCharanChetpelly31 Can you please explain me for the cypress test case requirement here? Can we test with Unit test ?

sagar-qa007 avatar Aug 05 '24 06:08 sagar-qa007

@SaiCharanChetpelly31 Can you please explain me for the cypress test case requirement here? Can we test with Unit test ?

@sagar-qa007 For other fields like visible and disabled, they are checking if currentRow is accessible in the Cypress tests itself. I didn't find any unit tests related to this in the Table widget code, which is why I have only written Cypress tests.

You can check the already existing cypress tests for visible and disable fields here https://github.com/appsmithorg/appsmith/blob/486662a2ed37a9e616d478c5ce296426d522c9c8/app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/menubutton_spec.js#L100C1-L165C8

SaiCharanChetpelly31 avatar Aug 05 '24 06:08 SaiCharanChetpelly31

This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected.

github-actions[bot] avatar Aug 12 '24 16:08 github-actions[bot]

@sagar-qa007 @ApekshaBhosale I hope you're well. Iā€™m curious and excited to hear your thoughts on my pull request raised a while ago. Looking forward to your review! Thank you. cc: @ramsaptami

SaiCharanChetpelly31 avatar Aug 12 '24 17:08 SaiCharanChetpelly31

This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected.

github-actions[bot] avatar Aug 21 '24 16:08 github-actions[bot]

@sagar-qa007 @ApekshaBhosale I hope you're well. Iā€™m curious and excited to hear your thoughts on my pull request raised a while ago. Looking forward to your review! Thank you. cc: @ramsaptami

SaiCharanChetpelly31 avatar Aug 22 '24 04:08 SaiCharanChetpelly31

This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected.

github-actions[bot] avatar Sep 03 '24 16:09 github-actions[bot]

@ApekshaBhosale Could you please review this PR?

SaiCharanChetpelly31 avatar Sep 04 '24 04:09 SaiCharanChetpelly31

This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected.

github-actions[bot] avatar Sep 19 '24 16:09 github-actions[bot]

@SaiCharanChetpelly31 Please fix merge conflicts.

rahulbarwal avatar Sep 26 '24 06:09 rahulbarwal

@SaiCharanChetpelly31 Please fix merge conflicts.

sure, working on it

SaiCharanChetpelly31 avatar Sep 27 '24 04:09 SaiCharanChetpelly31

@rahulbarwal I resolved merge conflicts. can you please check? Thanks in advance.

SaiCharanChetpelly31 avatar Sep 27 '24 05:09 SaiCharanChetpelly31

@jsartisan could you please review this PR?

KelvinOm avatar Sep 27 '24 11:09 KelvinOm

@rahulbarwal can you check this PR?

jsartisan avatar Sep 30 '24 06:09 jsartisan