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.

Both routes carry auth.user, and both endpoint registrations carry authz groups: SuperAdmins, Administrators, SummitAdministrators, mirroring update-event minus the track chair roles. The middleware is a coarse global group check and the summit scoped admin check still runs in the controller. It narrows nothing, because Member::hasPermissionForOnGroup() already ends in && isOnGroup($slug), so every caller that passes the controller check passes the middleware; a client credentials token with no member passes the middleware and is still refused by the controller. The groups are what make the middleware usable at all: UserAuthEndpoint::handle() returns 403 when an endpoint's required group list is empty.
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 is an expandable relation, declared on AdminPresentationSerializer rather than on the base. The payload carries submission_reopened_by_id, and ?expand=submission_reopened_by replaces it with the serialized member. The mapping is deliberately subclass local: AbstractSerializer::getExpandsMappings() merges lineage parent to child only, and SubmissionPresentationSerializer is a sibling of AdminPresentationSerializer, so the relation is unreachable from the Submission and Public variants. A case in the base expand switch would not be, which is the leak the Admin-only design prevents and the reason the SDS rejected id plus expand when a base class switch was the only mechanism considered. The expand serializes the member as Private, matching how created_by is already serialized on this class. Show Admin therefore asks for the expand; that ticket is not yet started, so nothing downstream changes.
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 schema change is split across two migrations, mirroring Version2026061500000{0,1}. Within one migration every addSql() statement runs before the Builder schema diff, so a single migration doing both would execute the foreign key against a column that did not exist yet. Version20260807120000 carries the three nullable columns and the index, entirely through Builder. Version20260807120001 carries the foreign key as its single addSql() statement and only reads the schema, so it emits no diff and the hazard cannot reappear. Both guard per component, so an interrupted run repairs on retry. Confirmed against a scratch database: the resulting schema has one index named SubmissionReopenedByID and unchanged column types.
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.
A model migration that fails partway is safe to re-run. The DDL statements commit separately, so a failure at the index or foreign key step leaves the added columns in place with the migration unrecorded. Both model migrations guard each component on itself rather than on the column, so a retry adds only what is missing instead of failing on duplicate schema objects.
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.
Two new optional config values, and no .env change is required to deploy. CFP_MAX_REOPEN_HOURS (ceiling on an admin granted window, default 168, seven days) and CFP_DEFAULT_REOPEN_HOURS (window used when the caller sends no hours, default 24). Both are documented in .env.example, and both defaults live in config/cfp.php, so an environment that sets neither behaves as specified. Set them per environment only if a show needs a different ceiling. A CFP_DEFAULT_REOPEN_HOURS configured above the ceiling is clamped down rather than rejected, per reviewer note 7 above, so a typo there raises no error anywhere.

On k8s specifically: neither key has to be added to the configmap for this to deploy, because the fallbacks in config/cfp.php are the intended values. They only need adding where an environment wants a non-default ceiling or default. If they are added, they have to be present in the pod environment before the container's entrypoint runs php artisan config:cache, which fn-docker/summit-api/php-entry-point.sh does on every start: config/cfp.php resolves env() at cache build time, so a value injected into the pod after that point is baked out and reads back as the default.
Run both migrations with or before the application deploy. The entity maps the three new columns, which only Version20260807120000 and Version20260807120001 (--em=model_write) create, and the routes read their authz groups from the config database, which only Version20260807130000 (--em=config) populates. Application code live ahead of either one fails: presentation reads hit missing columns, and the reopen routes answer 400 for an unregistered endpoint.

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
-- each also carrying authz_groups super-admins, administrators, summit-front-end-administrators

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

Testing

97 new tests across five files, 45 integration and 52 unit:

File	Tests	Kind	Covers
`tests/PresentationReopenModelTest.php`	8	integration	predicates, derived deadline, the `isSubmissionClosed()` fold
`tests/PresentationReopenApiTest.php`	25	integration	endpoints, serialization, CFP table feeds, acceptance under an active grant, open-window parity
`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 (59 tests, 385 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.

Summary by CodeRabbit

New Features
- Administrators can reopen closed presentation submission periods for a configurable duration.
- Administrators can immediately close reopened submission periods.
- Reopening details, deadlines, and administrator information are visible where applicable.
- Configurable default and maximum reopening windows are now supported.
Bug Fixes
- Validly reopened presentations can be updated and completed during their reopening window, while existing permissions remain enforced.
Tests
- Added comprehensive coverage for reopening, closing, authorization, validation, serialization, and configuration behavior.

… 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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fd41f23e-3fb9-4bb2-b21e-a2cac7726308

📥 Commits

Reviewing files that changed from the base of the PR and between 8e51951 and 5a05eda.

📒 Files selected for processing (2)

app/Services/Model/IPresentationSubmissionReopenService.php
database/migrations/model/Version20260807120001.php

🚧 Files skipped from review as they are similar to previous changes (1)

app/Services/Model/IPresentationSubmissionReopenService.php

📝 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, migration, 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`, `database/migrations/model/Version20260807120001.php`	`Presentation` stores reopening duration, timestamp, and actor. It calculates active deadlines and treats submissions as open during valid reopening windows. The migrations add the columns, index, and foreign key.
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 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. Serializers expose reopening fields according to presentation serializer type and authorization.
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#543: Both changes modify OAuth2SummitSpeakersApiController and related speaker presentation API behavior.

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 35.95% 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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: an administrator-controlled, time-limited CFP submission reopening override.
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.

✨ 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.

