fix: deprecate JS object confirm before calling #32527

rishabhrathod01 · 2024-04-09T12:46:39Z

Description

We deprecate confirm before calling for JSObjects

For new JSObject, confirm before calling option won't be visible
For already present JSObject, only for function where confirm before calling was enabled will be visible and once turned off will not be visible anymore.

Fixes #29248

Test scenarios

For an application where JSObject confirmed before calling was enabled, it should work same.
For new JSObject, confirm before calling option shouldn't be visible.
Once user disable confirm before calling it should not be visible anymore.
Deprecation warning on info.

Automation

/ok-to-test tags="@tag.All"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/8654538946
Commit: 6ac458b
Cypress dashboard url: Click here!

coderabbitai · 2024-04-10T21:43:34Z

Walkthrough

The changes across various files primarily focus on the removal and simplification of features related to the "Confirm Before Calling" functionality in JavaScript functions and associated test adjustments. This includes removing redundant user confirmation steps, streamlining test flows, and updating component visibility settings related to function execution settings.

Changes

Files	Change Summary
`JSOnLoad2_Spec.ts`, `JSOnLoad4_Spec.ts`	Restructured test scenarios related to JS Objects OnLoad Actions and updated function verification logic.
`JSEditor.ts`, `JSFunctionSettings.tsx`, `constants.ts`	Modified function settings handling, simplified logic, and removed "Confirm Before Calling" settings.
`messages.ts`, `RemoveConfirmBeforeCallingDialog.tsx`	Added deprecation warning constants and implemented a modal dialog for confirming remove actions.

Assessment against linked issues

