flightcheck: add SN-CFG-002 — ServiceNow portal base URL populated in template config - #169
Conversation
… template config
Extends the ServiceNow deep validation from config *presence* (SN-CFG-001)
to config *value populated*. SN-CFG-002 reads the `msdyn_value` column of
the `msdyn_employeeselfservicetemplateconfigs` Dataverse entity for each
ServiceNow template config and verifies it carries a populated, well-formed
absolute http(s) portal base URL. When the base URL is blank, an
unsubstituted placeholder (e.g. `{{ServiceNowBaseUrl}}`), or only a relative
path, the check WARNs and names the offending configs — catching the silent
failure where "read all tickets" / "read all cases" responses omit
hyperlinks at runtime without surfacing any deploy-time error.
API tier: Dataverse Web API v9.2 is the `documented` tier per
tests/fixtures/cassettes/INDEX.md. No cassette required. The `msdyn_value`
field name is confirmed against the production selector in
backup_template_configs.py; a documented mock builder
(`dataverse.template_config`) is added for the tests. Gating mirrors the rest
of run_servicenow_checks (early-return when no ServiceNow flows are detected)
and the no-token / no-config / query-error paths mirror SN-CFG-001.
Tests in tests/flightcheck/checks/test_servicenow_deep.py cover GOOD (all
populated -> Passed), BAD (blank value, unsubstituted placeholder, relative
path -> Warning with remediation), plus NotConfigured, Skipped, and
query-error branches.
Fixes #168
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GrahamMcMynn
left a comment
GrahamMcMynn
left a comment
There was a problem hiding this comment.
Code review by ESS Code Review Agent (Auto mode)
|
|
||
| candidates = _ABSOLUTE_URL_RE.findall(value) | ||
| has_real_url = any(_is_absolute_http_url(c) for c in candidates) | ||
| if has_real_url: |
There was a problem hiding this comment.
SN-CFG-002 can still pass an unsubstituted placeholder when the placeholder appears in the host. For example, https://{{ServiceNowBaseUrl}}/api/now/table/incident is matched as https://{{ServiceNowBaseUrl, and _is_absolute_http_url() accepts it because it has an http(s) scheme and non-empty netloc, so has_real_url becomes true. That is one of the bad states this check is meant to catch. Please reject placeholder tokens before accepting URL candidates (or validate the parsed host excludes template tokens) and add a test for this scheme-prefixed placeholder case.
There was a problem hiding this comment.
Fixed in d240a6c. _is_absolute_http_url now rejects any netloc containing placeholder characters ({}<>%$ ), so https://{{ServiceNowBaseUrl}}/... no longer passes. Added a regression test pinning the scheme-prefixed form (plus a bare-token case).
| runner = SimpleNamespace(env_url="https://org.crm.dynamics.com", dv_token="t") | ||
| cfg = _by_id(_check_template_config_base_urls(runner), "SN-CFG-002") | ||
|
|
||
| assert cfg.status == "Warning" |
There was a problem hiding this comment.
tests/AGENTS.md requires every WARNING test to assert specific phrases from both result and remediation, including the impact and concrete fix path. This warning test only pins result; test_base_url_relative_path_only_warns and test_base_url_query_error_warns below have the same gap. As written, the remediation text for these actionable warnings could regress or disappear without failing tests. Please add remediation assertions for each SN-CFG-002 Warning branch.
There was a problem hiding this comment.
Fixed in d240a6c. Both warning tests now assert remediation phrases (https://.service-now.com and the omit/hyperlinks impact), and a new bare-placeholder warning test was added.
| verify the ServiceNow portal base URL inside the value is a | ||
| populated, well-formed absolute http(s) URL. | ||
|
|
||
| Source (documented): |
There was a problem hiding this comment.
The documented-tier mock requirements in tests/AGENTS.md require the builder docstring to cite an anchored vendor doc and include the documented example response verbatim. This builder cites generic Dataverse query docs plus backup_template_configs.py, but it does not include a documented example for the msdyn_employeeselfservicetemplateconfigs shape/fields it returns. That leaves tests depending on a shape that is not verified by one of the permitted tiers. Please add the required documented example evidence, or promote this surface with cassette/schema evidence before using the builder in FlightCheck tests.
There was a problem hiding this comment.
Fixed in d240a6c. The emplate_config builder docstring now carries the verbatim field contract (cited to backup_template_configs.py \ + record mapping) wrapped in a documented OData v4 collection-envelope example.
…holder hosts Code review of #169 found SN-CFG-002 would falsely PASS a value like `https://{{ServiceNowBaseUrl}}/api/...`: the placeholder host parses to a non-empty netloc, so the old `_is_absolute_http_url` accepted it. Tighten the host check to reject any netloc containing placeholder characters (`{}<>%$ `) so an unsubstituted base-URL token never counts as a populated base URL. Also strengthen the tests and mock per the review: - The placeholder warning test now pins the scheme-prefixed form (`https://{{ServiceNowBaseUrl}}/...`) that triggered the bug, plus a new bare-token case; both warning tests now assert remediation phrases as required by tests/AGENTS.md. - The documented-tier `dataverse.template_config` builder docstring now includes the verbatim field contract (cited to the backup_template_configs.py $select + mapping) inside a documented OData envelope example. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
👋 Friendly reminder: This PR has been inactive for a while. Could the author or reviewers take a look and either push it forward, request changes, or close it if it's no longer needed? Let's keep our PR queue healthy! 🚀 |
…-base-url-populated-in-te
…r (fix false positives found in PROD validation) Validated SN-CFG-002 against a real PROD tenant: of 21 ServiceNow-named template configs, only 2 (msdyn_ServiceNowHRSD / msdyn_ServiceNowITSM) carry the portal base URL as a ServiceNowPortalBaseURI field. The original check flagged all 21, producing 19 false positives. - Scope the check to only configs that carry a ServiceNowPortalBaseURI field; scenario/field-mapping configs never hold a URL and are ignored. - Validate the ServiceNowPortalBaseURI field value specifically rather than scanning the whole config blob for any URL. - Defer per-pack to the update-safe SN-URL env var (US 7535608, PR #174) when it holds a valid URL, so SN-CFG-002 and SN-URL-001/002 do not double-fire on tenants migrated to the env-var mechanism. - Rewrite SN-CFG-002 tests to the real config shape and add scoping and deference coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cae8da1a-6f8b-44db-af8c-52a559fc6a94
…ests/AGENTS.md compliance dropped in the SN-CFG-002 rewrite) Every WARNING test must pin phrases from both result and remediation per tests/AGENTS.md. The scoping rewrite dropped remediation assertions on three WARNING branches (query-error, root-config-amongst-scenarios, partial-env-deference) and left the bare-placeholder test pinning only the fix path, not the impact. Restore full remediation coverage so the actionable warning text cannot regress silently. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cae8da1a-6f8b-44db-af8c-52a559fc6a94
|
Closing this in favor of #174. Both checks validate the same ServiceNow portal base URL. #174 validates the update-safe environment variables (SN-URL-001/002), which are the real source of truth after the URL moved out of template config under US 7535608 to fix ICM 820635151. The template-config value ships empty and resets on managed-solution updates, so the env-var check supersedes this one. #174 has been reviewed, fixed, and validated end to end against a live tenant. Thanks Graham for the original work here. |
3 participants
Implements the FlightCheck check requested in #168, scoped and validated against a real PROD tenant.
Checkpoint:
SN-CFG-002Category: ServiceNow
Priority: MEDIUM
Conditional? Yes — only runs when ServiceNow flows are detected (same gate as
run_servicenow_checks).What it validates
Extends SN-CFG-001 from config presence to portal base URL value populated. The portal base URL is what renders clickable ticket/case hyperlinks; an empty value is the root cause of ICM 820635151 (links silently omitted).
Scoping (validated against PROD)
Live validation against a production tenant showed that of 21 ServiceNow-named template configs, only 2 carry the portal base URL:
msdyn_ServiceNowHRSDandmsdyn_ServiceNowITSM, each holding a{ "ServiceNowPortalBaseURI": "<url>" }object. The other 19 are scenario/field-mapping configs that never contain a URL. The check therefore evaluates only configs that carry aServiceNowPortalBaseURIfield, and validates that field's value specifically. This eliminates the 19 false positives the original name-only filter produced.Detection rules (unchanged): WARNs (naming the offending configs) when the base URL value is blank, carries an unsubstituted placeholder (e.g.
{{ServiceNowBaseUrl}}), or is only a relative path.Deference to the update-safe env var (pairs with #174)
US 7535608 (PR #174) moves the base URL to update-safe environment variables (
ServiceNow{HRSD,ITSM}PortalBaseURI, checked by SN-URL-001/002). Once a tenant migrates, the template config is intentionally left empty. To avoid SN-CFG-002 and SN-URL-001/002 double-firing, SN-CFG-002 defers per-pack: when the corresponding env var holds a valid URL, the empty template config is treated as expected (not a warning). Configs with no matching env var are still validated normally.Tests
tests/flightcheck/checks/test_servicenow_deep.py— rewritten to the real config shape (msdyn_ServiceNow{HRSD,ITSM}withServiceNowPortalBaseURI). Covers: all-populated → Passed; blank/placeholder/relative-path → Warning; scenario-configs-ignored (the 19-false-positive regression); root-config-only-flagged-amongst-scenarios (mirrors the PROD tenant); full and partial env-var deference; env-var-only; plus NotConfigured, Skipped, and query-error branches. 20 passing in the file, 76 passing across the ServiceNow suites.Fixes #168