WD-8528 - feat: Display errors in Actions Panel #1691

vladimir-cucu · 2024-01-29T20:21:27Z

Done

Display inline error when trying to get actions for application.
Display error in toast when trying to execute action on units.

QA

To test the loading state of the button, you can add the following code at the beginning of executeAction function definition:

const executeActionPromise = new Promise((resolve, reject) => {
  setTimeout(() => {
    reject(new Error());
  }, 2000);
});
await executeActionPromise;

Details

https://warthogs.atlassian.net/browse/WD-8528

webteam-app · 2024-01-29T20:21:31Z

Demo starting at https://juju-dashboard-1691.demos.haus

src/panels/ActionsPanel/ActionsPanel.tsx

codecov · 2024-01-30T12:17:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aa059d2) 95.04% compared to head (53e1808) 95.07%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1691      +/-   ##
==========================================
+ Coverage   95.04%   95.07%   +0.03%     
==========================================
  Files         182      183       +1     
  Lines        5325     5359      +34     
  Branches     1545     1555      +10     
==========================================
+ Hits         5061     5095      +34     
  Misses        243      243              
  Partials       21       21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vladimir-cucu · 2024-01-30T13:53:32Z

src/panels/ActionsPanel/ActionsPanel.tsx

@@ -211,12 +244,21 @@ export default function ActionsPanel(): JSX.Element {
              cancelButtonLabel={Label.CANCEL_BUTTON}
              confirmButtonLabel={Label.CONFIRM_BUTTON}
              confirmButtonAppearance="positive"
-              onConfirm={() => {
+              onConfirm={(event: React.MouseEvent<HTMLElement, MouseEvent>) => {
+                event.nativeEvent.stopImmediatePropagation();


This line of code (and maybe similar lines in other places) might become redundant in the case this gets implemented.

Thanks for filing that issue. I think it would be good to leave a comment here about why this stopImmediatePropagation is necessary and probably link to the issue you filed as well.

huwshimi

I've ended up making quite a few small comments so I'll do another review of this once those changes have been made.

huwshimi · 2024-01-30T22:26:23Z

src/panels/ActionsPanel/ActionsPanel.tsx

@@ -119,9 +145,16 @@ export default function ActionsPanel(): JSX.Element {
            setActionData(actions.results[0].actions);
          }
          setFetchingActionData(false);
+          setInlineErrors((prevInlineErrors) => [null, prevInlineErrors[1]]);


This case (and the other setInlineErrors() calls below) would lead to bugs if the total number of errors in the tuple changed (e.g. if a third error was added it would get lost during this call). Instead we should only update the slot that matches the error we're concerned with.

If you're concerned about mutating state then you could do something like:

errors = [... prevInlineErrors]; errors[0] = null;

huwshimi · 2024-01-30T22:28:45Z

src/panels/ActionsPanel/ActionsPanel.tsx

@@ -211,12 +244,21 @@ export default function ActionsPanel(): JSX.Element {
              cancelButtonLabel={Label.CANCEL_BUTTON}
              confirmButtonLabel={Label.CONFIRM_BUTTON}
              confirmButtonAppearance="positive"
-              onConfirm={() => {
+              onConfirm={(event: React.MouseEvent<HTMLElement, MouseEvent>) => {
+                event.nativeEvent.stopImmediatePropagation();


Thanks for filing that issue. I think it would be good to leave a comment here about why this stopImmediatePropagation is necessary and probably link to the issue you filed as well.

huwshimi · 2024-01-30T22:34:49Z

src/panels/ActionsPanel/ActionsPanel.tsx

-                  console.error(Label.EXECUTE_ACTION_ERROR, error),
-                );
-                handleRemovePanelQueryParams();
+                executeAction()


I think this should clear the errors here in case the user submits the form again. Also, depending on how long this action takes there could be a pause before the panel closes. You could show a loading state in the button. You'd need a new state (e.g. "const [saving, setSaving] = useState(false);".

Suggested change

executeAction()

setSaving(true);

executeAction()

You'll also need to update the button on line 287 to be <ActionButton and then give it the props disabled={saving} loading={saving}.

huwshimi · 2024-01-30T22:35:28Z

src/panels/ActionsPanel/ActionsPanel.tsx

+                  .catch((error) => {
+                    setInlineErrors((prevInlineError) => [


If you set the saving state when the action starts then you'll need to update it again here:

Suggested change

.catch((error) => {

setInlineErrors((prevInlineError) => [

.catch((error) => {

setSaving(false);

setInlineErrors((prevInlineError) => [

huwshimi · 2024-01-30T22:38:24Z

src/panels/ActionsPanel/ActionsPanel.tsx

          return;
        })
-        .catch((error) => console.error(Label.GET_ACTIONS_ERROR, error));
+        .catch((error) => {
+          setInlineErrors((prevInlineErrors) => [


Here also we need to only update a single error slot so we don't accidentally remove errors if the number of errors in the tuple changes.

huwshimi · 2024-01-30T22:39:02Z

src/panels/ActionsPanel/ActionsPanel.tsx

+                      prevInlineError[0],
+                      Label.EXECUTE_ACTION_ERROR,


Also need to only update a single slot in the tuple.

huwshimi · 2024-01-30T22:42:30Z

src/panels/ConfigPanel/ConfigPanel.tsx

@@ -392,7 +395,7 @@ export default function ConfigPanel(): JSX.Element {

            <div className="config-panel__list">
              {formErrors


I think this should also have .some(... or at the very least check that this isn't an empty array.

huwshimi · 2024-01-30T22:44:04Z

src/panels/ConfigPanel/ConfigPanel.tsx

+const generateErrors = (errors: string[], scrollArea: HTMLElement | null) => (
+  <ScrollOnRender scrollArea={scrollArea}>
+    {errors.map((error) => (
+      <Notification key={error} severity="negative">
+        {error}
+      </Notification>
+    ))}
+  </ScrollOnRender>
+);


Possibly in a follow-up branch this could become a shared component, something like PanelErrors and include the .some(...) like you do in the other panel, as we're doing the same thing in a number of places.

huwshimi · 2024-01-30T22:51:13Z

src/panels/ActionsPanel/ActionsPanel.tsx

+        {inlineErrors.some((error) => error)
+          ? generateInlineErrors(inlineErrors, scrollArea.current)
+          : null}


This needs to be outside of the wrapping <p> as this is currently giving errors about nested <p> tags.

…n button

huwshimi

Thanks for all the changes. I've made a couple of small comments but this can land once those have been fixed.

huwshimi · 2024-01-31T22:44:06Z

src/panels/ActionsPanel/ActionsPanel.tsx

          return;
        })
-        .catch((error) => console.error(Label.GET_ACTIONS_ERROR, error));
+        .catch((error) => {


I think this also needs to call setFetchingActionData(false); so that the spinner hides when the error appears.

huwshimi · 2024-01-31T22:45:36Z

src/panels/PanelInlineErrors/PanelInlineErrors.tsx

+  scrollArea?: HTMLElement | null;
+};
+
+const PanelInlineErrors = ({


Nice, thanks for adding this.

huwshimi · 2024-01-31T22:47:08Z

src/panels/PanelInlineErrors/PanelInlineErrors.test.tsx

+
+import PanelInlineErrors from "./PanelInlineErrors";
+
+describe("PanelInlineErrors", () => {


I think it would be good to also test that the ScrollOnRender component does not appear if inlineErrors is null, an empty array or only contains falsey items.

huwshimi

Just one tiny comment.

src/panels/ActionsPanel/ActionsPanel.tsx

feat: Display errors in Actions Panel

2427148

vladimir-cucu added Review: QA needed Review: Code needed labels Jan 29, 2024

vladimir-cucu requested a review from huwshimi January 29, 2024 20:21

huwshimi reviewed Jan 29, 2024

View reviewed changes

src/panels/ActionsPanel/ActionsPanel.tsx Outdated Show resolved Hide resolved

fix: Display both errors inline

3faa736

vladimir-cucu added 2 commits January 30, 2024 15:08

test: Test error display in ActionsPanel

10c84dc

refactor: Use RegExp instead of hardcoded strings

8276a38

vladimir-cucu marked this pull request as ready for review January 30, 2024 13:22

vladimir-cucu requested a review from huwshimi January 30, 2024 13:22

vladimir-cucu commented Jan 30, 2024

View reviewed changes

huwshimi reviewed Jan 30, 2024

View reviewed changes

feat: Add PanelInlineErrors component, Add loading state to Run Actio…

dccd993

…n button

huwshimi approved these changes Jan 31, 2024

View reviewed changes

huwshimi added Review: QA +1 Review: Code +1 (with changes) and removed Review: QA needed Review: Code needed labels Jan 31, 2024

vladimir-cucu added 5 commits February 1, 2024 20:49

fix: Implement review comments

c64fe72

Merge branch 'main' into actions-panel-errors

b68ae53

test: Add PanelInlineErrors tests

d29ecb5

test: Add missing tests in ActionsPanel

f31338c

fix: Rename ActionsPanel error button

7ddaa3b

huwshimi approved these changes Feb 2, 2024

View reviewed changes

src/panels/ActionsPanel/ActionsPanel.tsx Show resolved Hide resolved

vladimir-cucu added 3 commits February 2, 2024 09:54

refactor: Add comment to refetch actions button

73cd5a0

refactor: Modify comment

6c48493

refactor: Modify comment

53e1808

vladimir-cucu merged commit 0559f36 into canonical:main Feb 2, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WD-8528 - feat: Display errors in Actions Panel #1691

WD-8528 - feat: Display errors in Actions Panel #1691

vladimir-cucu commented Jan 29, 2024 •

edited

Loading

webteam-app commented Jan 29, 2024

codecov bot commented Jan 30, 2024 •

edited

Loading

vladimir-cucu Jan 30, 2024

huwshimi Jan 30, 2024

huwshimi left a comment

huwshimi Jan 30, 2024

huwshimi Jan 30, 2024

huwshimi Jan 30, 2024

huwshimi Jan 30, 2024

huwshimi Jan 30, 2024

huwshimi Jan 30, 2024

huwshimi Jan 30, 2024

huwshimi Jan 30, 2024

huwshimi Jan 30, 2024

huwshimi left a comment

huwshimi Jan 31, 2024

huwshimi Jan 31, 2024

huwshimi Jan 31, 2024

huwshimi left a comment

		@@ -392,7 +395,7 @@ export default function ConfigPanel(): JSX.Element {

		<div className="config-panel__list">
		{formErrors


		import PanelInlineErrors from "./PanelInlineErrors";

		describe("PanelInlineErrors", () => {

WD-8528 - feat: Display errors in Actions Panel #1691

WD-8528 - feat: Display errors in Actions Panel #1691

Conversation

vladimir-cucu commented Jan 29, 2024 • edited Loading

Done

QA

Details

webteam-app commented Jan 29, 2024

codecov bot commented Jan 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huwshimi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huwshimi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huwshimi left a comment

Choose a reason for hiding this comment

vladimir-cucu commented Jan 29, 2024 •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading