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

This reverts commit 4a956f3.

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 #3449

Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>

Author: rrahir

src/components/selection_input/selection_input.ts

@@ -103,7 +103,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,
@@ -381,6 +381,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;
   }
@@ -442,10 +446,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();
@@ -592,15 +595,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

@@ -9,11 +9,7 @@ import {
   paste,
   setSelection,
 } from "../test_helpers/commands_helpers";
-import {
-  setInputValueAndTrigger,
-  simulateClick,
-  triggerMouseEvent,
-} from "../test_helpers/dom_helper";
+import { setInputValueAndTrigger, triggerMouseEvent } from "../test_helpers/dom_helper";
 import {
   createColorScale,
   createEqualCF,
@@ -354,47 +350,20 @@ describe("UI of conditional formats", () => {
     });
 
     test("cannot create a new CF with invalid range", async () => {
-      await simulateClick(selectors.buttonAdd);
+      triggerMouseEvent(selectors.buttonAdd, "click");
+      await nextTick();
       await nextTick();
 
       setInputValueAndTrigger(selectors.ruleEditor.range, "hello", "change");
 
       const dispatch = spyDispatch(parent);
       //  click save
-      await simulateClick(selectors.buttonSave);
-      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 simulateClick(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 simulateClick(selectors.buttonAdd);
+      triggerMouseEvent(selectors.buttonSave, "click");
       await nextTick();
-      const dispatch = spyDispatch(parent);
-
-      setInputValueAndTrigger(selectors.ruleEditor.range, "", "change");
-      await simulateClick(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", "change");
-      await nextTick();
-      expect(document.querySelector(selectors.error)).toBe(null);
-
-      setInputValueAndTrigger(selectors.ruleEditor.range, "s!A1", "change");
-      await simulateClick(selectors.buttonSave);
-      expect(dispatch).not.toHaveBeenCalledWith("ADD_CONDITIONAL_FORMAT");
-      const errorString1 = document.querySelector(selectors.error);
-      expect(errorString1!.textContent).toBe("The range is invalid");
+      expect(errorString!.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");
@@ -775,6 +744,8 @@ describe("UI of conditional formats", () => {
     await nextTick();
     setInputValueAndTrigger(selectors.ruleEditor.range, "", "change");
     await nextTick();
+    triggerMouseEvent(selectors.buttonSave, "click");
+    await nextTick();
     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

@@ -343,14 +343,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 () => {