feature (registration): add order extra question answers to ticket CSV import#567
Merged
Merged
Conversation
…port
Adds extra_question:{question name} column support to the ticket data
import, template endpoint and OpenAPI docs. Answers are upserted through
the existing ExtraQuestionAnswerHolder persistence path; unknown
questions, order-scoped questions, disallowed questions, empty values
and locked answers are logged and skipped without failing the row.
List type questions accept value name/label/id ('|' separated for
CheckBoxList) and store value ids.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a standardized ChangesExtra Question CSV Import
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuth2SummitTicketApiController
participant SummitOrderService
participant SummitAttendee
Client->>OAuth2SummitTicketApiController: request ticket CSV template or import
OAuth2SummitTicketApiController->>SummitOrderService: load order extra questions
OAuth2SummitTicketApiController->>Client: template / documented columns
Client->>SummitOrderService: import CSV rows
SummitOrderService->>SummitAttendee: evict allowed badge-features cache
SummitOrderService->>SummitOrderService: resolve extra-question columns
SummitOrderService->>SummitOrderService: upsert attendee extra-question answers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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-567/ This page is automatically updated on each push to this PR. |
The import re-reads tickets with HINT_REFRESH, so it sees DB state: the fixture's assigned tickets share one badge entity whose FK points at only the last of them. Use an unassigned ticket, which gets its own DB-consistent badge from SummitTicketType::applyTo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-567/ 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: 3
🤖 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/Services/Model/Imp/SummitOrderService.php`:
- Around line 4745-4786: The multi-value comparison in
SummitOrderService::upsertAttendeeExtraQuestionAnswers is order-sensitive, so
re-importing the same CheckBoxList selections in a different order can
incorrectly hit the “cannot change” skip. Normalize the selected ids before
comparing by applying a stable ordering to the $value_ids-derived string, and
apply the same normalization to $former_value before the $former_value != $value
check so equivalent answers are treated as unchanged.
- Around line 4806-4841: Wrap the `hadCompletedExtraQuestions()` call in
`SummitOrderService::upsertAttendeeExtraQuestionAnswers` with a
`ValidationException` catch and return early so the current row is skipped
instead of failing the whole per-row transaction. Keep the existing
debug/warning behavior, and handle the exception locally in this method so
`processTicketData()` can continue with later rows.
In `@tests/SummitOrderServiceTest.php`:
- Around line 388-421: The test helper buildTicketDataImportService sets up
$upload_strategy without the save() behavior, but processTicketData() invokes
save() on IFileUploadStrategy during this path. Add the missing save expectation
on the $upload_strategy mock in buildTicketDataImportService so
SummitOrderService can complete the ticket data import flow without Mockery
throwing.
🪄 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: 091dda31-c9fd-417e-beac-e98f68bb7eb4
📒 Files selected for processing (4)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitTicketApiController.phpapp/Services/Model/ISummitOrderService.phpapp/Services/Model/Imp/SummitOrderService.phptests/SummitOrderServiceTest.php
…on answer persistence Per CodeRabbit review: sort list-question value ids so the same selection in a different order is not treated as a changed answer, and catch ValidationException from the extra-question persistence path so one bad payload cannot strand the remaining import rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-567/ This page is automatically updated on each push to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for importing attendee order extra-question answers from the ticket CSV import flow, and exposes corresponding extra_question:{name} columns in the CSV template endpoint so admins can populate these answers via bulk operations.
Changes:
- Extend ticket CSV import to upsert attendee extra-question answers from
extra_question:{question name}columns (including list/checkbox parsing and guardrails). - Extend
GET /summits/{id}/tickets/csv/templateto include oneextra_question:{name}column per relevant order extra question. - Add/expand test coverage around new create/update/skip behaviors for extra-question imports and badge-feature interaction.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/SummitOrderServiceTest.php | Adds helper methods + new tests validating extra-question import behaviors and badge-feature interactions. |
| app/Services/Model/ISummitOrderService.php | Introduces a shared constant for the extra_question: CSV column prefix. |
| app/Services/Model/Imp/SummitOrderService.php | Implements extra-question parsing/upsert during ticket CSV processing. |
| app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitTicketApiController.php | Updates CSV template generation and OpenAPI text to document/include extra-question columns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
smarcet
reviewed
Jul 3, 2026
smarcet
reviewed
Jul 3, 2026
smarcet
reviewed
Jul 3, 2026
smarcet
reviewed
Jul 3, 2026
smarcet
reviewed
Jul 3, 2026
Per review: process badge data before extra questions and flush + evict the badge-features result cache so same-row badge/feature grants are visible to the question permission gates; split the upsert into resolve/merge helpers; ctype_digit for raw value ids; targeted cache forget in tests. Adds regression tests for the same-row feature grant (new + existing attendee), order-scoped skip, locked-answer skip, and badge creation for badge-less tickets (folds in #568). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-567/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Services/Model/Imp/SummitOrderService.php`:
- Around line 4867-4909: The resolved choice ids in
SummitOrderService::resolveListQuestionValue can be added more than once when
the same token appears repeatedly or matches by multiple lookup paths, which
then breaks single-value validation and CheckBoxList comparison. Deduplicate
$value_ids before the count check and before sorting/returning, so the logic in
resolveListQuestionValue stays set-based and matches the equality handling used
by resolveExtraQuestionColumns.
🪄 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: fbf34d7f-bc31-401e-8c6c-5ad8f6cd67df
📒 Files selected for processing (3)
app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.phpapp/Services/Model/Imp/SummitOrderService.phptests/SummitOrderServiceTest.php
…ault badge type SummitTicketType::getBadgeType falls back to the summit default badge type, so on the main fixture summit applyTo always builds a badge and a badge-less ticket cannot be constructed. Build the scenario on the second fixture summit, which has no badge types — the real case the badge creation fix covers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JpMaxMan
added a commit
that referenced
this pull request
Jul 3, 2026
…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>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-567/ This page is automatically updated on each push to this PR. |
SummitAttendeeTicketEmail's constructor requires the summit support email; the second fixture summit never sets one, so the ticket reassignment path threw during the badge-less import test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JpMaxMan
added a commit
that referenced
this pull request
Jul 3, 2026
…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>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-567/ This page is automatically updated on each push to this PR. |
smarcet
reviewed
Jul 4, 2026
smarcet
requested changes
Jul 4, 2026
smarcet
left a comment
smarcet
left a comment
Collaborator
There was a problem hiding this comment.
@JpMaxMan please review
#567 (comment)
…import Duplicated CSV tokens (drag-fill artifacts, or the same choice spelled as name/label/raw id) resolve to the same value id: they falsely tripped the single-value guard on radio/combo questions and persisted duplicated id strings for CheckBoxList, which also resisted later correction under the answer-change lock. Adds both regression tests from review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JpMaxMan
added a commit
that referenced
this pull request
Jul 6, 2026
…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>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-567/ This page is automatically updated on each push to this PR. |
smarcet
pushed a commit
that referenced
this pull request
Jul 7, 2026
…V import (#567) * feat(registration): add order extra question answers to ticket CSV import Adds extra_question:{question name} column support to the ticket data import, template endpoint and OpenAPI docs. Answers are upserted through the existing ExtraQuestionAnswerHolder persistence path; unknown questions, order-scoped questions, disallowed questions, empty values and locked answers are logged and skipped without failing the row. List type questions accept value name/label/id ('|' separated for CheckBoxList) and store value ids. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(registration): fix badge-features import test fixture assumptions The import re-reads tickets with HINT_REFRESH, so it sees DB state: the fixture's assigned tickets share one badge entity whose FK points at only the last of them. Use an unassigned ticket, which gets its own DB-consistent badge from SummitTicketType::applyTo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(registration): normalize list answer ordering + guard import row on answer persistence Per CodeRabbit review: sort list-question value ids so the same selection in a different order is not treated as a changed answer, and catch ValidationException from the extra-question persistence path so one bad payload cannot strand the remaining import rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(registration): review fixes for extra-question CSV import Per review: process badge data before extra questions and flush + evict the badge-features result cache so same-row badge/feature grants are visible to the question permission gates; split the upsert into resolve/merge helpers; ctype_digit for raw value ids; targeted cache forget in tests. Adds regression tests for the same-row feature grant (new + existing attendee), order-scoped skip, locked-answer skip, and badge creation for badge-less tickets (folds in #568). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(registration): badge-less ticket requires a summit without a default badge type SummitTicketType::getBadgeType falls back to the summit default badge type, so on the main fixture summit applyTo always builds a badge and a badge-less ticket cannot be constructed. Build the scenario on the second fixture summit, which has no badge types — the real case the badge creation fix covers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(registration): set support email on summit2 fixture path SummitAttendeeTicketEmail's constructor requires the summit support email; the second fixture summit never sets one, so the ticket reassignment path threw during the badge-less import test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(registration): dedupe resolved list-question value ids on ticket import Duplicated CSV tokens (drag-fill artifacts, or the same choice spelled as name/label/raw id) resolve to the same value id: they falsely tripped the single-value guard on radio/combo questions and persisted duplicated id strings for CheckBoxList, which also resisted later correction under the answer-change lock. Adds both regression tests from review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
smarcet
pushed a commit
that referenced
this pull request
Jul 7, 2026
…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>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
ref: https://app.clickup.com/t/9014802374/86baraz3d
SummitAttendee::hadCompletedExtraQuestions, same methodSummitAttendeeFactory::populatedelegates to).populate()itself is not called with a minimal payload because it has member/email side effects (clearMember()when no member passed) that are wrong for a partial update.|-separated for CheckBoxList (mirrors theattendee_tagsconvention); stored as comma-separated value ids as the persistence path and consumers (badge-print-app) expect. Multiple values on single-select types are rejected with a warning.GET /summits/{id}/tickets/csv/templateemits oneextra_question:{name}column per Ticket/Both-usage order extra question. OpenAPI descriptions and docblocks updated.Gating decision
Import respects the
hasAllowedExtraQuestionsgate fromAttendeeService::updateAttendee. The persistence path clears-and-rebuilds all answers, so an attendee with zero allowed questions would have every existing answer wiped with nothing rebuilt — the gate is data protection, not permissions. Bypassing it gains nothing (no CSV answer could apply anyway) and risks destroying historical answers.The per-question answer-change lock (
canChangeAnswerValue) is also respected: the import job runs without an authenticated admin, so on summits withallow_update_attendee_extra_questionsoff, import fills blank answers but skips (warns on) changes to existing non-empty ones — matching what the platform enforces for the same actor context, pre-checked deterministically so a locked answer skips one column instead of rolling back the whole row.Tests
tests/SummitOrderServiceTest.php: answer set on newly created attendee; answer updated on existing attendee; unknown question skipped without failing the row; empty value ignored (existing answer preserved); list question stores value ids; badge feature columns still cleared + re-set alongside extra-question columns.Summary by CodeRabbit
extra_question:{question name}columns for ticket-scoped extra questions, including list/checkbox-list inputs stored as value IDs.