fix: update date field uneditable and Ignore Update Window not sent to API (#44)#140
Conversation
…ate absent, ignoreUpdateWindow always false Bug 1: DateFieldEditable required Latest Selectable Date to be non-zero. When the BC Admin API omits latestSelectableDate or returns it as a date-only string (which AsDateTime() cannot parse), the date field became non-editable, preventing users from setting any date including today. Fixed by: - Making DateFieldEditable depend only on Rec.Available - Defaulting User Selected Date to Today() when Latest Selectable Date is absent, and to Latest Selectable Date when present - Using Evaluate() with a Date fallback before AsDateTime() for both latestSelectableDate and latestSelectableDateTime JSON fields Bug 2: SelectTargetVersion always sent ignoreUpdateWindow: false in the PATCH request body regardless of user intent. The Ignore Update Window field was populated from the API but never exposed as editable in D4PUpdateSelectionDialog, so users could not bypass the update window. Fixed by: - Adding an editable Ignore Update Window column to the dialog repeater - Extending GetSelectedVersion to return IgnoreUpdateWindow - Extending SelectTargetVersion to accept IgnoreUpdateWindow and pass it in both the API request body and the environment record update
Apply consistent Evaluate()-based date parsing to GetEnvironmentUpdates, matching the pattern used in GetAvailableUpdates. The BC Admin API may return latestSelectableDate as a plain ISO date string (no time component), which causes AsDateTime() to return 0DT. Use Evaluate() with Date/DateTime fallback and prefer latestSelectableDateTime when available. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Extract GetJsonText, GetJsonBoolean, GetJsonInteger, GetJsonObject, FormatExpectedAvailability, TryGetJsonDate, TryGetJsonDateTime helpers - Eliminate ~45 repeated JSON field-reading patterns across 5 procedures - Remove dead commented-out code in GetAvailableAppUpdates - Add CCMSTests app with 13 automated test cases - Tests cover Bug 1 (date parsing) and Bug 2 (IgnoreUpdateWindow) regressions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
There was a problem hiding this comment.
Pull request overview
Fixes update scheduling UX/API integration issues for BC environment updates by correcting date parsing from the Admin API and wiring through the ignoreUpdateWindow setting end-to-end. Also introduces JSON parsing helpers to reduce duplication and adds an automated test app to prevent regressions.
Changes:
- Fix parsing of date-only/datetime JSON fields and refactor JSON field extraction into helper procedures.
- Add “Ignore Update Window” to the update selection dialog and include it in the PATCH payload when scheduling updates.
- Add a new test app (
CCMSTests) with automated regression coverage and enable it in AL-Go CI.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
CCMS/src/Environment/D4PBCEnvironmentMgt.codeunit.al |
Refactors JSON parsing, fixes date handling, and sends ignoreUpdateWindow in PATCH body. |
CCMS/src/Environment/D4PUpdateSelectionDialog.page.al |
Makes date editable for available updates and exposes “Ignore Update Window” in the dialog. |
CCMS/src/Environment/D4PBCEnvironmentCard.page.al |
Passes IgnoreUpdateWindow from dialog into scheduling API call. |
CCMS/app.json |
Exposes internals to the test app via internalsVisibleTo. |
CCMSTests/app.json |
Adds a dedicated test app that depends on the main app. |
CCMSTests/src/D4PUpdateSelectionDialogTests.codeunit.al |
Adds dialog regression tests, including ignore-window roundtrip. |
CCMSTests/src/D4PJsonHelperTests.codeunit.al |
Adds tests for JSON date/datetime parsing helpers. |
.AL-Go/settings.json |
Enables CI discovery/execution of tests in CCMSTests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Remove unused appId variable from GetInstalledApps - Fix GetAvailableAppUpdates loop to clear appId/appVersion per iteration and guard Modify on both fields reading successfully - Mark SelectTargetVersion as internal (only called within the same app) - Add ExpectedMonth and ExpectedYear assertions to AllParameters_RoundTripCorrectly
Rename 'D4P Update Selection Dialog Tests' (34 chars) to 'D4P Update Selection Dlg Tests' (30 chars) to comply with AL compiler rule AL0305 which enforces a 30-character maximum for application object identifier names.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This reverts commit 483a5fa.
- Use Evaluate(..., 9) in TryGetJsonDate and TryGetJsonDateTime for locale-safe ISO 8601 parsing, consistent with Operations Helper and AppInsights client patterns in the repo - Move BCEnvironment.Init() inside the GetEnvironments foreach loop so each iteration starts with a clean record, preventing stale field values from bleeding across environments - Move InstalledApp.Init() inside the GetInstalledApps foreach loop for the same reason
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Rename 'D4P Update Selection Dialog Tests' (34 chars) to 'D4P Update Selection Dlg Tests' (30 chars) to comply with AL compiler rule AL0305 (max 30 chars for object identifiers). Also includes second bot review fixes from prior commit: Evaluate(..., 9) for ISO 8601 parsing, BCEnvironment.Init() and InstalledApp.Init() moved inside foreach loops.
…response In GetAvailableAppUpdates, the inner InstalledApp.Get call was reachable even when appId was not present in the JSON object, because the two GetJsonText checks were independent. A missing appId left the Guid at its zero value, causing an unnecessary table lookup that would always fail silently. Nest the version lookup inside the appId block so the database call only runs when both fields are successfully read from the response.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ResultDate: Date; | ||
| Found: Boolean; | ||
| begin | ||
| // Arrange: datetime string — the root cause scenario (AsDateTime() returned 0DT for this) |
There was a problem hiding this comment.
The comment says the datetime string case is “the root cause scenario (AsDateTime() returned 0DT for this)”, but the PR description attributes the 0DT behavior to date-only strings like YYYY-MM-DD. Consider adjusting the comment so it matches the actual root cause being tested (e.g., “datetime fallback parsing”) to avoid misleading future readers.
| // Arrange: datetime string — the root cause scenario (AsDateTime() returned 0DT for this) | |
| // Arrange: datetime string — validates datetime fallback parsing when extracting the date portion |
| procedure GetSelectedVersion(var TargetVersion: Text[100]; var SelectedDate: Date; var ExpectedMonth: Integer; var ExpectedYear: Integer; var IgnoreUpdateWindow: Boolean) | ||
| begin | ||
| TargetVersion := Rec."Target Version"; | ||
| SelectedDate := Rec."User Selected Date"; | ||
| ExpectedMonth := Rec."Expected Month"; | ||
| ExpectedYear := Rec."Expected Year"; | ||
| IgnoreUpdateWindow := Rec."Ignore Update Window"; | ||
| end; |
There was a problem hiding this comment.
GetSelectedVersion returns values from the current Rec, not from the row where Selected = true. If the user selects one version and then moves focus to a different row before pressing OK, this can return (and schedule) the wrong target version/date/window. Consider locating the selected record (e.g., filter Selected = true, FindFirst) and erroring if none is selected; also enforce that an available update has a non-0D selected date before returning.
| Editable = DateFieldEditable; | ||
| Visible = DateFieldsVisible; | ||
| StyleExpr = RowStyleExpr; | ||
| ToolTip = 'Specifies the date for the update. Select a date between today and the latest selectable date.'; |
There was a problem hiding this comment.
The new tooltip says the date must be between today and the latest selectable date, but this dialog explicitly supports cases where Latest Selectable Date is 0D (no upper bound) and defaults to Today(). Consider rewording the tooltip to reflect that the upper bound applies only when a latest selectable date is provided.
| ToolTip = 'Specifies the date for the update. Select a date between today and the latest selectable date.'; | |
| ToolTip = 'Specifies the date for the update. Select a date from today onward, and no later than the latest selectable date when one is provided.'; |
Fixes #44
What was broken
Two separate bugs were causing the update scheduling dialog to misbehave.
Bug 1: Update date field was always grayed out
The BC Admin API returns
latestSelectableDateas a date-only ISO string like2025-01-15. The original code calledJsonValue.AsDateTime()on that string, which returns0DTfor date-only input.DT2Date(0DT)gives0D, so the editability check (Latest Selectable Date <> 0D) always failed and the date picker was permanently disabled.Bug 2: Ignore Update Window was hardcoded to false
The
ignoreUpdateWindowfield was never read from the API response and was not shown in the dialog. The PATCH request body always sentfalseregardless of what the user intended.What was fixed
Date parsing (
D4PBCEnvironmentMgt.codeunit.al)Replaced
AsDateTime()withEvaluate()plus a date/datetime fallback in bothGetAvailableUpdatesandGetEnvironmentUpdates. Both paths now correctly handle date-only strings and datetime strings from the BC Admin API.Dialog fields (
D4PUpdateSelectionDialog.page.al)UpdateFieldVisibilityto make the date field editable whenever the version is available (removed the brokenLatest Selectable Date <> 0Dguard)SetDefaultDateTimeto fall back toToday()when Latest Selectable Date is zeroAPI call (
D4PBCEnvironmentMgt.codeunit.al,D4PBCEnvironmentCard.page.al)SelectTargetVersionnow acceptsIgnoreUpdateWindowas a parameter and includes it in the PATCH body. The value is stored back to the environment record on success.Refactoring
While fixing the date parsing, it was clear the codeunit had about 45 repeated 3-line JSON field-reading blocks spread across 5 procedures. These are now replaced with 7 parameterized helpers.
GetJsonTextGetJsonBooleanGetJsonIntegerGetJsonObjectFormatExpectedAvailabilityTryGetJsonDateTryGetJsonDateTimeThe codeunit went from 905 to 819 lines. Dead commented-out code in
GetAvailableAppUpdateswas also removed.Tests
Added a
CCMSTests/test app (ID range 62050-62099) with 13 automated test cases across two codeunits. No external test library or HTTP calls needed.D4PUpdateSelectionDialogTests (codeunit 62050) covers 6 scenarios:
IgnoreUpdateWindowtrue/false roundtrip (Bug 2 regression)D4PJsonHelperTests (codeunit 62051) covers 7 scenarios:
TryGetJsonDateTryGetJsonDateTimehappy path and null/missing cases