smarcet · 2026-08-10T15:14:58Z

+    // (Presentation.php:1264-1270).
+    // ---------------------------------------------------------------------------------------------
+
+    public function testActiveGrantLetsTheSpeakerUpdateAfterTheWindowClosed()


@caseylocker The SDS's own acceptance bar — open-window parity (§8, D7) — has no test in this suite. §8 asks for three assertions under an active reopen grant: a field the selection plan marks non-editable stays non-editable, a field it marks editable stays editable, and the allowed media-upload types match the open-window set. The acceptance tests here prove the gate opens and closes correctly, but nothing pins the editable surface, so a future change that widens what a speaker can touch during a reopen window would pass this suite unnoticed.

Parity does hold today by construction — the diff only ORs the time gate, and curatePayloadByPresentationAllowedQuestions / checkPresentationAllowedEdtiableQuestions run unchanged on the update path (PresentationService.php:557-558) — but D7 calls this "the acceptance bar", which is exactly the kind of invariant that deserves a tripwire rather than an argument.

Suggested addition: two tests in this file — (1) grant a window, send an update touching a field the plan does not list as an allowed editable question, assert it is refused/stripped exactly as during the open window; (2) assert getAllowedMediaUploadTypes() under an active grant equals the open-window set. Note the scalar-field half of (1) is currently blocked by the pre-existing SelectionPlan::areFieldsEqual self-compare bug you already disclosed (SelectionPlan.php:1570) — that half belongs with that fix's ticket; the extra-questions and media-upload-type halves are writable now.

Added in c91f721. Five tests.

Two of them are the real tripwires, and they use links rather than a scalar. areFieldsEqual has two branches: the array branch is correct, the scalar branch compares a value to itself, so the guard is dead for scalars. links is array typed in AllowedEditableFields and in getSnapshot(), so it takes the working branch and neither test needs the comparator fixed first.

testNonEditableFieldIsRefusedUnderReopen: with links removed from the plan's allowed editable questions, an update under an active grant is refused with the plan's own message, and the link is not written.

testEditableFieldStaysEditableUnderReopen: with it left in place, the same update returns 201 and the value persists.

You were right that the scalar half is blocked. It cannot assert enforcement at all, so it is covered as parity only, by testNonEditableScalarHandledIdenticallyInTheOpenWindow and testNonEditableScalarHandledIdenticallyUnderReopen. Both assert 201, which is the comparator defect showing through rather than a property worth keeping. When that comparator is fixed, both move to 412 together and both should be updated together. Only one moving is the signal the pair exists to give. Two tests rather than one because this suite gets one successful HTTP write per entity.

The media upload types assertion is in as testAllowedMediaUploadTypesAreUnchangedUnderReopen, with one caveat recorded in its docblock so a green is not over read. getAllowedMediaUploadTypes() returns the presentation type's stored collection, and addMediaUploadTo checks type, allowance and cap without consulting the submission window or the selection plan, so the set cannot vary with reopen state today. Media and speaker subresource mutations are currently ungated in every window, which is 86bba8388. The assertion therefore holds by construction right now, and its value is as a guard against that changing rather than as evidence the gate is doing the work.

One fixture note for whoever reads these next: SelectionPlan::__construct seeds every allowed field as both a question and editable, so the restrictive case is built by removing a question, not by adding one.

Full file is green at 25 tests, 174 assertions. The whole PresentationMediaUploads shard is green at 59 tests, 385 assertions, and passed in CI on this commit.

smarcet

@caseylocker please review

Answers smarcet's review comment on #581. The SDS names open-window parity as the acceptance bar (section 8, D7) and the suite had no test for it: the existing acceptance tests prove the gate opens and closes, but nothing pinned the editable surface, so a change that widened what a speaker can touch during a reopen window would have passed unnoticed. Five tests. Two are real tripwires: with links removed from the plan's allowed editable questions an update under an active grant is refused with the plan's own message, and with it left in place the same update succeeds and persists. links rather than a scalar because areFieldsEqual's array branch is correct while its scalar branch compares a value to itself, so the scalar guard is dead. That defect is pre existing, outside this diff, and deliberately not fixed here. The scalar half of the bullet therefore cannot assert enforcement at all and is covered as parity only, by a pair asserting that a non editable scalar is handled identically in the open window and under a grant. A pair rather than one test because this suite gets one successful HTTP write per entity. Both arms move to 412 together when the comparator is fixed; only one moving is the signal they exist to give. The media upload type test is a structural guard, not coverage, and its docblock says so. getAllowedMediaUploadTypes returns a stored collection and addMediaUploadTo never consults the window or the plan, so media and speaker subresource mutations are ungated in every window. That guard is 86bba8388 and is out of scope here. Fixture note worth keeping: SelectionPlan's constructor seeds every allowed field as both a question and editable, so the restrictive case is built by removal, not by addition. Full file green at 25 tests, 174 assertions; the whole push.yml shard green at 59 tests, 385 assertions. Co-Authored-By: Claude <noreply@anthropic.com>

github-actions · 2026-08-10T16:24:34Z

📘 OpenAPI / Swagger preview

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

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

caseylocker · 2026-08-10T17:51:32Z

@smarcet Your request has been addressed. Ready for review

smarcet · 2026-08-10T19:32:29Z

+        'overflow_stream_key',
+        'submission_reopened_until',
+        'submission_reopened_by_id',
+        'submission_reopened_by',


@caseylocker submission_reopened_by is a relation, and it should be declared as an expand mapping rather than flattened into a preformatted "Full Name (email)" string.

