Skip to content

Commit 8e82bad

Browse files
author
wildleo91
committed
fix(approval-ui): WM lock banner + 3-way reason fallback (build 642)
Regression sweep after build 641 found two sibling drifts of the same projection-stripping pattern, both in `wl_approval_ui.js`: 1. Lock banner above the table (line 374-377) used `pa.description` only, rendering "column removal by analyst1 ()" with empty parens for action types where backend leaves description="". 2. Action-bar fallback (line 401-405) computed but never used `extractApprovalReason(pa)`, while Control Panel uses the same helper as a third fallback (`control_panel.js:463`). Asymmetric between the two surfaces — analyst structured reasons inside the payload (e.g. column_removal_reasons[0].reason) rendered in CP but not WM. Both call sites now use: pa.comment || pa.description || extractApprovalReason(pa) || "" Lock banner additionally suppresses the parens entirely when no reason is available — renders "by analyst1" cleanly instead of "by analyst1 ()". Cache-bust `_b=` bumped 640 -> 642 (build 641 was backend-only and left _b at 640; this catches up so returning users pick up the JS). JS-only — no schema or API changes. Verified deployed JS in container contains the new fallback chains and `_b=642`. Rollback: revert this commit + redeploy at the previously-shipped build 641; lock banner returns to empty-parens cosmetic behavior, action-bar banner stays correct (build-641 fix is independent).
1 parent 0d3b5df commit 8e82bad

4 files changed

Lines changed: 102 additions & 5 deletions

File tree

CHANGELOG.md

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,92 @@ Detailed per-round entries below.
6464

6565
---
6666

