feat: add per-activity CFP reopen, an admin-set time-boxed submission override by caseylocker · Pull Request #581 · OpenStackweb/summit-api

caseylocker · 2026-08-08T20:27:04Z

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

Spec: sds/per-activity-cfp-reopen.md in the ftn-docsnsklz vault, on main (merged bd2c2c3), sections 3, 4 and 7.

What this does

A summit admin can reopen CFP submission for a single presentation for a chosen window (default 24h, ceiling 168h), so the speaker can edit that one talk through the existing submission flow after the selection plan's window has closed. Expiry is passive: no cron, no cleanup job.

This is step 1 of 3. The call-for-presentations and summit-admin work are ClickUp 86bba82ph and 86bba82y3. Until step 2 ships, nothing a speaker can see changes.

How it works

Three nullable columns on Presentation store the grant: raw hours, stamp date, granting member. The deadline is derived on read and never stored, so the granted duration and the window end cannot drift out of lockstep.

A new isSubmissionReopened() predicate gates on that derived deadline plus plan invariants (assigned plan, enabled, submission window already ended). It is folded into isSubmissionClosed() as an early return, which covers both delete guards with no change at either call site, and it is OR'd into the two isSubmissionOpen() checks on update and complete. Two admin-only endpoints stamp and clear the grant.

The plan invariants are not optional. A deadline-only check would grant edits before the CFP ever opened, and via the isSubmissionClosed() fold it would open speaker deletes on a plan disabled after the grant, because the delete path never re-checks IsEnabled() itself.

Notes for reviewers

Seven things in this diff look like defects but are deliberate. Each cost a review round to establish, so they are recorded here rather than rediscovered.

The new routes carry no auth.user middleware, and the registered endpoints carry no authz groups. UserAuthEndpoint::handle() returns 403 when an endpoint's required group list is empty, so auth.user would reject every request. The controller performs the summit aware admin check instead, matching the media upload routes.
Serialization uses SerializerType_Private, not _Admin. Presentation has no Admin key, and an unknown type silently falls back to Public, which would strip every field these endpoints exist to set.
Scopes are the OR trio WriteSummitData, WriteEventData, WritePresentationData. Validation is any of, so the trio admits existing Show Admin tokens. Narrowing to WritePresentationData alone would 403 every current caller.
submission_reopened_by renders "Full Name (email)" as a plain Admin-only scalar rather than the repo's usual id plus expand idiom. The SDS considered that idiom and rejected it: the expand switch lives in the base PresentationSerializer, so a case added there is reachable from the Public and Submission variants, which is the exact leak the Admin-only design prevents. Show Admin reads the field straight off the getEvent payload with no expand. The rationale is also in a comment on Presentation::getSubmissionReopenedByNice().
The two speaker presentation-list endpoints previously serialized Public and now serialize Submission, which also selects Admin serializers for the nested created_by, updated_by, moderator and speakers relations. This grants no new access: getPresentationSubmission already serializes the same fields for the same audience, gated by the same memberCanEdit check (creator, moderator, or assigned speaker). The switch itself is what the SDS mandates, since PagingResponse::toArray() defaults to Public and the CFP portal table would otherwise never receive the new field.
The service's $hours < 1 check is unreachable over HTTP, because the endpoint validates sometimes|integer|min:1 and refuses first. It is kept deliberately: it is the only guard for a non HTTP caller, and a persisted non positive value would make getSubmissionReopenedUntil() throw on every read, since new \DateInterval('PT-1H') is invalid.
A misconfigured default is clamped, while an explicit out of range value is refused. If default_reopen_hours is configured above max_reopen_hours, the resolved default is clamped down to the ceiling rather than rejected. An explicitly supplied hours above the ceiling still gets a 412. The asymmetry is deliberate: a configuration error the caller can neither see nor fix should not surface to an admin as an unexplainable refusal of a request that supplied nothing, whereas a caller who names an out of range value should be told. Covered by testMisconfiguredDefaultAboveMaxIsClampedRatherThanRefused.

Two further points on the design:

The model migration is all raw addSql() on purpose. Mixing the Doctrine Builder schema diff with addSql() reorders them, addSql() first, which would execute the foreign key before its column exists.
Both the seeder entry and the config migration are needed and are not redundant. The deploy flow does not re-run seeders, and an unregistered route returns 400 before the controller runs.