Reviewer note 4 says the SDS rejected the id+expand idiom because the expand switch lives in the base PresentationSerializer, so a case added there would be reachable from the Public and Submission variants. That concern is real — but it only applies to the base-class switch, and the fix is not to abandon expand. Your own docblock on Presentation::getSubmissionReopenedByNice() names the right mechanism: "the safe mechanism is a subclass-local $expand_mappings, never a base-class switch case." That is precisely the pattern in the file I linked: SummitAttendeeSerializer.php:181 declares protected static $expand_mappings on the subclass, keyed per relation, with original_attribute, getter, has and an explicit serializer_type. Declared on AdminPresentationSerializer it is unreachable from the Public and Submission variants, so the leak the SDS was guarding against does not occur.

Why it matters beyond style: the flattened string is not consumable. A client that needs the actor's id, or wants to link to the member, has to parse a display string and guess at names containing parentheses. It also cannot be expanded, filtered, or reused, and it bakes a presentation decision into the model layer — getSubmissionReopenedByNice() exists only because the serializer could not express the relation.

Suggested fix: keep submission_reopened_by_id:json_int in $array_mappings, drop SubmissionReopenedByNice and the submission_reopened_by string field, and add to AdminPresentationSerializer:

protected static $expand_mappings = [ 'submission_reopened_by' => [ 'type' => One2ManyExpandSerializer::class, 'original_attribute' => 'submission_reopened_by_id', 'getter' => 'getSubmissionReopenedBy', 'has' => 'hasSubmissionReopenedBy', 'serializer_type' => SerializerRegistry::SerializerType_Private, ], ];

Show Admin then reads submission_reopened_by_id off the getEvent payload as it does today, and gets the full member only when it asks. Worth noting the plumbing is already in place: reopenSubmissionPeriod forwards SerializerUtils::getExpand(), getFields() and getRelations() into serialize(), so ?expand=submission_reopened_by would work the moment the mapping exists — the declaration is the only missing piece. If the SDS text needs an amendment to record this, that is worth doing — the constraint it encoded is satisfied by the subclass-local form.

Done in c1cffca. submission_reopened_by is a relation now: the payload carries submission_reopened_by_id, and ?expand=submission_reopened_by returns the serialized member. The preformatted string is gone.

One deliberate detail. The mapping is declared on AdminPresentationSerializer, not on the base. AbstractSerializer::getExpandsMappings() merges the class lineage parent to child only, and SubmissionPresentationSerializer is a sibling of this class, so the relation stays unreachable from the Submission and Public variants. That was the SDS's objection to id plus expand, and it does not apply to a subclass local mapping. serializer_type is Private, matching how created_by is already serialized here; the default is Public, which blanks the actor's email.

Covered by testReopenActorIsExpandableOnTheAdminResponse, which asserts the id key is replaced by the relation and that the relation carries the email.

On your last point, the SDS amendment: written and open as fntechgit/ftn-docsnsklz#115. It records the shape change, why the subclass local form satisfies the constraint §4 encoded, and the two places the body is now wrong about the shipped contract (§6's "no getEvent expand entry is needed" and §8's "plain scalars, no expand"). The body itself is untouched, the entry is additive. Vault main needs one non-author approval, so it is waiting on a reviewer.

smarcet · 2026-08-10T19:34:49Z

+     */
+    #[ORM\JoinColumn(name: 'SubmissionReopenedByID', referencedColumnName: 'ID', onDelete: 'SET NULL')]
+    #[ORM\ManyToOne(targetEntity: \models\main\Member::class, fetch: 'EXTRA_LAZY')]
+    protected $submission_reopened_by = null;


@caseylocker getSubmissionReopenedById() reimplements One2ManyPropertyTrait::getPropertyId() line for line — same null check, same getId(), same catch (\Exception) { return 0; }. The trait is already in play here: SummitEvent uses it (SummitEvent.php:64) and declares $getIdMappings for created_by / updated_by, so Presentation inherits the mechanism and hand-writing the getter opts out of it for no gain.

Why it matters: the trait is the single place where "id of a nullable relation" semantics live. A hand-rolled copy stops tracking it — if the trait's null/exception behaviour is ever changed (0 vs null, logging the swallowed exception), every declared mapping follows and this one does not.

Suggested fix — declare the mapping instead of the method, and carry the parent's entries forward, since a redeclared $getIdMappings on the subclass shadows SummitEvent's rather than merging with it, which would silently kill getCreatedById() / getUpdatedById():

protected $getIdMappings = [ 'getCreatedById' => 'created_by', 'getUpdatedById' => 'updated_by', 'getSubmissionReopenedById' => 'submission_reopened_by', ]; protected $hasPropertyMappings = [ 'hasCreatedBy' => 'created_by', 'hasUpdatedBy' => 'updated_by', 'hasSubmissionReopenedBy' => 'submission_reopened_by', ];

The hasSubmissionReopenedBy half is what the expand mapping in my serializer comment needs. Your existing unit tests (PresentationSubmissionReopenTest.php, the getSubmissionReopenedById cases) cover both the 0 and the actor-id paths and should pass unchanged — worth confirming, since __call() only fires for methods that do not exist, so the hand-written one must actually be deleted.

Done in c1cffca. The hand written accessor is gone and getSubmissionReopenedById() resolves through One2ManyPropertyTrait, which Presentation inherits from SummitEvent. hasSubmissionReopenedBy() comes along with it and is what the new expand mapping uses for its has check.

