[FIX] side_panel: cf rule range bugs

Steps to reproduce :-
Bug 1 : 
- Change the range to ""  
- Focus out => selection input should have a red border even without clicking on
  confirm
Bug 2 : 
- Enter "" as range and save
- Now change it to a valid range and confirm => red border and error message do
  not disappear even if you confirm
Bug 3 :
- Enter an invalid range (eg. A2:B3:C3) and save
- Now change it to a valid range and confirm => red border disappears but the
  error message does not disappear
Bug 4 :
- Enter an invalid range (with invalid sheet name) => red border appears
  (correct), but there is no error message and you can still save the cf rule

With this commit, the following amendments have been made:

Earlier, the condition checked to display selection input border in red
considered 'empty range' as a valid input. That condition is changed to
consider it as an invalid range.

The condition to check for invalid ranges, while dispatching command result
'InvalidRange', is altered to use the getter `isRangeValid` so that cf rules
with ranges containing invalid sheet name cannot be saved.

To ensure that the error message appears if and only if the input is invalid
(i.e. it disappears on entering a valid range as input, even without clicking on
confirm or save), error messages for invalid range are now handled from
the `onRangesChanged` method instead of `saveConditionalFormat` method.

Task ID : 3651293

closes #3446

X-original-commit: 4a956f3
Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>

src/components/selection_input/selection_input.ts

@@ -122,7 +122,7 @@ export class SelectionInput extends Component<Props, SpreadsheetChildEnv> {
       : [];
     return ranges.map((range) => ({
       ...range,
-      isValidRange: range.xc === "" || this.env.model.getters.isRangeValid(range.xc),
+      isValidRange: range.xc !== "" && this.env.model.getters.isRangeValid(range.xc),
     }));
   }
 

src/components/side_panel/conditional_formatting/conditional_formatting.ts

@@ -1,6 +1,6 @@
 import { Component, onWillUpdateProps, useExternalListener, useState } from "@odoo/owl";
 import { DEFAULT_COLOR_SCALE_MIDPOINT_COLOR } from "../../../constants";
-import { colorNumberString, rangeReference } from "../../../helpers/index";
+import { colorNumberString } from "../../../helpers/index";
 import { _t } from "../../../translation";
 import {
   CancelledReason,
@@ -385,10 +385,6 @@ export class ConditionalFormattingPanel extends Component<Props, SpreadsheetChil
     return this.env.model.getters.getConditionalFormats(this.env.model.getters.getActiveSheetId());
   }
 
-  get isRangeValid(): boolean {
-    return this.state.errors.includes(CommandResult.EmptyRange);
-  }
-
   errorMessage(error: CancelledReason): string {
     return CfTerms.Errors[error] || CfTerms.Errors.Unexpected;
   }