Deploy notes

The foreign key addition copies the table. On MySQL 8.0 an in place foreign key addition requires foreign_key_checks to be disabled; otherwise it rebuilds. Presentation is a large production table, so run this migration in a low traffic window and monitor lock time. The three nullable columns are combined into a single ALTER, and the explicit index on the FK column is the index MySQL would create anyway.
Do not blind-rerun the model migration if it fails partway. The three DDL statements commit separately, so a failure at the index or foreign key step leaves the added columns in place while the migration is not recorded as complete, and a naive retry then fails on duplicate schema objects. Check actual schema state before retrying.
Migrations use --em=config and --em=model_write. There is no model entity manager; --em=model silently finds zero migrations and reports "already at latest".
Safe rollback is application only. The columns are additive and inert when null. Rolling the schema back destroys live grants and the actor and date audit trail, so prefer rolling back the application. If a schema rollback is unavoidable, confirm all serving instances run the old application first.

Endpoint registration evidence

Against the config database, before the migration:

SELECT name FROM api_endpoints WHERE name LIKE '%presentation-submission-period%';
-- 0 rows

After php artisan doctrine:migrations:migrate --em=config:

reopen-presentation-submission-period   PUT     /api/v1/summits/{id}/presentations/{presentation_id}/submission-period/reopen
close-presentation-submission-period    DELETE  /api/v1/summits/{id}/presentations/{presentation_id}/submission-period/reopen
-- 2 rows, each carrying summits/write, summits/write-event, summits/write-presentation
-- 0 authz_groups, by design (see reviewer note 1)

Both route strings byte match the Laravel routes as printed by route:list.

Testing

92 new tests across five files, 40 integration and 52 unit:

File	Tests	Kind	Covers
`tests/PresentationReopenModelTest.php`	8	integration	predicates, derived deadline, the `isSubmissionClosed()` fold
`tests/PresentationReopenApiTest.php`	20	integration	endpoints, serialization, CFP table feeds, acceptance under an active grant
`tests/PresentationReopenAuthzTest.php`	12	integration	non-admin 403s, both delete paths, authorship, the create gate
`tests/Unit/Models/PresentationSubmissionReopenTest.php`	29	unit	the full deadline and predicate matrix, including half-states and boundaries
`tests/Unit/Services/PresentationSubmissionReopenServiceTest.php`	23	unit	every validation branch of the reopen service

Integration shard: OK (54 tests, 351 assertions). Unit files: OK (29 tests, 59 assertions) and OK (23 tests, 77 assertions).

All five files are registered in the push.yml matrix. No CI job covers the tests/ root, and tests/Unit/Models/ was not previously sharded either, so a test in either location runs nowhere unless it is named.

The two unit files are genuinely database-free and framework-free: they extend plain PHPUnit\Framework\TestCase, bind a real config repository into a bare container for the facade, and mock ITransactionService so the transaction callback still executes. 52 tests run in about 120ms, against roughly 50 seconds for the integration shard. They exist because several cases are impractical or impossible to reach otherwise: the expiry boundary, the half-states where hours is set with a null date or the reverse, and the service's $hours < 1 guard, which the endpoint's min:1 rule makes unreachable over HTTP so the unit test is its only coverage.

Two notes on test rigour, both of which changed how these were written:

The authz tests run against a genuinely non-admin identity. ProtectedApiTestCase produces a global admin through two independent levers, the persisted group and the access token stub's default IdP groups, and both delete guards short circuit for a global admin. Both levers are defeated and a canary asserts it, because an admin run 403 suite proves nothing.
Several tests arrange their precondition directly on the model rather than through the reopen endpoint. A BrowserKit test cannot perform two sequential HTTP writes against the same entity: DoctrineMiddleware closes the model entity manager after every request and singleton repositories pin the manager they were first resolved with, so the second write is silently dropped while still returning success. Each affected test asserts its precondition landed in the database before issuing the request under test.

