Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: update property name automatically when custom column name is changed #18048

Merged
merged 6 commits into from
Dec 7, 2022
Merged

feat: update property name automatically when custom column name is changed #18048

merged 6 commits into from
Dec 7, 2022

Conversation

souma-ghosh
Copy link
Contributor

@souma-ghosh souma-ghosh commented Nov 2, 2022 •
edited

Description

This feature is only for custom columns. Users needed to change property name of custom columns every-time they changed the column name.
Column name can be updated from 2 places

  1. Table widget property pane > Columns
  2. Column settings > title edit

This PR uses updateHook to change column alias of a custom column whenever the column label (column name) is changed.

Fixes #17142

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Cypress tests
  • Jest tests

Test Plan

Add Testsmith test cases links that relate to this PR

Issues raised during DP testing

Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR)

Checklist:

Dev activity

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • PR is being merged under a feature flag

QA activity:

  • Test plan has been approved by relevant developers
  • Test plan has been peer reviewed by QA
  • Cypress test cases have been added and approved by either SDET or manual QA
  • Organized project review call with relevant stakeholders after Round 1/2 of QA
  • Added Test Plan Approved label after reveiwing all Cypress test

@souma-ghosh
feat: update property name automatically when custom column name is c…
7a502ff 
…hanged

- adds the feature
- adds cypress tests
@vercel
Copy link

vercel bot commented Nov 2, 2022 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
appsmith ✅ Ready (Inspect) Visit Preview Nov 30, 2022 at 6:27AM (UTC)

@github-actions github-actions bot added App Viewers Pod This label assigns issues to the app viewers pod Enhancement New feature or request Medium Issues that frustrate users due to poor UX Table Widget V2 Issues related to Table Widget V2 labels Nov 2, 2022
@souma-ghosh
Copy link
Contributor Author

/ok-to-test sha=7a502ff

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3376898337.
Workflow: Appsmith External Integration Test Workflow.
Commit: 7a502ff.
PR: 18048.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=18048&runId=3376898337_1

@souma-ghosh
Copy link
Contributor Author

/ok-to-test sha=7a502ff

@github-actions
Copy link

github-actions bot commented Nov 3, 2022

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3383739278.
Workflow: Appsmith External Integration Test Workflow.
Commit: 7a502ff.
PR: 18048.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=18048&runId=3383739278_1

@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Nov 14, 2022
sbalaji1192
sbalaji1192 reviewed Nov 23, 2022
View reviewed changes
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/Data.ts Show resolved Hide resolved
app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/index.ts Outdated Show resolved Hide resolved
app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts Outdated Show resolved Hide resolved
app/client/src/widgets/TableWidgetV2/widget/propertyUtils.ts Outdated Show resolved Hide resolved
app/client/src/widgets/TableWidgetV2/widget/propertyUtils.test.ts Outdated Show resolved Hide resolved
app/client/src/widgets/TableWidgetV2/widget/propertyUtils.test.ts Outdated Show resolved Hide resolved
app/client/src/widgets/TableWidgetV2/widget/propertyUtils.test.ts Outdated Show resolved Hide resolved
app/client/src/widgets/TableWidgetV2/widget/propertyUtils.test.ts Outdated Show resolved Hide resolved
@souma-ghosh
Review changes
2c1772d 
- rename function to updateCustomColumnAliasOnLabelChange
- more suitable titles to jest tests
- modify updateCustomColumnAliasOnLabelChange logic to be more concise
@souma-ghosh
Copy link
Contributor Author

/ok-to-test sha=2c1772d

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3540678374.
Workflow: Appsmith External Integration Test Workflow.
Commit: 2c1772d.
PR: 18048.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=18048&runId=3540678374_1

@github-actions github-actions bot removed the Stale label Nov 24, 2022
@souma-ghosh
Merge branch 'release' of github.com:appsmithorg/appsmith into feat/1…
d3d1968 
…7142-propertyname-customcolumn-change-with-label
@vercel
Copy link

vercel bot commented Nov 25, 2022

Deployment failed with the following error:

Resource is limited - try again in 17 minutes (more than 100, code: "api-deployments-free-per-day").

@souma-ghosh
Copy link
Contributor Author

/ok-to-test sha=d3d1968

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3545625936.
Workflow: Appsmith External Integration Test Workflow.
Commit: d3d1968.
PR: 18048.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=18048&runId=3545625936_1

@souma-ghosh
Copy link
Contributor Author