Objective	Addressed	Explanation
Sunset "Confirm Before Calling" functionality (#29248)	✅	The changes align with the objective of sunsetting the "Confirm Before Calling" feature.
Address test failures caused by changes in PR (#30378)	❓	The PR does not explicitly mention addressing specific test failures related to PR #30378. Further details are needed to confirm if all failures are resolved.

Possibly related issues

Issue [Bug]: Function execution does not wait for user confirmation before running and does not display response when run on page laod #13646: This issue could be linked as it discusses the behavior of JS functions regarding user confirmation and page load, which are affected by the PR changes.
Issue [Bug]: Function settings of a JS Object are not obeyed when copied/moved to another page #31475: Although not directly addressed, the changes related to function settings in this PR could impact the behavior described in this issue, suggesting a potential link for further investigation.

Recent Review Details

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 499bc03 and 6ac458b.

Files selected for processing (7)

app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad2_Spec.ts (4 hunks)
app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad4_Spec.ts (3 hunks)
app/client/cypress/support/Pages/JSEditor.ts (3 hunks)
app/client/src/ce/constants/messages.ts (1 hunks)
app/client/src/pages/Editor/JSEditor/JSFunctionSettings.tsx (8 hunks)
app/client/src/pages/Editor/JSEditor/RemoveConfirmBeforeCallingDialog.tsx (1 hunks)
app/client/src/pages/Editor/JSEditor/constants.ts (2 hunks)

Files skipped from review as they are similar to previous changes (3)

app/client/cypress/support/Pages/JSEditor.ts
app/client/src/pages/Editor/JSEditor/JSFunctionSettings.tsx
app/client/src/pages/Editor/JSEditor/constants.ts

Additional comments not posted (6)

app/client/src/pages/Editor/JSEditor/RemoveConfirmBeforeCallingDialog.tsx (1)

22-58: The implementation of RemoveConfirmationModal is clean and follows best practices for React component structure. The use of constants for text values enhances maintainability.

app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad4_Spec.ts (2)

34-55: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [16-52]

The test case structure is consistent and uses appropriate Cypress commands and custom commands from jsEditor for creating and verifying JSObject functions.

68-77: The implementation of VerifyFunctionDropdown correctly handles the dropdown interactions and validations using Cypress commands.

app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad2_Spec.ts (1)

153-170: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [13-167]

The modifications to the test scenarios in JSOnLoad2_Spec.ts are aligned with the PR objectives and correctly focus on verifying JSObject functionalities without the "confirm before calling" feature.

app/client/src/ce/constants/messages.ts (2)

627-632: Consider adding localization support for UI messages to enhance user experience across different languages.

627-632: The messages for the deprecation of the "confirm before calling" feature are clear and effectively communicate the irreversible nature of the action.

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>.
- 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 generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @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.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

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

Actionable comments posted: 2

coderabbitai · 2024-04-10T21:49:12Z

app/client/src/pages/Editor/JSEditor/constants.ts

+export const CONFIRM_BEFORE_CALLING_HEADING = {
+  text: "Confirm before calling ",
+  hasInfo: true,
+  info: `Ask for confirmation before executing function. 
+  (This setting is deprecated and will be removed in the future.)`,
+  key: "run_before_calling",
+  hidden: true,
+};


The new CONFIRM_BEFORE_CALLING_HEADING object is well-defined with appropriate properties. However, ensure that the deprecation note is clear about when this feature will be removed to avoid confusion.

coderabbitai · 2024-04-10T21:49:12Z

app/client/src/pages/Editor/JSEditor/constants.ts

-    label: "No",
-    value: "false",
-  },
+  CONFIRM_BEFORE_CALLING_HEADING,


Adding CONFIRM_BEFORE_CALLING_HEADING directly to SETTINGS_HEADINGS without conditions might not be ideal since it's deprecated. Consider adding a condition to include it only if necessary.

- CONFIRM_BEFORE_CALLING_HEADING, + ...(shouldIncludeConfirmBeforeCalling ? [CONFIRM_BEFORE_CALLING_HEADING] : []),

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change

CONFIRM_BEFORE_CALLING_HEADING,

...(shouldIncludeConfirmBeforeCalling ? [CONFIRM_BEFORE_CALLING_HEADING] : []),

rishabhrathod01 · 2024-04-11T07:01:21Z

/build-deploy-preview env=release skip-tests=true

github-actions · 2024-04-11T07:01:42Z

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8642786354.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: release.
PR: 32527.
recreate: .

github-actions · 2024-04-11T07:14:20Z

Deploy-Preview-URL: https://appsmith-kpreziifs-get-appsmith.vercel.app

🚨 Note: The release environment runs EE code, so using a frontend-only DP on this repo, will
likely behave unexpectedly. Consider using a full DP instead.
Learn more.

app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad1_Spec.ts

app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad2_Spec.ts

app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JsOnLoad3_Spec.ts

app/client/cypress/support/Pages/JSEditor.ts

sneha122 · 2024-04-11T07:47:08Z

/build-deploy-preview skip-tests=true

github-actions · 2024-04-11T07:47:29Z

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8643279346.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 32527.
recreate: .

github-actions · 2024-04-11T07:59:58Z

Deploy-Preview-URL: https://ce-32527.dp.appsmith.com

sneha122 · 2024-04-11T08:15:54Z

@rishabhrathod01 Tested the above test scenarios and have a couple of observations:

I see this Log in to comment which looks irrelevant to this PR but I did not see this on release or prod but only on DP (Looks like it was only there for release-ee DP, we can prolly ignore it)

Can we please align these two columns in center now, as third column does not exist anymore? (Quality issue)

Deprecation warning is seen only on hovering over ? in the header, but as soon as I turn toggle the switch off this option disappears, if a user has not seen the deprecation warning, do we think it will confuse the user? As to why the option suddenly disappeared. Perhaps we can show the warning once the option disappears to let the user know that option went away because deprecated

Screen.Recording.2024-04-11.at.1.39.35.PM.mov

rishabhrathod01 · 2024-04-11T08:30:45Z

@sneha122

I see this Log in to comment which looks irrelevant to this PR but I did not see this on release or prod but only on DP (Looks like it was only there for release-ee DP, we can prolly ignore it)

This is a vercel feature on deploy preview to add comments once we log in. For creating DP pointing to release we use vercel hence this popup is visible.

Can we please align these two columns in center now, as third column does not exist anymore? (Quality issue)

I would like to keep this UI consistent until we remove this feature. Cases where even a single confirm before calling option is enabled we will see the column and position would move for both of the cases. IMO, it is better to keep the position consistent as few JSObject settings shouldn’t look different from others for users. Thoughts?

Deprecation warning is seen only on hovering over ? in the header, but as soon as I turn toggle the switch off this option disappears, if a user has not seen the deprecation warning, do we think it will confuse the user? As to why the option suddenly disappeared.

Yeah, I agree. Ideally, we should show a confirmation before the toggle itself but as we don't have the UI for that.

Perhaps we can show the warning once the option disappears to let the user know that option went away because deprecated

Any idea, Where shall we show the warning message?

sneha122 · 2024-04-11T08:46:51Z

This is a vercel feature on deploy preview to add comments once we log in. For creating DP pointing to release we use vercel hence this popup is visible.

Got it

I would like to keep this UI consistent until we remove this feature. Cases where even a single confirm before calling option is enabled we will see the column and position would move for both of the cases. IMO, it is better to keep the position consistent as few JSObject settings shouldn’t look different from others for users. Thoughts?

Agree with you, let's do that once we remove this feature completely, but in meantime can we please confirm the UI with someone from design team? IMO this is a small UI issue but still a quality issue, so let's confirm with design team once

Any idea, Where shall we show the warning message?

Got it for now, we can probably show a warning toast message to just let the user know why it disappeared

rishabhrathod01 · 2024-04-12T05:32:12Z

/build-deploy-preview skip-tests=true

github-actions · 2024-04-12T05:32:33Z

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8657477247.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 32527.
recreate: .

github-actions · 2024-04-12T05:45:23Z

Deploy-Preview-URL: https://ce-32527.dp.appsmith.com

trishaanand · 2024-04-12T07:19:03Z

app/client/cypress/e2e/Regression/ServerSide/Datasources/Oracle_Spec.ts

@@ -446,11 +445,7 @@ WHERE aircraft_type = 'Passenger Plane'`;
    propPane.EnterJSContext("onClick", `{{Query1.run()}}`);
    deployMode.DeployApp(locators._widgetInDeployed(draggableWidgets.TABLE));
    agHelper.ClickButton("Submit");
-    jsEditor.ConfirmationClick("No"); //Handling both No & Yes from confirmation dialog


@rishabhrathod01 @sneha122 : Aren't these lines asserting if the confirm before calling feature of the API/Query is working? This wasn't about JS Object's feature. Can you please confirm if this was the correct change?

@trishaanand you are correct. This code was misleading as it uses jsEditor class method to assert ApiEditor confirmation. I will undo this and will use generic or specific class names here to avoid this confusion in the future.

Thanks @trishaanand for catching these 🚀, Missed during the review because I mainly relied on the fact that since tests are passing in CI, we should be okay with the changes, I will be more diligent about test cases review too from now onwards.

Adding the check back in this PR #32957

app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/JSFunctionExecution_spec.ts

app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad2_Spec.ts

app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad4_Spec.ts

trishaanand · 2024-04-12T07:27:12Z

app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JsOnLoad3_Spec.ts

-        //agHelper.WaitUntilToastDisappear('The action "Quotes" has failed');No toast appears!
-
-        agHelper.AssertElementAbsence(jsEditor._dialogBody("WhatTrumpThinks")); //Since JS call is NO, dependent API confirmation should not appear
-
        agHelper.RefreshPage("getConsolidatedData");


@rishabhrathod01 @sneha122 : Any reason to refresh the page considering the last line is to deploy the app. We should already have the page open at this point right?

@trishaanand I will need to dig deeper here, i am not sure why was this present earlier. Let me check and get back on this one.

@vsvamsi1 could you please check if we could skip the refresh page that was added here. https://github.com/appsmithorg/appsmith/pull/30506/files#diff-25b467f92a60af5a65ad19e00f03aca3e9aa2c9ee324b4744cf15d1e233473e7

app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JsOnLoad3_Spec.ts

trishaanand · 2024-04-12T07:30:25Z

app/client/cypress/support/Pages/JSEditor.ts

-    onLoad = true,
-    bfrCalling = true,
-  ) {
+  public VerifyAsyncFuncSettings(funName: string, onLoad = true) {
    // this.agHelper.AssertExistingToggleState(this._functionSetting(Cypress.env("MESSAGES").JS_SETTINGS_ONPAGELOAD()), onLoad)


@rishabhrathod01 @sneha122 : Could we please get rid of these commented lines. Thanks!

Sure, I will remove the commented code in a new PR.

fix: deprecate JS object confirm before calling

0a723ee

rishabhrathod01 added the ok-to-test Required label for CI label Apr 9, 2024

fix cypress test

8a4831b

rishabhrathod01 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Apr 9, 2024

rishabhrathod01 added 2 commits April 10, 2024 20:05

fix cypress test

489e76f

fix unused imports

57104a8

rishabhrathod01 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Apr 10, 2024

fix cypress test

499bc03

rishabhrathod01 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Apr 10, 2024

rishabhrathod01 marked this pull request as ready for review April 10, 2024 21:43

rishabhrathod01 requested review from AmanAgarwal041 and sneha122 April 10, 2024 21:45

coderabbitai bot reviewed Apr 10, 2024

View reviewed changes

sneha122 reviewed Apr 11, 2024

View reviewed changes

app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad1_Spec.ts Show resolved Hide resolved

sneha122 reviewed Apr 11, 2024

View reviewed changes

app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad2_Spec.ts Show resolved Hide resolved

sneha122 reviewed Apr 11, 2024

View reviewed changes

app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JsOnLoad3_Spec.ts Show resolved Hide resolved

sneha122 reviewed Apr 11, 2024

View reviewed changes

app/client/cypress/support/Pages/JSEditor.ts Show resolved Hide resolved

Add confirmation dialog for disable confirmBeforeCalling

6ac458b

rishabhrathod01 self-assigned this Apr 11, 2024

rishabhrathod01 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Apr 11, 2024

sneha122 approved these changes Apr 12, 2024

View reviewed changes

rishabhrathod01 merged commit 6a26602 into release Apr 12, 2024
83 checks passed

rishabhrathod01 deleted the chore/deprecateJSConfirmBeforeCalling branch April 12, 2024 06:48