The full suite is green in CI, including the tests/oauth2/ shard at 1069 tests. Local full-suite runs need two setup steps that CI does for itself: freshly created test databases (php artisan db:create_initial_db for both schemas, as push.yml does), and max_connections above MySQL's default of 151, which the suite exceeds. Without those, a local run reports failures that are entirely environmental.

Not in scope

Cache key work, already shipped in fix: gate public presentation serialization on display_on_site for media uploads #577.
The delete bypass role narrowness, ClickUp 86bba832v.
A window guard on media and speaker subresource mutations, ClickUp 86bba8388.
Presentation creation under a closed plan. A test asserts creation stays blocked.

Three pre-existing defects were found while implementing this and deliberately left untouched, so they are not mistaken for regressions or for oversights:

SelectionPlan::areFieldsEqual compares its first argument to itself, so the scalar branch always reports equal and the allowed editable question guard never fires for scalar fields.
PresentationType::isAreSpeakersMandatory() ignores the are_speakers_mandatory column and returns min_speakers > 0, so calling the setter has no effect.
getPresentationMediaUploads is registered twice in routes/api_v1.php.

Scope compatibility with Show Admin, checked

Scope validation is any of, and the endpoints accept WriteSummitData (summits/write), WriteEventData (summits/write-event) or WritePresentationData (summits/write-presentation). summit-admin's requested scopes, per its .env.example, are:

summits/read
summits/read/all
summits/write                          <- WriteSummitData
summits/write-event                    <- WriteEventData
summits/write-presentation-materials   <- note: NOT write-presentation

Two of the three, so Show Admin is admitted without any client change.

This is also why the trio matters rather than being belt and braces. summit-admin holds write-presentation-materials, not write-presentation, so registering WritePresentationData alone would have returned 403 to every Show Admin caller in production while passing every test here, because the test token does carry it.

One residual check for whoever deploys: .env.example is the template, and a deployed .env can drift from it. Worth confirming the deployed summit-admin .env still lists summits/write before the control in 86bba82y3 goes live.

… override A summit admin can reopen CFP submission for a single presentation for a chosen window (default 24h, ceiling 168h), letting the speaker edit that one talk through the existing submission flow after the selection plan's window has closed. Expiry is passive: no cron, no cleanup job. Three nullable columns on Presentation store the grant (raw hours, stamp date, granting member). The deadline is derived on read and never stored, so the duration and the window end cannot drift out of lockstep. A new isSubmissionReopened() predicate gates on that derived deadline plus plan invariants (assigned plan, enabled, submission window already ended), and is folded into isSubmissionClosed() as an early return so both delete guards honor a reopen with no change at either call site. Implemented in eight steps: 1. Presentation gains SubmissionReopenedHours, SubmissionReopenedDate and SubmissionReopenedByID plus the derived accessor, the predicate and the isSubmissionClosed() fold, with the schema migration. 2. updatePresentationSubmission and completePresentationSubmission relax only their isSubmissionOpen() condition. The adjacent IsEnabled() and isAllowedMember() checks stay enforced, so a plan disabled after a grant still refuses. 3. New PresentationSubmissionReopenService owns the whole hours rule, default and ceiling together, and rejects a grant on a plan that could never honor it. closeNow() is deliberately exempt so a stale grant is always clearable. 4. Two admin-only endpoints, PUT and DELETE on summits/{id}/presentations/{presentation_id}/submission-period/reopen. 5. Endpoint registration in both the seeder and a config migration. The deploy flow does not re-run seeders, and an unregistered route returns 400 before the controller runs. 6. submission_reopened_until serialized as an epoch on the Submission and Admin subclasses only, with the by-fields Admin-only. All names are added to $allowed_fields or they are dropped from any response omitting fields=. 7. The two speaker presentation-list endpoints now request the Submission serializer, since PagingResponse::toArray() defaults to Public and the CFP portal table would otherwise never see the field. 8. 91 tests. 40 integration tests across three files cover the model predicates, the endpoints, acceptance behavior under an active grant, and everything requiring a non-admin identity. 51 database-free unit tests cover the derived deadline and the predicate matrix, including half-states and boundaries, plus every validation branch of the service, one of which is unreachable over HTTP and has no other coverage. All five test files are registered in the CI matrix, since no job covers the tests/ root and tests/Unit/Models was not previously sharded. Decisions that look wrong without context: - The new routes carry no auth.user middleware and register no authz groups. UserAuthEndpoint returns 403 when an endpoint's required group list is empty, so auth.user would reject every request. The controller performs the summit aware admin check instead, matching the media upload routes. - Serialization uses SerializerType_Private, not _Admin. Presentation has no Admin key and an unknown type silently falls back to Public, which would strip the new fields. - Scopes are the OR trio WriteSummitData, WriteEventData and WritePresentationData. Validation is any of, so the trio admits existing Show Admin tokens; narrowing to WritePresentationData alone would 403 every current caller. - submission_reopened_by renders "Full Name (email)" as a plain Admin-only scalar rather than the usual id plus expand idiom, because the expand switch lives in the base PresentationSerializer and a case there would be reachable from the Public and Submission variants. ref: https://app.clickup.com/t/86bba82ch Co-Authored-By: Claude <noreply@anthropic.com>

