forked from OpenStackweb/summit-admin
-
Notifications
You must be signed in to change notification settings - Fork 4
fix(sponsor-reports): make Purchase Details describe itself on every view #1046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
3e3f98a
fix(sponsor-reports): keep Payment Method applied across grain switches
caseylocker d545ce5
docs(sponsor-reports): fix second stale paymentMethod-drop comment
caseylocker 552b469
fix(sponsor-reports): show the line's own state in the manifest
caseylocker dbc5521
test(sponsor-reports): strengthen manifest freshness and live-count a…
caseylocker 5046275
fix(sponsor-reports): By Item drill-down shows line state and freshness
caseylocker d7885ea
test(sponsor-reports): pin By Item chip copy and drill-down column pa…
caseylocker 1d95ea3
feat(sponsor-reports): surface refunded amount and freshness on Orders
caseylocker 3afa8b0
fix(sponsor-reports): right-align refunded amount, pin full Orders he…
caseylocker c8562c9
fix(sponsor-reports): label the gross total and the canceled default
caseylocker b9c1c39
fix(sponsor-reports): flow the status helper text in-page instead of …
caseylocker 5eb2fa9
fix(sponsor-reports): reword lines chip, add header/cell guard, fix t…
caseylocker e34b5a6
test(sponsor-reports): pin the lines_count copy in the catalog
caseylocker 0fab48b
fix(sponsor-reports): announce the canceled-default note with its con…
caseylocker e077974
fix(sponsor-reports): carry the canceled-default note as a hover info…
caseylocker c8bc53f
feat(sponsor-reports): give canceled rows their own filter axis
caseylocker d0b0ec6
refactor(sponsor-reports): resolve line status in the mapper, trim co…
caseylocker File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caseylocker This branch cannot render once sponsor-reports-api#40 is deployed: the only
line rows that reach it are ones whose parent order is already Canceled, where both
branches print the same label with the same tone.
include_cancelledis written in exactly one place —sponsor-reports-actions.js:226,if (status === "Canceled")— so asking for canceled rows also emitsstatus==Canceled,which
PurchaseLineDetailsFilterresolves topurchase__status(
purchase_line_details_filter.py:20). A soft-canceled line on a Paid order — the exactrow this change targets — is excluded by default, then excluded again by the status clause
the moment you try to un-hide it.
statusTone("Canceled")returnsdefaulteither way, soin every reachable state this renders identically to the previous
line.purchase?.status.The API deliberately made this a separate axis, and it is already exercised there:
test_include_cancelled_true_restores_line_level_canceled_items(
apps/reports/tests/test_purchase_details_lines_endpoints.py:123) sends?include_cancelled=truewith no status filter and gets AV3 back —is_canceled: trueinside a Paid order. The fixture comment states it directly: "NO status selection could
ever remove it -- line-level cancellation is a separate axis". The frontend is the only
side coupling the two.
Suggested fix: give
include_cancelledits own control rather than deriving it from thestatus value — a "Show canceled" checkbox in
FilterBar, carried as its own filter key andemitted independently in
buildReportQuery. That is the shape this repo already uses forshowArchived(.claude/rules/summit-admin-archive-pattern.md). The current coupling stayscorrect at order grain; only the line grain needs the extra axis. The same fix makes the
By Item contributor branch at
ByItemView.js:467reachable, and lets the Purchase Statusnote describe both axes truthfully on the line views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and fixed in c8bc53f.
You're right that the branch was unreachable.
include_cancelledhad exactly one write site,sponsor-reports-actions.js:226, andbuildReportQueryis the single builder for the details, lines, By Item and both CSV paths, so the coupling closed the axis everywhere at once. Every row that could reach the pill already hadpurchase.status === "Canceled", and sinceStatusPillmaps both spellings of canceled to tonedefaultandstatus_canceledis literally "Canceled", the two arms rendered an identical chip. Same forByItemView.js:467.Took the separate control. One change to the shape you suggested: the status coupling stays, as an OR rather than a replacement.
Emitting
include_cancelledonly from the checkbox would regress the Orders view.purchase_details_report_view.py:44excludesstatus='Canceled'by default, so a Canceled selection without the flag returns zero rows and the dropdown option goes dead. There's a test pinning that direction specifically, so a later cleanup can't quietly drop it.The checkbox is on every grain, not the line views only. Filters are carried across a view switch, so a control that appears and vanishes would leave a live filter behind an invisible control, which is the same silent-filter class this report set out to fix.
Also updated the Purchase Status note per your last point: it's now grain-aware, since the line grains hide a second kind of row the order grain has no concept of.
One correction on the citation:
.claude/rules/summit-admin-archive-pattern.mdisn't in this repo, there's no.claude/directory here. The doc lives in the fn-skills vault atpatterns/show-admin/summit-admin-archive-pattern.md. The precedent itself is real in-repo,showArchivedinform-template-actions.js,show-pages-actions.jsand others, and the shape matched, so the checkbox follows it.Full suite 1528 passing.