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! 🚀 |
2 participants
Implements the FlightCheck check requested in #168.
Checkpoint:
SN-CFG-002Category: ServiceNow
Priority: MEDIUM
Conditional? Yes — only runs when ServiceNow flows are detected (same gate as
run_servicenow_checks, which early-returns whenrunner._servicenow_flowsis empty).API tier: documented
Mock source:
documentedpertests/fixtures/cassettes/INDEX.mdAPI tier registry).msdyn_valuefield name confirmed against the production selector insolutions/ess-maker-skills/scripts/backup_template_configs.py; new documented mock buildertests/mocks/dataverse.py::template_configcites that plus the MS Learn Dataverse query docs.What it validates: Extends SN-CFG-001 from config presence to value populated. Reads
msdyn_valuefor each ServiceNow template config and verifies it contains a populated, well-formed absolute http(s) portal base URL. WARNs (naming the offending configs) when the value is blank, carries an unsubstituted placeholder (e.g.{{ServiceNowBaseUrl}}), or is only a relative path — the silent failure where "read all tickets" / "read all cases" responses omit hyperlinks with no deploy-time error.Tests:
tests/flightcheck/checks/test_servicenow_deep.py— covers GOOD (all populated → Passed), BAD (blank / placeholder / relative-path → Warning with remediation), plus NotConfigured, Skipped, and query-error branches. 27 passing locally across the ServiceNow suites.Fixes #168