asf-tooling commented on issue #1187:
URL:
https://github.com/apache/tooling-trusted-releases/issues/1187#issuecomment-4409738605
<!-- gofannon-issue-triage-bot v2 -->
**Automated triage** — analyzed at `main@2da7807a`
**Type:** `refactor` • **Classification:** `actionable` •
**Confidence:** `medium`
**Application domain(s):** `release_lifecycle`, `automated_checks`, `voting`
### Summary
The issue requests making the compose and vote check result file tables
visually consistent. Currently the compose page (rendered via
`check-selected-path-table.html` template) color-codes filenames by highest
severity, while the checks page (`atr/get/checks.py` `_render_checks_table`)
uses columns for Pass/Warning/Error. The proposal is to adopt the column
approach everywhere and remove the Warning column (pending outcome definitions
from issue #851). The key code for the column-based table is in
`atr/get/checks.py`, but the compose template source is not available in the
provided files.
### Where this lives in the code today
#### `atr/get/checks.py` — `_render_checks_table` (lines 297-325)
_needs modification_
This is the vote/checks page table with Pass/Warning/Error columns - the
Warning column should be removed per the issue.
```python
def _render_checks_table(
page: htm.Block,
release: sql.Release,
paths: list[safe.RelPath],
per_file_stats: dict[safe.RelPath, FileStats],
) -> None:
if not paths:
page.div(".alert.alert-info")["This release candidate does not have
any files."]
return
table = htm.Block(htpy.table,
classes=".table.table-striped.align-middle.table-sm.mb-0.border")
thead = htm.Block(htpy.thead, classes=".table-light")
thead.tr[
htpy.th(".py-2.ps-3")["Path"],
htpy.th(".py-2.text-center", style="width: 5em")["Pass"],
htpy.th(".py-2.text-center", style="width: 5em")["Warning"],
htpy.th(".py-2.text-center", style="width: 5em")["Error"],
htpy.th(".py-2.text-end.pe-3")[""],
]
table.append(thead.collect())
tbody = htm.Block(htpy.tbody)
for path in paths:
stats = per_file_stats.get(path, FileStats(0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0))
_render_file_row(tbody, release, path, stats)
table.append(tbody.collect())
page.div(".table-responsive.card.mb-4")[table.collect()]
```
#### `atr/get/checks.py` — `_render_file_row` (lines 409-422)
_needs modification_
Each file row renders pass/warn/err cells - warning cell needs to be removed
and logic adjusted.
```python
def _render_file_row(
tbody: htm.Block,
release: sql.Release,
path: safe.RelPath,
stats: FileStats,
) -> None:
path_str = str(path)
num_style = "font-size: 1.1rem;"
pass_count = stats.file_pass_after
warn_count = stats.file_warn_after
err_count = stats.file_err_after
has_checks_before = (stats.file_pass_before + stats.file_warn_before +
stats.file_err_before) > 0
has_checks_after = (pass_count + warn_count + err_count) > 0
```
#### `atr/get/vote.py` — `_render_section_checks` (lines 447-461)
_needs modification_
The vote page summary section shows pass/warning/error counts - warning
display should be removed to match the proposed column structure.
```python
def _render_section_checks(page: htm.Block, release: sql.Release,
file_totals: checks.FileStats) -> None:
import atr.get.checks as checks
page.h2("#checks")["2. Review file checks"]
page.p["ATR has checked this release candidate with the following
results:"]
summary = htm.Block(htm.div, classes=".card.mb-4")
summary.div(".card-header.bg-light")["Automated checks"]
body = htm.Block(htm.div, classes=".card-body")
pass_count = file_totals.file_pass_after
warn_count = file_totals.file_warn_after
err_count = file_totals.file_err_after
```
#### `atr/get/checks.py` — `FileStats` (lines 48-60)
_needs modification_
FileStats tracks warning counts separately - if warnings are being removed
as a display concept, this may need restructuring depending on how #851
outcomes are adopted.
```python
class FileStats(NamedTuple):
file_pass_before: int
file_warn_before: int
file_err_before: int
file_pass_after: int
file_warn_after: int
file_err_after: int
member_pass_before: int
member_warn_before: int
member_err_before: int
member_pass_after: int
member_warn_after: int
member_err_after: int
```
### Where new code would go
- `templates/check-selected-path-table.html` — existing file (not provided)
This Jinja2 template renders the compose file table and would need to be
rewritten to use column-based layout matching the checks page approach.
### Proposed approach
The change has two parts: (1) Remove the Warning column from the checks page
table in `atr/get/checks.py`, and (2) Make the compose page file table (in the
Jinja2 template `check-selected-path-table.html`) use the same column-based
approach instead of color-coding filenames.
For part 1, modify `_render_checks_table` to remove the Warning header
column and `_render_file_row` to remove warning cells. Warning counts could be
folded into error counts or simply not displayed, depending on the outcome of
#851. For part 2, the template needs to be rewritten but I don't have its
source. The compose page would also need to compute and pass per-file stats
(similar to how `_compute_stats` works in the checks page) rather than relying
solely on the `PathInfo` color-coding approach. The vote page summary in
`_render_section_checks` should also drop the warning display for consistency.
### Suggested patches
#### `atr/get/checks.py`
Remove the Warning column from the checks table header and file rows, making
the table show only Pass and Error columns (plus path and actions).
````diff
--- a/atr/get/checks.py
+++ b/atr/get/checks.py
@@ -271,7 +271,6 @@
thead.tr[
htpy.th(".py-2.ps-3")["Path"],
htpy.th(".py-2.text-center", style="width: 5em")["Pass"],
- htpy.th(".py-2.text-center", style="width: 5em")["Warning"],
htpy.th(".py-2.text-center", style="width: 5em")["Error"],
htpy.th(".py-2.text-end.pe-3")[""],
]
@@ -306,10 +305,9 @@
path_str = str(path)
num_style = "font-size: 1.1rem;"
pass_count = stats.file_pass_after
- warn_count = stats.file_warn_after
err_count = stats.file_err_after
- has_checks_before = (stats.file_pass_before + stats.file_warn_before +
stats.file_err_before) > 0
- has_checks_after = (pass_count + warn_count + err_count) > 0
+ has_checks_before = (stats.file_pass_before + stats.file_warn_before +
stats.file_err_before) > 0
+ has_checks_after = (pass_count + stats.file_warn_after + err_count) > 0
report_url = util.as_url(
report.selected_path,
@@ -332,26 +330,22 @@
if not has_checks_before:
path_display = htpy.code(".text-muted")[path_str]
pass_cell = htpy.span(".text-muted", style=num_style)["-"]
- warn_cell = htpy.span(".text-muted", style=num_style)["-"]
err_cell = htpy.span(".text-muted", style=num_style)["-"]
report_btn =
htpy.span(".btn.btn-sm.btn-outline-secondary.disabled")["No checks"]
elif not has_checks_after:
path_display = htpy.code[path_str]
pass_cell = htpy.span(".text-muted", style=num_style)["0"]
- warn_cell = htpy.span(".text-muted", style=num_style)["0"]
err_cell = htpy.span(".text-muted", style=num_style)["0"]
report_btn = htpy.a(".btn.btn-sm.btn-outline-secondary",
href=report_url)["Show details"]
elif err_count > 0:
path_display = htpy.strong[htpy.code(".text-danger")[path_str]]
pass_cell = (
htpy.span(".text-success", style=num_style)[str(pass_count)]
if (pass_count > 0)
else htpy.span(".text-muted", style=num_style)["0"]
)
- warn_cell = (
- htpy.span(".text-warning", style=num_style)[str(warn_count)]
- if (warn_count > 0)
- else htpy.span(".text-muted", style=num_style)["0"]
- )
err_cell = htpy.span(".text-danger.fw-bold",
style=num_style)[str(err_count)]
report_btn = htpy.a(".btn.btn-sm.btn-outline-danger",
href=report_url)["Show details"]
- elif warn_count > 0:
- path_display = htpy.strong[htpy.code(".text-warning")[path_str]]
- pass_cell = (
- htpy.span(".text-success", style=num_style)[str(pass_count)]
- if (pass_count > 0)
- else htpy.span(".text-muted", style=num_style)["0"]
- )
- warn_cell = htpy.span(".text-warning.fw-bold",
style=num_style)[str(warn_count)]
- err_cell = htpy.span(".text-muted", style=num_style)["0"]
- report_btn = htpy.a(".btn.btn-sm.btn-outline-warning",
href=report_url)["Show details"]
else:
path_display = htpy.code[path_str]
pass_cell = htpy.span(".text-success",
style=num_style)[str(pass_count)]
- warn_cell = htpy.span(".text-muted", style=num_style)["0"]
err_cell = htpy.span(".text-muted", style=num_style)["0"]
report_btn = htpy.a(".btn.btn-sm.btn-outline-success",
href=report_url)["Show details"]
@@ -380,7 +364,6 @@
tbody.tr[
htpy.td(".py-2.ps-3")[path_display],
htpy.td(".py-2.text-center")[pass_cell],
- htpy.td(".py-2.text-center")[warn_cell],
htpy.td(".py-2.text-center")[err_cell],
htpy.td(".text-end.text-nowrap.py-2.pe-3")[
htpy.div(".d-flex.justify-content-end.align-items-center.gap-2")[
````
#### `atr/get/vote.py`
Remove the warning display from the vote page checks summary to match the
table column removal.
````diff
--- a/atr/get/vote.py
+++ b/atr/get/vote.py
@@ -298,7 +298,6 @@
pass_count = file_totals.file_pass_after
- warn_count = file_totals.file_warn_after
err_count = file_totals.file_err_after
check_word = util.plural(pass_count, "check", include_count=False)
- warn_word = util.plural(warn_count, "warning", include_count=False)
err_word = util.plural(err_count, "error", include_count=False)
checks_list = htm.Block(htm.div, classes=".d-flex.flex-wrap.gap-4.mb-3")
@@ -306,14 +305,6 @@
htpy.i(".bi.bi-check-circle-fill.me-2"),
f"{pass_count} {check_word} passed",
]
- if warn_count > 0:
- checks_list.span(".text-warning")[
- htpy.i(".bi.bi-exclamation-triangle-fill.me-2"),
- f"{warn_count} {warn_word}",
- ]
- else:
- checks_list.span(".text-muted")[
- htpy.i(".bi.bi-exclamation-triangle.me-2"),
- "0 warnings",
- ]
if err_count > 0:
checks_list.span(".text-danger")[
htpy.i(".bi.bi-x-circle-fill.me-2"),
````
### Open questions
- The compose page file table is rendered by
`check-selected-path-table.html` (a Jinja2 template not provided in the source
files). This template needs to be rewritten to use a column-based layout. What
does the current template look like?
- Issue #851 proposes new check result outcomes (possibly five statuses).
Until those are finalized, it's unclear exactly which columns should remain.
Should this change wait for #851 to be resolved?
- When warnings are removed as a column, should warning counts be folded
into error counts, or should warnings simply not appear in the table at all
(only visible in detailed report)?
- The `shared.web.render_checks_summary` function (used by the compose page)
also likely shows warnings - its source was not provided but may also need
modification.
- Should `FileStats` be refactored to remove warning fields, or kept for
internal computation even though warnings aren't displayed in the table?
### Files examined
- `atr/get/compose.py`
- `atr/get/vote.py`
- `atr/get/checks.py`
- `atr/storage/readers/checks.py`
- `atr/storage/readers/releases.py`
- `atr/tasks/checks/__init__.py`
### Related issues
This issue appears related to: #1202.
_Both address UI consistency improvements across phase pages and result
tables_
---
*Draft from a triage agent. A human reviewer should validate before merging
any change. The agent did not run tests or verify diffs apply.*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