coderabbitai · 2026-08-08T20:27:28Z

📝 Walkthrough

Walkthrough

This change adds configurable, administrator-controlled reopening windows for presentation submissions. It persists reopening metadata, validates plan and time conditions, exposes reopen and close APIs, updates serializers, and adds model, service, API, authorization, and CI coverage.

Changes

Presentation submission reopening

Layer / File(s)	Summary
Persisted reopening state `app/Models/Foundation/Summit/Events/Presentations/Presentation.php`, `database/migrations/model/Version20260807120000.php`	`Presentation` stores reopening duration, timestamp, and actor. It calculates active deadlines and treats submissions as open during valid reopening windows.
Reopening service and integration `app/Services/Model/IPresentationSubmissionReopenService.php`, `app/Services/Model/Imp/PresentationSubmissionReopenService.php`, `app/Services/Model/Imp/PresentationService.php`, `app/Services/ModelServicesProvider.php`, `config/cfp.php`, `.env.example`	The service validates configured duration limits and selection-plan state, then reopens or closes submissions in transactions. Updates and completion remain allowed during active reopening windows.
Administrator API and serialization `routes/api_v1.php`, `app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php`, `database/seeders/ApiEndpointsSeeder.php`, `database/migrations/config/Version20260807130000.php`, `app/ModelSerializers/Summit/Presentation/*`, `app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php`	New PUT and DELETE submission-period endpoints support administrator reopening and closure. Admin and submission serializers expose the applicable reopening fields.
Validation and test coverage `tests/PresentationReopenApiTest.php`, `tests/PresentationReopenAuthzTest.php`, `tests/PresentationReopenModelTest.php`, `tests/Unit/Models/PresentationSubmissionReopenTest.php`, `tests/Unit/Services/PresentationSubmissionReopenServiceTest.php`, `.github/workflows/push.yml`	Tests cover duration limits, plan state, authorization, serialization, deletion, updates, completion, closure, expiry, and service transactions. CI runs the added model and integration coverage.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

OpenStackweb/summit-api#517: Shares presentation reopening API tests and CI coverage.
OpenStackweb/summit-api#543: Shares speaker-list controller and API test areas.
OpenStackweb/summit-api#549: Shares changes to the Presentation model.

Suggested reviewers: smarcet

Sequence Diagram(s)

sequenceDiagram
  participant Administrator
  participant PresentationAPI
  participant ReopenService
  participant Presentation
  Administrator->>PresentationAPI: Reopen submission period
  PresentationAPI->>ReopenService: Validate and reopen presentation
  ReopenService->>Presentation: Persist reopening grant
  Presentation-->>PresentationAPI: Return serialized presentation
  Administrator->>PresentationAPI: Close submission period
  PresentationAPI->>ReopenService: Clear reopening grant
  ReopenService->>Presentation: Persist closure

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 31.88% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main feature: an administrator-controlled, time-boxed CFP submission reopening capability.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feature/per-activity-cfp-reopen

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.

❤️ Share

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

github-actions · 2026-08-08T20:29:11Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-581/

This page is automatically updated on each push to this PR.

coderabbitai

Actionable comments posted: 3

🧹 Nitpick comments (5)

app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php (1)
594-596: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Pass the request serialization parameters.

