Namespace ticket CSV column families + shared value separator for export/import round-trip#569
Namespace ticket CSV column families + shared value separator for export/import round-trip#569JpMaxMan wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates ticket CSV export/import to use ChangesTicket CSV Column Namespacing
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-569/ This page is automatically updated on each push to this PR. |
JpMaxMan
force-pushed
the
feature/ticket-csv-column-prefixes
branch
from
a5ea1f9 to
21ecf73
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-569/ This page is automatically updated on each push to this PR. |
JpMaxMan
force-pushed
the
feature/ticket-csv-column-prefixes
branch
from
21ecf73 to
c3ac0d1
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-569/ This page is automatically updated on each push to this PR. |
JpMaxMan
force-pushed
the
feature/ticket-csv-column-prefixes
branch
from
c3ac0d1 to
7b22c5c
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-569/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Pull request overview
This PR aligns ticket CSV export/import semantics to support lossless round-trips by (1) namespacing additive “column families” and (2) sharing a consistent multi-value separator between export and import, while extending import to recognize extra questions by label (to match exported headers).
Changes:
- Namespaced badge feature columns as
badge_feature:{name}(while preserving legacy bare feature-name columns on import). - Exported extra-question headers as
extra_question:{label}and updated import to match questions by name or label. - Export now formats multi-value answers using the shared
|separator and resolves each stored value ID to its label.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/SummitOrderServiceTest.php |
Adds regression tests for namespaced badge-feature columns, label-based extra-question matching, and multi-value round-trip. |
app/Services/Model/ISummitOrderService.php |
Introduces shared constants for column prefixes and multi-value separator. |
app/Services/Model/Imp/SummitOrderService.php |
Updates import to accept badge_feature: columns, match extra questions by label, and use shared multi-value separator. |
app/ModelSerializers/Summit/Registration/SummitAttendeeTicketCSVSerializer.php |
Updates export headers to use namespaced prefixes and formats list answers with ` |
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitTicketApiController.php |
Updates template generation and OpenAPI descriptions to reflect namespaced badge-feature columns and label acceptance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $question = $summit->getOrderExtraQuestionByName($question_name); | ||
| // fall back to the label: the ticket csv export emits question labels as column names | ||
| if (is_null($question)) | ||
| $question = $summit->getOrderExtraQuestionByLabel($question_name); |
| ]; | ||
| }, | ||
| function () use ($summit) { | ||
| $allowed_columns = [ |
There was a problem hiding this comment.
Export's columns= query-param allowlist wasn't updated to the new namespaced keys. $allowed_columns
still pushes bare $featuresType->getName() / $question->getLabel(), never the badge_feature:/extra_question: prefixed forms the serializer now emits. Confirmed via full call-trace: the
default (no columns=) export is unaffected since CSVExporter::export() ignores this list entirely and just uses the serialized row's actual keys — but a client explicitly requesting
?columns=badge_feature:VIP (the real, current column name) gets rejected with a ValidationException since that string isn't in the stale allowlist.
There was a problem hiding this comment.
Fixed in 970b38a — both allowlist loops now emit the namespaced forms, and the question entries use the sanitized label (html_entity_decode(strip_tags(...))) so they match what the serializer actually emits (the raw-label form in the old allowlist was already drifting from the emitted header for HTML-bearing labels). Confirmed your call-trace read: the default no-columns= path was unaffected either way.
| @@ -4781,6 +4785,9 @@ private function resolveExtraQuestionColumns(Summit $summit, SummitAttendee $att | |||
| } | |||
|
|
|||
| $question = $summit->getOrderExtraQuestionByName($question_name); | |||
There was a problem hiding this comment.
Extra-question name→label fallback can silently mis-file an answer to the wrong question. getOrderExtraQuestionByName() and the new getOrderExtraQuestionByLabel() fallback are independent lookups with no cross-check. If Question A's name equals Question B's label (both plausible, free-text fields with no cross-field uniqueness constraint), a CSV column extra_question:{that string} — including one produced by this PR's own export, which always emits labels as headers — resolves to Question A via the name-match and never reaches the label fallback intended for B. The answer is silently written to the wrong question; no warning is logged since a question was found. This directly undermines the PR's stated round-trip goal. (Independently confirmed by 3 of the 6 finder agents via the same mechanism.)
There was a problem hiding this comment.
Fixed in 970b38a — agreed silent mis-file is the worst outcome for an importer, and reordering the lookups would just mirror the failure mode. The fix runs both lookups side by side: when name-match and label-match disagree, the column is logged as ambiguous (naming both question ids) and skipped entirely. Regression test added (testImportTicketDataSkipsAmbiguousExtraQuestionColumn): question A's name == question B's label → the ambiguous column touches neither question, row still processes.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-569/ This page is automatically updated on each push to this PR. |
| // column names ); when the two lookups disagree the column is ambiguous — one | ||
| // question's name is another question's label — skip it rather than silently | ||
| // filing the answer under the wrong question | ||
| $question = $summit->getOrderExtraQuestionByName($question_name); |
There was a problem hiding this comment.
@JpMaxMan The ambiguity check runs before the usage/eligibility filters, causing false-positive skips. The name-vs-label collision check
(lines 4791-4806) executes before the OrderQuestionUsage check (line 4822) and isAllowedQuestion() check (line 4834). So if a legitimately-matched Ticket-usage question's name collides
with the label of an unrelated Order-scoped (or attendee-disallowed) question, the column is now flagged "ambiguous" and dropped entirely — even though the label-matched question would
have been rejected by the usage/eligibility check moments later anyway and was never a real candidate. This is a regression in import completeness introduced by the fix: previously
(name-match-only, no fallback) this exact column would have imported correctly. No test exercises this cross-usage case — only same-usage true collisions are covered.
There was a problem hiding this comment.
Good catch — fixed in 4a69e57. The name/label lookups now build a candidate list that's vetted through the existing order-scoped and isAllowedQuestion() rejections first; a column is only skipped as ambiguous when two eligible questions survive, so an ineligible collision partner can no longer shadow a legitimate match. Added testImportTicketDataIneligibleQuestionDoesNotMakeColumnAmbiguous covering the cross-usage case (ticket-usage question named X vs order-scoped question labeled X → resolves to the ticket-usage question).
Worth noting for severity: order-scoped questions aren't surfaced anywhere in the registration FE today, so that half of the path is defensive — the realistic leg was the isAllowedQuestion() one, which is now also decided per-candidate before ambiguity.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-569/ This page is automatically updated on each push to this PR. |
smarcet
changed the base branch from
feature/ticket-import-extra-question-answers
to
main
…ator for export/import round-trip
Adopts badge_feature:{name} alongside extra_question:{name} uniformly
across the import template, the import (bare feature names still
accepted for legacy CSVs) and the ticket CSV export; import matches
questions by label since export emits labels; multi-value answer cells
now export with the import's '|' separator (per-id label resolution, so
comma-bearing labels stay intact). Closes the export->import round-trip
gap raised in #567 review.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ion-column skip Per review: the columns= allowlist now emits the namespaced forms (sanitized labels, matching the serializer exactly), and import matching runs the name and label lookups side by side — a column that is one question's name and another's label is logged as ambiguous and skipped instead of silently filed under the name match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…kip on ticket import
smarcet
force-pushed
the
feature/ticket-csv-column-prefixes
branch
from
4a69e57 to
f9198e5
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-569/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Services/Model/Imp/SummitOrderService.php (1)
4910-4910: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRound-trip is not faithful when a choice label itself contains the
|separator.Export joins multi-values with
|and import splits on|here, so a value whose name/label contains|(e.g.A|B) becomes ambiguous tokens on re-import. The PR already handles labels containing commas via per-value resolution; labels containing the separator itself are the remaining gap. Likely rare, but worth confirming such labels can't be created (or documenting the constraint).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Services/Model/Imp/SummitOrderService.php` at line 4910, The round-trip parsing in SummitOrderService::importExtraQuestionValue is ambiguous because it splits exported multi-values on the same ExtraQuestionValueSeparator used for joining, so labels containing “|” cannot be faithfully re-imported. Update the import/export handling around the explode(self::ExtraQuestionValueSeparator, $value) logic to either reject or escape separator characters in choice labels, or resolve values without relying on raw separator splitting; make the constraint explicit if such labels are not allowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/ModelSerializers/Summit/Registration/SummitAttendeeTicketCSVSerializer.php`:
- Around line 159-171: In formatAnswerValue, the current empty($label) check
will incorrectly drop a legitimate label of "0" from exported CSV values.
Replace that loose emptiness check with a strict null/empty-string validation so
only missing labels are skipped while preserving valid "0" labels. Keep the rest
of the label collection logic in
SummitAttendeeTicketCSVSerializer::formatAnswerValue unchanged.
---
Nitpick comments:
In `@app/Services/Model/Imp/SummitOrderService.php`:
- Line 4910: The round-trip parsing in
SummitOrderService::importExtraQuestionValue is ambiguous because it splits
exported multi-values on the same ExtraQuestionValueSeparator used for joining,
so labels containing “|” cannot be faithfully re-imported. Update the
import/export handling around the explode(self::ExtraQuestionValueSeparator,
$value) logic to either reject or escape separator characters in choice labels,
or resolve values without relying on raw separator splitting; make the
constraint explicit if such labels are not allowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19113cca-2a9f-4085-8976-03418d9cc03b
📒 Files selected for processing (5)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitTicketApiController.phpapp/ModelSerializers/Summit/Registration/SummitAttendeeTicketCSVSerializer.phpapp/Services/Model/ISummitOrderService.phpapp/Services/Model/Imp/SummitOrderService.phptests/SummitOrderServiceTest.php
| private static function formatAnswerValue(SummitOrderExtraQuestionType $question, string $value): string | ||
| { | ||
| if (!$question->allowsValues()) | ||
| return strval($question->getNiceValue($value)); | ||
|
|
||
| $labels = []; | ||
| foreach (explode(ExtraQuestionType::QuestionChoicesCharSeparator, $value) as $value_id) { | ||
| $label = $question->getNiceValue(trim($value_id)); | ||
| if (empty($label)) continue; | ||
| $labels[] = $label; | ||
| } | ||
| return implode(ISummitOrderService::ExtraQuestionValueSeparator, $labels); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
empty($label) silently drops a legitimate label equal to "0".
PHP's empty() treats the string "0" as empty (empty("0") === true), so a checkbox/list value whose nice label is literally "0" would be silently excluded from the exported cell, causing data loss during export.
🐛 Proposed fix using a strict check
$labels = [];
foreach (explode(ExtraQuestionType::QuestionChoicesCharSeparator, $value) as $value_id) {
$label = $question->getNiceValue(trim($value_id));
- if (empty($label)) continue;
+ if ($label === null || $label === '') continue;
$labels[] = $label;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static function formatAnswerValue(SummitOrderExtraQuestionType $question, string $value): string | |
| { | |
| if (!$question->allowsValues()) | |
| return strval($question->getNiceValue($value)); | |
| $labels = []; | |
| foreach (explode(ExtraQuestionType::QuestionChoicesCharSeparator, $value) as $value_id) { | |
| $label = $question->getNiceValue(trim($value_id)); | |
| if (empty($label)) continue; | |
| $labels[] = $label; | |
| } | |
| return implode(ISummitOrderService::ExtraQuestionValueSeparator, $labels); | |
| } | |
| private static function formatAnswerValue(SummitOrderExtraQuestionType $question, string $value): string | |
| { | |
| if (!$question->allowsValues()) | |
| return strval($question->getNiceValue($value)); | |
| $labels = []; | |
| foreach (explode(ExtraQuestionType::QuestionChoicesCharSeparator, $value) as $value_id) { | |
| $label = $question->getNiceValue(trim($value_id)); | |
| if ($label === null || $label === '') continue; | |
| $labels[] = $label; | |
| } | |
| return implode(ISummitOrderService::ExtraQuestionValueSeparator, $labels); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/ModelSerializers/Summit/Registration/SummitAttendeeTicketCSVSerializer.php`
around lines 159 - 171, In formatAnswerValue, the current empty($label) check
will incorrectly drop a legitimate label of "0" from exported CSV values.
Replace that loose emptiness check with a strict null/empty-string validation so
only missing labels are skipped while preserving valid "0" labels. Keep the rest
of the label collection logic in
SummitAttendeeTicketCSVSerializer::formatAnswerValue unchanged.
|
@JpMaxMan Deep review pass on this PR, adversarially verified (two independent checks per finding, actively trying to disprove each one before posting). Label sanitization mismatch breaks the round-trip for HTML/entity-bearing labelsExport builds the header as So any question whose label contains HTML tags or entities exports a sanitized header that can never equal the raw stored label. If the question's This isn't a hypothetical edge case:
Proof — this test reproduces the drop (label public function testTicketCSVExportHtmlEntityLabelDoesNotRoundTripThroughImport()
{
Queue::fake();
// label as stored e.g. by the Eventbrite ingestion path (SummitOrderExtraQuestionTypeService
// ::seedSummitOrderExtraQuestionTypesFromEventBrite) or by an admin entering basic markup
// through the normal API (HTMLPurifier permits <b>/<i>/<a>/... by default)
$question = $this->insertOrderExtraQuestion
(
'DIET_REQ',
ExtraQuestionTypeConstants::TextQuestionType,
[],
SummitOrderExtraQuestionTypeConstants::TicketQuestionUsage,
'Café Preference'
);
$attendee = $this->getDefaultAttendee();
$ticket = $attendee->getTickets()->first();
// export: header is the sanitized label ( html_entity_decode(strip_tags(...)) ), not the raw one
$values = SerializerRegistry::getInstance()
->getSerializer($ticket, SerializerRegistry::SerializerType_CSV)
->serialize(null, [], [], [
'ticket_questions' => [$question],
'answers_by_owner' => [$attendee->getId() => [$question->getId() => 'Vegan']],
]);
$question_column = 'extra_question:Café Preference';
$this->assertArrayHasKey($question_column, $values);
$this->assertEquals('Vegan', $values[$question_column]);
// re-import the exact exported header + cell, verbatim, for a new attendee
$unassigned = $this->getUnassignedTicket();
$csv_content = <<<CSV
number,attendee_email,attendee_first_name,attendee_last_name,{$question_column}
{$unassigned->getNumber()},new.attendee@nowhere.com,New,Attendee,Vegan
CSV;
$service = $this->buildTicketDataImportService($csv_content);
$service->processTicketData(self::$summit->getId(), 'tickets.csv');
$new_attendee = App::make(ISummitAttendeeRepository::class)
->getBySummitAndEmail(self::$summit, 'new.attendee@nowhere.com');
$this->assertNotNull($new_attendee);
// getOrderExtraQuestionByLabel() matches the RAW stored label ('Café Preference'),
// which never equals the sanitized exported header ('Café Preference'), so the column is
// treated as an unknown question and the answer is silently dropped
$answer = $new_attendee->getExtraQuestionAnswerByQuestion($question);
$this->assertNotNull($answer, 'expected the CSV export/import round trip to preserve the answer');
$this->assertEquals('Vegan', $answer->getValue());
}Ran against the current branch head ( Suggested fix: apply the same Extra-question column resolution issues two DB queries per column per row
I found this is a deliberate trade-off (an earlier commit on this branch short-circuited the label lookup; a later one removed the short-circuit specifically to fix the ambiguous name/label collision case), and with realistic CSVs having ~1-3 extra-question columns against a small per-summit question table in a background queued job, it's minor, not a hot-path regression. Flagging mainly as a follow-up: consider pre-loading |
…sanitized label, not just the raw stored one Also folds the by-name/by-label resolution into a single per-summit index built once per import job instead of two DB round-trips per extra_question column per row (order_extra_questions is EXTRA_LAZY).
|
@smarcet Both landed in Finding 1 (HTML/entity label mismatch): fixed by folding the by-name and by-label resolution into one per-summit lookup index ( Finding 2 (2x EXTRA_LAZY queries per column per row): resolved as a side effect of the same change — the index is built once per summit (cached on the service instance) instead of two Re-requesting your review on this one — CI is running now. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-569/ This page is automatically updated on each push to this PR. |
…rt of every import job ISummitOrderService is bound as an application singleton (ModelServicesProvider.php:356) -- on a warm queue worker, the per-summit index cache introduced in the previous commit would survive across import jobs, so a question added/edited between two runs for the same summit would resolve against stale in-memory data. Force a rebuild once at the top of processTicketData instead of relying on lazy caching alone.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-569/ This page is automatically updated on each push to this PR. |
|
@JpMaxMan please review unit tests ( CI is red )
/home/runner/work/summit-api/summit-api/tests/SummitOrderServiceTest.php:1088 |
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@JpMaxMan please review unit tests
CI is red
https://github.com/OpenStackweb/summit-api/actions/runs/28919500453/job/85793392737?pr=569#step:10:816
- Tests\SummitOrderServiceTest::testTicketCSVExportHtmlEntityLabelRoundTripsThroughImport
expected the CSV export/import round trip to preserve the answer
Failed asserting that null is not null.
/home/runner/work/summit-api/summit-api/tests/SummitOrderServiceTest.php:1088
ref: https://app.clickup.com/t/86baraz3d
Summary
Follow-up promised on #567's column-naming thread: rather than defend the asymmetry, both additive per-name column families are now namespaced —
badge_feature:{name}joinsextra_question:{name}— uniformly across the import template, the import (bare feature names remain accepted, so existing CSVs keep working), and the ticket CSV export. Import additionally matches questions by label, since export emits sanitized labels as headers.Multi-value answers round-trip too: export cells now use the import's
|separator (sharedISummitOrderService::ExtraQuestionValueSeparatorconst), with each stored value id resolved to its label individually — post-splittinggetNiceValue()'s comma-joined output would corrupt labels containing commas. A CSV exported fromGET /summits/{id}/tickets/csvnow re-imports without any column renaming or cell editing.Note: export headers and multi-value cell separators change. We have no scripted consumers of the export, so no compat shim is included. Remaining edge: a value label that itself contains
|won't round-trip — inherent to any textual separator.Tests
testImportTicketDataAcceptsPrefixedBadgeFeatureColumns(canonical prefixed form; legacy bare form still covered by the existing badge-features test),testImportTicketDataMatchesExtraQuestionByLabel, andtestTicketCSVExportMultiValueAnswerRoundTripsThroughImport— the last one exports a two-value answer and feeds the exported cell verbatim back through the import.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features / Improvements
Bug Fixes