[REV][FIX] side_panel: cf rule range bugs

This reverts commit f752519.
The task was not fixing bugs (in the sens of tracebacks or corrupted
behaviour) but rather seemingly backporting a feture introduced in
later versions. Since the implementation was not accounting for every
case and that it' not fixing any behaviour, just improving the user
experience, it's better to cancel it.

closes #3452

X-original-commit: 1c77cc3
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 } from "../../../helpers/index";
+import { colorNumberString, rangeReference } from "../../../helpers/index";
 import { _t } from "../../../translation";
 import {
   CancelledReason,
@@ -385,6 +385,10 @@ 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;
   }
@@ -443,10 +447,9 @@ export class ConditionalFormattingPanel extends Component<Props, SpreadsheetChil
 
   saveConditionalFormat() {
     if (this.state.currentCF) {
-      if (
-        this.state.errors.includes(CommandResult.EmptyRange) ||
-        this.state.errors.includes(CommandResult.InvalidRange)
-      ) {
+      const invalidRanges = this.state.currentCF.ranges.some((xc) => !xc.match(rangeReference));
+      if (invalidRanges) {
+        this.state.errors = [CommandResult.InvalidRange];
         return;
       }
       const sheetId = this.env.model.getters.getActiveSheetId();
@@ -593,15 +596,6 @@ 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,6 +37,7 @@
               <SelectionInput
                 ranges="state.currentCF.ranges"
                 class="'o-range'"
+                isInvalid="isRangeValid"
                 onSelectionChanged="(ranges) => this.onRangesChanged(ranges)"
                 required="true"
               />

tests/components/conditional_formatting.test.ts

@@ -342,36 +342,7 @@ 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");
@@ -716,6 +687,7 @@ 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

@@ -399,14 +399,10 @@ 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");
-    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("don't show red border initially", async () => {
+    await createSelectionInput();
+    expect(fixture.querySelectorAll("input")[0].classList).not.toContain("o-invalid");
   });
 
   test("pressing and releasing control has no effect on future clicks", async () => {