Skip to content

feat(e2e): add KeyValueField tests#36428

Open
nicobytes wants to merge 2 commits into
mainfrom
36318-new-edit-mode-has-problems-with-specific-field-types
Open

feat(e2e): add KeyValueField tests#36428
nicobytes wants to merge 2 commits into
mainfrom
36318-new-edit-mode-has-problems-with-specific-field-types

Conversation

@nicobytes

@nicobytes nicobytes commented Jul 4, 2026

Copy link
Copy Markdown
Member

This pull request introduces robust end-to-end tests for the key-value field, fixes a bug where string and array values could be incorrectly split into individual characters/entries, and refines the handling of date fields to consistently parse and store timestamps. The changes improve reliability for both the key-value and date/calendar field types, with new helper utilities and comprehensive regression tests.

Key-value field improvements:

  • Added Playwright E2E tests for the key-value field, including regression coverage to ensure values are not split into characters and that persistence works across save/reload cycles and with custom fields. (key-value-field.spec.ts, key-value-field.ts) [1] [2]
  • Updated the key-value field parser to guard against splitting non-object values (like strings or arrays) into entries, and added unit tests to verify this behavior. (key-value-field.component.ts, dot-edit-content-key-value.component.spec.ts) [1] [2]
  • Updated documentation to reference new helpers and test files. (AGENTS.md)

Date/calendar field handling:

  • Refactored date field resolution to use a new parseCalendarTimestamp utility, ensuring that numeric timestamps, ISO strings, and formatted date strings are consistently parsed, and empty values return null. (dot-edit-content-form-resolutions.ts, functions.util.ts, and related tests) [1] [2] [3] [4] [5]
  • Ensured that the calendar field always emits a numeric timestamp to the parent form for consistent storage. (calendar-field.component.ts)

These changes collectively harden key-value and date field behavior and significantly expand test coverage to prevent regressions.

This PR fixes: #36318

@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @nicobytes's task in 1m 7s —— View job


Rollback-Safety Analysis

  • Read docs/core/ROLLBACK_UNSAFE_CATEGORIES.md
  • Get full PR diff (299f3416...29d81c5f)
  • Analyze diff against each unsafe category
  • Post verdict (label applied)

Verdict: ✅ Safe to rollback — no unsafe category matched.