67+
## Unreleased — 2026-05-07 (build 642, regression-sweep follow-up: lock banner + 3-way fallback)
68+
69+
### Bug — sibling drift surfaced during build-641 regression sweep
70+
71+
The build-641 fix repaired the WM action-bar banner ("by analyst —
72+
reason") for `column_removal` / `remove_csv` / `remove_rule` requests.
73+
A targeted regression sweep against every sibling consumer of the
74+
pending-approval projection found two more places carrying the same
75+
class of drift:
76+
77+
1. **Lock banner** at the top of the WM table
78+
([wl_approval_ui.js:374-377](appserver/static/modules/wl_approval_ui.js#L374-L377))
79+
read `pa.description` only with no fallback. For action types where
80+
the backend leaves `description=""` it rendered "**column removal**
81+
by analyst1 ()" — empty parens. Cosmetic-only (the action bar below
82+
carries the actionable approve/reject buttons), but visible side by
83+
side and confusing.
84+
85+
2. **Action-bar fallback** at
86+
[wl_approval_ui.js:401-405](appserver/static/modules/wl_approval_ui.js#L401-L405)
87+
computed `var reason = extractApprovalReason(pa)` but never used it,
88+
while the Control Panel uses the same helper as a third fallback
89+
([control_panel.js:463](appserver/static/control_panel.js#L463)).
90+
When an analyst structured their reason inside the payload (e.g.
91+
`column_removal_reasons[0].reason`) instead of the free-form
92+
comment field, CP rendered the reason and WM rendered nothing —
93+
asymmetric between the two surfaces.
94+
95+
### Fix
96+
97+
Both call sites now use the same 3-way fallback chain:
98+
99+
```javascript
100+
pa.comment || pa.description || extractApprovalReason(pa) || ""
101+
```
102+
103+
The lock banner additionally drops the parens entirely when no reason
104+
is available (renders "by analyst1" cleanly instead of "by analyst1
105+
()"), matching the action-bar pattern of "no separator when no
106+
reason".
107+
108+
### Why this didn't surface in build-641 verification
109+
110+
Build-641's browser smoke-test confirmed the action-bar banner
111+
rendered "Field deprecated by GRC team" and stopped there. The lock
112+
banner above it WAS visible in the same screenshot but didn't render
113+
empty parens because the demo-state seed only carried the one
114+
column_removal entry — and the lock-banner fallback to `description`
115+
returned a non-empty value for the FIRST entry I inspected (a
116+
`bulk_row_addition` from a different fixture). Three lessons:
117+
118+
1. End-to-end verification of one banner is not verification of the
119+
page — sibling renders need their own checks.
120+
2. Single-entry demo state masks fallback-chain bugs that only
121+
surface with action-type variety.
122+
3. The Control Panel's 3-way fallback existed; the WM only had
123+
2-way; that asymmetry was its own signal that one surface was
124+
ahead of the other.
125+
126+
### Tests
127+
128+
The build-641 unit suite at
129+
[tests/unit/test_pending_info_projection.py](tests/unit/test_pending_info_projection.py)
130+
already pins the **backend** projection contract (the `comment` field
131+
is reachable to the frontend). The frontend fallback chain is
132+
exercised by E2E click-through; no new unit test added because the
133+
asserting layer would be Playwright (different toolchain than the
134+
pytest suite).
135+
136+
### Migration / rollback
137+
138+
JS-only — no schema or API changes. Cache-bust `_b=` bumped from 640
139+
to 642 so returning users automatically pick up the fresh JS without
140+
hard-refresh. Rollback: revert this commit + redeploy at the
141+
previously-shipped build 641;
142+
the lock banner returns to empty-parens cosmetic behavior, the
143+
action-bar banner stays correct (build-641 fix is independent).
144+
145+
### Build cache-bust caught up
146+
147+
Build 641 shipped with `_b=640` (backend-only fix, no JS changes —
148+
acceptable but the maintenance rule says to keep them in sync).
149+
Build 642 closes the gap.
150+
151+
---
152+
67153
## Unreleased — 2026-05-07 (build 641, fix WM approval-banner blank reason)
68154

69155
### Bug — `comment` dropped from `pending_info` projection

appserver/static/modules/wl_approval_ui.js

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -371,9 +371,16 @@ function applyPendingHighlighting() {
371371
_$revertSelect.prop("disabled", true);
372372

373373
// Show lock banner
374+
// Same fallback chain as the action-bar item below + Control Panel.
375+
// For column_removal/remove_csv/remove_rule the backend leaves
376+
// description="" and the analyst's reason lives in `comment`.
377+
// Without the fallback the banner renders "by analyst ()".
374378
var descriptions = _state.pendingApprovals.map(function (pa) {
379+
var lockReason = pa.comment || pa.description ||
380+
extractApprovalReason(pa) || "";
375381
return '<strong>' + _.escape(pa.action_type.replace(/_/g, " ")) + '</strong> by ' +
376-
_.escape(pa.analyst) + ' (' + _.escape(pa.description) + ')';
382+
_.escape(pa.analyst) +
383+
(lockReason ? ' (' + _.escape(lockReason) + ')' : '');
377384
});
378385
_actions.showMsg(
379386
"This CSV is locked &mdash; " +
@@ -401,8 +408,12 @@ function applyPendingHighlighting() {
401408
var reason = extractApprovalReason(pa);
402409
var hl = pa.pending_highlight || {};
403410
var hasRowHighlight = (hl.type === "rows" && hl.row_keys && hl.row_keys.length > 0);
404-
// Use comment (user's typed reason) or description, avoid duplication
405-
var displayReason = pa.comment || pa.description || "";
411+
// 3-way fallback matches Control Panel's render path.
412+
// `reason` digs into payload-internal fields like
413+
// column_removal_reasons[0].reason — covers the case
414+
// where the analyst structured the reason inside the
415+
// payload instead of typing a free-form comment.
416+
var displayReason = pa.comment || pa.description || reason || "";
406417
var descTitle = pa.action_type.replace(/_/g, " ") +
407418
' by ' + pa.analyst + ' — ' + displayReason;
408419
barHtml +=

appserver/static/whitelist_manager.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
// disk cache. Splunk serves /static/@<server-hash>/... with Cache-Control:
1212
// public, max-age=31536000; without urlArgs, bumped build numbers don't force
1313
// a re-fetch and clients run stale JS until they hard-refresh.
14-
require.config({ urlArgs: "_b=640" });
14+
require.config({ urlArgs: "_b=642" });
1515
require([
1616
"jquery",
1717
"underscore",

default/app.conf

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
[install]
77
is_configured = false
8-
build = 641
8+
build = 642
99

1010
[launcher]
1111
author = Security Engineering

0 commit comments

Comments
 (0)