Every other action in this controller forwards SerializerUtils::getExpand(), getFields(), and getRelations() to serialize(). This call passes none, so expand and fields query parameters are ignored for this endpoint only. The inline comment explains the serializer type choice but not the omitted arguments.
♻️ Proposed change
             return $this->updated(SerializerRegistry::getInstance()->getSerializer(
                 $presentation, SerializerRegistry::SerializerType_Private
-            )->serialize());
+            )->serialize(
+                SerializerUtils::getExpand(),
+                SerializerUtils::getFields(),
+                SerializerUtils::getRelations()
+            ));
🤖 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/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php`
around lines 594 - 596, Update the presentation serialization call in the
controller action to pass SerializerUtils::getExpand(), getFields(), and
getRelations() as the request serialization parameters, matching the other
actions in this controller while preserving SerializerType_Private.
config/cfp.php (1)
21-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider guarding against default_reopen_hours greater than max_reopen_hours.

The two values are independent. If an operator sets CFP_DEFAULT_REOPEN_HOURS above CFP_MAX_REOPEN_HOURS, then every reopen request that omits hours fails validation in PresentationSubmissionReopenService::reopen. A clamp at resolution time, or a startup check, prevents this silent misconfiguration.
🤖 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 `@config/cfp.php` around lines 21 - 23, Validate the resolved CFP reopen-hour
settings in config/cfp.php so default_reopen_hours cannot exceed
max_reopen_hours, preferably clamping the default to the maximum while
preserving the existing environment defaults. Ensure
PresentationSubmissionReopenService::reopen receives a valid default for
requests that omit hours.
tests/PresentationReopenApiTest.php (1)
124-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated entity-manager reload helper clears the whole identity map. Both test classes carry a copy of the same refresh block, and both call self::$em->clear(). clear() detaches every managed entity, including the fixture statics that clearSummitTestData() uses during tearDown(). Extract one shared helper and confirm the teardown path still holds managed entities.

tests/PresentationReopenApiTest.php#L124-L132: move the refresh block into a shared trait or base helper, and replace clear() with a targeted refresh() on the presentation if the teardown depends on managed fixtures.

tests/PresentationReopenAuthzTest.php#L187-L195: delete the local copy and call the shared helper with the presentation id.
🤖 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 `@tests/PresentationReopenApiTest.php` around lines 124 - 132, In
tests/PresentationReopenApiTest.php#L124-L132, extract the duplicated
reloadPresentation entity-manager logic into a shared trait or base helper,
replace the full self::$em->clear() with a targeted refresh of the presentation,
and verify clearSummitTestData() still has its managed teardown fixtures. In
tests/PresentationReopenAuthzTest.php#L187-L195, remove the local helper copy
and call the shared helper using the presentation id.
app/Services/Model/Imp/PresentationSubmissionReopenService.php (1)
77-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

closeNow accepts $actor but never uses it.

The use clause omits $actor, so the acting administrator is discarded. The reopen path records the granting member, but the close path records nothing. Either log the actor for the audit trail, or remove the parameter from the interface and the two call sites. Logging is the smaller change and keeps the administrative action traceable.
♻️ Proposed change: record the acting administrator
+use Illuminate\Support\Facades\Log;
     public function closeNow(Summit $summit, int $presentation_id, Member $actor): void
     {
-        $this->tx_service->transaction(function () use ($summit, $presentation_id) {
+        $this->tx_service->transaction(function () use ($summit, $presentation_id, $actor) {
 
             $presentation = $summit->getEvent($presentation_id);
             if (!$presentation instanceof Presentation)
                 throw new EntityNotFoundException(sprintf("Presentation %s not found.", $presentation_id));
 
+            Log::info(sprintf(
+                "PresentationSubmissionReopenService::closeNow presentation %s closed by member %s",
+                $presentation_id, $actor->getId()
+            ));
             // no plan-state checks on purpose: a stale grant must always be clearable
             $presentation->closeSubmissionNow();
         });
     }
🤖 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/PresentationSubmissionReopenService.php` around lines
77 - 88, Update closeNow to capture the provided actor in its transaction
closure and record that Member through the presentation’s existing close/audit
mechanism, matching how the reopen path records its granting member. Keep the
$actor parameter and both call sites unchanged.
Source: Linters/SAST tools
.github/workflows/push.yml (1)
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rename or split the PresentationMediaUploads suite entry.