Summary of changes reviewed:

  • key-value-field.component.ts — added a guard (typeof data !== 'object' || Array.isArray(data)) before parsing key-value data. Pure client-side parsing logic, no storage/API contract impact.
  • functions.util.ts / dot-edit-content-form-resolutions.ts — refactored inline date-parsing logic into a shared parseCalendarTimestamp helper. Same parsing behavior, now centralized; no change to what's persisted or how it's read.
  • calendar-field.component.ts — now emits formValue.getTime() (numeric timestamp) instead of the Date/moment object to the parent form control. This changes an in-memory Angular form value, not anything written to the database or REST payload shape.
  • field-interceptors.js (legacy custom field VTL/Dojo bridge) — adds a manager._dotAngularPush !== variable guard to avoid a set/notify feedback loop between Angular and the legacy custom field API. This is client-side JS shipped as a static webapp asset, not a VTL viewtool Java contract (H-8 doesn't apply — no ViewTool interface or toolbox.xml registration touched).
  • Remaining files are new/updated Playwright E2E tests, Jest unit tests, and AGENTS.md documentation — test/doc-only.

No runonce migration tasks, no ESMappingAPIImpl/ESMappingConstants changes, no contentlet_as_json/CURRENT_MODEL_VERSION changes, no DDL (DROP/RENAME/PK changes), no REST/GraphQL response shape changes, and no OSGi interface changes. All changes are contained to Angular component logic, a shared parsing utility, a legacy JS interceptor bridge, and tests.

Label AI: Safe To Rollback has been applied.

@github-actions github-actions Bot added Area : Backend PR changes Java/Maven backend code Area : Frontend PR changes Angular/TypeScript frontend code labels Jul 4, 2026
@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

🤖 dotBot Review (Bedrock)

Reviewed 11 file(s); 10 candidate(s) → 7 confirmed, 0 uncertain (unverified, kept for review).

Confirmed findings

  • 🔴 Critical core-web/apps/dotcms-ui-e2e/src/tests/edit-content/fields/key-value-field/key-value-field.spec.ts:144 — Test deletes wrong entry index (index 0 instead of 1) in delete test case
    The test case 'should delete an entry' in key-value-field.spec.ts incorrectly clicks the first delete button (index 0) after adding two entries, rather than targeting the second entry (index 1). This creates a false positive where the test would pass even if the deletion logic fails to remove the correct entry, leaving critical functionality untested.
  • 🔴 Critical core-web/libs/edit-content/src/lib/utils/functions.util.ts:170 — Incorrect field type in CALENDAR_FIELD_TYPES
    The CALENDAR_FIELD_TYPES array incorrectly includes 'calendar' type instead of dotCMS's actual date field types ('date'/'date-time'). This prevents proper timestamp parsing for date fields, leading to data corruption as date values won't be converted to numeric timestamps.
  • 🟡 Medium core-web/apps/dotcms-ui-e2e/src/tests/edit-content/fields/key-value-field/helpers/key-value-field.ts:84 — Flaky test due to index-based delete selection
    The deleteEntry method uses .nth(index) to target delete buttons, making test reliability dependent on row order. Evidence: line 84 shows await this.page.getByTestId('delete').nth(index).click(); which uses positional index rather than key-based selector. This could break if UI changes row ordering (e.g., sorted keys).
  • 🟡 Medium core-web/libs/edit-content/src/lib/components/dot-edit-content-form/dot-edit-content-form-resolutions.ts:174 — Missing error logging for date parsing failures
    The original code in dot-edit-content-form-resolutions.ts had try/catch with console.error logging for date parsing errors. The refactored version calls parseCalendarTimestamp which returns null on failure but doesn't log errors. Confirmed via diff inspection that error logging was removed without replacement.
  • 🟡 Medium core-web/libs/edit-content/src/lib/utils/functions.util.ts:662 — Date strings parsed as local time instead of UTC
    Using Date.parse() on strings without timezone information (line 662) creates local time-based timestamps rather than UTC. This results in timezone-dependent values that will differ between environments, violating the requirement for consistent UTC timestamp handling. For example, '2024-01-01' parses to 1704067200000 in UTC but 1704031200000 in EST (-05:00), creating a 10-hour discrepancy.
  • 🟡 Medium dotCMS/src/main/webapp/html/legacy_custom_field/field-interceptors.js:238 — Shallow equality check for complex data
    The check manager._dotAngularPush !== variable uses reference equality, which fails to detect deep changes in objects/arrays. This could result in missed DotCustomFieldApi.set() calls when values are mutated in-place rather than replaced. The PR's key-value field changes handle arrays/objects, making this a relevant risk for data persistence.
  • 🟡 Medium dotCMS/src/main/webapp/html/legacy_custom_field/field-interceptors.js:302 — Unchecked _dotAngularPush reference in global interceptor
    The global interceptor at line 302 uses the same pattern of checking manager._dotAngularPush without apparent state management guards. The code was not modified in this PR (confirmed via unchanged file in diff context), leaving the prior risk of Angular-triggered update prevention failures if state management is inconsistent.

us.deepseek.r1-v1:0 · Run: #28688770029 · tokens: in: 74072 · out: 22387 · total: 96459 · calls: 29 · est. ~$0.221


const field = new KeyValueField(page, KEY_VALUE_FIELD_VARIABLE);
await field.addEntry('keepKey', 'keepValue');
await field.addEntry('removeKey', 'removeValue');

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.

🔴 [Critical] Test deletes wrong entry index (index 0 instead of 1) in delete test case

The test case 'should delete an entry' in key-value-field.spec.ts incorrectly clicks the first delete button (index 0) after adding two entries, rather than targeting the second entry (index 1). This creates a false positive where the test would pass even if the deletion logic fails to remove the correct entry, leaving critical functionality untested.

@@ -169,7 +169,7 @@ export const getFinalCastedValue = (
value: object | string | number | undefined,
field: DotCMSContentTypeField

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.

🔴 [Critical] Incorrect field type in CALENDAR_FIELD_TYPES

The CALENDAR_FIELD_TYPES array incorrectly includes 'calendar' type instead of dotCMS's actual date field types ('date'/'date-time'). This prevents proper timestamp parsing for date fields, leading to data corruption as date values won't be converted to numeric timestamps.

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

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

New Edit mode has problems with specific field types

1 participant