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

fix:prevent execution of js function when jscollection is saving #13592

Merged
merged 4 commits into from
May 19, 2022
Merged

fix:prevent execution of js function when jscollection is saving #13592

merged 4 commits into from
May 19, 2022

Conversation

ohansFavour
Copy link
Contributor

@ohansFavour ohansFavour commented May 5, 2022 •
edited by github-actions bot

Description

This PR prevents execution of JS functions with stale values. execution only starts after editors values are saved.

While still saving

Screenshot 2022-05-12 at 09 58 28

After saving
Screenshot 2022-05-12 at 10 10 56

Fixes #13512
Fixes #13960

Type of change

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

How Has This Been Tested?

  • Unit tests

  • getJSResponseViewState

  • Manual

  • Tested by throttling the internet speed and ensuring that recent values are used for execution even when application is still in the saving state when execution is triggered.

Checklist:

  • 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

Test coverage results 🧪

🟢 Total coverage has increased 
// Code coverage diff between base branch:release and head branch: fix/prevent-running-js-function-when-jsobject-is-loading
Status File % Stmts % Branch % Funcs % Lines
🟢 total 56.66 (0.02) 38.66 (0.06) 35.96 (0) 56.91 (0.02)
🔴 app/client/src/ce/constants/messages.ts 77.78 (-0.02) 100 (0) 34.26 (-0.05) 81.74 (0.02)
🟢 app/client/src/components/editorComponents/JSResponseView.tsx 58.44 (8.44) 31.88 (6) 8.33 (0) 60.81 (8.96)
🟢 app/client/src/components/editorComponents/utils.ts 82.61 (49.28) 91.67 (91.67) 33.33 (33.33) 80 (60)
🔴 app/client/src/sagas/JSPaneSagas.ts 16.39 (-0.28) 1 (-0.02) 4.55 (0) 19.02 (-0.29)
🟢 app/client/src/utils/WorkerUtil.ts 89.76 (0) 72.55 (1.96) 100 (0) 93.33 (0)

@vercel
Copy link

vercel bot commented May 5, 2022 •
edited

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

Name Status Preview Updated
appsmith ✅ Ready (Inspect) Visit Preview May 19, 2022 at 11:26AM (UTC)

@ohansFavour ohansFavour marked this pull request as draft May 5, 2022 08:39
@github-actions github-actions bot added the Bug Something isn't working label May 5, 2022
@github-actions
Copy link

github-actions bot commented May 5, 2022

Unable to find test scripts. Please add necessary tests to the PR.

@ohansFavour ohansFavour force-pushed the fix/prevent-running-js-function-when-jsobject-is-loading branch from e002d74 to 264e564 Compare May 12, 2022 08:53
@ohansFavour ohansFavour marked this pull request as ready for review May 12, 2022 08:54
@github-actions github-actions bot added Actions Pod FE Coders Pod Issues related to users writing javascript in appsmith High This issue blocks a user from building or impacts a lot of users JS Objects Issues related to JS Objects Needs Triaging Needs attention from maintainers to triage Release labels May 12, 2022
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

3 similar comments
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@ohansFavour
Copy link
Contributor Author

/ok-to-test sha=264e564

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2312408113.
Workflow: Appsmith External Integration Test Workflow.
Commit: 264e564.
PR: 13592.

@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2312408113.
Commit: 264e564.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 1980.9 1931.89 1937.52 2045.21 1875.07 1937.52 1954.12 3.24 2.90
painting 11.14 10.24 6.96 7.69 17.51 10.24 10.71 39.03 34.92
rendering 626.67 620.06 617.52 635.48 638.86 626.67 627.72 1.49 1.33
SELECT_WIDGET_SELECT_OPTION
scripting 360.66 236.33 239.47 398.51 296.81 296.81 306.36 23.60 21.11
painting 3.72 3.06 3.49 5.36 4.6 3.72 4.05 22.72 20.49
rendering 16.45 24.11 16.49 16.14 16.56 16.49 17.95 19.22 17.16

1 similar comment
@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2312408113.
Commit: 264e564.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 1980.9 1931.89 1937.52 2045.21 1875.07 1937.52 1954.12 3.24 2.90
painting 11.14 10.24 6.96 7.69 17.51 10.24 10.71 39.03 34.92
rendering 626.67 620.06 617.52 635.48 638.86 626.67 627.72 1.49 1.33
SELECT_WIDGET_SELECT_OPTION
scripting 360.66 236.33 239.47 398.51 296.81 296.81 306.36 23.60 21.11
painting 3.72 3.06 3.49 5.36 4.6 3.72 4.05 22.72 20.49
rendering 16.45 24.11 16.49 16.14 16.56 16.49 17.95 19.22 17.16

eco-monk
eco-monk reviewed May 13, 2022
View reviewed changes
Copy link
Contributor

@eco-monk eco-monk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any unit or cypress tests around this?

@ohansFavour
Copy link
Contributor Author

ohansFavour commented May 14, 2022 •
edited

Any unit or cypress tests around this?

@eco-monk I have added unit tests. Because of the nature of the issue, cypress tests can't be used here. Manually, testing this requires throttling the internet speed.

