Skip to content

Commit 7341865

Browse files
authored
Merge pull request #121 from magfest/fix/display-of-rejected-lines
Fix/Fixed Display and Reachability of lines after admin decision
2 parents 85ef28f + 3fe73c0 commit 7341865

4 files changed

Lines changed: 194 additions & 12 deletions

File tree

app/templates/budget/_work_item_lines_table.html

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,25 @@ <h3 style="margin:0;">Budget Lines</h3>
1111
{% endif %}
1212
</div>
1313

14+
{# A board-held budget is persisted as FINALIZED, so FINALIZED covers both
15+
"finalized" and "Pending FY Budget Approval". The line page stays reachable
16+
there because it renders read-only: every decision form on it requires a
17+
checkout, and FINALIZED items cannot be checked out. #}
18+
{% set show_line_actions = status in ("SUBMITTED", "UNDER_REVIEW", "NEEDS_INFO", "FINALIZED") %}
19+
{% set line_action_label = "View" if status == "FINALIZED" else "Review" %}
20+
1421
{% if lines %}
15-
{% set show_approved_column = status == "FINALIZED" or totals.approved > 0 %}
22+
{# Show the Approved column once an admin has decided any line, not once the
23+
approved total goes positive. A rejection and a $0 approval are both real
24+
decisions worth 0 cents; gating on the amount hid the column on exactly the
25+
requests that needed it, leaving a rejected line's requested amount as the
26+
only figure on the page. Comped badges and third-party-pays rooms make $0
27+
approvals routine, not an edge case. #}
28+
{% set has_admin_decision = lines
29+
| selectattr("current_review_stage", "equalto", "ADMIN_FINAL")
30+
| list | length > 0 %}
31+
{% set show_approved_column = status == "FINALIZED"
32+
or has_admin_decision or totals.approved > 0 %}
1633
<table class="responsive-cards">
1734
<thead>
1835
<tr>
@@ -27,7 +44,7 @@ <h3 style="margin:0;">Budget Lines</h3>
2744
<th style="width: 120px;" class="right">Approved</th>
2845
{% endif %}
2946
<th style="width: 120px;">Status</th>
30-
{% if status == "SUBMITTED" or status == "UNDER_REVIEW" or status == "NEEDS_INFO" %}
47+
{% if show_line_actions %}
3148
<th style="width: 80px;"></th>
3249
{% endif %}
3350
</tr>
@@ -123,10 +140,10 @@ <h3 style="margin:0;">Budget Lines</h3>
123140
{{ render_status_pill(line_status, label=friendly_status(line_status) if friendly_status else line_status) }}
124141
{% endif %}
125142
</td>
126-
{% if status == "SUBMITTED" or status == "UNDER_REVIEW" or status == "NEEDS_INFO" %}
143+
{% if show_line_actions %}
127144
<td>
128145
<a class="btn btn-muted" href="{{ url_for('approvals.line_review', event=ctx.event_cycle.code, dept=ctx.department.code, public_id=work_item.public_id, line_num=line.line_number) }}" style="padding: 4px 8px; font-size: 12px;">
129-
Review
146+
{{ line_action_label }}
130147
</a>
131148
</td>
132149
{% endif %}
@@ -143,7 +160,7 @@ <h3 style="margin:0;">Budget Lines</h3>
143160
<td class="right num">{{ format_currency(g.approved_cents) }}</td>
144161
{% endif %}
145162
<td></td>
146-
{% if status == "SUBMITTED" or status == "UNDER_REVIEW" or status == "NEEDS_INFO" %}
163+
{% if show_line_actions %}
147164
<td></td>
148165
{% endif %}
149166
</tr>
@@ -156,7 +173,7 @@ <h3 style="margin:0;">Budget Lines</h3>
156173
<td class="right num" style="color: #059669;">{{ format_currency(visible_approved_cents) }}</td>
157174
{% endif %}
158175
<td></td>
159-
{% if status == "SUBMITTED" or status == "UNDER_REVIEW" or status == "NEEDS_INFO" %}
176+
{% if show_line_actions %}
160177
<td></td>
161178
{% endif %}
162179
</tr>
@@ -175,7 +192,7 @@ <h3 style="margin:0;">Budget Lines</h3>
175192
<td class="right num" style="color: #059669;">{{ format_currency(totals.approved) }}</td>
176193
{% endif %}
177194
<td></td>
178-
{% if status == "SUBMITTED" or status == "UNDER_REVIEW" or status == "NEEDS_INFO" %}
195+
{% if show_line_actions %}
179196
<td></td>
180197
{% endif %}
181198
</tr>
@@ -188,7 +205,7 @@ <h3 style="margin:0;">Budget Lines</h3>
188205
{% if total_diff > 0 %}+{% endif %}{{ format_currency(total_diff) }}
189206
</td>
190207
<td></td>
191-
{% if status == "SUBMITTED" or status == "UNDER_REVIEW" or status == "NEEDS_INFO" %}
208+
{% if show_line_actions %}
192209
<td></td>
193210
{% endif %}
194211
</tr>

app/templates/budget/work_item_detail.html

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,8 +117,11 @@ <h1 style="margin:0;">
117117
</form>
118118
{% endif %}
119119

120-
{# Quick Review button for reviewers/admins (not shown for drafts) #}
121-
{% if not perms.is_draft and (perms.is_worktype_admin or perms.can_checkout or perms.is_checked_out_by_current_user) %}
120+
{# Quick Review button for reviewers/admins. Hidden for drafts, and for
121+
FINALIZED items because it then repeats what the lines table already
122+
shows; the per-line View link covers the read-only case. #}
123+
{% if not perms.is_draft and status != "FINALIZED"
124+
and (perms.is_worktype_admin or perms.can_checkout or perms.is_checked_out_by_current_user) %}
122125
<a class="btn btn-primary" href="{{ url_for('work.quick_review', event=ctx.event_cycle.code, dept=ctx.department.code, public_id=work_item.public_id) }}">
123126
Quick Review
124127
</a>
Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
"""Integration tests: per-line action link on a FINALIZED budget request.
2+
3+
A board-held budget is persisted as FINALIZED; "Pending FY Budget Approval"
4+
is a derived display string. Both cases must still expose the line detail
5+
page, which renders read-only because every decision form requires a
6+
checkout and FINALIZED items cannot be checked out.
7+
"""
8+
import re
9+
from datetime import datetime
10+
11+
from app import db
12+
from app.models import (
13+
WorkItem, WorkLine, BudgetLineDetail,
14+
REQUEST_KIND_PRIMARY,
15+
WORK_ITEM_STATUS_FINALIZED, WORK_ITEM_STATUS_SUBMITTED,
16+
WORK_LINE_STATUS_APPROVED, WORK_LINE_STATUS_PENDING,
17+
REVIEW_STAGE_ADMIN_FINAL, REVIEW_STAGE_APPROVAL_GROUP,
18+
)
19+
20+
DETAIL_URL = "/TST2026/TESTDEPT/budget/item/TST2026-TESTDEPT-BUD-1"
21+
22+
# The per-line action cell, captured with its label so the test can tell
23+
# "Review" (actionable) from "View" (finalized) apart.
24+
LINE_ACTION = re.compile(
25+
r'<a class="btn btn-muted" href="[^"]*/line/1/review"[^>]*>\s*(\w+)\s*</a>'
26+
)
27+
28+
29+
def _make_item(data, status, line_status, stage):
30+
work_item = WorkItem(
31+
portfolio_id=data["portfolio"].id,
32+
request_kind=REQUEST_KIND_PRIMARY,
33+
status=status,
34+
public_id="TST2026-TESTDEPT-BUD-1",
35+
created_by_user_id=data["admin"].id,
36+
)
37+
if status == WORK_ITEM_STATUS_FINALIZED:
38+
work_item.finalized_at = datetime.utcnow()
39+
work_item.finalized_by_user_id = data["admin"].id
40+
db.session.add(work_item)
41+
db.session.flush()
42+
43+
line = WorkLine(
44+
work_item_id=work_item.id, line_number=1,
45+
status=line_status, current_review_stage=stage,
46+
approved_amount_cents=450_000 if line_status == WORK_LINE_STATUS_APPROVED else None,
47+
)
48+
db.session.add(line)
49+
db.session.flush()
50+
db.session.add(BudgetLineDetail(
51+
work_line_id=line.id,
52+
expense_account_id=data["expense_account"].id,
53+
spend_type_id=data["spend_type"].id,
54+
quantity=1, unit_price_cents=450_000,
55+
routed_approval_group_id=data["approval_group"].id,
56+
))
57+
db.session.commit()
58+
return work_item
59+
60+
61+
def _finalized(data):
62+
return _make_item(data, WORK_ITEM_STATUS_FINALIZED,
63+
WORK_LINE_STATUS_APPROVED, REVIEW_STAGE_ADMIN_FINAL)
64+
65+
66+
def _submitted(data):
67+
return _make_item(data, WORK_ITEM_STATUS_SUBMITTED,
68+
WORK_LINE_STATUS_PENDING, REVIEW_STAGE_APPROVAL_GROUP)
69+
70+
71+
def _login(client, user_id):
72+
with client.session_transaction() as sess:
73+
sess["active_user_id"] = user_id
74+
75+
76+
def test_finalized_item_keeps_a_link_to_the_line(app, client, seed_workflow_data):
77+
_finalized(seed_workflow_data)
78+
_login(client, "test:admin")
79+
80+
resp = client.get(DETAIL_URL)
81+
assert resp.status_code == 200
82+
html = resp.get_data(as_text=True)
83+
84+
# The board hold is what the team reported against.
85+
assert "Pending FY Budget Approval" in html
86+
assert LINE_ACTION.search(html), "no per-line link on a finalized request"
87+
88+
89+
def test_finalized_line_link_is_labelled_view(app, client, seed_workflow_data):
90+
_finalized(seed_workflow_data)
91+
_login(client, "test:admin")
92+
93+
html = client.get(DETAIL_URL).get_data(as_text=True)
94+
assert LINE_ACTION.search(html).group(1) == "View"
95+
96+
97+
def test_finalized_item_hides_quick_review(app, client, seed_workflow_data):
98+
_finalized(seed_workflow_data)
99+
_login(client, "test:admin")
100+
101+
html = client.get(DETAIL_URL).get_data(as_text=True)
102+
assert "quick-review" not in html
103+
104+
105+
def test_submitted_item_still_shows_review_and_quick_review(app, client, seed_workflow_data):
106+
_submitted(seed_workflow_data)
107+
_login(client, "test:admin")
108+
109+
html = client.get(DETAIL_URL).get_data(as_text=True)
110+
assert LINE_ACTION.search(html).group(1) == "Review"
111+
assert "quick-review" in html

tests/integration/test_group_subtotals_view.py

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@
44
WorkItem, WorkLine, BudgetLineDetail, ApprovalGroup, UserRole,
55
REQUEST_KIND_PRIMARY, WORK_ITEM_STATUS_SUBMITTED,
66
WORK_LINE_STATUS_PENDING, WORK_LINE_STATUS_APPROVED,
7-
REVIEW_STAGE_APPROVAL_GROUP, ROLE_APPROVER,
7+
WORK_LINE_STATUS_REJECTED,
8+
REVIEW_STAGE_APPROVAL_GROUP, REVIEW_STAGE_ADMIN_FINAL, ROLE_APPROVER,
89
)
910

1011

@@ -216,4 +217,54 @@ def test_approved_subtotals_render_when_approved_column_shown(app, client, seed_
216217

217218
assert "Tech Team (" in html
218219
assert "Hotel Team (" in html
219-
assert "$350.00" in html # approved subtotal for the TECH group
220+
assert "$350.00" in html # approved subtotal for the TECH group
221+
222+
def test_admin_final_rejection_shows_approved_column(app, client, seed_workflow_data):
223+
"""A rejected line needs the Approved column to show its $0.00.
224+
225+
Regression: the column gate tested totals.approved > 0, so an item whose
226+
only admin decision was a rejection rendered requested amounts alone.
227+
"""
228+
data = seed_workflow_data
229+
tech = data["approval_group"]
230+
work_item = _make_multi_group_item(data, [
231+
(tech.id, 500_00, 1), # 500.00 still pending
232+
(tech.id, 76_00, 6), # 456.00 rejected by the admin
233+
])
234+
235+
rejected = WorkLine.query.filter_by(
236+
work_item_id=work_item.id, line_number=2,
237+
).first()
238+
rejected.status = WORK_LINE_STATUS_REJECTED
239+
rejected.current_review_stage = REVIEW_STAGE_ADMIN_FINAL
240+
rejected.approved_amount_cents = None
241+
db.session.commit()
242+
243+
with client.session_transaction() as sess:
244+
sess["active_user_id"] = "test:admin"
245+
246+
resp = client.get("/TST2026/TESTDEPT/budget/item/TST2026-TESTDEPT-BUD-1")
247+
assert resp.status_code == 200
248+
html = resp.get_data(as_text=True)
249+
250+
assert ">Approved</th>" in html
251+
assert "Difference:" in html
252+
253+
254+
def test_undecided_request_hides_approved_column(app, client, seed_workflow_data):
255+
"""No decisions yet means no Approved column; the gate must stay narrow."""
256+
data = seed_workflow_data
257+
tech = data["approval_group"]
258+
_make_multi_group_item(data, [
259+
(tech.id, 500_00, 1),
260+
(tech.id, 76_00, 6),
261+
])
262+
263+
with client.session_transaction() as sess:
264+
sess["active_user_id"] = "test:admin"
265+
266+
resp = client.get("/TST2026/TESTDEPT/budget/item/TST2026-TESTDEPT-BUD-1")
267+
assert resp.status_code == 200
268+
html = resp.get_data(as_text=True)
269+
270+
assert ">Approved</th>" not in html

0 commit comments

Comments
 (0)