/ok-to-test sha=d3d1968

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3573264011.
Workflow: Appsmith External Integration Test Workflow.
Commit: d3d1968.
PR: 18048.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=18048&runId=3573264011_1

@souma-ghosh
Review Changes
9175e20 
- Modify updateCustomColumnAliasOnLabelChange method to return undefined instead of empty array
- Adjust test cases accordingly
@souma-ghosh
Copy link
Contributor Author

/ok-to-test sha=9175e20

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3581507016.
Workflow: Appsmith External Integration Test Workflow.
Commit: 9175e20.
PR: 18048.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=18048&runId=3581507016_1

@souma-ghosh
Copy link
Contributor Author

@dilippitchika About updating other widget bindings when we change the name of a custom column; we should create a separate issue to work on that.
As far as I understand the logic here has to be very much similar to what is happening to bindings when we update the name of a widget. e.g. when we update the name of a table from Table1 to Table12 all the dependencies that had Table1 in them get updated to Table12.
But we will need careful planning before going about this. I will create a separate issue and tag it here soon

@chandannkumar
Copy link

Test Plan for this PR: https://github.com/appsmithorg/TestSmith/issues/2095

@souma-ghosh
Copy link
Contributor Author

/ok-to-test sha=9175e20

@chandannkumar
Copy link

Observation: @souma-ghosh

@github-actions
Copy link

github-actions bot commented Dec 2, 2022

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3599569365.
Workflow: Appsmith External Integration Test Workflow.
Commit: 9175e20.
PR: 18048.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-63465d4789020c7ac296d08d?pr=18048&runId=3599569365_1

@souma-ghosh
Copy link
Contributor Author

souma-ghosh commented Dec 2, 2022 •
edited

Observation: @souma-ghosh

  1. For the first one, the issue already existed before and is not as a result of this PR. We should create an issue for this.
  2. For the second one, @sbalaji1192 @dilippitchika could you comment on the expected behaviour for this scenario. According to my understanding since there is a duplicate column label the table should not be able to identify which column to pick up and show in the table or in TableName.selectedRow. We are maintaining separate ids for each column but we are using column label to identify and make the columns unique. We show an error when non unique columns are encountered. Should this behaviour be sufficient?

@dilippitchika
Copy link
Contributor

For 1, I didn't understand how this is an existing issue @souma-ghosh. What are the steps to reproduce?

For 2, It's expected behavior to show the key which matches at the end. So not an issue.

@souma-ghosh
Copy link
Contributor Author

For 1, I didn't understand how this is an existing issue @souma-ghosh. What are the steps to reproduce?

For 2, It's expected behavior to show the key which matches at the end. So not an issue.

The issue mentioned in point 1 happens for any column
Steps to reproduce 1

  • Go into the column settings of any column
  • Click on column title to edit it
  • Delete the last character and type back the deleted letter again, e.g. backspace on status so it becomes statu then type back the s again (don't save the column title anywhere in between this step)
  • Now save the title and come outside to the main property pane and observe that the column name is still statu and not status

@souma-ghosh
Copy link
Contributor Author

@chandannkumar Both the above mentioned issues are already happening in prod. We can move ahead

@souma-ghosh
Copy link
Contributor Author

/ok-to-test sha=9175e20

@github-actions
Copy link

github-actions bot commented Dec 7, 2022

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3636461385.
Workflow: Appsmith External Integration Test Workflow.
Commit: 9175e20.
PR: 18048.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18048&runId=3636461385_1

@chandannkumar
Copy link

Tested this PR and working as expected

@chandannkumar chandannkumar added the Test Plan Approved Manual/Cypress tests covers changes made on the PR. Else, add skip-testPlan label if not applicable label Dec 7, 2022
@trishaanand trishaanand merged commit b2f67e1 into release Dec 7, 2022
@trishaanand trishaanand deleted the feat/17142-propertyname-customcolumn-change-with-label branch December 7, 2022 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbalaji1192 sbalaji1192 sbalaji1192 approved these changes

Assignees
No one assigned
Labels
App Viewers Pod This label assigns issues to the app viewers pod Enhancement New feature or request Medium Issues that frustrate users due to poor UX Table Widget V2 Issues related to Table Widget V2 Test Plan Approved Manual/Cypress tests covers changes made on the PR. Else, add skip-testPlan label if not applicable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: When user updates the name of the custom column also update the property name
5 participants
@souma-ghosh @chandannkumar @dilippitchika @sbalaji1192 @trishaanand