@@ -447,9 +443,10 @@ export class ConditionalFormattingPanel extends Component<Props, SpreadsheetChil
 
   saveConditionalFormat() {
     if (this.state.currentCF) {
-      const invalidRanges = this.state.currentCF.ranges.some((xc) => !xc.match(rangeReference));
-      if (invalidRanges) {
-        this.state.errors = [CommandResult.InvalidRange];
+      if (
+        this.state.errors.includes(CommandResult.EmptyRange) ||
+        this.state.errors.includes(CommandResult.InvalidRange)
+      ) {
         return;
       }
       const sheetId = this.env.model.getters.getActiveSheetId();
@@ -596,6 +593,15 @@ export class ConditionalFormattingPanel extends Component<Props, SpreadsheetChil
   }
 
   onRangesChanged(ranges: string[]) {
+    if (ranges.length === 0) {
+      this.state.errors = [CommandResult.EmptyRange];
+      return;
+    }
+    if (ranges.some((xc) => !this.env.model.getters.isRangeValid(xc))) {
+      this.state.errors = [CommandResult.InvalidRange];
+      return;
+    }
+    this.state.errors = [];
     if (this.state.currentCF) {
       this.state.currentCF.ranges = ranges;
     }

src/components/side_panel/conditional_formatting/conditional_formatting.xml

@@ -37,7 +37,6 @@
               <SelectionInput
                 ranges="state.currentCF.ranges"
                 class="'o-range'"
-                isInvalid="isRangeValid"
                 onSelectionChanged="(ranges) => this.onRangesChanged(ranges)"
                 required="true"
               />

tests/components/conditional_formatting.test.ts

@@ -342,7 +342,36 @@ describe("UI of conditional formats", () => {
       expect(dispatch).not.toHaveBeenCalledWith("ADD_CONDITIONAL_FORMAT");
       const errorString = document.querySelector(selectors.error);
       expect(errorString!.textContent).toBe("The range is invalid");
+
+      setInputValueAndTrigger(selectors.ruleEditor.range, "s!A1", "change");
+      await click(fixture, selectors.buttonSave);
+      expect(dispatch).not.toHaveBeenCalledWith("ADD_CONDITIONAL_FORMAT");
+      const errorString2 = document.querySelector(selectors.error);
+      expect(errorString2!.textContent).toBe("The range is invalid");
     });
+
+    test("display error message if and only if invalid range", async () => {
+      await click(fixture, selectors.buttonAdd);
+      await nextTick();
+      const dispatch = spyDispatch(parent);
+
+      setInputValueAndTrigger(selectors.ruleEditor.range, "", "input");
+      await click(fixture, selectors.buttonSave);
+      expect(dispatch).not.toHaveBeenCalledWith("ADD_CONDITIONAL_FORMAT");
+      const errorString = document.querySelector(selectors.error);
+      expect(errorString!.textContent).toBe("A range needs to be defined");
+
+      setInputValueAndTrigger(selectors.ruleEditor.range, "A1", "input");
+      await nextTick();
+      expect(document.querySelector(selectors.error)).toBe(null);
+
+      setInputValueAndTrigger(selectors.ruleEditor.range, "s!A1", "input");
+      await click(fixture, selectors.buttonSave);
+      expect(dispatch).not.toHaveBeenCalledWith("ADD_CONDITIONAL_FORMAT");
+      const errorString1 = document.querySelector(selectors.error);
+      expect(errorString1!.textContent).toBe("The range is invalid");
+    });
+
     test("displayed range is updated if range changes", async () => {
       const previews = document.querySelectorAll(selectors.listPreview);
       expect(previews[0].querySelector(selectors.description.range)!.textContent).toBe("A1:A2");
@@ -687,7 +716,6 @@ describe("UI of conditional formats", () => {
     await nextTick();
     setInputValueAndTrigger(selectors.ruleEditor.range, "", "input");
     await nextTick();
-    await click(fixture, selectors.buttonSave);
     expect(errorMessages()).toEqual(["A range needs to be defined"]);
     expect(fixture.querySelector(selectors.ruleEditor.range)?.className).toContain("o-invalid");
   });

tests/components/selection_input.test.ts

@@ -401,10 +401,14 @@ describe("Selection Input", () => {
     expect(fixture.querySelectorAll("input")[0].value).toBe("B1");
     expect(fixture.querySelectorAll("input")[0].getAttribute("style")).not.toBe("color: #000;");
     expect(fixture.querySelectorAll("input")[0].classList).not.toContain("o-invalid");
-  });
-  test("don't show red border initially", async () => {
-    await createSelectionInput();
-    expect(fixture.querySelectorAll("input")[0].classList).not.toContain("o-invalid");
+    await writeInput(0, "");
+    expect(fixture.querySelectorAll("input")[0].value).toBe("");
+    expect(fixture.querySelectorAll("input")[0].getAttribute("style")).toBe("color: #000;");
+    expect(fixture.querySelectorAll("input")[0].classList).toContain("o-invalid");
+    await writeInput(0, "s!A1");
+    expect(fixture.querySelectorAll("input")[0].value).toBe("s!A1");
+    expect(fixture.querySelectorAll("input")[0].getAttribute("style")).toBe("color: #000;");
+    expect(fixture.querySelectorAll("input")[0].classList).toContain("o-invalid");
   });
 
   test("pressing and releasing control has no effect on future clicks", async () => {