Skip to content

feat: show form item Additional Input Fields in the Purchase Details lines view - #1074

Merged
smarcet merged 3 commits into
masterfrom
feature/purchase-details-lines-additional-fields
Sep 17, 2026
Merged

smarcet merged 3 commits into
masterfrom
feature/purchase-details-lines-additional-fields

Conversation

@caseylocker

Copy link
Copy Markdown

ref: https://app.clickup.com/t/86bc2ekbd

What this does

Show organizers use the Purchase Details report (Sponsors > Reports > Purchase Details, Lines view) to fulfil sponsor orders on site. The answers sponsors give in a form item's Additional Input Fields (chair color, labor start time, number of people) are part of what has to be delivered, but the Lines view rendered a fixed set of 13 columns and would have silently ignored them.

This adds an Additional Fields column so those answers are visible next to each line on screen and on the printed manifest, not only in the CSV export.

How

Three files, purely additive presentation:

  • src/components/sponsors/reports/LinesManifestView.js: a module-private AdditionalFields cell component, one HEADERS entry after col_notes, one <TableCell> after the notes cell.
  • src/i18n/en.json: sponsor_reports_page.col_additional_fields, wording matched to the backend CSV header so the screen, the print view and the export all name the column the same thing.
  • the Lines view component tests.

No reducer or action change was needed, and I confirmed that rather than assuming it: buildPurchaseLinesQuery sends no sparse-fieldset param and the lines reducer destructures data wholesale, so the new field reaches the cell untouched. That was the one thing that could have made this a no-op in production.

API contract

Each row of GET purchase-details/lines carries additional_fields, either null or a list of {label, value}. Both are always strings and arrive display-ready: option text already resolved to the option name, DateTime already formatted in UTC, CheckBox already Yes or No. The key can also be absent on older rows.

The UI deliberately does not format or translate the values. The API owns the display format so the screen, the print view and the CSV always agree. Field labels are sponsor data, so they are correctly not run through i18n.

Shipped in sponsor-reports-api https://github.com/fntechgit/sponsor-reports-api/pull/43.

Three decisions worth a reviewer's attention

The cell component lives in LinesManifestView.js, not its own file, and is not exported. The repo's conventions fold a single-consumer helper into the component that owns the concept, and this file is the cited exemplar of that rule. It is private because nothing outside the file consumes it, matching the in-file lineTotalLabel precedent; Destination is exported only because ByItemView imports it.

The React key is index based, with the eslint disable kept. An answer carries no stable id and a form may legitimately define two fields sharing a label, so a label key is not safe. This file already carries the identical disable on its row key for the identical reason. The disable sits inside the tag directly above key= because the one line form exceeds prettier's printWidth, and after prettier rewraps it the disable no longer applies and eslint --fix hard errors.

No width cap on the new cell. The stated consistency target is the Notes cell, which is a bare <TableCell> with no width or wrap styling, so consistency means adding none. MUI cells already wrap.

Tests

Two new tests, plus one existing test updated.

  • the populated case: both fields render as "Label: value", one element each, in the order received
  • the empty case: null, an empty list, and the key being absent all render an empty cell without throwing
  • the existing header and cell cardinality guard moved from 13 to 14. It was updated rather than weakened, because it is the only thing that catches a HEADERS versus cells desync, which would silently misalign every column to its right.

A deliberate trade on test coverage. Two of the ticket's acceptance criteria, values rendering markup literally and a canceled line's cell being struck through, have no direct unit test. Both are satisfied by not writing something: React escapes by default, and the strikethrough comes from the pre-existing row level & td sx, which needed no new code. A test whose subject is framework behavior is what the testing playbook's "What NOT to Test" rules out, so an acceptance criterion does not automatically earn a test here. Raising it explicitly rather than leaving the gap silent.

Verification

  • yarn test src/components/sponsors/reports: 15 suites, 121 tests, all passing
  • yarn lint on the changed files: 0 errors
  • prettier: clean on all three files
  • the new i18n key is wired both ways, one reference and one catalog entry
  • convention pass run against skills/react-frontend.md sections 1 through 8 and the show-admin playbooks for i18n, testing, list pages and reuse before build: no violations

Two notes on the wider suite. The full yarn test run has one failure, in sponsor-purchases-list.test.js, which reproduces on a clean master checkout and is not from this branch. And the ticket's criterion "yarn lint reports 0 errors" is not satisfiable as written: eslint src reports over a thousand pre-existing errors repo wide, and what CI actually gates is lint-staged on changed files, which are clean. Worth rewording that criterion on the ticket.

Still open on the ticket

Two acceptance criteria need a deployed sponsor-reports-api and stay open after this merges:

  • verifying the screen in a browser against an environment returning additional_fields
  • verifying the print preview, specifically for clipping

Print is the real risk and it is not unit testable. TableContainer gives horizontal scrolling on screen, but in print the report body is absolutely positioned at full width, so overflow clips rather than scrolls, and this is now column 14. The browser check should look specifically at clipping, not merely at whether the column appears.

Please do not close the ticket on this PR alone.

Out of scope

  • the By Item view, which is a per item aggregate and does not show per line detail such as Notes either
  • the Orders view
  • filtering, sorting or grouping by an additional field value
  • the CSV export, which the sponsor-reports-api ticket already covers
  • printing the full filtered report rather than the loaded page, an existing limitation
  • enforcing the required flag on additional fields, which is separate purchases-api and sponsor-services work

@coderabbitai

coderabbitai Bot commented Sep 17, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 07c9ba8a-bcf8-4796-a36b-509b5f1dbe9a


Comment @coderabbitai help to get the list of available commands.

@smarcet smarcet left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smarcet
smarcet merged commit 93a462d into master Sep 17, 2026
9 checks passed
smarcet pushed a commit that referenced this pull request Sep 17, 2026
…lines view (#1074)

* feat: show additional input fields in the purchase details lines view

* test: name the canceled-line additional fields test for what it asserts

* refactor: trim additional fields comments and drop two framework-level tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants