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

fix: Refactor entities based on AST parsing logic

Open nidhi-nair opened this issue 3 years ago • 31 comments

Description

As a result of this PR, incorrectly refactored references to other entities (widgets, actions, JS objects, JS functions) like when the same string happened to be in a comment, a nested path, a string or any other invalid reference, will now be ignored during refactors. This eliminates another source of possible cyclic dependency errors in Appsmith apps.

Fixes #17047 Fixes #17564 Fixes #17556 Fixes #18493 Fixes #18492 Fixes #18699

Waiting on:

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Manual

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

  • [x] My code follows the style guidelines of this project
  • [x] 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
  • [x] 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

nidhi-nair avatar Nov 28 '22 08:11 nidhi-nair

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

Name Status Preview Updated
appsmith ✅ Ready (Inspect) Visit Preview Dec 7, 2022 at 6:18AM (UTC)

vercel[bot] avatar Nov 28 '22 08:11 vercel[bot]

LGTM!

ayushpahwa avatar Nov 30 '22 17:11 ayushpahwa

/ok-to-test sha=1656d0c

ChandanBalajiBP avatar Nov 30 '22 17:11 ChandanBalajiBP

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

github-actions[bot] avatar Nov 30 '22 18:11 github-actions[bot]

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

github-actions[bot] avatar Nov 30 '22 18:11 github-actions[bot]

FYI - @ChandanBalajiBP - the perf infra dashboard seems to have some issue - could u ask performance team to take a look at this? unable to see the page.

btsgh avatar Dec 01 '22 05:12 btsgh

While executing cases, found a few issues -

  1. https://ddpa7mfuvv.vmaker.com/record/m0dAXHeTAitF7CWk
  2. https://ddpa7mfuvv.vmaker.com/record/RZlBgfWm4zUlLjOQ
  3. https://ddpa7mfuvv.vmaker.com/record/cuzDP28VwdW2Qexq
  4. https://ddpa7mfuvv.vmaker.com/record/rlCgxZVq410inhPe
  5. The following scenario still has an issue - OriginalWidgetName

This is how it looks after entity name change - AfternameChange

Expected behavior -

  • The first reference in quotes should not get changed,
  • The second binding should get changed
  • The third, 4th and 5th references should not get changed as they are not in bindings. Spaces and comments / slashes should be retained
  • The 6th one should be changed and the new line should be retained
  • The 7th reference should be changed and the new line as well as spaces should be retained
  1. https://ddpa7mfuvv.vmaker.com/record/d6eyutc3cBJ9xF7t

btsgh avatar Dec 01 '22 07:12 btsgh

/ok-to-test sha=d906ffc

ChandanBalajiBP avatar Dec 05 '22 18:12 ChandanBalajiBP

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

github-actions[bot] avatar Dec 05 '22 18:12 github-actions[bot]

/ok-to-test sha=7316e18

nidhi-nair avatar Dec 05 '22 23:12 nidhi-nair

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

github-actions[bot] avatar Dec 05 '22 23:12 github-actions[bot]

/ok-to-test sha=c0cada5

nidhi-nair avatar Dec 05 '22 23:12 nidhi-nair

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

github-actions[bot] avatar Dec 05 '22 23:12 github-actions[bot]

  1. @nidhi-nair - Raised a separate issue for this. Assigned it at present to Chandan ..

  2. @ChandanBalajiBP you mean this is also not related to AST? This happens on changing the widget name isn't it? The issue looks to be resolved now as well. Will run more cases around this Note: @nidhi-nair An update here is if there is a binding within comments, that gets refactored, while it should not. Here is the screenshot - BindingWithinComment.png

  3. I can still seem to reproduce this issue.

  4. Though this is resolved, the above issue 3 still exists in this scenario.

  5. This issue has been resolved

  6. This issue has been resolved.

btsgh avatar Dec 06 '22 07:12 btsgh

For 3rd point - This issue exists in production and has not been introduced by AST-related changes. @btsgh has created an issue for the same!!

ChandanBalajiBP avatar Dec 06 '22 12:12 ChandanBalajiBP

/ok-to-test sha=b5a0496

nidhi-nair avatar Dec 07 '22 05:12 nidhi-nair

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

github-actions[bot] avatar Dec 07 '22 05:12 github-actions[bot]

  1. With new line characters, when refactoring happens, the curly braces lose their dark blue color AST_BindingColorLost with newline char

Raised a separate bug to track this

  1. Another issue found in the API value area. - Discussed with @nidhi-nair - not related to entity refactoring - Note - logged this issue for the same

  2. Here is a behavior which is inconsistent when a binding is referenced inside a comment. Treating this as acceptable behavior as per this slack thread - @nidhi-nair

  3. Here is another issue found. Shared it with @nidhi-nair .. was asked to raise a separate issue

Issue 1 and 4 are the ones that need to be looked into. Have completed my round of testing.

Testsmith link

  1. Also - The Performance link that was shared , for this particular PR, shows up a lot of red values in the SD column. Do get this checked.

btsgh avatar Dec 07 '22 08:12 btsgh

/ok-to-test sha=bab7186

nidhi-nair avatar Dec 07 '22 13:12 nidhi-nair

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

github-actions[bot] avatar Dec 07 '22 13:12 github-actions[bot]

/perf-test sha=bab7186

nidhi-nair avatar Dec 08 '22 13:12 nidhi-nair

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3648592718. Workflow: Performance Tests Workflow. Commit: bab7186. PR: 18517. Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18517&runId=3648592718_1

github-actions[bot] avatar Dec 08 '22 13:12 github-actions[bot]

/perf-test

nidhi-nair avatar Dec 08 '22 14:12 nidhi-nair

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3649355355. Workflow: Performance Tests Workflow. Commit: ``. PR: 18517. Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18517&runId=3649355355_1

github-actions[bot] avatar Dec 08 '22 14:12 github-actions[bot]

/perf-test

nidhi-nair avatar Dec 09 '22 07:12 nidhi-nair

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3655467511. Workflow: Performance Tests Workflow. Commit: ``. PR: 18517. Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18517&runId=3655467511_1

github-actions[bot] avatar Dec 09 '22 07:12 github-actions[bot]

With 3 open issues around this which will be handled later -

  1. https://github.com/appsmithorg/appsmith/issues/18723
  2. https://github.com/appsmithorg/appsmith/issues/18795
  3. https://github.com/appsmithorg/appsmith/issues/18813

Moving the status to Done

btsgh avatar Dec 09 '22 08:12 btsgh

/perf-test

ChandanBalajiBP avatar Dec 09 '22 12:12 ChandanBalajiBP

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3657330043. Workflow: Performance Tests Workflow. Commit: ``. PR: 18517. Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18517&runId=3657330043_1

github-actions[bot] avatar Dec 09 '22 12:12 github-actions[bot]

/perf-test

ChandanBalajiBP avatar Dec 09 '22 14:12 ChandanBalajiBP