This entry now runs six files, three of which are reopen tests. The suite name and the uploaded artifact name results_PresentationMediaUploads no longer describe the contents. The six files also run serially in one matrix slot while other slots are free. Add a separate PresentationReopen entry for the three new files.
♻️ Proposed split
-                    - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php tests/PresentationSerializerCacheKeyTest.php tests/PresentationReopenModelTest.php tests/PresentationReopenApiTest.php tests/PresentationReopenAuthzTest.php" }
+                    - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php tests/PresentationSerializerCacheKeyTest.php" }
+                    - { name: "PresentationReopen", filter: "tests/PresentationReopenModelTest.php tests/PresentationReopenApiTest.php tests/PresentationReopenAuthzTest.php" }
🤖 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 @.github/workflows/push.yml at line 72, Split the suite configuration entry
in the workflow matrix: keep the three media-upload/serializer tests under
PresentationMediaUploads, and add a separate PresentationReopen entry containing
the three PresentationReopen* test files. This ensures the suite and generated
artifact names accurately reflect their contents and allows the reopen tests to
run in their own matrix slot.

🤖 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/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php`:
- Around line 556-560: Update the OpenAPI response declaration in the
presentation action to use Response::HTTP_OK instead of Response::HTTP_CREATED,
matching the 200 status returned by $this->updated(...).

In `@app/Services/ModelServicesProvider.php`:
- Around line 198-199: Add IPresentationSubmissionReopenService::class to the
provides() array in ModelServicesProvider, matching the deferred singleton
registration so the provider loads when this interface is resolved directly.

In `@tests/PresentationReopenApiTest.php`:
- Around line 462-473: Update
testReopenFieldsNeverAppearOnAPublicSerializedResponse to assert that
reopen(['hours' => 24]) succeeds before serializing and checking field absence.
Also add the equivalent success assertion to
testByFieldsAreAbsentFromTheSubmissionSerializer, preserving their existing
serializer assertions.

---

Nitpick comments:
In @.github/workflows/push.yml:
- Line 72: Split the suite configuration entry in the workflow matrix: keep the
three media-upload/serializer tests under PresentationMediaUploads, and add a
separate PresentationReopen entry containing the three PresentationReopen* test
files. This ensures the suite and generated artifact names accurately reflect
their contents and allows the reopen tests to run in their own matrix slot.

In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php`:
- Around line 594-596: Update the presentation serialization call in the
controller action to pass SerializerUtils::getExpand(), getFields(), and
getRelations() as the request serialization parameters, matching the other
actions in this controller while preserving SerializerType_Private.

In `@app/Services/Model/Imp/PresentationSubmissionReopenService.php`:
- Around line 77-88: Update closeNow to capture the provided actor in its
transaction closure and record that Member through the presentation’s existing
close/audit mechanism, matching how the reopen path records its granting member.
Keep the $actor parameter and both call sites unchanged.

In `@config/cfp.php`:
- Around line 21-23: Validate the resolved CFP reopen-hour settings in
config/cfp.php so default_reopen_hours cannot exceed max_reopen_hours,
preferably clamping the default to the maximum while preserving the existing
environment defaults. Ensure PresentationSubmissionReopenService::reopen
receives a valid default for requests that omit hours.

In `@tests/PresentationReopenApiTest.php`:
- Around line 124-132: In tests/PresentationReopenApiTest.php#L124-L132, extract
the duplicated reloadPresentation entity-manager logic into a shared trait or
base helper, replace the full self::$em->clear() with a targeted refresh of the
presentation, and verify clearSummitTestData() still has its managed teardown
fixtures. In tests/PresentationReopenAuthzTest.php#L187-L195, remove the local
helper copy and call the shared helper using the presentation id.

🪄 Autofix

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 Plus

Run ID: 621f037a-3017-4fc7-b8e5-0277ea4d1d97

📥 Commits

Reviewing files that changed from the base of the PR and between e829aed and 82940f8.

📒 Files selected for processing (21)