One trap worth recording for the next person. A child property shadows the parent default, so Presentation::$getIdMappings has to repeat SummitEvent's created_by and updated_by entries. __call returns null for an unmapped name rather than raising, so omitting them would have made getCreatedById() return null silently instead of failing. testInheritedCreatedByAccessorsStillResolve pins that.

smarcet · 2026-08-10T19:37:18Z

    'scopes' => env('CFP_OAUTH2_SCOPES', null),
-];
+
+    // ceiling on an admin-granted per-presentation reopen window


@caseylocker The PR's Deploy notes cover the two migrations but not these two new config keys, and they are the ones an operator will get wrong. CFP_MAX_REOPEN_HOURS and CFP_DEFAULT_REOPEN_HOURS are read through env(), so on a deployed instance they resolve from the environment, not from .env.example — an unset variable falls back to 168 and 24, which happens to be the intended default, so a misconfiguration is invisible until an admin sees a window they did not expect.

Please add to the Deploy notes: the two variable names, their defaults and units (hours), which environments need them set explicitly versus left to default, and — given reviewer note 7 — that configuring CFP_DEFAULT_REOPEN_HOURS above CFP_MAX_REOPEN_HOURS is silently clamped rather than rejected, so a typo there produces no error anywhere. Also confirm whether the k8s configmap / secret for this service needs the keys added, since that is a separate change from .env.example.

Added to the PR description as deploy notes 5 and 6.

Note 5 covers both values: CFP_MAX_REOPEN_HOURS (default 168, seven days) and CFP_DEFAULT_REOPEN_HOURS (default 24). Both are documented in .env.example and both defaults live in config/cfp.php, so no .env change is required to deploy; set them per environment only if a show needs a different ceiling.

Note 6 covers migration ordering, which the auth.user change makes load bearing: both migrations have to run with or before the application, the model one because the entity maps three columns only it creates, the config one because the routes now read their authz groups from it.

Following up on your last question, the k8s side. I went and looked rather than guessing, and the answer is no change is needed, on either path.

The declaration site is argocd-apps/summit-api/values-{prod,stage}.yaml, under the config: block, which templates/deployment.yaml mounts onto the containers with envFrom: configMapRef: <fullname>-config. That is where the three existing keys live:

CFP_APP_BASE_URL: https://speakermgmt.fnvirtual.app CFP_SUPPORT_EMAIL: support@fntech.com # CFP_OAUTH2_CLIENT_ID: secret

Neither CFP_MAX_REOPEN_HOURS nor CFP_DEFAULT_REOPEN_HOURS appears anywhere in that repo on origin/main, in values.yaml, values-prod.yaml or values-stage.yaml.

You asked about the configmap or the secret, so I checked the secret side too rather than only the values files: the Doppler secret this chart's ExternalSecret syncs into <fullname>-secrets, in both the prod and stage configs. Neither key is present there either. That check mattered rather than being belt-and-braces, because envFrom lists configMapRef before secretRef on every php container, so a key set on the secret side would silently win over the values file.

So both environments take the compiled fallbacks of 168 and 24, which are the intended values, and this ships without touching the chart or the secret. They only need adding where a show wants a different ceiling, and since they are plain non-secret integers they belong inline in the config: block like CFP_APP_BASE_URL, not in the commented : secret form that routes through externalsecrets.yaml.

One timing constraint if they ever are added, which is not obvious and is the same invisible-misconfiguration shape you flagged for the clamp: fn-docker/summit-api/php-entry-point.sh runs php artisan config:cache on every container start, for every command. config/cfp.php resolves env() at cache build time, so the value has to be in the pod environment before the entrypoint runs. Anything injected after that is baked out and silently reads back as the default. That is now in deploy note 5 along with the names, defaults, units and the clamp.

The one thing I have not read is the parent chart that renders <fullname>-config, since it is a dependency rather than part of that repo. It does not change the answer: the consumption side is confirmed in deployment.yaml, and the three existing CFP_* keys prove the path works in prod.

smarcet · 2026-08-10T19:37:40Z

