Skip to content

[WC-3501]: fix(combobox-web): fix inverted editability logic#2332

Open
r0b1n wants to merge 1 commit into
mainfrom
fix/combobox-editability-inverted
Open

[WC-3501]: fix(combobox-web): fix inverted editability logic#2332
r0b1n wants to merge 1 commit into
mainfrom
fix/combobox-editability-inverted

Conversation

@r0b1n

@r0b1n r0b1n commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixed a bug in getReadonly() where the conditionally editability mode had inverted logic: expression.value = true (meaning "editable") was being returned directly as the read-only flag, making the widget read-only when it should be editable, and vice versa.
  • Fixed a related bug where the presence of an editable targetAttribute caused the function to return early with false (editable), bypassing the custom editability condition entirely. Now targetAttribute only short-circuits when it is explicitly read-only.
  • Added unit tests covering all branches of getReadonly.

Root cause

Two issues in getReadonly:

  1. The original early-return on targetAttribute presence meant any editable attribute would always report the widget as editable, ignoring the never or conditionally settings.
  2. The conditionally branch returned customEditabilityExpression.value ?? true directly, where the expression represents editability (true = editable), so the boolean was inverted.

@r0b1n
r0b1n requested a review from a team as a code owner July 16, 2026 09:04
@github-actions

This comment has been minimized.

@r0b1n r0b1n changed the title fix(combobox-web): fix inverted editability logic [WC-3501] fix(combobox-web): fix inverted editability logic Jul 16, 2026
@r0b1n r0b1n changed the title [WC-3501] fix(combobox-web): fix inverted editability logic [WC-3501]: fix(combobox-web): fix inverted editability logic Jul 16, 2026
@r0b1n
r0b1n force-pushed the fix/combobox-editability-inverted branch from aecac76 to 2351483 Compare July 16, 2026 13:02
@github-actions

This comment has been minimized.

The `conditionally` branch returned `expression.value` directly, so a
truthy value (meaning "editable") made the widget read-only, and vice
versa.  Fixed by returning `true` only when the expression is not `true`,
and added unit tests for all `getReadonly` branches.
@r0b1n
r0b1n force-pushed the fix/combobox-editability-inverted branch from 2351483 to 86b58b8 Compare July 17, 2026 10:42
@github-actions

Copy link
Copy Markdown
Contributor

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge


What was reviewed

File Change
packages/pluggableWidgets/combobox-web/src/helpers/Database/utils.ts Fix inverted editability logic in getReadonly() — two bugs corrected
packages/pluggableWidgets/combobox-web/src/helpers/Database/__tests__/utils.spec.ts New unit tests covering all branches of getReadonly
packages/pluggableWidgets/combobox-web/CHANGELOG.md Unreleased entries for both fixes + breaking change note

Skipped (out of scope): dist/, pnpm-lock.yaml

CI checks could not be retrieved — please verify all checks pass before merging.


Findings

⚠️ Low — Breaking changes section placed inside ### Fixed

File: packages/pluggableWidgets/combobox-web/CHANGELOG.md lines 19–21
Note: The ### Breaking changes heading appears nested inside the ## [Unreleased] block, but Keep a Changelog only defines standard sections: Added, Changed, Deprecated, Removed, Fixed, Security. A custom ### Breaking changes section is non-standard and may not be picked up by changelog tooling. Consider putting the breaking-change notice inside ### Changed or as a callout within ### Fixed using bold text, e.g.:

### Fixed

- **Breaking change:** Apps that worked around the previous bug by inverting their editability expression...

- We fixed an issue where the custom editability condition...

⚠️ Low — Missing test for targetAttribute readOnly + conditionally expression false

File: packages/pluggableWidgets/combobox-web/src/helpers/Database/__tests__/utils.spec.ts
Note: The "when targetAttribute is readOnly" describe block only tests dynamic.available(true) as the expression. The fix made targetAttribute.readOnly unconditionally short-circuit the rest of the logic, so a case where both readOnly is true AND customEditabilityExpression.value === false would be useful to confirm read-only still wins. Low confidence this matters in practice, but worth one extra case:

it("is read-only regardless of customEditability 'conditionally' with expression false", () => {
    const targetAttribute = new EditableValueBuilder<string>().withValue("value").isReadOnly().build();
    const result = getReadonly(
        targetAttribute as unknown as EditableValue<string | Big>,
        "conditionally",
        dynamic.available(false)
    );
    expect(result).toBe(true);
});

Positives

  • The fix is minimal and surgical — only the two confirmed-buggy conditions were changed; surrounding logic is untouched.
  • targetAttribute?.readOnly optional-chaining cleanly handles the undefined case without a separate guard.
  • customEditabilityExpression.value !== true is a safe, explicit comparison: it correctly treats loading, unavailable, and false all as "not editable" with a single expression.
  • Tests use EditableValueBuilder and dynamic.* helpers from @mendix/widget-plugin-test-utils — no manual mock objects, fully aligned with repo conventions.
  • All three targetAttribute states (readOnly, editable, undefined) are covered, and all customEditability modes are exercised.
  • CHANGELOG entry is present and user-facing, as required; the breaking-change notice is a valuable heads-up for upgrading apps.

@yordan-st yordan-st left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants