[IMP] DV, CF: Enforce range on CF & DV #5679

fdamhaut · 2025-02-10T16:01:53Z

Description:

Enforce that the ranges on CF & DV belong to the same sheet they do.

Task: 4072640

review checklist

robodoo · 2025-02-10T16:01:57Z

hokolomopo

pouet pouet 🎺

hokolomopo · 2025-02-19T10:10:11Z

src/components/side_panel/conditional_formatting/cf_editor/cf_editor.ts

+    const ranges = rangesXC.map((xc) => this.env.model.getters.getRangeDataFromXc(sheetId, xc));
+    const OOSranges = ranges.some((range) => range._sheetId != sheetId);
+    if (OOSranges) {
+      if (!newCf.suppressErrors) {
+        this.state.errors = [CommandResult.TargetOutOfSheet];
+      }
+      return [CommandResult.TargetOutOfSheet];
+    }


should be handled in the allowDispatch of the core plugin, not here (like the data validations)

src/components/side_panel/conditional_formatting/conditional_formatting.ts

hokolomopo · 2025-02-19T10:19:53Z

src/components/translations_terms.ts

@@ -28,6 +28,7 @@ export const CfTerms = {
    [CommandResult.ValueCellIsInvalidFormula]: _t(
      "At least one of the provided values is an invalid formula"
    ),
+    [CommandResult.TargetOutOfSheet]: _t("The range cannot target out of the current sheet."),


Message isn't very clear IMO. Since we can leave the side panel open while changing sheet, talking about current sheet is dubious. "The range" when there is no clear way to know about which range we are talking about is also not very clear.

"All the ranges should be in the same sheet as the conditional format" maybe ?

Still valid, the message

does not seems very english/clear. Maybe it's just me. And we should probably put the range between quotes.

tests/conditional_formatting/conditional_formatting_panel_component.test.ts

hokolomopo · 2025-02-19T10:24:46Z

src/components/side_panel/conditional_formatting/cf_editor/cf_editor.ts

@@ -198,6 +200,7 @@ export class ConditionalFormattingEditor extends Component<Props, SpreadsheetChi
  setup() {
    this.state = useState<State>({
      errors: [],
+      sheetId: this.env.model.getters.getActiveSheetId(),


theoretically we could open the CF editor pane on a CF that is not on the active sheet. sheetId should probably be a props.

not addressed

- Enforce that the target range of CF & DV belong to the same sheet than them - Allow to switch sheet while editing CF to set formulas task: 4072640

hokolomopo

👍

hokolomopo · 2025-02-20T09:03:08Z

src/components/side_panel/conditional_formatting/cf_editor/cf_editor.ts

+  get firstInvalidRangeString(): string | undefined {
+    const sheetId = this.sheetId;
+    return this.state.ranges.find((xc) => {
+      const range = this.env.model.getters.getRangeDataFromXc(sheetId, xc);


Suggested change

const range = this.env.model.getters.getRangeDataFromXc(sheetId, xc);

const range = this.env.model.getters.getRangeFromSheetXc(sheetId, xc);

don't use RangeData when you do'nt have to. They are a bit ugly, and only meant for commands. We shouldn't want to access range._sheetId, it's "private"

hokolomopo · 2025-02-20T09:05:54Z

src/components/side_panel/conditional_formatting/cf_editor/cf_editor.ts

+      case CommandResult.InvalidRange:
+        return CfTerms.Errors[reason](this.firstInvalidRangeString);
+      case CommandResult.ValueCellIsInvalidFormula:
+        return CfTerms.Errors[reason](this.firstInvalidFormula);


It's not really necessary, the composer is already highlighted red to show the user it's invalid.

And the message only displays the first invalid formula, but there could be multiple. Either make a message for every invalid formula, or don't bother making a dynamic message.

hokolomopo · 2025-02-20T09:07:56Z

src/components/translations_terms.ts

+    [CommandResult.MinBiggerThanMax]: () => _t("Minimum must be smaller then Maximum"),
+    [CommandResult.MinBiggerThanMid]: () => _t("Minimum must be smaller then Midpoint"),
+    [CommandResult.MidBiggerThanMax]: () => _t("Midpoint must be smaller then Maximum"),


while you're changing this: you could fix the message. "than" rather than "then". And remove random uppercasing of words

hokolomopo · 2025-02-20T09:09:01Z

src/plugins/core/conditional_format.ts

+          this.checkEmptyRange,
+          this.checkValidRange,
          this.checkCFRule,
          this.checkEmptyRange,


this.checkEmptyRange is there twice

hokolomopo · 2025-02-20T09:17:15Z

src/plugins/core/conditional_format.ts

+    const stringRanges = ranges.map((range) => this.getters.getRangeString(range, cmd.sheetId));
+    if (stringRanges.some((xc) => !this.getters.isRangeValid(xc))) {
+      return CommandResult.InvalidRange;
+    }


probably wrong way to do that/useless. We're coming from a RangeData, so either it's a normal rangeData and the XC will always be valid, or it's really malformed and the conversion to range/string will throw and we'll never return CommandResult.InvalidRange.

And we already test for invalid zones inside the sheet core plugin.

Oh you copied what was already in the data validation. Well either i'm wrong and there is cases where you return CommandResult.InvalidRange here, or you can remove it from both.

hokolomopo · 2025-02-20T09:23:24Z

tests/conditional_formatting/conditional_formatting_panel_component.test.ts

+      setInputValueAndTrigger(selectors.ruleEditor.range, "Sheet2!A1");
+
+      const dispatch = spyModelDispatch(model);
+      //  click save


nitpick: comment looks useless

hokolomopo · 2025-02-20T09:25:15Z

tests/data_validation/data_validation_generics_side_panel_component.test.ts

+    createSheet(model, { sheetId: "ID", name: "Sheet2", activate: false });
+    await simulateClick(".o-dv-add");
+    await nextTick();
+    await changeCriterionType("dateIs");


useless no ?

hokolomopo · 2025-02-20T09:26:04Z

tests/data_validation/data_validation_generics_side_panel_component.test.ts

@@ -18,7 +18,7 @@ mockGetBoundingClientRect({
  "o-dv-type": () => dataValidationSelectBoundingRect,
 });

-export async function mountDataValidationPanel(model?: Model) {
+async function mountDataValidationPanel(model?: Model) {


remove this and use the new helper

fdamhaut force-pushed the master-spreadsheet-dv-cf-range-enforcement-flda branch from e51aea4 to b30c4c3 Compare February 10, 2025 16:03

hokolomopo reviewed Feb 19, 2025

View reviewed changes

[IMP] DV, CF: Enforce range on CF & DV

f360106

- Enforce that the target range of CF & DV belong to the same sheet than them - Allow to switch sheet while editing CF to set formulas task: 4072640

fdamhaut force-pushed the master-spreadsheet-dv-cf-range-enforcement-flda branch from b30c4c3 to f360106 Compare February 19, 2025 14:21

hokolomopo reviewed Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IMP] DV, CF: Enforce range on CF & DV #5679

[IMP] DV, CF: Enforce range on CF & DV #5679

fdamhaut commented Feb 10, 2025

robodoo commented Feb 10, 2025

hokolomopo left a comment

hokolomopo Feb 19, 2025

hokolomopo Feb 19, 2025

hokolomopo Feb 20, 2025

hokolomopo Feb 19, 2025

hokolomopo Feb 20, 2025

hokolomopo left a comment

hokolomopo Feb 20, 2025

hokolomopo Feb 20, 2025

hokolomopo Feb 20, 2025

hokolomopo Feb 20, 2025

hokolomopo Feb 20, 2025

hokolomopo Feb 20, 2025

hokolomopo Feb 20, 2025

hokolomopo Feb 20, 2025

hokolomopo Feb 20, 2025

	const range = this.env.model.getters.getRangeDataFromXc(sheetId, xc);
	const range = this.env.model.getters.getRangeFromSheetXc(sheetId, xc);

[IMP] DV, CF: Enforce range on CF & DV #5679

Are you sure you want to change the base?

[IMP] DV, CF: Enforce range on CF & DV #5679

Conversation

fdamhaut commented Feb 10, 2025

Description:

review checklist

robodoo commented Feb 10, 2025

hokolomopo 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

hokolomopo 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