+            'name' => 'reopen-presentation-submission-period',
+            'route' => '/api/v1/summits/{id}/presentations/{presentation_id}/submission-period/reopen',
+            'http_method' => 'PUT',
+            'scopes' => [


@caseylocker Both entries need authz_groups. The migration's own docblock states "No authz_groups: the routes do not use auth.user" — that reasoning is inverted, see my comment on routes/api_v1.php:867: the empty group list is what forces the middleware out, so filling it is what lets the middleware back in. Fix both endpoint definitions here in the same change as the route and the seeder, and update the docblock so the next reader does not inherit the original rationale.

Added in 38d2714: SuperAdmins, Administrators and SummitAdministrators, mirroring update-event minus the track chair roles, which have no business reopening a submission window.

registerEndpoints() already consumed an authz_groups key, so this is a data only change, and rollback is already covered since api_endpoints cascades to endpoint_api_authz_groups. The seeder carries the identical list.

smarcet · 2026-08-10T19:39:26Z

+    public function up(Schema $schema): void
+    {
+        $this->addSql(<<<SQL
+            ALTER TABLE `Presentation`


@caseylocker This migration should go through LaravelDoctrine\Migrations\Schema\Builder rather than raw addSql() throughout.

The docblock justifies the raw form by the ordering hazard: mixing Builder schema-diff with addSql() in one migration runs the addSql() statements first and the diff last, so the FK would execute before its column exists. That hazard is real — Version20260615000001 documents exactly the same ordering in its own header, and relies on it deliberately. But the repo's answer to it is not to abandon Builder; it is to split the migration. Version20260615000000 adds the column and its index through Builder:

$builder->table(self::AddOnTable, function (Table $table) { $table->integer('AddOnTypeID', false, false)->setNotnull(false); $table->index('AddOnTypeID', 'IDX_SummitSponsorshipAddOn_AddOnTypeID'); });

and Version20260615000001 adds the FK constraint separately, as the one statement that genuinely needs raw DDL. The ordering problem never arises because the two never share a migration.

Why it matters: raw DDL bypasses the platform abstraction and the hasTable() / hasColumn() guards the rest of the model migrations use, which is what makes them safe to re-run. That is directly relevant here — deploy note 2 warns that a partial failure leaves the columns in place while the migration is unrecorded, so a naive retry fails on duplicate objects and an operator has to inspect the schema by hand. With Builder's existence guards, the retry is a no-op instead and the warning becomes unnecessary.

Suggested fix: split into two migrations, matching Version2026061500000{0,1}:

the three columns plus the index on SubmissionReopenedByID, through $builder->table('Presentation', ...) with a hasColumn() guard;

the FK constraint as addSql(), in its own migration.

down() should mirror that split. If a single migration is required for some reason, then at minimum guard the column adds so a partial failure is recoverable, and say so in the docblock.

Reworked to the split in 8e51951, matching Version2026061500000{0,1} as you laid out.

Version20260807120000: the three nullable columns plus the index on SubmissionReopenedByID, entirely through Builder.

Version20260807120001: the foreign key as its single addSql() statement. It only reads the schema, never mutates it, so it produces no diff and the ordering hazard cannot reappear inside it.

down() mirrors the split, and the version order matters: 120001 drops the constraint first, which is what lets 120000 drop the index it sits on.

Your point about the guards is what the split made obvious. Both migrations now check each component on itself, so an interrupted run repairs on retry rather than failing on duplicate objects, and deploy note 2 has been rewritten accordingly. Driving the pair against synthetic schemas: fresh emits the columns and index then the constraint; columns-only emits the index and the constraint alone; a fully applied schema emits nothing from either.

One improvement that fell out of splitting: it restores the original statement order, index before constraint, so MySQL builds the constraint on the existing index instead of creating its own. Verified against a scratch database, one index named SubmissionReopenedByID, column types unchanged.

smarcet · 2026-08-10T19:39:41Z

+                'name' => 'reopen-presentation-submission-period',
+                'route' => '/api/v1/summits/{id}/presentations/{presentation_id}/submission-period/reopen',
+                'http_method' => 'PUT',
+                'scopes' => [


@caseylocker Same as the config migration: both entries need authz_groups, and the inline comment "admin-only; authorization enforced in the controller" should go with them. Full reasoning on routes/api_v1.php:867. Keep the seeder and Version20260807130000.php byte-identical on the group list — they drift silently otherwise, and a fresh install then behaves differently from a deployed one.

Added in 38d2714, the same three groups as the config migration, kept in parity with it deliberately: the seeder covers fresh installs and the migration covers deployed environments, since the deploy flow does not re-run seeders.

smarcet · 2026-08-10T19:40:22Z

+                // NOT via auth.user -- that middleware 403s any endpoint with no authz groups)
+                Route::group(['prefix' => 'submission-period'], function () {
+                    Route::group(['prefix' => 'reopen'], function () {
+                        Route::put('', 'OAuth2PresentationApiController@reopenSubmissionPeriod');


@caseylocker Reviewer note 1 is factually right about the mechanism and wrong about the conclusion. UserAuthEndpoint::handle() does 403 when the endpoint's group list is empty (app/Http/Middleware/UserAuthEndpoint.php:129-149 — the loop finds no match and falls through to the 403), but that is a consequence of registering the endpoint with zero authz_groups, not an independent reason to drop auth.user. Add the groups and the failure mode disappears.

Why it matters: as written, the only authorization on these two routes is the summit-aware admin check inside the controller. That check runs after routing and after the controller is resolved, so it is one refactor, one inherited method, or one copy-pasted action away from being silently absent — and nothing in the route definition would show it. Every other admin-mutating presentation route in this group goes through the middleware, which fails closed at the routing layer regardless of what the controller does. Note also that handle() returns early at line 73 when there is no current member, so auth.user does not break pure service-token callers.

The media-upload routes cited as precedent are a known exception, not the house pattern; reproducing an exception widens it.

Suggested fix, one change across three files:

here and at line 868, add 'middleware' => 'auth.user';

add the appropriate authz_groups to both entries in database/migrations/config/Version20260807130000.php;

same for both entries in database/seeders/ApiEndpointsSeeder.php:7094-7112.

Use the same group set as the sibling presentation write endpoints (summit admins / super admins). If there is a specific group that cannot be assigned here, name it and I will take another look — but "no groups" is not a viable end state for an admin-only mutating endpoint.

Added in 38d2714, on both routes, together with the authz groups the middleware reads.

Those two asks go together. auth.user returns 403 for any endpoint whose required group list is empty, which is why the middleware was left off originally; registering the groups is the right fix rather than skipping the middleware. It narrows nothing, because Member::hasPermissionForOnGroup() already ends in && isOnGroup($slug), so every caller that passes the controller's summit scoped admin check passes the middleware's global group check too. A client credentials token with no member passes the middleware and is still refused by the controller.

PresentationReopenAuthzTest is still green at 12 tests.

smarcet

@caseylocker please re review

smarcet · 2026-08-10T20:04:45Z

+        });
+    }
+
+    public function closeNow(Summit $summit, int $presentation_id, Member $actor): void


@caseylocker Backing CodeRabbit's nitpick here, and I'd rate it above trivial. $actor is not just unused — it never enters the closure's use list, so the parameter is inert by construction rather than by an oversight in one branch.

The reason it matters is what closeSubmissionNow() does: it nulls all three columns, submission_reopened_by included. So closing a grant erases who granted it and records nothing about who revoked it. Both ends of an admin-only privilege operation end up unauditable, which sits badly against the migration's own down() docblock describing these columns as "the durable actor/date audit trail".

Two ways out, either is fine:

log the revocation (Log::info with summit id, presentation id, actor id — cheap, keeps the schema as is); or

drop $actor from IPresentationSubmissionReopenService::closeNow() and from this implementation, so the signature stops implying an audit that does not happen.

What should not stay is a parameter that documents an intent the code does not honour.

Agreed, and the diagnosis was exactly right: $actor never entered the closure's use list, so it was inert by construction rather than by a missed branch. Took the logging option, in 1f0346d and d8572c5.

The line is emitted after transaction() returns, not inside the closure. That mattered more than it first looked: DoctrineTransactionService::transaction() runs the callback and only then flushes and commits, and it re-runs the whole callback on a retryable failure. Logging inside would have announced a completed revocation that had not happened yet, and could have announced it more than once for a single request. The audit fields are still read before closeSubmissionNow() nulls them, then carried out of the closure. A transaction that throws logs nothing.

Two details in the line itself worth naming, since both were wrong in my first cut:

getSubmissionReopenedById() returns 0 rather than null for an absent relation, and closing with no active grant is a supported no-op, so the line read "granted by member 0". It now reads "no active grant".

The window end is normalized back to UTC and stamped with a Z. The value is written UTC but Doctrine hydrates datetimes in the application default timezone, so formatting it raw was one config change away from being wrong.

The signature is unchanged, so the audit the parameter implies is now the audit the code performs. Three tests cover it: testCloseNowLogsTheRevocationWithBothActors, testCloseNowWithNoGrantLogsNoActiveGrantRatherThanMemberZero, and testCloseNowDoesNotLogWhenTheTransactionFails.

…ble relation submission_reopened_by was a preformatted "Full Name (email)" scalar. It is a relation, so it is now declared as one: the payload carries submission_reopened_by_id, and ?expand=submission_reopened_by replaces it with the serialized member. The expand mapping is declared on AdminPresentationSerializer rather than on the base. AbstractSerializer::getExpandsMappings() merges lineage parent to child only, and SubmissionPresentationSerializer is a sibling of this class, so the relation is unreachable from the Submission and Public variants. A case in the base expand switch would not be, which is why the SDS rejected id plus expand when a base class switch was the only mechanism considered. serializer_type is Private, matching how created_by is already serialized here; the default is Public, which blanks the actor's email. The hand written id accessor is replaced by One2ManyPropertyTrait, which SummitEvent already uses. Presentation's $getIdMappings repeats the inherited created_by and updated_by entries because a child property shadows the parent default, and __call returns null for an unmapped name rather than raising, so the omission would have degraded silently. testInheritedCreatedByAccessorsStillResolve pins that. Co-Authored-By: Claude <noreply@anthropic.com>

…schema builder up() is now entirely Builder and down() is entirely addSql(); neither mixes the two. Within one migration every addSql() statement runs before the Builder schema diff, so a mixed up() would have executed the foreign key before its column existed. One mechanism per direction removes the ordering question instead of managing it. The emitted DDL is equivalent to the SQL it replaces: one ALTER adding the three nullable columns, the constraint, and a single index named SubmissionReopenedByID. Each component is guarded on itself rather than on the column. DDL does not roll back, so a run interrupted between the ADD COLUMN and the ADD CONSTRAINT leaves the columns in place with the migration unrecorded, and a column-only guard would then skip past the missing foreign key and record the migration as complete. Re-running now repairs only what is missing. Co-Authored-By: Claude <noreply@anthropic.com>

Both routes now carry the auth.user middleware, and both endpoint registrations, the config migration and the seeder, carry authz groups: SuperAdmins, Administrators and SummitAdministrators. That mirrors update-event minus the track chair roles, which have no business reopening a submission window. The middleware was omitted originally because it 403s any endpoint registered with no authz groups. Registering the groups is the fix for that, rather than skipping the middleware. It narrows nothing. Member::hasPermissionForOnGroup() already ends in && isOnGroup($slug), so every caller that passes the controller's summit scoped admin check also passes the middleware's global group check. A client credentials token with no member passes the middleware and is still refused by the controller. Co-Authored-By: Claude <noreply@anthropic.com>

closeNow() took a Member $actor that never entered the transaction closure's use list, so the parameter was inert by construction rather than by an oversight in one branch. Since closeSubmissionNow() nulls all three columns, the granting actor included, closing a grant erased who granted it and recorded nothing about who revoked it, leaving both ends of an admin-only privilege unauditable. The revocation is now logged before the write, carrying the summit, the presentation, the revoking member and the granting member with the window the grant would have run to. The signature is unchanged, so the audit the parameter implies is now the audit the code performs. The unit test harness binds a logger spy as the Log facade root, which the existing closeNow tests need anyway now that the service logs, and testCloseNowLogsTheRevocationWithBothActors asserts both member ids reach the line. Verified by mutation: dropping $actor from the use list again turns that test red. Co-Authored-By: Claude <noreply@anthropic.com>

…ction commits The line was logged inside the transaction closure, before the mutation. Flush and commit happen after the closure returns, and DoctrineTransactionService re-runs the closure on a retryable failure, so the log could announce a completed revocation that had not happened yet, and could announce it more than once for a single request. The audit fields are still read before closeSubmissionNow() nulls them, but they are now carried out of the closure and logged once transaction() has returned. A transaction that throws logs nothing. Two things the line itself got wrong. getSubmissionReopenedById() returns 0 rather than null for an absent relation, so the supported no-grant close read as "granted by member 0"; it now reads "no active grant". And the window end is normalized back to UTC and stamped with a Z, since the value is written UTC but hydrated in the application default timezone. Covered by testCloseNowWithNoGrantLogsNoActiveGrantRatherThanMemberZero and testCloseNowDoesNotLogWhenTheTransactionFails. Co-Authored-By: Claude <noreply@anthropic.com>

…igrations Mirrors Version2026061500000{0,1}, which is the repo's answer to the ordering hazard the previous docblock described. Within one migration every addSql() statement runs before the Builder schema diff, so a migration doing both would execute the foreign key against a column that did not exist yet. Version20260807120000 now carries the three nullable columns and the index on SubmissionReopenedByID, entirely through Builder. Version20260807120001 carries the foreign key as its single addSql() statement, and only reads the schema, so it produces no diff and the hazard cannot reappear. Both are guarded per component, so a run interrupted partway repairs on retry rather than failing on duplicate schema objects. Driving the pair against synthetic schemas: fresh emits the columns and index then the constraint, columns-only emits the index and the constraint alone, and a fully applied schema emits nothing from either. The split also restores the original statement order, index before constraint, so MySQL builds the constraint on the existing index rather than creating its own. Confirmed against a scratch database: one index named SubmissionReopenedByID, and the column types are unchanged. down() mirrors the split. Version20260807120001 drops the constraint first, by version order, which is what lets Version20260807120000 drop the index it sits on. Co-Authored-By: Claude <noreply@anthropic.com>

github-actions · 2026-08-10T21:44:46Z

📘 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: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

database/migrations/model/Version20260807120000.php (1)
86-95: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Make down() safe to retry after a partial rollback.

If the index drop succeeds and the column drop fails, the next rollback fails because Line 89 drops an absent index. Guard the index and each column with the current schema before queueing each drop.
🤖 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 `@database/migrations/model/Version20260807120000.php` around lines 86 - 95,
Update down() to inspect the current schema before queueing destructive SQL:
only drop the index named by self::ActorColumn when it exists, and independently
drop each of SubmissionReopenedHours, SubmissionReopenedDate, and
SubmissionReopenedByID only when present. Preserve the existing rollback order
while making retries after partial rollback safe.

🤖 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 `@database/migrations/model/Version20260807120001.php`:
- Around line 50-53: Update the guard in the migration method containing the
ActorColumn and FkName checks: if hasColumn(self::ActorColumn) is false, throw
the migration exception used by this migration instead of returning. Preserve
the existing early return when hasForeignKey(self::FkName) is already true, and
allow constraint creation to proceed when the column exists.

---

Outside diff comments:
In `@database/migrations/model/Version20260807120000.php`:
- Around line 86-95: Update down() to inspect the current schema before queueing
destructive SQL: only drop the index named by self::ActorColumn when it exists,
and independently drop each of SubmissionReopenedHours, SubmissionReopenedDate,
and SubmissionReopenedByID only when present. Preserve the existing rollback
order while making retries after partial rollback safe.

🪄 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: b332f847-3132-41e3-972e-1975eceab57b

📥 Commits

Reviewing files that changed from the base of the PR and between c91f721 and 8e51951.

📒 Files selected for processing (12)

app/ModelSerializers/Summit/Presentation/AdminPresentationSerializer.php
app/Models/Foundation/Summit/Events/Presentations/Presentation.php
app/Services/Model/Imp/PresentationSubmissionReopenService.php
database/migrations/config/Version20260807130000.php
database/migrations/model/Version20260807120000.php
database/migrations/model/Version20260807120001.php
database/seeders/ApiEndpointsSeeder.php
routes/api_v1.php
tests/PresentationReopenApiTest.php
tests/PresentationReopenModelTest.php
tests/Unit/Models/PresentationSubmissionReopenTest.php
tests/Unit/Services/PresentationSubmissionReopenServiceTest.php

🚧 Files skipped from review as they are similar to previous changes (5)

routes/api_v1.php
app/Services/Model/Imp/PresentationSubmissionReopenService.php
tests/PresentationReopenModelTest.php
database/seeders/ApiEndpointsSeeder.php
app/Models/Foundation/Summit/Events/Presentations/Presentation.php

…missing up() returned quietly if Presentation.SubmissionReopenedByID was absent. A plain return is the ordinary success path, so Doctrine records the version as applied: the constraint would never be created, and the column would be left with no ON DELETE SET NULL and no migration remaining to add it. abortIf() throws AbortMigration, which DbalExecutor treats as an error and rethrows, so the run stops and the version stays unrecorded and re-runnable once the prerequisite migration is repaired. skipIf() would be wrong for the same reason as the plain return: it marks the version skipped and records it. The hasForeignKey() early return stays, since an idempotent no-op is the correct outcome there and recording the version is right. Raised by CodeRabbit. Verified against the three states: a missing column aborts with a message naming the prerequisite, columns-without-constraint emits one statement, and an existing constraint emits none. Co-Authored-By: Claude <noreply@anthropic.com>

github-actions · 2026-08-10T22:08:50Z

📘 OpenAPI / Swagger preview

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

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

romanetar · 2026-08-11T12:03:53Z

Code review

Found 1 issue:

The closeNow() interface docblock still documents $actor as unused, but the implementation now uses it. This is the stale half of the fix for smarcet's review comment ("What should not stay is a parameter that documents an audit that does not happen") — the implementation was updated in 1f0346d81, the interface contract was not. As written it invites a future reader to drop $actor and silently kill the revocation audit line.

summit-api/app/Services/Model/IPresentationSubmissionReopenService.php

Lines 41 to 45 in b3410a7

    
                * 
        
                * $actor is unused by the implementation and is kept because the signed-off SDS specifies it: 
        
                * clearing nulls the ByID column rather than restamping it, and the caller is already captured 
        
                * by generic request auditing. Do not "tidy" it away without amending the SDS. 
        
                *

The implementation does use it, at $actor->getId():

summit-api/app/Services/Model/Imp/PresentationSubmissionReopenService.php

Lines 115 to 124 in b3410a7

    
           Log::info( 
        
               sprintf( 
        
                   "PresentationSubmissionReopenService::closeNow summit %s presentation %s revoked by member %s (granted by %s, ran until %s).", 
        
                   $summit->getId(), 
        
                   $presentation_id, 
        
                   $actor->getId(), 
        
                   $audit['granted_by'], 
        
                   $audit['granted_until'] 
        
               ) 
        
           );

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

romanetar

@caseylocker please review

…unused The interface docblock still described $actor as unused by the implementation and justified keeping it on the grounds that the caller is captured by generic request auditing. 1f0346d changed that: closeNow now emits a revocation audit line interpolating $actor->getId(), which was the fix for the review comment asking not to keep a parameter documenting an audit that does not happen. The stale text is worse than merely out of date. Its "do not tidy it away" warning reaches the right conclusion through a premise a reader can check and find false, which invites discounting the warning and dropping the parameter. Comment-only; no behavior change. Co-Authored-By: Claude <noreply@anthropic.com>

caseylocker · 2026-08-11T13:08:47Z

Confirmed and fixed in 5a05eda04.

You're right that the interface contract was the stale half. 1f0346d81 made $actor the subject of the revocation audit line but left the docblock calling it unused and justifying that on the grounds that the caller is already captured by generic request auditing, which is now the opposite of what the code does.

One thing worth adding to your read: the stale text is a bit worse than out of date. The "do not tidy it away" warning still lands on the right conclusion, but it gets there through a premise a reader can check and find false, which invites discounting the warning and dropping the parameter anyway. So I replaced the whole rationale rather than just the word "unused":

 * $actor is logged as the revoking member in the audit line this method emits. Removing it
 * silently drops that attribution from the revocation record.

Comment only, no behavior change.

github-actions · 2026-08-11T13:09:15Z

📘 OpenAPI / Swagger preview

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

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

caseylocker · 2026-08-11T13:11:12Z

@romanetar @smarcet ready for review.

smarcet

LGTM

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 Outdated

caseylocker requested a review from smarcet August 8, 2026 23:13

caseylocker mentioned this pull request Aug 9, 2026

feat: honor per-activity CFP reopen in the submission edit gate fntechgit/call-for-presentations#90

Merged

smarcet requested a review from romanetar August 10, 2026 13:14

smarcet reviewed Aug 10, 2026

View reviewed changes

smarcet requested changes Aug 10, 2026

View reviewed changes

caseylocker requested a review from smarcet August 10, 2026 16:41

smarcet reviewed Aug 10, 2026

View reviewed changes

smarcet requested changes Aug 10, 2026

View reviewed changes

smarcet reviewed Aug 10, 2026

View reviewed changes

caseylocker and others added 5 commits August 10, 2026 15:31

coderabbitai Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread database/migrations/model/Version20260807120001.php Outdated

caseylocker requested a review from smarcet August 10, 2026 22:14

romanetar requested changes Aug 11, 2026

View reviewed changes

caseylocker requested a review from romanetar August 11, 2026 13:11

smarcet approved these changes Aug 11, 2026

View reviewed changes

smarcet merged commit 6078887 into main Aug 11, 2026
22 checks passed

caseylocker mentioned this pull request Aug 11, 2026

feat: per-activity CFP submission reopen on the activity form fntechgit/summit-admin#1042

Merged

Uh oh!

Conversation

caseylocker commented Aug 8, 2026 • edited by coderabbitai Bot 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

Summary by CodeRabbit

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarcet left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 10, 2026

Uh oh!

caseylocker commented Aug 10, 2026

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caseylocker commented Aug 8, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 8, 2026 •

edited

Loading

smarcet Aug 10, 2026 •

edited

Loading

caseylocker Aug 10, 2026 •

edited

Loading

smarcet Aug 10, 2026 •

edited

Loading

smarcet Aug 10, 2026 •

edited

Loading

caseylocker Aug 10, 2026 •

edited

Loading

smarcet Aug 10, 2026 •

edited

Loading

smarcet Aug 10, 2026 •

edited

Loading

caseylocker Aug 10, 2026 •

edited

Loading

smarcet Aug 10, 2026 •

edited

Loading

smarcet Aug 10, 2026 •

edited

Loading

romanetar commented Aug 11, 2026 •

edited

Loading