.env.example
.github/workflows/push.yml
app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php
app/ModelSerializers/Summit/Presentation/AdminPresentationSerializer.php
app/ModelSerializers/Summit/Presentation/SubmissionPresentationSerializer.php
app/Models/Foundation/Summit/Events/Presentations/Presentation.php
app/Services/Model/IPresentationSubmissionReopenService.php
app/Services/Model/Imp/PresentationService.php
app/Services/Model/Imp/PresentationSubmissionReopenService.php
app/Services/ModelServicesProvider.php
config/cfp.php
database/migrations/config/Version20260807130000.php
database/migrations/model/Version20260807120000.php
database/seeders/ApiEndpointsSeeder.php
routes/api_v1.php
tests/PresentationReopenApiTest.php
tests/PresentationReopenAuthzTest.php
tests/PresentationReopenModelTest.php
tests/Unit/Models/PresentationSubmissionReopenTest.php
tests/Unit/Services/PresentationSubmissionReopenServiceTest.php

Three review follow ups on the per-activity CFP reopen endpoints. The reopen response now forwards expand, fields and relations to serialize(), matching every sibling action in this controller. The Private serializer type is unchanged, so the Admin only fields stay Admin only. A resolved default_reopen_hours above max_reopen_hours is now clamped to the ceiling instead of rejecting every request that omits hours. That is a config error the caller can neither see nor fix, so failing them is the wrong behaviour. An explicitly supplied hours is still validated strictly and never clamped, which the existing out of range test continues to cover. testReopenFieldsNeverAppearOnAPublicSerializedResponse now asserts the reopen returned 201 and that the grant persisted before asserting the three fields are absent from the Public payload. Without that the absence assertions held vacuously: a failed reopen leaves no grant, so Public omits the fields whether or not the mappings are correctly scoped to the Admin subclass. ref: https://app.clickup.com/t/86bba82ch Co-Authored-By: Claude <noreply@anthropic.com>

caseylocker · 2026-08-08T20:59:49Z

Dispositions for the five non-blocking items in the review body, since they are not inline threads. Two taken, three declined with reasons. The two inline bug reports are refuted in their own threads.

Taken, in d673aa7:

Pass the request serialization parameters. The reopen response now forwards expand, fields and relations to serialize(), matching every sibling action in this controller. The SerializerType_Private argument is unchanged, so the Admin only fields stay Admin only.
Guard against default_reopen_hours greater than max_reopen_hours. A resolved default above the ceiling is now clamped to it rather than rejecting every request that omits hours. That is a configuration error the caller can neither see nor fix, so failing them is the wrong behaviour. An explicitly supplied hours is still validated strictly and never clamped, and a new unit test covers the clamp.

Declined:

Duplicated reload helper, and clear() versus refresh(). The duplication between the two test classes is deliberate. They need materially different identities, one global admin and one genuinely non admin, so a shared trait would need conditionals. On the mechanism: clear() is required rather than preferred. Each simulated HTTP dispatch closes the model entity manager, because DoctrineMiddleware closes it after every request and singleton repositories pin whichever manager instance they were first resolved with. A targeted refresh() cannot refresh an entity that is already detached. Teardown re-fetches its own roots, and the suite passes in both orders.
closeNow accepts $actor but never uses it. Intentional and documented on the interface. Closing nulls the granting actor column rather than restamping it, and the caller is already captured by generic request auditing. The signed off SDS specifies the parameter, so removing it would need an amendment.
Rename or split the PresentationMediaUploads suite entry. Accurate observation, no correctness impact. Deliberately left alone to keep this diff scoped.

The docstring coverage warning is not a PR specific correctness finding.

github-actions · 2026-08-08T21:00:49Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-581/

This page is automatically updated on each push to this PR.

Copilot

Pull request overview

Adds a per-presentation, time-boxed “CFP submission reopen” override that allows summit admins to temporarily re-enable edits for a single presentation after the selection plan’s submission window has ended, without persisting the derived deadline.

Changes:

Adds nullable reopen-grant columns to Presentation (hours, stamp date, granting member) plus derived deadline + predicates, and folds the predicate into isSubmissionClosed().
Introduces admin-only reopen/close endpoints and wires serializers so admin reads include grant metadata while speaker submission flows receive only the derived deadline.
Adds comprehensive integration + unit test coverage and updates CI sharding so the new tests run in push.yml.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`app/Models/Foundation/Summit/Events/Presentations/Presentation.php`	Stores reopen grant fields, derives `submission_reopened_until`, adds `isSubmissionReopened()` and folds into `isSubmissionClosed()`.
`database/migrations/model/Version20260807120000.php`	Adds DB columns/index/FK for reopen grant on `Presentation`.
`config/cfp.php`	Adds configurable max/default reopen hours.
`app/Services/Model/IPresentationSubmissionReopenService.php`	Defines service contract for reopen/close operations.
`app/Services/Model/Imp/PresentationSubmissionReopenService.php`	Implements reopen/close logic (config-based hours rules + plan invariants + summit scoping).
`app/Services/ModelServicesProvider.php`	Registers the new reopen service in the container.
`app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php`	Adds admin-only reopen/close endpoints with controller-level summit-admin authorization.
`routes/api_v1.php`	Registers the new reopen/close routes.
`database/migrations/config/Version20260807130000.php`	Registers new API endpoints in config DB so OAuth validation recognizes the routes.
`database/seeders/ApiEndpointsSeeder.php`	Seeds endpoint definitions for fresh installs.
`app/Services/Model/Imp/PresentationService.php`	Allows update/complete when plan window is closed but a valid reopen grant is active.
`app/ModelSerializers/Summit/Presentation/AdminPresentationSerializer.php`	Exposes reopen fields for admin/private presentation serialization.
`app/ModelSerializers/Summit/Presentation/SubmissionPresentationSerializer.php`	Exposes `submission_reopened_until` for submission serializer output.
`app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php`	Switches speaker presentation list endpoints to `Submission` serializer type so the derived deadline is returned.
`tests/PresentationReopenApiTest.php`	Integration coverage for reopen/close endpoints, serialization, and reopened edit/complete acceptance paths.
`tests/PresentationReopenAuthzTest.php`	Integration coverage for non-admin authorization (403s), delete-guard behavior, authorship invariants, and non-leakage of admin-only fields.
`tests/PresentationReopenModelTest.php`	Integration coverage for model predicate/deadline behavior and `isSubmissionClosed()` fold behavior.
`tests/Unit/Models/PresentationSubmissionReopenTest.php`	Unit coverage for full deadline/predicate state matrix and boundary conditions.
`tests/Unit/Services/PresentationSubmissionReopenServiceTest.php`	Unit coverage for service validation branches and config-driven default/ceiling behavior.
`.github/workflows/push.yml`	Adds new test files/paths to CI matrix so they execute.
`.env.example`	Documents new CFP reopen environment variables.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

caseylocker self-assigned this Aug 8, 2026

coderabbitai Bot reviewed Aug 8, 2026

View reviewed changes

Comment thread app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php

Comment thread app/Services/ModelServicesProvider.php

Comment thread tests/PresentationReopenApiTest.php

caseylocker requested a lite review from Copilot August 8, 2026 21:02

Copilot started reviewing on behalf of caseylocker August 8, 2026 21:02 View session

Copilot AI reviewed Aug 8, 2026

View reviewed changes

Comment thread app/Models/Foundation/Summit/Events/Presentations/Presentation.php

Navigation Menu

Uh oh!

feat: add per-activity CFP reopen, an admin-set time-boxed submission override - #581

feat: add per-activity CFP reopen, an admin-set time-boxed submission override#581
caseylocker wants to merge 2 commits into
mainfrom
feature/per-activity-cfp-reopen

caseylocker commented Aug 8, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 8, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Aug 8, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

caseylocker commented Aug 8, 2026

Uh oh!

github-actions Bot commented Aug 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

caseylocker commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this does

How it works

Notes for reviewers

Deploy notes

Endpoint registration evidence

Testing

Not in scope

Scope compatibility with Show Admin, checked

Uh oh!

coderabbitai Bot commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Aug 8, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

caseylocker commented Aug 8, 2026

Uh oh!

github-actions Bot commented Aug 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

caseylocker commented Aug 8, 2026 •

edited

Loading

coderabbitai Bot commented Aug 8, 2026 •

edited

Loading