@ohansFavour ohansFavour force-pushed the fix/prevent-running-js-function-when-jsobject-is-loading branch from 264e564 to f99ace7 Compare May 14, 2022 09:57
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

1 similar comment
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2323625717.
Commit: f99ace7.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 2514.64 2431.94 2440.55 2571.97 2581.18 2514.64 2508.06 2.81 2.51
painting 29.27 12.98 20.06 22.19 19.81 20.06 20.86 28.00 25.02
rendering 898.7 950.52 821.45 906.35 890.32 898.7 893.47 5.20 4.65
SELECT_WIDGET_SELECT_OPTION
scripting 357.59 423.42 408.83 399.64 388.81 399.64 395.66 6.26 5.60
painting 5.63 15.99 6.1 7.27 8.65 7.27 8.73 48.34 43.30
rendering 21.09 22.1 25.15 21.63 27.59 22.1 23.51 11.78 10.55

1 similar comment
@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2323625717.
Commit: f99ace7.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 2514.64 2431.94 2440.55 2571.97 2581.18 2514.64 2508.06 2.81 2.51
painting 29.27 12.98 20.06 22.19 19.81 20.06 20.86 28.00 25.02
rendering 898.7 950.52 821.45 906.35 890.32 898.7 893.47 5.20 4.65
SELECT_WIDGET_SELECT_OPTION
scripting 357.59 423.42 408.83 399.64 388.81 399.64 395.66 6.26 5.60
painting 5.63 15.99 6.1 7.27 8.65 7.27 8.73 48.34 43.30
rendering 21.09 22.1 25.15 21.63 27.59 22.1 23.51 11.78 10.55

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

5 similar comments
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@rishabhrathod01 rishabhrathod01 removed their request for review May 14, 2022 21:17
eco-monk
eco-monk previously approved these changes May 16, 2022
View reviewed changes
@ramsaptami
Copy link
Contributor

ramsaptami commented May 19, 2022 •
edited

Test plan : https://github.com/appsmithorg/TestSmith/issues/1711

@ramsaptami ramsaptami added the Test Plan Approved Manual/Cypress tests covers changes made on the PR. Else, add skip-testPlan label if not applicable label May 19, 2022
@ohansFavour ohansFavour force-pushed the fix/prevent-running-js-function-when-jsobject-is-loading branch from f99ace7 to c3689dd Compare May 19, 2022 11:15
@ohansFavour
Copy link
Contributor Author

/ok-to-test sha=c3689dd

@ohansFavour ohansFavour requested a review from eco-monk May 19, 2022 11:16
@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2351585665.
Workflow: Appsmith External Integration Test Workflow.
Commit: c3689dd.
PR: 13592.

eco-monk
eco-monk approved these changes May 19, 2022
View reviewed changes
Copy link
Contributor

@eco-monk eco-monk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge conflicts resolved.

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2351585665.
Commit: c3689dd.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 2843.42 2732.36 2809.95 2877.26 2995.55 2843.42 2851.71 3.39 3.03
painting 23.56 15.14 15.75 21.4 19.07 19.07 18.98 19.02 17.02
rendering 1170.91 1191.58 1166.88 1207.19 1267.72 1191.58 1200.86 3.40 3.04
SELECT_WIDGET_SELECT_OPTION
scripting 429.65 453.52 409.75 456.24 431.37 431.37 436.11 4.39 3.93
painting 7.45 7.74 12.69 4.53 9.9 7.74 8.46 35.93 32.15
rendering 30.3 25.76 25.17 31.84 24.92 25.76 27.6 11.70 10.47

1 similar comment
@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2351585665.
Commit: c3689dd.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_WIDGET_MENU_OPEN
scripting 2843.42 2732.36 2809.95 2877.26 2995.55 2843.42 2851.71 3.39 3.03
painting 23.56 15.14 15.75 21.4 19.07 19.07 18.98 19.02 17.02
rendering 1170.91 1191.58 1166.88 1207.19 1267.72 1191.58 1200.86 3.40 3.04
SELECT_WIDGET_SELECT_OPTION
scripting 429.65 453.52 409.75 456.24 431.37 431.37 436.11 4.39 3.93
painting 7.45 7.74 12.69 4.53 9.9 7.74 8.46 35.93 32.15
rendering 30.3 25.76 25.17 31.84 24.92 25.76 27.6 11.70 10.47

@ohansFavour ohansFavour merged commit cad1620 into release May 19, 2022
@ohansFavour ohansFavour deleted the fix/prevent-running-js-function-when-jsobject-is-loading branch May 19, 2022 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eco-monk eco-monk eco-monk approved these changes

@ApekshaBhosale ApekshaBhosale Awaiting requested review from ApekshaBhosale

Assignees
No one assigned
Labels
Bug Something isn't working FE Coders Pod Issues related to users writing javascript in appsmith High This issue blocks a user from building or impacts a lot of users JS Objects Issues related to JS Objects Needs Triaging Needs attention from maintainers to triage Release 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
3 participants
@ohansFavour @ramsaptami @eco-monk