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

feat: Added warning message in response pane while prepared statement is on #36407

Merged
merged 6 commits into from
Sep 23, 2024
Merged

feat: Added warning message in response pane while prepared statement is on #36407

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
46410cc
feat: Added warning message in response pane while prepared statement…
albinAppsmith Sep 19, 2024
5ed0dfa
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Sep 19, 2024
cc19110
feat: Added unit test for prepared statement warning
albinAppsmith Sep 19, 2024
1ad67e1
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Sep 19, 2024
9d451ab
fix: codemirror suggetion
albinAppsmith Sep 19, 2024
debc9f3
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/client/src/ce/constants/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2502,3 +2502,9 @@ export const EMPTY_DATASOURCE_TOOLTIP_SIDEBUTTON = () =>
"Create a datasource to power your app with data.";

export const FIELD_REQUIRED_MESSAGE = () => `This field is required`;

export const PREPARED_STATEMENT_WARNING = {
MESSAGE: () =>
"Prepared Statements are currently enabled, which may be causing the query error. Turn them off and try running the query again",
LINK: () => "Open settings",
};
Comment on lines +2507 to +2511
Copy link
Contributor

Choose a reason for hiding this comment

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

Well done on introducing the PREPARED_STATEMENT_WARNING constant

You've defined a new constant to display a warning message when prepared statements are enabled and an error occurs. This aligns perfectly with the PR objectives and enhances user awareness during error scenarios.

However, let's consider a small improvement to keep our code consistent and maintainable.

To maintain consistency with other message constants in this file, it would be better to define MESSAGE and LINK as constant strings rather than arrow functions, unless dynamic computation is required. Many existing constants use direct string assignments.

Here's how you might refactor the code:

-export const PREPARED_STATEMENT_WARNING = {
-  MESSAGE: () =>
-      "Prepared Statements are currently enabled, which may be causing the query error. Turn them off and try running the query again",
-  LINK: () => "Open settings",
+export const PREPARED_STATEMENT_WARNING_MESSAGE =
+  "Prepared Statements are currently enabled, which may be causing the query error. Turn them off and try running the query again.";
+export const PREPARED_STATEMENT_WARNING_LINK = "Open settings";

This change makes the constants consistent with the rest of the file and simplifies their usage.

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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const PREPARED_STATEMENT_WARNING = {
MESSAGE: () =>
"Prepared Statements are currently enabled, which may be causing the query error. Turn them off and try running the query again",
LINK: () => "Open settings",
};
export const PREPARED_STATEMENT_WARNING_MESSAGE =
"Prepared Statements are currently enabled, which may be causing the query error. Turn them off and try running the query again.";
export const PREPARED_STATEMENT_WARNING_LINK = "Open settings";

39 changes: 37 additions & 2 deletions app/client/src/pages/Editor/QueryEditor/QueryResponseTab.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useState } from "react";
import React, { useCallback, useState } from "react";
import { useDispatch, useSelector } from "react-redux";
import ReactJson from "react-json-view";
import {
Expand All @@ -13,7 +13,12 @@ import LogAdditionalInfo from "components/editorComponents/Debugger/ErrorLogs/co
import LogHelper from "components/editorComponents/Debugger/ErrorLogs/components/LogHelper";
import LOG_TYPE from "entities/AppsmithConsole/logtype";
import { JsonWrapper } from "components/editorComponents/Debugger/ErrorLogs/components/LogCollapseData";
import { Callout, Flex, SegmentedControl } from "@appsmith/ads";
import {
Callout,
Flex,
SegmentedControl,
type CalloutLinkProps,
} from "@appsmith/ads";
import styled from "styled-components";
import { DEBUGGER_TAB_KEYS } from "components/editorComponents/Debugger/helpers";
import AnalyticsUtil from "ee/utils/AnalyticsUtil";
Expand All @@ -32,6 +37,12 @@ import ActionExecutionInProgressView from "components/editorComponents/ActionExe
import { EditorTheme } from "components/editorComponents/CodeEditor/EditorConfig";
import BindDataButton from "./BindDataButton";
import { getQueryPaneDebuggerState } from "selectors/queryPaneSelectors";
import { setQueryPaneConfigSelectedTabIndex } from "actions/queryPaneActions";
import { EDITOR_TABS } from "constants/QueryEditorConstants";
import {
createMessage,
PREPARED_STATEMENT_WARNING,
} from "ee/constants/messages";

const HelpSection = styled.div``;

Expand Down Expand Up @@ -151,6 +162,7 @@ const QueryResponseTab = (props: Props) => {

let error = runErrorMessage;
let hintMessages: Array<string> = [];
let showPreparedStatementWarning = false;
// TODO: Fix this the next time the file is edited
// eslint-disable-next-line @typescript-eslint/no-explicit-any
let output: Record<string, any>[] | null = null;
Expand Down Expand Up @@ -189,8 +201,26 @@ const QueryResponseTab = (props: Props) => {
error = "";
hintMessages = actionResponse.messages;
}

const { pluginSpecifiedTemplates } =
currentActionConfig.actionConfiguration;

if (error && pluginSpecifiedTemplates[0].value === true) {
showPreparedStatementWarning = true;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure pluginSpecifiedTemplates is defined before accessing

To prevent potential runtime errors, it's important to check if pluginSpecifiedTemplates is defined and has elements before accessing pluginSpecifiedTemplates[0].value. This validation will make your code more robust and error-resistant.

Consider applying this change:

        const { pluginSpecifiedTemplates } =
          currentActionConfig.actionConfiguration;

-        if (error && pluginSpecifiedTemplates[0].value === true) {
+        if (
+          error &&
+          pluginSpecifiedTemplates &&
+          pluginSpecifiedTemplates.length > 0 &&
+          pluginSpecifiedTemplates[0].value === true
+        ) {
          showPreparedStatementWarning = true;
        }
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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { pluginSpecifiedTemplates } =
currentActionConfig.actionConfiguration;
if (error && pluginSpecifiedTemplates[0].value === true) {
showPreparedStatementWarning = true;
}
const { pluginSpecifiedTemplates } =
currentActionConfig.actionConfiguration;
if (
error &&
pluginSpecifiedTemplates &&
pluginSpecifiedTemplates.length > 0 &&
pluginSpecifiedTemplates[0].value === true
) {
showPreparedStatementWarning = true;
}


const navigateToSettings = useCallback(() => {
dispatch(setQueryPaneConfigSelectedTabIndex(EDITOR_TABS.SETTINGS));
}, []);

const preparedStatementCalloutLinks: CalloutLinkProps[] = [
{
onClick: navigateToSettings,
children: createMessage(PREPARED_STATEMENT_WARNING.LINK),
},
];

if (isRunning) {
return (
<ActionExecutionInProgressView
Expand All @@ -202,6 +232,11 @@ const QueryResponseTab = (props: Props) => {

return (
<ResponseContentWrapper isError={!!error}>
{showPreparedStatementWarning && (
<Callout kind="warning" links={preparedStatementCalloutLinks}>
{createMessage(PREPARED_STATEMENT_WARNING.MESSAGE)}
</Callout>
)}
{error && (
<ResponseTabErrorContainer>
<ResponseTabErrorContent>
Expand Down
Loading