From 82940f8172ed66c41886edf18e8cb00d03cb7c2d Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Sat, 8 Aug 2026 11:49:15 -0500 Subject: [PATCH 1/3] feat: add per-activity CFP reopen, an admin-set time-boxed submission 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 --- .env.example | 3 + .github/workflows/push.yml | 3 +- .../OAuth2PresentationApiController.php | 114 ++- .../OAuth2SummitSpeakersApiController.php | 9 +- .../AdminPresentationSerializer.php | 8 +- .../SubmissionPresentationSerializer.php | 8 + .../Events/Presentations/Presentation.php | 127 ++++ .../IPresentationSubmissionReopenService.php | 49 ++ .../Model/Imp/PresentationService.php | 4 +- .../PresentationSubmissionReopenService.php | 89 +++ app/Services/ModelServicesProvider.php | 4 + config/cfp.php | 6 +- .../config/Version20260807130000.php | 76 ++ .../model/Version20260807120000.php | 73 ++ database/seeders/ApiEndpointsSeeder.php | 21 + routes/api_v1.php | 9 + tests/PresentationReopenApiTest.php | 701 ++++++++++++++++++ tests/PresentationReopenAuthzTest.php | 560 ++++++++++++++ tests/PresentationReopenModelTest.php | 169 +++++ .../PresentationSubmissionReopenTest.php | 423 +++++++++++ ...resentationSubmissionReopenServiceTest.php | 482 ++++++++++++ 21 files changed, 2930 insertions(+), 8 deletions(-) create mode 100644 app/Services/Model/IPresentationSubmissionReopenService.php create mode 100644 app/Services/Model/Imp/PresentationSubmissionReopenService.php create mode 100644 database/migrations/config/Version20260807130000.php create mode 100644 database/migrations/model/Version20260807120000.php create mode 100644 tests/PresentationReopenApiTest.php create mode 100644 tests/PresentationReopenAuthzTest.php create mode 100644 tests/PresentationReopenModelTest.php create mode 100644 tests/Unit/Models/PresentationSubmissionReopenTest.php create mode 100644 tests/Unit/Services/PresentationSubmissionReopenServiceTest.php diff --git a/.env.example b/.env.example index a76c0ab5b..d08ef75a8 100644 --- a/.env.example +++ b/.env.example @@ -156,6 +156,9 @@ CFP_APP_BASE_URL= CFP_SUPPORT_EMAIL= CFP_OAUTH2_SCOPES= CFP_OAUTH2_CLIENT_ID= +# ceiling and default for an admin-granted per-presentation submission reopen window, in hours +CFP_MAX_REOPEN_HOURS=168 +CFP_DEFAULT_REOPEN_HOURS=24 # RABBIT MQ RABBITMQ_EXCHANGE_NAME=databus-exchange diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 2e2aec304..0e665c5b0 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -59,6 +59,7 @@ jobs: - { name: "SummitRSVPServiceTest", filter: "--filter SummitRSVPServiceTest" } - { name: "SummitRSVPInvitationServiceTest", filter: "--filter SummitRSVPInvitationServiceTest" } - { name: "EntityModelUnitTests", filter: "tests/Unit/Entities/" } + - { name: "ModelUnitTests", filter: "tests/Unit/Models/" } - { name: "AuditUnitTests", filter: "tests/Unit/Audit/" } - { name: "AuditOtlpStrategyTest", filter: "--filter AuditOtlpStrategyTest" } - { name: "AuditEventTypesTest", filter: "--filter AuditEventTypesTest" } @@ -68,7 +69,7 @@ jobs: - { name: "CacheOptimizations", filter: "--filter '(PresentationSpeakerCacheTest|ResourceServerContextTest)'" } # Named by path because no job in this matrix runs the tests/ root, only its # subdirectories - a file added there runs nowhere unless it is listed here. - - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php tests/PresentationSerializerCacheKeyTest.php" } + - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php tests/PresentationSerializerCacheKeyTest.php tests/PresentationReopenModelTest.php tests/PresentationReopenApiTest.php tests/PresentationReopenAuthzTest.php" } env: OTEL_SERVICE_ENABLED: false APP_ENV: testing diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php index 99647c11c..1d544e065 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php @@ -43,6 +43,7 @@ use ModelSerializers\SerializerRegistry; use OpenApi\Attributes as OA; use services\model\IPresentationService; +use services\model\IPresentationSubmissionReopenService; use utils\Filter; use utils\FilterElement; use utils\FilterParser; @@ -84,6 +85,11 @@ final class OAuth2PresentationApiController extends OAuth2ProtectedController */ private $presentation_comments_repository; + /** + * @var IPresentationSubmissionReopenService + */ + private $presentation_submission_reopen_service; + /** * OAuth2PresentationApiController constructor. * @param IPresentationService $presentation_service @@ -92,6 +98,7 @@ final class OAuth2PresentationApiController extends OAuth2ProtectedController * @param IMemberRepository $member_repository * @param ISummitPresentationCommentRepository $presentation_comments_repository * @param IResourceServerContext $resource_server_context + * @param IPresentationSubmissionReopenService $presentation_submission_reopen_service */ public function __construct ( @@ -100,7 +107,8 @@ public function __construct ISummitEventRepository $presentation_repository, IMemberRepository $member_repository, ISummitPresentationCommentRepository $presentation_comments_repository, - IResourceServerContext $resource_server_context + IResourceServerContext $resource_server_context, + IPresentationSubmissionReopenService $presentation_submission_reopen_service ) { parent::__construct($resource_server_context); @@ -109,6 +117,7 @@ public function __construct $this->member_repository = $member_repository; $this->summit_repository = $summit_repository; $this->presentation_comments_repository = $presentation_comments_repository; + $this->presentation_submission_reopen_service = $presentation_submission_reopen_service; } //presentations @@ -525,6 +534,109 @@ public function updatePresentationSubmission($summit_id, $presentation_id) }); } + #[OA\Put( + path: "/api/v1/summits/{id}/presentations/{presentation_id}/submission-period/reopen", + summary: "Admin-only: reopen the submission period for a presentation", + operationId: "reopenSubmissionPeriod", + security: [['summit_presentations_auth' => [SummitScopes::WriteSummitData, SummitScopes::WriteEventData, SummitScopes::WritePresentationData]]], + tags: ['Presentations'], + parameters: [ + new OA\Parameter(name: 'id', in: 'path', required: true, schema: new OA\Schema(type: 'integer')), + new OA\Parameter(name: 'presentation_id', in: 'path', required: true, schema: new OA\Schema(type: 'integer')), + ], + requestBody: new OA\RequestBody( + required: false, + content: new OA\JsonContent( + properties: [ + new OA\Property(property: 'hours', type: 'integer'), + ] + ) + ), + responses: [ + new OA\Response( + response: Response::HTTP_CREATED, + description: "Created", + content: new OA\JsonContent(ref: "#/components/schemas/Presentation") + ), + new OA\Response(response: Response::HTTP_UNAUTHORIZED, description: "Unauthorized"), + new OA\Response(response: Response::HTTP_FORBIDDEN, description: "Forbidden"), + new OA\Response(response: Response::HTTP_NOT_FOUND, description: "Not Found"), + new OA\Response(response: Response::HTTP_PRECONDITION_FAILED, description: "Validation Error"), + new OA\Response(response: Response::HTTP_INTERNAL_SERVER_ERROR, description: "Server Error"), + ] + )] + public function reopenSubmissionPeriod($summit_id, $presentation_id) + { + return $this->processRequest(function () use ($summit_id, $presentation_id) { + + $summit = SummitFinderStrategyFactory::build($this->summit_repository, $this->resource_server_context)->find($summit_id); + if (is_null($summit)) return $this->error404(); + + $current_member = $this->resource_server_context->getCurrentUser(); + if (is_null($current_member)) return $this->error403(); + + $isAdmin = $current_member->isAdmin() + || $current_member->hasPermissionForOnGroup($summit, IGroup::SummitAdministrators); + if (!$isAdmin) return $this->error403(); + + $payload = $this->getJsonPayload(['hours' => 'sometimes|integer|min:1']); + + // null, not the default: the hours rule (default AND ceiling) lives in the service. + $presentation = $this->presentation_submission_reopen_service->reopen( + $summit, + intval($presentation_id), + isset($payload['hours']) ? intval($payload['hours']) : null, + $current_member + ); + + // Private, NOT Admin: SerializerRegistry has no Admin key for Presentation and an + // unknown type silently falls back to Public, stripping the reopen fields. + return $this->updated(SerializerRegistry::getInstance()->getSerializer( + $presentation, SerializerRegistry::SerializerType_Private + )->serialize()); + }); + } + + #[OA\Delete( + path: "/api/v1/summits/{id}/presentations/{presentation_id}/submission-period/reopen", + summary: "Admin-only: close the reopened submission period for a presentation", + operationId: "closeSubmissionPeriod", + security: [['summit_presentations_auth' => [SummitScopes::WriteSummitData, SummitScopes::WriteEventData, SummitScopes::WritePresentationData]]], + tags: ['Presentations'], + parameters: [ + new OA\Parameter(name: 'id', in: 'path', required: true, schema: new OA\Schema(type: 'integer')), + new OA\Parameter(name: 'presentation_id', in: 'path', required: true, schema: new OA\Schema(type: 'integer')), + ], + responses: [ + new OA\Response(response: Response::HTTP_NO_CONTENT, description: "No Content"), + new OA\Response(response: Response::HTTP_UNAUTHORIZED, description: "Unauthorized"), + new OA\Response(response: Response::HTTP_FORBIDDEN, description: "Forbidden"), + new OA\Response(response: Response::HTTP_NOT_FOUND, description: "Not Found"), + new OA\Response(response: Response::HTTP_INTERNAL_SERVER_ERROR, description: "Server Error"), + ] + )] + public function closeSubmissionPeriod($summit_id, $presentation_id) + { + return $this->processRequest(function () use ($summit_id, $presentation_id) { + + $summit = SummitFinderStrategyFactory::build($this->summit_repository, $this->resource_server_context)->find($summit_id); + if (is_null($summit)) return $this->error404(); + + $current_member = $this->resource_server_context->getCurrentUser(); + if (is_null($current_member)) return $this->error403(); + + $isAdmin = $current_member->isAdmin() + || $current_member->hasPermissionForOnGroup($summit, IGroup::SummitAdministrators); + if (!$isAdmin) return $this->error403(); + + $this->presentation_submission_reopen_service->closeNow( + $summit, intval($presentation_id), $current_member + ); + + return $this->deleted(); + }); + } + #[OA\Put( path: "/api/v1/summits/{id}/presentations/{presentation_id}/completed", summary: "Mark a presentation submission as completed", diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php index 1ef9c6e4f..38054a885 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php @@ -31,6 +31,7 @@ use models\summit\ISummitEventRepository; use models\summit\ISummitRepository; use models\summit\PresentationSpeaker; +use ModelSerializers\IPresentationSerializerTypes; use ModelSerializers\ISerializerTypeSelector; use ModelSerializers\SerializerRegistry; use services\model\ISpeakerService; @@ -2342,7 +2343,9 @@ public function getMySpeakerPresentationsByRoleAndBySelectionPlan($role, $select return $this->ok($response->toArray( SerializerUtils::getExpand(), SerializerUtils::getFields(), - SerializerUtils::getRelations() + SerializerUtils::getRelations(), + [], + IPresentationSerializerTypes::Submission )); }); } @@ -2449,7 +2452,9 @@ public function getMySpeakerPresentationsByRoleAndBySummit($role, $summit_id) return $this->ok($response->toArray( SerializerUtils::getExpand(), SerializerUtils::getFields(), - SerializerUtils::getRelations() + SerializerUtils::getRelations(), + [], + IPresentationSerializerTypes::Submission )); }); } diff --git a/app/ModelSerializers/Summit/Presentation/AdminPresentationSerializer.php b/app/ModelSerializers/Summit/Presentation/AdminPresentationSerializer.php index 5b79d3108..8299f51e5 100644 --- a/app/ModelSerializers/Summit/Presentation/AdminPresentationSerializer.php +++ b/app/ModelSerializers/Summit/Presentation/AdminPresentationSerializer.php @@ -41,6 +41,9 @@ class AdminPresentationSerializer extends PresentationSerializer 'OverflowStreamIsSecure' => 'overflow_stream_is_secure:json_boolean', 'OverflowStreamKey' => 'overflow_stream_key:json_string', 'TrackChairAvgScoresPerRakingType' => 'track_chair_scores_avg:json_string_array', + 'SubmissionReopenedUntil' => 'submission_reopened_until:datetime_epoch', + 'SubmissionReopenedById' => 'submission_reopened_by_id:json_int', + 'SubmissionReopenedByNice' => 'submission_reopened_by:json_string', ]; protected static $allowed_fields = [ @@ -64,7 +67,10 @@ class AdminPresentationSerializer extends PresentationSerializer 'etherpad_link', 'overflow_streaming_url', 'overflow_stream_is_secure', - 'overflow_stream_key' + 'overflow_stream_key', + 'submission_reopened_until', + 'submission_reopened_by_id', + 'submission_reopened_by', ]; /** diff --git a/app/ModelSerializers/Summit/Presentation/SubmissionPresentationSerializer.php b/app/ModelSerializers/Summit/Presentation/SubmissionPresentationSerializer.php index a5589986e..a3e9777bf 100644 --- a/app/ModelSerializers/Summit/Presentation/SubmissionPresentationSerializer.php +++ b/app/ModelSerializers/Summit/Presentation/SubmissionPresentationSerializer.php @@ -21,6 +21,14 @@ */ class SubmissionPresentationSerializer extends PresentationSerializer { + protected static $array_mappings = [ + 'SubmissionReopenedUntil' => 'submission_reopened_until:datetime_epoch', + ]; + + protected static $allowed_fields = [ + 'submission_reopened_until', + ]; + /** * @param string|null $relation * @return string diff --git a/app/Models/Foundation/Summit/Events/Presentations/Presentation.php b/app/Models/Foundation/Summit/Events/Presentations/Presentation.php index 251be8730..045d52451 100644 --- a/app/Models/Foundation/Summit/Events/Presentations/Presentation.php +++ b/app/Models/Foundation/Summit/Events/Presentations/Presentation.php @@ -231,6 +231,25 @@ public static function getAllowedEditableFields(): array #[ORM\Column(name: 'CustomOrder', type: 'integer')] protected $custom_order; + /** + * @var int|null the raw admin-granted duration; the window end is derived, never stored + */ + #[ORM\Column(name: 'SubmissionReopenedHours', type: 'integer', nullable: true)] + protected $submission_reopened_hours = null; + + /** + * @var \DateTime|null + */ + #[ORM\Column(name: 'SubmissionReopenedDate', type: 'datetime', nullable: true)] + protected $submission_reopened_date = null; + + /** + * @var Member|null + */ + #[ORM\JoinColumn(name: 'SubmissionReopenedByID', referencedColumnName: 'ID', onDelete: 'SET NULL')] + #[ORM\ManyToOne(targetEntity: \models\main\Member::class, fetch: 'EXTRA_LAZY')] + protected $submission_reopened_by = null; + /** * @var PresentationSpeaker */ @@ -363,6 +382,9 @@ public function __construct() $this->attending_media = false; $this->will_all_speakers_attend = false; $this->disclaimer_accepted_date = null; + $this->submission_reopened_hours = null; + $this->submission_reopened_date = null; + $this->submission_reopened_by = null; $this->custom_order = 0; $this->track_chairs_scores = new ArrayCollection(); } @@ -2524,12 +2546,117 @@ public function getReviewStatusNice(): string return $review_status; } + public function getSubmissionReopenedHours(): ?int + { + return $this->submission_reopened_hours; + } + + public function getSubmissionReopenedDate(): ?\DateTime + { + return $this->submission_reopened_date; + } + + public function setSubmissionReopenedDate(?\DateTime $date): void + { + $this->submission_reopened_date = $date; + } + + public function getSubmissionReopenedBy(): ?Member + { + return $this->submission_reopened_by; + } + + public function getSubmissionReopenedById(): int + { + try { + return is_null($this->submission_reopened_by) ? 0 : $this->submission_reopened_by->getId(); + } catch (\Exception $ex) { + return 0; + } + } + + /** + * Preformatted for Show Admin. Serialized as a plain string because an $array_mappings + * entry invokes scalar getters only and cannot serialize a Member. + * + * Yes, this departs from the repo's usual "who did this" idiom (a *_by_id scalar plus an + * expand case, e.g. CreatedById on SummitEventSerializer). That idiom was considered and + * REJECTED by the signed-off SDS, because the expand switch lives in the base + * PresentationSerializer, so a case added there is reachable from the Public and Submission + * variants too -- which is the exact leak the Admin-only design exists to prevent. The SDS + * mandates this shape ("no expand needed"), and Show Admin reads the field straight off the + * getEvent payload. If a relation is ever wanted, the safe mechanism is a subclass-local + * $expand_mappings, never a base-class switch case. Do not "correct" this to id+expand. + */ + public function getSubmissionReopenedByNice(): ?string + { + $member = $this->submission_reopened_by; + if (is_null($member)) return null; + return sprintf("%s (%s)", $member->getFullName(), $member->getEmail()); + } + + /** + * Derived, never stored — so hours and date cannot drift out of lockstep. + */ + public function getSubmissionReopenedUntil(): ?\DateTime + { + if (is_null($this->submission_reopened_date) || is_null($this->submission_reopened_hours)) + return null; + return (clone $this->submission_reopened_date) + ->add(new \DateInterval(sprintf("PT%dH", $this->submission_reopened_hours))); + } + + /** + * A grant is only honored on a plan that is assigned, enabled, and whose submission + * window has actually ended. Deadline alone would grant edits before the CFP opened, + * and (via the isSubmissionClosed() fold) would open speaker deletes on a plan disabled + * after the grant — the delete path never re-checks IsEnabled() itself. + */ + public function isSubmissionReopened(): bool + { + $selection_plan = $this->selection_plan; + if (is_null($selection_plan)) return false; + if (!$selection_plan->IsEnabled()) return false; + + $submission_end_date = $selection_plan->getSubmissionEndDate(); + if (is_null($submission_end_date)) return false; + + $now = new \DateTime('now', new \DateTimeZone('UTC')); + // window not ended yet (pre-open or still open): there is nothing to reopen + if ($now <= $submission_end_date) return false; + + $until = $this->getSubmissionReopenedUntil(); + return !is_null($until) && $now < $until; + } + + public function reopenSubmission(int $hours, Member $actor): void + { + $this->submission_reopened_hours = $hours; + $this->submission_reopened_date = new \DateTime('now', new \DateTimeZone('UTC')); + $this->submission_reopened_by = $actor; + } + + /** + * Deliberately exempt from the invariants above: a stale grant must always be clearable. + */ + public function closeSubmissionNow(): void + { + $this->submission_reopened_hours = null; + $this->submission_reopened_date = null; + $this->submission_reopened_by = null; + } + /** * @return bool * @throws \Exception */ public function isSubmissionClosed(): bool { + // Fold: covers both production callers (PresentationService::deletePresentation and + // SummitService) with no change at either call site. Review-status-safe because + // getReviewStatus() recomputes submission_closed inline rather than calling this. + if ($this->isSubmissionReopened()) return false; + $selection_plan = $this->selection_plan; if (is_null($selection_plan)) return false; diff --git a/app/Services/Model/IPresentationSubmissionReopenService.php b/app/Services/Model/IPresentationSubmissionReopenService.php new file mode 100644 index 000000000..028fd3ee3 --- /dev/null +++ b/app/Services/Model/IPresentationSubmissionReopenService.php @@ -0,0 +1,49 @@ +isSubmissionOpen()) { + if (!$current_selection_plan->isSubmissionOpen() && !$presentation->isSubmissionReopened()) { throw new ValidationException(sprintf("Submission Period is Closed.")); } @@ -668,7 +668,7 @@ public function completePresentationSubmission(Summit $summit, $presentation_id) throw new ValidationException(sprintf("Submission Period is Closed.")); } - if (!$current_selection_plan->isSubmissionOpen()) { + if (!$current_selection_plan->isSubmissionOpen() && !$presentation->isSubmissionReopened()) { throw new ValidationException(sprintf("Submission Period is Closed.")); } diff --git a/app/Services/Model/Imp/PresentationSubmissionReopenService.php b/app/Services/Model/Imp/PresentationSubmissionReopenService.php new file mode 100644 index 000000000..82ab7b7ca --- /dev/null +++ b/app/Services/Model/Imp/PresentationSubmissionReopenService.php @@ -0,0 +1,89 @@ +tx_service->transaction(function () use ($summit, $presentation_id, $hours, $actor) { + + // summit-scoped unconditionally for every caller role -- deliberately stricter than + // the media-endpoint precedent, which scopes only its non-admin branch. + $presentation = $summit->getEvent($presentation_id); + if (!$presentation instanceof Presentation) + throw new EntityNotFoundException(sprintf("Presentation %s not found.", $presentation_id)); + + // whole hours rule in one place: null means "unspecified", so the default is resolved + // here rather than in the controller, right next to the ceiling it has to respect. + $hours = $hours ?? intval(Config::get('cfp.default_reopen_hours', 24)); + + // The lower bound is currently unreachable over HTTP -- the endpoint validates + // 'hours' => 'sometimes|integer|min:1' and refuses first -- but it is deliberate, not + // dead code: this is the only guard for any non-HTTP caller (job, console, another + // service), and a persisted non-positive value would make + // Presentation::getSubmissionReopenedUntil() throw on every read, since + // new \DateInterval('PT-1H') is invalid. Do not remove it as unused. + $max = intval(Config::get('cfp.max_reopen_hours', 168)); + if ($hours < 1 || $hours > $max) + throw new ValidationException(sprintf("hours must be between 1 and %s.", $max)); + + // Same invariants isSubmissionReopened() enforces, checked up front so the admin + // gets a clear error rather than a grant that silently never activates. + $selection_plan = $presentation->getSelectionPlan(); + if (is_null($selection_plan)) + throw new ValidationException("Presentation is not assigned to any selection plan."); + if (!$selection_plan->IsEnabled()) + throw new ValidationException("Selection plan is not enabled."); + + $submission_end_date = $selection_plan->getSubmissionEndDate(); + if (is_null($submission_end_date)) + throw new ValidationException("Selection plan has no submission end date."); + + $now = new \DateTime('now', new \DateTimeZone('UTC')); + if ($now <= $submission_end_date) + throw new ValidationException("Submission period has not ended yet; nothing to reopen."); + + $presentation->reopenSubmission($hours, $actor); + + return $presentation; + }); + } + + public function closeNow(Summit $summit, int $presentation_id, Member $actor): void + { + $this->tx_service->transaction(function () use ($summit, $presentation_id) { + + $presentation = $summit->getEvent($presentation_id); + if (!$presentation instanceof Presentation) + throw new EntityNotFoundException(sprintf("Presentation %s not found.", $presentation_id)); + + // no plan-state checks on purpose: a stale grant must always be clearable + $presentation->closeSubmissionNow(); + }); + } +} diff --git a/app/Services/ModelServicesProvider.php b/app/Services/ModelServicesProvider.php index c7554d647..27926fda6 100644 --- a/app/Services/ModelServicesProvider.php +++ b/app/Services/ModelServicesProvider.php @@ -149,6 +149,7 @@ use services\model\ChatTeamService; use services\model\IChatTeamService; use services\model\IPresentationService; +use services\model\IPresentationSubmissionReopenService; use services\model\ISpeakerService; use services\model\ISubmitterService; use services\model\ISummitAttendeeBadgePrintService; @@ -156,6 +157,7 @@ use services\model\ISummitService; use services\model\ISummitSponsorService; use services\model\PresentationService; +use services\model\PresentationSubmissionReopenService; use services\model\SpeakerService; use services\model\SubmitterService; use services\model\SummitAttendeeBadgePrintService; @@ -193,6 +195,8 @@ public function register() App::singleton(IPresentationService::class, PresentationService::class); + App::singleton(IPresentationSubmissionReopenService::class, PresentationSubmissionReopenService::class); + App::singleton(IChatTeamService::class, ChatTeamService::class); App::singleton diff --git a/config/cfp.php b/config/cfp.php index b46d96a21..54b8dc9ea 100644 --- a/config/cfp.php +++ b/config/cfp.php @@ -17,4 +17,8 @@ 'support_email' => env('CFP_SUPPORT_EMAIL', null), 'client_id' => env('CFP_OAUTH2_CLIENT_ID', null), 'scopes' => env('CFP_OAUTH2_SCOPES', null), -]; \ No newline at end of file + + // ceiling on an admin-granted per-presentation reopen window + 'max_reopen_hours' => (int) env('CFP_MAX_REOPEN_HOURS', 168), // 7 days + 'default_reopen_hours' => (int) env('CFP_DEFAULT_REOPEN_HOURS', 24), +]; diff --git a/database/migrations/config/Version20260807130000.php b/database/migrations/config/Version20260807130000.php new file mode 100644 index 000000000..e989d0cb5 --- /dev/null +++ b/database/migrations/config/Version20260807130000.php @@ -0,0 +1,76 @@ + 'reopen-presentation-submission-period', + 'route' => '/api/v1/summits/{id}/presentations/{presentation_id}/submission-period/reopen', + 'http_method' => 'PUT', + 'scopes' => [ + SummitScopes::WriteSummitData, + SummitScopes::WriteEventData, + SummitScopes::WritePresentationData, + ], + ], + [ + 'name' => 'close-presentation-submission-period', + 'route' => '/api/v1/summits/{id}/presentations/{presentation_id}/submission-period/reopen', + 'http_method' => 'DELETE', + 'scopes' => [ + SummitScopes::WriteSummitData, + SummitScopes::WriteEventData, + SummitScopes::WritePresentationData, + ], + ], + ]; + + public function getDescription(): string + { + return 'Register the per-activity CFP reopen/close endpoints.'; + } + + public function up(Schema $schema): void + { + $this->registerEndpoints(self::API_NAME, self::ENDPOINTS); + } + + public function down(Schema $schema): void + { + $this->unregisterEndpoints(self::API_NAME, array_column(self::ENDPOINTS, 'name')); + } +} diff --git a/database/migrations/model/Version20260807120000.php b/database/migrations/model/Version20260807120000.php new file mode 100644 index 000000000..d752c93da --- /dev/null +++ b/database/migrations/model/Version20260807120000.php @@ -0,0 +1,73 @@ +addSql(<<addSql('CREATE INDEX `SubmissionReopenedByID` ON `Presentation` (`SubmissionReopenedByID`)'); + + $this->addSql(<<addSql('ALTER TABLE `Presentation` DROP FOREIGN KEY `FK_Presentation_SubmissionReopenedBy`'); + $this->addSql('DROP INDEX `SubmissionReopenedByID` ON `Presentation`'); + $this->addSql(<< 'reopen-presentation-submission-period', + 'route' => '/api/v1/summits/{id}/presentations/{presentation_id}/submission-period/reopen', + 'http_method' => 'PUT', + 'scopes' => [ + SummitScopes::WriteSummitData, + SummitScopes::WriteEventData, + SummitScopes::WritePresentationData + ], + ], + [ + 'name' => 'close-presentation-submission-period', + 'route' => '/api/v1/summits/{id}/presentations/{presentation_id}/submission-period/reopen', + 'http_method' => 'DELETE', + 'scopes' => [ + SummitScopes::WriteSummitData, + SummitScopes::WriteEventData, + SummitScopes::WritePresentationData + ], + ], // presentation speakers [ 'name' => 'add-presentation-speaker', diff --git a/routes/api_v1.php b/routes/api_v1.php index 05bffbdc3..d0ef2dbe2 100644 --- a/routes/api_v1.php +++ b/routes/api_v1.php @@ -860,6 +860,15 @@ }); }); + // submission period (admin-only reopen; authorization is done in the controller, + // 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'); + Route::delete('', 'OAuth2PresentationApiController@closeSubmissionPeriod'); + }); + }); + // attendees votes Route::group(['prefix' => 'attendee-votes'], function () { Route::get('', ['uses' => 'OAuth2PresentationApiController@getAttendeeVotes']); diff --git a/tests/PresentationReopenApiTest.php b/tests/PresentationReopenApiTest.php new file mode 100644 index 000000000..0564a25c5 --- /dev/null +++ b/tests/PresentationReopenApiTest.php @@ -0,0 +1,701 @@ +setTitle("REOPEN API TEST"); + self::$presentation->setType(self::$defaultPresentationType); + self::$presentation->setSelectionPlan(self::$default_selection_plan); + // Creator is set HERE, not in a later task: getPresentationSubmission (Task 6) and the + // role=creator table feeds (Task 7) both require it. memberCanEdit() recognizes only + // creator / moderator / assigned speaker (Presentation.php:1254-1261), so without this + // every read in Tasks 6-8 returns 403. + self::$presentation->setCreatedBy(self::$member); + // update/complete also need a track (SummitEventValidationRulesFactory requires track_id) + self::$presentation->setCategory(self::$defaultTrack); + self::$summit->addEvent(self::$presentation); + + // the feature only applies once the window has ended + self::$default_selection_plan->setIsEnabled(true); + self::$default_selection_plan->setSubmissionBeginDate( + (new \DateTime('now', new \DateTimeZone('UTC')))->sub(new \DateInterval('P10D')) + ); + self::$default_selection_plan->setSubmissionEndDate( + (new \DateTime('now', new \DateTimeZone('UTC')))->sub(new \DateInterval('P1D')) + ); + + self::$em->persist(self::$summit); + self::$em->flush(); + } + + protected function tearDown(): void + { + self::clearSummitTestData(); + parent::tearDown(); + } + + protected function reopen(array $payload, ?int $presentation_id = null, ?int $summit_id = null) + { + $params = [ + 'id' => $summit_id ?? self::$summit->getId(), + 'presentation_id' => $presentation_id ?? self::$presentation->getId(), + ]; + $headers = $this->getAuthHeaders(); // includes CONTENT_TYPE: application/json + + return $this->action( + "PUT", "OAuth2PresentationApiController@reopenSubmissionPeriod", + $params, [], [], [], $headers, json_encode($payload) + ); + } + + protected function closeNow() + { + $params = [ + 'id' => self::$summit->getId(), + 'presentation_id' => self::$presentation->getId(), + ]; + $headers = $this->getAuthHeaders(); // includes CONTENT_TYPE: application/json + + return $this->action( + "DELETE", "OAuth2PresentationApiController@closeSubmissionPeriod", + $params, [], [], [], $headers + ); + } + + /** + * Reload from the DB rather than reading the response body. + * + * These assertions deliberately do NOT read submission_reopened_until out of the response: + * the serializer mappings that would put it there do not exist until Task 6, so asserting + * on the response here would make Task 5's "the API test now passes" gate unreachable. + * Task 6 adds the response-shape assertions once the mappings land. + * + * Uses self::$em (the 'model' manager, set in InsertSummitTestData::insertSummitTestData + * via Registry::getManager(SilverstripeBaseModel::EntityManager)), NOT the bare EntityManager + * facade: the facade defaults to the 'config' manager (config/doctrine.php lists 'config' + * first with no default override), and Presentation lives in the 'model' DB, so the facade + * throws TableNotFoundException. + * + * Each simulated HTTP dispatch ($this->action(...)) closes the 'model' entity manager at + * request end (confirmed live: after $this->reopen(), self::$em->isOpen() is already false + * with no exception/retry logged -- this is normal per-request teardown, not an error path). + * A second request (e.g. closeNow()) then transparently reopens 'model' via its own + * Registry::resetManager() call -- but that only updates the framework's registry entry, + * not our locally-cached self::$em, so re-fetching self::$em from Registry::getManager() + * (not blindly resetManager()-ing our stale copy again, which would discard that + * already-open, already-correct instance and start a THIRD one) is required before reading. + * Mirrors the exact pattern InsertSummitTestData::insertSummitTestData() itself uses + * (tests/InsertSummitTestData.php:311-314) to pick up the current 'model' manager. + */ + private function reloadPresentation(): Presentation + { + self::$em = Registry::getManager(SilverstripeBaseModel::EntityManager); + if (!self::$em->isOpen()) { + self::$em = Registry::resetManager(SilverstripeBaseModel::EntityManager); + } + self::$em->clear(); + return self::$em->getRepository(Presentation::class)->find(self::$presentation->getId()); + } + + /** + * buildForSubmission(..., update: true) (SummitEventValidationRulesFactory.php:139-157) marks + * exactly four fields 'required': title, type_id, track_id, selection_plan_id. Everything else + * is 'sometimes'. A partial payload 400s in getJsonPayload BEFORE the reopen gate runs, which + * would make the success case fail and every refusal case pass for the wrong reason. + * + * type_id/track_id must additionally be on the selection plan: saveOrUpdatePresentation checks + * hasEventType() (PresentationService.php:427) and hasTrack() (:451). The fixture's + * defaultPresentationType and defaultTrack both are (InsertSummitTestData.php:687, :686 via + * defaultTrackGroup), and type_id must equal the presentation's current type or ":436" refuses + * the change. + */ + private function validUpdatePayload(): array + { + return [ + 'title' => 'EDITED DURING REOPEN', + 'type_id' => self::$defaultPresentationType->getId(), + 'track_id' => self::$defaultTrack->getId(), + 'selection_plan_id' => self::$default_selection_plan->getId(), + ]; + } + + protected function updateSubmission() + { + $params = [ + 'id' => self::$summit->getId(), + 'presentation_id' => self::$presentation->getId(), + ]; + $headers = $this->getAuthHeaders(); // includes CONTENT_TYPE: application/json + + return $this->action( + "PUT", "OAuth2PresentationApiController@updatePresentationSubmission", + $params, [], [], [], $headers, json_encode($this->validUpdatePayload()) + ); + } + + protected function completeSubmission() + { + $params = [ + 'id' => self::$summit->getId(), + 'presentation_id' => self::$presentation->getId(), + ]; + $headers = $this->getAuthHeaders(); // includes CONTENT_TYPE: application/json + + return $this->action( + "PUT", "OAuth2PresentationApiController@completePresentationSubmission", + $params, [], [], [], $headers + ); + } + + /** + * A 412 alone does not prove the window gate refused: update/complete raise ValidationException + * (-> 412) from a dozen other places, and complete's post-gate media-upload and speaker checks + * are indistinguishable from the gate at the HTTP layer. Both window guards + * (PresentationService.php:542-548 for update, :666-672 for complete) carry the literal message + * "Submission Period is Closed.", which processRequest surfaces in `errors` + * (RequestProcessor.php:47 -> JsonController::error412 :140-144), so assert on that. + */ + private function assertRefusedBySubmissionWindow($response): void + { + $this->assertResponseStatus(412); + $body = json_decode($response->getContent(), true); + $this->assertIsArray($body['errors'] ?? null, $response->getContent()); + $this->assertContains( + 'Submission Period is Closed.', + $body['errors'], + 'refused with 412, but NOT by the submission-window gate: ' . $response->getContent() + ); + } + + private function assertErrorsContain($response, string $needle): void + { + $body = json_decode($response->getContent(), true); + $this->assertIsArray($body['errors'] ?? null, $response->getContent()); + $this->assertContains($needle, $body['errors'], $response->getContent()); + } + + /** + * assertArrayHasKey() alone cannot catch a broken serializer mapping. AbstractSerializer + * (libs/ModelSerializers/AbstractSerializer.php) assigns $new_values[$mapping[0]] = $value + * UNCONDITIONALLY, and swallows a missing-getter exception into $value = null with only a log + * warning -- so renaming getSubmissionReopenedUntil() without updating the 'datetime_epoch' + * mapping would ship null to the Show Admin column and both speaker tables with the key still + * present and a presence-only assertion still green. Every caller below grants a window first, + * so the value must be a real epoch in the future. + */ + private function assertFutureEpoch(array $payload, string $key, string $context = ''): void + { + $this->assertArrayHasKey($key, $payload, $context); + $this->assertNotNull( + $payload[$key], + sprintf('%s present but null -- the serializer mapping no longer resolves a getter. %s', $key, $context) + ); + $this->assertIsInt( + $payload[$key], + sprintf('%s is not an epoch integer: %s', $key, var_export($payload[$key], true)) + ); + $this->assertGreaterThan( + time(), + $payload[$key], + sprintf('%s is not in the future; the granted window never reached the response', $key) + ); + } + + /** + * Grant the window directly on the model instead of through reopen(). + * + * Same reason as testCloseNowClearsTheWindow below: a BrowserKit test cannot do two sequential + * HTTP-simulated writes against the same entity -- DoctrineMiddleware::handle closes the 'model' + * entity manager after every request and singleton repositories pin the manager instance they + * were first resolved with, so the second write mutates an untracked object and flush() silently + * persists nothing while still returning a success status. Every test below therefore arranges + * its precondition on the model and spends its one HTTP write on the endpoint under test. + */ + private function grantWindow(int $hours = 24): void + { + self::$presentation->reopenSubmission($hours, self::$member); + self::$em->flush(); + } + + public function testAdminCanReopenAClosedSubmission() + { + $this->reopen(['hours' => 24]); + $this->assertResponseStatus(201); + + $reloaded = $this->reloadPresentation(); + $this->assertTrue($reloaded->isSubmissionReopened()); + $this->assertEquals(24, $reloaded->getSubmissionReopenedHours()); + $this->assertGreaterThan(new \DateTime('now', new \DateTimeZone('UTC')), $reloaded->getSubmissionReopenedUntil()); + } + + /** + * 7, not the shipped 24, and asserted as a literal: building the expectation out of the same + * Config key the production code reads made this pass against a hardcoded 24 just as happily. + * Config::set reaches the dispatched request -- seedCompletionEmailConfig() below relies on + * exactly that. + */ + public function testReopenDefaultsToTheConfiguredWindowWhenHoursIsOmitted() + { + Config::set('cfp.default_reopen_hours', 7); + + $this->reopen([]); + $this->assertResponseStatus(201); + + $this->assertEquals(7, $this->reloadPresentation()->getSubmissionReopenedHours()); + } + + /** + * Max 9, not the shipped 168: sending Config::get('cfp.max_reopen_hours')+1 against a ceiling + * the service reads from the same key passed against a hardcoded 168 too. The message assertion + * is what proves the CEILING refused rather than one of the plan-state guards, all of which also + * surface as a bare 412. + */ + public function testHoursAboveMaxIsRejected() + { + Config::set('cfp.max_reopen_hours', 9); + + $response = $this->reopen(['hours' => 10]); + $this->assertResponseStatus(412); + $this->assertErrorsContain($response, 'hours must be between 1 and 9.'); + + $this->assertNull( + $this->reloadPresentation()->getSubmissionReopenedUntil(), + 'refused with 412 but the grant was written anyway' + ); + } + + /** + * The other side of the same boundary, so an off-by-one ceiling cannot pass. Its own test + * rather than a second request inside the one above: a BrowserKit test cannot do two + * sequential HTTP writes against the same entity (see grantWindow()). + */ + public function testHoursExactlyAtMaxIsAccepted() + { + Config::set('cfp.max_reopen_hours', 9); + + $this->reopen(['hours' => 9]); + $this->assertResponseStatus(201); + + $this->assertEquals(9, $this->reloadPresentation()->getSubmissionReopenedHours()); + } + + /** + * T3: nothing else sends a non-positive window. A2 moved this refusal to the request-validation + * layer ('hours' => 'sometimes|integer|min:1'), so the body is the validator's field-keyed shape + * -- NOT the service's flat "hours must be between 1 and %s." list. Asserted as what the code + * actually returns. + */ + public function testNonPositiveHoursIsRejected() + { + foreach ([0, -1] as $hours) { + $response = $this->reopen(['hours' => $hours]); + $this->assertResponseStatus(412); + + $body = json_decode($response->getContent(), true); + $this->assertEquals( + ['The hours must be at least 1.'], + $body['errors']['hours'] ?? null, + sprintf('hours=%d was not refused by the min:1 rule: %s', $hours, $response->getContent()) + ); + } + + $this->assertNull( + $this->reloadPresentation()->getSubmissionReopenedUntil(), + 'refused but a grant was written anyway' + ); + } + + public function testPresentationFromAnotherSummitReturns404() + { + // summit2 ships with no event types (InsertSummitTestData:587-602), so it needs one. + // Build a DEDICATED type: Summit::addEventType() calls $event_type->setSummit($this) + // (Summit.php:2748-2752), so reusing self::$defaultPresentationType would reassign it + // away from self::$summit and corrupt the primary fixture mid-test. + $foreign_type = new \models\summit\PresentationType(); + $foreign_type->setType("FOREIGN PRESENTATION TYPE"); + $foreign_type->setShouldBeAvailableOnCfp(true); + self::$summit2->addEventType($foreign_type); + + $foreign = new Presentation(); + $foreign->setTitle("FOREIGN"); + $foreign->setType($foreign_type); + self::$summit2->addEvent($foreign); + self::$em->persist(self::$summit2); + self::$em->flush(); + + $this->reopen(['hours' => 24], $foreign->getId()); + $this->assertResponseStatus(404); + + // §8 requires the scoping assertion on BOTH endpoints, not just reopen + $this->action( + "DELETE", "OAuth2PresentationApiController@closeSubmissionPeriod", + ['id' => self::$summit->getId(), 'presentation_id' => $foreign->getId()], + [], [], [], $this->getAuthHeaders() + ); + $this->assertResponseStatus(404); + } + + public function testCloseNowClearsTheWindow() + { + // Precondition ("already reopened") set directly via the model rather than through + // reopen(), so this test issues exactly one HTTP-simulated write instead of two. + // Verified live: the global DoctrineMiddleware (app/Http/Middleware/DoctrineMiddleware.php:38-42) + // closes the 'model' entity manager's connection after every request, and container-singleton + // repositories (e.g. DoctrineSummitRepository) pin to whichever manager instance existed at + // their first resolution -- so a second HTTP write in the same test resolves $summit via that + // now-stale repository, mutates a Presentation object the transaction's freshly-reset manager + // never tracked, and flush() silently has nothing to persist even though the response is 204. + // Raw-SQL-confirmed: with two sequential HTTP writes, SubmissionReopenedHours stayed at the + // reopen() value after a "successful" close. That's an orthogonal infra characteristic of + // per-request entity-manager closing, not a defect in reopenSubmission()/closeSubmissionNow() + // themselves, and out of scope for this test file -- so we avoid triggering it by using the + // same "set the precondition directly on the model, then flush" pattern setUp() already uses + // for the selection plan dates above. + self::$presentation->reopenSubmission(24, self::$member); + self::$em->flush(); + + $this->closeNow(); + $this->assertResponseStatus(204); + + $this->assertNull($this->reloadPresentation()->getSubmissionReopenedUntil()); + } + + public function testReopenIsRefusedWhileTheWindowIsStillOpen() + { + self::$default_selection_plan->setSubmissionEndDate( + (new \DateTime('now', new \DateTimeZone('UTC')))->add(new \DateInterval('P1D')) + ); + self::$em->flush(); + + $response = $this->reopen(['hours' => 24]); + $this->assertResponseStatus(412); + // a DIFFERENT message from assertRefusedBySubmissionWindow()'s: this one is the reopen + // service's own "still open" guard, so an unrelated 412 cannot stand in for it + $this->assertErrorsContain($response, 'Submission period has not ended yet; nothing to reopen.'); + } + + public function testReopenFieldsAppearOnDefaultAdminResponseWithoutAFieldsParam() + { + $this->reopen(['hours' => 24]); + $this->assertResponseStatus(201); + + $params = [ + 'id' => self::$summit->getId(), + 'event_id' => self::$presentation->getId(), + ]; + $headers = $this->getAuthHeaders(); // includes CONTENT_TYPE: application/json + + $response = $this->action( + "GET", "OAuth2SummitEventsApiController@getEvent", $params, [], [], [], $headers + ); + $this->assertResponseStatus(200); + + $payload = json_decode($response->getContent(), true); + $this->assertFutureEpoch($payload, 'submission_reopened_until', 'admin getEvent response'); + // the GRANTING member's id, not merely present: a bare new Presentation() serializes + // submission_reopened_by_id as 0 (getSubmissionReopenedById() returns 0 when unset), + // so a presence-only assertion proves nothing about the grant + $this->assertArrayHasKey('submission_reopened_by_id', $payload); + $this->assertEquals(self::$member->getId(), $payload['submission_reopened_by_id']); + $this->assertArrayHasKey('submission_reopened_by', $payload); + $this->assertIsString($payload['submission_reopened_by']); + } + + public function testByFieldsAreAbsentFromTheSubmissionSerializer() + { + $this->reopen(['hours' => 24]); + + $params = [ + 'id' => self::$summit->getId(), + 'presentation_id' => self::$presentation->getId(), + ]; + $headers = $this->getAuthHeaders(); // includes CONTENT_TYPE: application/json + + $response = $this->action( + "GET", "OAuth2PresentationApiController@getPresentationSubmission", + $params, [], [], [], $headers + ); + // 201, not 200: getPresentationSubmission returns $this->updated(...) + // (OAuth2PresentationApiController.php:457), and JsonController::updated() is 201. + $this->assertResponseStatus(201); + + $payload = json_decode($response->getContent(), true); + $this->assertFutureEpoch($payload, 'submission_reopened_until', 'submission serializer response'); + $this->assertArrayNotHasKey('submission_reopened_by_id', $payload); + $this->assertArrayNotHasKey('submission_reopened_by', $payload); + } + + public function testReopenFieldsNeverAppearOnAPublicSerializedResponse() + { + $this->reopen(['hours' => 24]); + + $payload = SerializerRegistry::getInstance()->getSerializer( + $this->reloadPresentation(), SerializerRegistry::SerializerType_Public + )->serialize(); + + $this->assertArrayNotHasKey('submission_reopened_until', $payload); + $this->assertArrayNotHasKey('submission_reopened_by_id', $payload); + $this->assertArrayNotHasKey('submission_reopened_by', $payload); + } + + public function testSpeakerPresentationListReturnsTheReopenWindow() + { + $this->reopen(['hours' => 24]); + $this->assertResponseStatus(201); + + $params = [ + 'id' => self::$summit->getId(), + 'role' => 'creator', + 'selection_plan_id' => self::$default_selection_plan->getId(), + ]; + $headers = $this->getAuthHeaders(); // includes CONTENT_TYPE: application/json + + $response = $this->action( + "GET", + "OAuth2SummitSpeakersApiController@getMySpeakerPresentationsByRoleAndBySelectionPlan", + $params, [], [], [], $headers + ); + $this->assertResponseStatus(200); + + $page = json_decode($response->getContent(), true); + $this->assertNotEmpty($page['data'], 'list returned no rows; the token member did not create the presentation'); + $this->assertFutureEpoch( + $page['data'][0], + 'submission_reopened_until', + 'if the key is missing the list is Public-serialized: PagingResponse::toArray() was called without a serializer type' + ); + } + + public function testSummitWideSpeakerListAlsoReturnsTheReopenWindow() + { + $this->reopen(['hours' => 24]); + $this->assertResponseStatus(201); + + // NOTE: this route's param is {summit_id}, not {id} -- it lives under a different + // prefix group (routes/api_v1.php:2309) than the selection-plan sibling above. + $params = [ + 'summit_id' => self::$summit->getId(), + 'role' => 'creator', + ]; + $headers = $this->getAuthHeaders(); // includes CONTENT_TYPE: application/json + + $response = $this->action( + "GET", + "OAuth2SummitSpeakersApiController@getMySpeakerPresentationsByRoleAndBySummit", + $params, [], [], [], $headers + ); + $this->assertResponseStatus(200); + + $page = json_decode($response->getContent(), true); + $this->assertNotEmpty($page['data']); + $this->assertFutureEpoch($page['data'][0], 'submission_reopened_until', 'summit-wide speaker list row'); + } + + // --------------------------------------------------------------------------------------------- + // Acceptance: an active grant actually lets the speaker edit after the window closed. + // The token member is both a global admin AND the presentation's creator with a speaker profile + // (InsertMemberTestData.php:144-149, plus setCreatedBy() in setUp above), so one token drives + // both roles. canEdit() matches on getCreatedById() == speaker->getMemberId() + // (Presentation.php:1264-1270). + // --------------------------------------------------------------------------------------------- + + public function testActiveGrantLetsTheSpeakerUpdateAfterTheWindowClosed() + { + $this->grantWindow(24); + // prove the arrange step landed in the DB: without this, a passing refusal sibling and a + // failing success case here would be indistinguishable from "the grant was never written" + $this->assertTrue( + $this->reloadPresentation()->isSubmissionReopened(), + 'grant did not persist; the assertions below would not be testing the gate' + ); + + $this->updateSubmission(); + $this->assertResponseStatus(201); + // assert the edit actually landed -- a 201 from an ignored payload must not pass + $this->assertEquals('EDITED DURING REOPEN', $this->reloadPresentation()->getTitle()); + } + + public function testWithoutAGrantTheSameUpdateIsRefused() + { + // precondition: no grant at all. setUp() never grants one, but assert it rather than assume. + $this->assertNull(self::$presentation->getSubmissionReopenedUntil()); + $this->assertFalse(self::$presentation->isSubmissionReopened()); + + $this->assertRefusedBySubmissionWindow($this->updateSubmission()); + } + + public function testExpiredGrantRefusesTheUpdate() + { + // a 1h grant whose start is backdated 2h => getSubmissionReopenedUntil() is 1h in the past + self::$presentation->reopenSubmission(1, self::$member); + self::$presentation->setSubmissionReopenedDate( + (new \DateTime('now', new \DateTimeZone('UTC')))->sub(new \DateInterval('PT2H')) + ); + self::$em->flush(); + + // precondition: a grant EXISTS (so this is not vacuously the no-grant case) but has expired + $reloaded = $this->reloadPresentation(); + $this->assertNotNull($reloaded->getSubmissionReopenedUntil(), 'grant did not persist'); + $this->assertLessThan(new \DateTime('now', new \DateTimeZone('UTC')), $reloaded->getSubmissionReopenedUntil()); + $this->assertFalse($reloaded->isSubmissionReopened()); + + $this->assertRefusedBySubmissionWindow($this->updateSubmission()); + } + + public function testCloseNowImmediatelyRefusesTheUpdate() + { + // closeSubmissionNow() is invoked on the model, not through the DELETE endpoint: chaining + // close + update would be two HTTP writes on the same entity (see grantWindow() above). + // testCloseNowClearsTheWindow already proves the endpoint clears the grant; this proves a + // cleared grant refuses the edit -- i.e. closing is not a no-op that leaves the window live. + self::$presentation->reopenSubmission(24, self::$member); + self::$em->flush(); + $this->assertTrue($this->reloadPresentation()->isSubmissionReopened(), 'grant did not persist'); + + self::$presentation = self::$em->getRepository(Presentation::class)->find(self::$presentation->getId()); + self::$presentation->closeSubmissionNow(); + self::$em->flush(); + + // precondition: the grant is gone + $this->assertNull($this->reloadPresentation()->getSubmissionReopenedUntil(), 'close did not persist'); + + $this->assertRefusedBySubmissionWindow($this->updateSubmission()); + } + + public function testDisablingThePlanAfterAGrantRefusesTheUpdate() + { + $this->grantWindow(24); + self::$default_selection_plan->setIsEnabled(false); + self::$em->flush(); + + // What the 412 below does and does NOT prove. The refusal comes from the PRE-EXISTING + // !IsEnabled() guard (PresentationService.php:543-545), which throws the identical + // "Submission Period is Closed." one branch before the reopen-aware guard at :547 -- so the + // message cannot discriminate the two, and this test does not by itself prove the grant was + // overridden rather than merely bypassed. The real proof that isSubmissionReopened() returns + // false BECAUSE of IsEnabled() is the model-level test (PresentationReopenModelTest) plus the + // preconditions asserted here: the grant is present and still in the future, the plan really + // is disabled, and isSubmissionReopened() is nonetheless false. What this test adds is that + // the HTTP path refuses too -- i.e. no controller/service layer re-opens the edit. + $reloaded = $this->reloadPresentation(); + $this->assertNotNull($reloaded->getSubmissionReopenedUntil(), 'grant did not persist'); + $this->assertGreaterThan(new \DateTime('now', new \DateTimeZone('UTC')), $reloaded->getSubmissionReopenedUntil()); + $this->assertFalse($reloaded->getSelectionPlan()->IsEnabled(), 'plan disable did not persist'); + $this->assertFalse($reloaded->isSubmissionReopened()); + + $this->assertRefusedBySubmissionWindow($this->updateSubmission()); + } + + // --------------------------------------------------------------------------------------------- + // complete is a SEPARATE gate from update (PresentationService.php:666-672) and runs three more + // checks AFTER it, so the fixture must satisfy all three or a failure downstream of the gate + // reads as a false negative on the feature. Two are already satisfied; one is not: + // - isSubmitted() (:653 -> Presentation.php:1345): PHASE_COMPLETE && STATUS_RECEIVED. A freshly + // built Presentation is neither, so complete is allowed. No fixture work. + // - fulfilMediaUploadsConditions() (Presentation.php:1290): the 5 media upload types attached + // to defaultPresentationType (InsertSummitTestData.php:438-448) never call + // setMinUploadsQty(), and SummitMediaUploadType::__construct defaults min_uploads_qty to 0 + // (SummitMediaUploadType.php:125), so getMandatoryAllowedMediaUploadTypesCount() is 0 and the + // method short-circuits true at :1297. No upload needs to be attached. + // - fulfilSpeakersConditions() (:1305) is the ONE that needs fixture work -> attachSpeaker(). + // useModerator=false skips the moderator branch, but useSpeakers=true with minSpeakers=1 + // (InsertSummitTestData.php:414-418) refuses an empty presentation at :1322: the fixture's + // setAreSpeakersMandatory(false) (:420) is inert, because + // PresentationType::isAreSpeakersMandatory() ignores that column and returns + // min_speakers > 0 (PresentationType.php:193-196). Verified empirically -- the precondition + // assertion below failed until attachSpeaker() was added. One speaker satisfies + // min 1 <= count 1 <= max 3. + // The remaining hazard is the notification email: complete dispatches + // PresentationCreatorNotificationEmail, whose constructor throws \InvalidArgumentException (-> + // 400, NOT 412) when cfp.base_url / idp.base_url / support email are empty. Set them so a config + // gap cannot be mistaken for a gate refusal. + // --------------------------------------------------------------------------------------------- + + private function seedCompletionEmailConfig(): void + { + Config::set('cfp.base_url', 'https://testcfp.openstack.org'); + Config::set('cfp.support_email', 'test@openstack.org'); + Config::set('idp.base_url', 'https://testidp.openstack.org'); + } + + /** + * self::$speaker is the token member's own speaker profile (InsertMemberTestData.php:144-149), + * so this satisfies fulfilSpeakersConditions() without introducing a second identity. canEdit() + * already passed via the creator branch, so this changes nothing about authorization. + */ + private function attachSpeaker(): void + { + self::$presentation->addSpeaker(self::$speaker); + self::$em->flush(); + } + + public function testActiveGrantLetsTheSpeakerCompleteAfterTheWindowClosed() + { + $this->seedCompletionEmailConfig(); + $this->attachSpeaker(); + $this->grantWindow(24); + + $reloaded = $this->reloadPresentation(); + $this->assertTrue($reloaded->isSubmissionReopened(), 'grant did not persist'); + // prove the post-gate checks cannot be what fails, so a non-201 below indicts the gate + $this->assertFalse($reloaded->isSubmitted()); + $this->assertTrue($reloaded->fulfilMediaUploadsConditions()); + $this->assertTrue($reloaded->fulfilSpeakersConditions()); + + $this->completeSubmission(); + $this->assertResponseStatus(201); + // the request passed THROUGH the gate rather than never reaching it: complete's only + // side effect is progress/status, and isSubmitted() is exactly that pair + $this->assertTrue($this->reloadPresentation()->isSubmitted()); + } + + public function testWithoutAGrantCompleteIsRefused() + { + $this->seedCompletionEmailConfig(); + $this->attachSpeaker(); + + // precondition: no grant, and the presentation is otherwise completable -- so the 412 below + // can only come from the window gate, not from isSubmitted()/media/speaker conditions + $this->assertFalse(self::$presentation->isSubmissionReopened()); + $this->assertFalse(self::$presentation->isSubmitted()); + $this->assertTrue(self::$presentation->fulfilMediaUploadsConditions()); + $this->assertTrue(self::$presentation->fulfilSpeakersConditions()); + + $this->assertRefusedBySubmissionWindow($this->completeSubmission()); + $this->assertFalse($this->reloadPresentation()->isSubmitted()); + } +} diff --git a/tests/PresentationReopenAuthzTest.php b/tests/PresentationReopenAuthzTest.php new file mode 100644 index 000000000..667e55aa5 --- /dev/null +++ b/tests/PresentationReopenAuthzTest.php @@ -0,0 +1,560 @@ +setCurrentGroup(IGroup::SummitRegistrationAdmins); + parent::setUp(); + + // Lever 2: the token's IdP groups. AccessTokenServiceStub's constructor DEFAULTS + // $idp_user_groups to ['badge-printers','administrators'] (ProtectedApiTestCase.php:38-47) + // and createApplication() installs it with no arguments (:327), so isAdmin() returns true + // via isOnExternalGroup(Administrators) (Member.php:916-917) no matter what lever 1 says. + // Replace the stub AND re-bind it: App::singleton() -> bind() drops any already-resolved + // instance, which reassigning the static alone would not do. + self::$service = new AccessTokenServiceStub([['slug' => IGroup::BadgePrinters]]); + App::singleton(IAccessTokenService::class, function () { return self::$service; }); + // parent::setUp() seeded the identity onto the OLD stub; re-seed it onto the new one or the + // token carries no user and every endpoint 403s on is_null($current_member) instead. + self::$service->setUserId(self::$member->getUserExternalId()); + self::$service->setUserExternalId(self::$member->getUserExternalId()); + self::$service->setUserEmail(self::$member->getEmail()); + self::$service->setUserFirstName(self::$member->getFirstName()); + self::$service->setUserLastName(self::$member->getLastName()); + + self::insertSummitTestData(); + + self::$presentation = new Presentation(); + self::$presentation->setTitle("REOPEN AUTHZ TEST"); + self::$presentation->setType(self::$defaultPresentationType); + self::$presentation->setSelectionPlan(self::$default_selection_plan); + self::$presentation->setCreatedBy(self::$member); + self::$presentation->setCategory(self::$defaultTrack); + self::$summit->addEvent(self::$presentation); + + // Step 5's fixture: same summit/plan/type/track, but created by a DIFFERENT member and with + // no speaker or moderator link to the token member, so canEdit() is false for it. + self::$foreign_presentation = new Presentation(); + self::$foreign_presentation->setTitle("OWNED BY SOMEONE ELSE"); + self::$foreign_presentation->setType(self::$defaultPresentationType); + self::$foreign_presentation->setSelectionPlan(self::$default_selection_plan); + self::$foreign_presentation->setCreatedBy(self::$member2); + self::$foreign_presentation->setCategory(self::$defaultTrack); + self::$summit->addEvent(self::$foreign_presentation); + + // the feature only applies once the window has ended + self::$default_selection_plan->setIsEnabled(true); + self::$default_selection_plan->setSubmissionBeginDate( + (new \DateTime('now', new \DateTimeZone('UTC')))->sub(new \DateInterval('P10D')) + ); + self::$default_selection_plan->setSubmissionEndDate( + (new \DateTime('now', new \DateTimeZone('UTC')))->sub(new \DateInterval('P1D')) + ); + + self::$em->persist(self::$summit); + self::$em->flush(); + + $this->assertIdentityIsNotAdmin(); + } + + protected function tearDown(): void + { + self::clearSummitTestData(); + parent::tearDown(); + } + + /** + * Both admin levers, asserted separately so a regression names which one came back. + * + * isAdmin(true) skips the external check, which isolates lever 1 (a real DB read). + * Lever 2 cannot be isolated through isAdmin(): isOnExternalGroup() (Member.php:987-1006) reads + * the request-scoped resource-server context, which is empty outside a dispatched request, so + * isAdmin() would report false here even with the admin-defaulted stub still installed -- a + * false negative. So assert on the token the container actually hands the middleware instead. + * + * Called from setUp(), i.e. it guards every test in this class, and separately as the named + * canary test below. + */ + private function assertIdentityIsNotAdmin(): void + { + // lever 1: persisted groups + $this->assertNull( + self::$member->getGroupByCode(IGroup::Administrators), + 'fixture member is persisted into the administrators group' + ); + $this->assertNull( + self::$member->getGroupByCode(IGroup::SuperAdmins), + 'fixture member is persisted into the super-admins group' + ); + $this->assertFalse( + self::$member->isAdmin(true), + 'fixture is still a global admin by persisted group; 403 tests here are meaningless' + ); + + // lever 2: the IdP groups on the token the container is serving + $slugs = array_column( + App::make(IAccessTokenService::class)->get($this->access_token)->getUserGroups(), + 'slug' + ); + $this->assertNotContains( + IGroup::Administrators, + $slugs, + 'the access-token stub still reports the administrators IdP group; isAdmin() will be ' + . 'true inside a request and every 403 test here is meaningless' + ); + $this->assertNotContains(IGroup::SuperAdmins, $slugs); + + $this->assertFalse(self::$member->isAdmin(), 'fixture is still a global admin'); + } + + /** + * Same manager-refresh dance as PresentationReopenApiTest::reloadPresentation(): self::$em is + * the 'model' manager, the bare EntityManager facade resolves to 'config'/api_config where + * Presentation does not exist, and each dispatched request closes 'model' on the way out + * (DoctrineMiddleware::handle), so re-fetch from Registry before reading. + */ + private function reload(int $id): ?Presentation + { + self::$em = Registry::getManager(SilverstripeBaseModel::EntityManager); + if (!self::$em->isOpen()) { + self::$em = Registry::resetManager(SilverstripeBaseModel::EntityManager); + } + self::$em->clear(); + return self::$em->getRepository(Presentation::class)->find($id); + } + + /** + * buildForSubmission() marks exactly title/type_id/track_id/selection_plan_id required + * (SummitEventValidationRulesFactory.php:139-157). A partial payload 400s before any gate runs. + */ + private function validUpdatePayload(): array + { + return [ + 'title' => 'EDITED DURING REOPEN', + 'type_id' => self::$defaultPresentationType->getId(), + 'track_id' => self::$defaultTrack->getId(), + 'selection_plan_id' => self::$default_selection_plan->getId(), + ]; + } + + /** + * Arrange the grant on the model, never through the reopen endpoint: a BrowserKit test cannot + * do two sequential HTTP writes against the same entity (DoctrineMiddleware::handle closes the + * 'model' EM after every request and singleton repositories pin the manager they were first + * resolved with, so the second write mutates an untracked object and flush() silently persists + * nothing while still returning success). Every test below spends its one HTTP write on the + * endpoint under test. This identity could not call the admin endpoint anyway. + */ + private function grantWindow(Presentation $presentation, int $hours = 24): void + { + $presentation->reopenSubmission($hours, self::$member2); + self::$em->flush(); + } + + private function assertRefusedByDeleteGuard($response, int $presentation_id): void + { + $this->assertResponseStatus(412); + $body = json_decode($response->getContent(), true); + $this->assertIsArray($body['errors'] ?? null, $response->getContent()); + $this->assertContains( + sprintf("Presentation %s can not be deleted because the submission is closed.", $presentation_id), + $body['errors'], + 'refused with 412, but NOT by the closed-submission delete guard: ' . $response->getContent() + ); + } + + private function assertErrorsContain($response, string $needle): void + { + $body = json_decode($response->getContent(), true); + $this->assertIsArray($body['errors'] ?? null, $response->getContent()); + $this->assertContains($needle, $body['errors'], $response->getContent()); + } + + // --------------------------------------------------------------------------------------------- + // Canary + // --------------------------------------------------------------------------------------------- + + public function testFixtureIdentityIsGenuinelyNotAdmin() + { + $this->assertFalse(self::$member->isAdmin(), 'fixture is still a global admin; 403 tests below are meaningless'); + // and the full two-lever check, in case isAdmin() alone ever stops covering both + $this->assertIdentityIsNotAdmin(); + } + + // --------------------------------------------------------------------------------------------- + // Step 3: a non-admin can neither reopen nor close. + // + // 403-vs-201 is self-validating against the admin trap: a global admin would get 201/204 here, + // so these cannot pass because the identity is too privileged. The other way to get a spurious + // 403 is the token failing to resolve to a member at all + // (OAuth2PresentationApiController.php:575-576) -- ruled out class-wide by the delete tests + // below, which 204 only because the token resolves to self::$member, the presentation's creator + // with a speaker profile. Each test also asserts the grant state is UNCHANGED, so a refusal + // that somehow still mutated would fail. + // --------------------------------------------------------------------------------------------- + + public function testMemberWithNoSummitAdminPermissionCannotReopen() + { + // precondition: no grant, and the window really has closed (so 403 is not standing in for + // the service's "nothing to reopen" 412) + $this->assertNull(self::$presentation->getSubmissionReopenedUntil()); + $this->assertFalse(self::$default_selection_plan->isSubmissionOpen()); + + $this->action( + "PUT", "OAuth2PresentationApiController@reopenSubmissionPeriod", + ['id' => self::$summit->getId(), 'presentation_id' => self::$presentation->getId()], + [], [], [], $this->getAuthHeaders(), json_encode(['hours' => 24]) + ); + $this->assertResponseStatus(403); + + $this->assertNull( + $this->reload(self::$presentation->getId())->getSubmissionReopenedUntil(), + 'refused with 403 but the grant was written anyway' + ); + } + + public function testMemberWithNoSummitAdminPermissionCannotClose() + { + $this->grantWindow(self::$presentation); + // precondition: there IS a grant to clear, so a 403 cannot be a no-op passing vacuously + $this->assertTrue( + $this->reload(self::$presentation->getId())->isSubmissionReopened(), + 'grant did not persist; this test would not be exercising the close endpoint' + ); + + $this->action( + "DELETE", "OAuth2PresentationApiController@closeSubmissionPeriod", + ['id' => self::$summit->getId(), 'presentation_id' => self::$presentation->getId()], + [], [], [], $this->getAuthHeaders() + ); + $this->assertResponseStatus(403); + + $this->assertTrue( + $this->reload(self::$presentation->getId())->isSubmissionReopened(), + 'refused with 403 but the grant was cleared anyway' + ); + } + + /** + * Persona (b): being a speaker ON the presentation is deliberately not a fallback -- §4 gives no + * speaker path to the reopen controls. This is the strongest form: the member is the creator AND + * an assigned speaker, i.e. memberCanEdit() is true, and it is still refused. + */ + public function testSpeakerOnThePresentationStillCannotReopen() + { + self::$presentation->addSpeaker(self::$speaker); + self::$em->flush(); + + $reloaded = $this->reload(self::$presentation->getId()); + $this->assertTrue($reloaded->memberCanEdit(self::$member), 'speaker/creator link did not persist'); + $this->assertNull($reloaded->getSubmissionReopenedUntil()); + + $this->action( + "PUT", "OAuth2PresentationApiController@reopenSubmissionPeriod", + ['id' => self::$summit->getId(), 'presentation_id' => self::$presentation->getId()], + [], [], [], $this->getAuthHeaders(), json_encode(['hours' => 24]) + ); + $this->assertResponseStatus(403); + + $this->assertNull( + $this->reload(self::$presentation->getId())->getSubmissionReopenedUntil(), + 'refused with 403 but the grant was written anyway' + ); + } + + public function testSpeakerOnThePresentationStillCannotClose() + { + self::$presentation->addSpeaker(self::$speaker); + $this->grantWindow(self::$presentation); + + $reloaded = $this->reload(self::$presentation->getId()); + $this->assertTrue($reloaded->memberCanEdit(self::$member), 'speaker/creator link did not persist'); + $this->assertTrue($reloaded->isSubmissionReopened(), 'grant did not persist'); + + $this->action( + "DELETE", "OAuth2PresentationApiController@closeSubmissionPeriod", + ['id' => self::$summit->getId(), 'presentation_id' => self::$presentation->getId()], + [], [], [], $this->getAuthHeaders() + ); + $this->assertResponseStatus(403); + + $this->assertTrue( + $this->reload(self::$presentation->getId())->isSubmissionReopened(), + 'refused with 403 but the grant was cleared anyway' + ); + } + + // --------------------------------------------------------------------------------------------- + // The _by fields are admin-only ON THE WIRE. + // + // PresentationReopenApiTest's Public-serializer test asserts this by calling SerializerRegistry + // directly, which bypasses the thing that actually decides the type on a real read: + // OAuth2SummitEventsApiController::getSerializerType() (:117-133) returns Private only for an + // ApplicationType_Service token or a current user who isAdmin()/isSummitAdmin(), and Public + // otherwise. This class holds the only genuinely non-admin identity in the feature's tests + // (SummitRegistrationAdmins satisfies neither predicate -- Member::isSummitAdmin() :923-931 keys + // on SummitAdministrators, and the harness token is WEB_APPLICATION, not SERVICE), so this is + // where that branch can be exercised end to end. getEvent carries no 'auth.user' middleware + // (routes/api_v1.php:698), so the request reaches the controller. + // --------------------------------------------------------------------------------------------- + + public function testNonAdminEventReadDoesNotExposeTheByFields() + { + $this->grantWindow(self::$presentation); + $this->assertTrue( + $this->reload(self::$presentation->getId())->isSubmissionReopened(), + 'grant did not persist; an absent field would prove nothing' + ); + + $response = $this->action( + "GET", "OAuth2SummitEventsApiController@getEvent", + ['id' => self::$summit->getId(), 'event_id' => self::$presentation->getId()], + [], [], [], $this->getAuthHeaders() + ); + $this->assertResponseStatus(200); + + $payload = json_decode($response->getContent(), true); + // the assertions below are not vacuous on an empty/error body + $this->assertEquals(self::$presentation->getId(), $payload['id'] ?? null, $response->getContent()); + $this->assertEquals('REOPEN AUTHZ TEST', $payload['title'] ?? null, $response->getContent()); + + $this->assertArrayNotHasKey('submission_reopened_by', $payload, $response->getContent()); + $this->assertArrayNotHasKey('submission_reopened_by_id', $payload, $response->getContent()); + } + + // --------------------------------------------------------------------------------------------- + // Step 4: both delete paths honor a reopen, with no change at either call site. + // + // Path A: DELETE /summits/{id}/presentations/{presentation_id} + // -> OAuth2PresentationApiController@deletePresentation -> PresentationService::deletePresentation + // guard at PresentationService.php:590-591. + // Path B: DELETE /summits/{id}/events/{event_id} + // -> OAuth2SummitEventsApiController@deleteEvent -> SummitService::deleteEvent + // guard at SummitService.php:1076-1077. + // Both guards read Presentation::isSubmissionClosed(), which Task 1 folded the reopen check + // into -- that fold is the only reason the reopened cases below can pass. + // --------------------------------------------------------------------------------------------- + + public function testReopenedPresentationCanBeDeletedByANonAdminCreator() + { + $this->grantWindow(self::$presentation); + + $reloaded = $this->reload(self::$presentation->getId()); + $this->assertTrue($reloaded->isSubmissionReopened(), 'grant did not persist'); + $this->assertFalse($reloaded->isSubmissionClosed(), 'the fold did not treat the grant as re-opening'); + + $id = self::$presentation->getId(); + $this->action( + "DELETE", "OAuth2PresentationApiController@deletePresentation", + ['id' => self::$summit->getId(), 'presentation_id' => $id], + [], [], [], $this->getAuthHeaders() + ); + $this->assertResponseStatus(204); + $this->assertNull($this->reload($id), '204 returned but the presentation is still there'); + } + + public function testMerelyClosedPresentationCannotBeDeletedByANonAdminCreator() + { + // precondition: no grant, window ended => isSubmissionClosed() is true + $reloaded = $this->reload(self::$presentation->getId()); + $this->assertNull($reloaded->getSubmissionReopenedUntil()); + $this->assertTrue($reloaded->isSubmissionClosed(), 'fixture window is not actually closed'); + + $id = self::$presentation->getId(); + $response = $this->action( + "DELETE", "OAuth2PresentationApiController@deletePresentation", + ['id' => self::$summit->getId(), 'presentation_id' => $id], + [], [], [], $this->getAuthHeaders() + ); + $this->assertRefusedByDeleteGuard($response, $id); + $this->assertNotNull($this->reload($id), 'refused with 412 but the presentation was deleted anyway'); + } + + public function testReopenedPresentationCanBeDeletedViaTheEventDeletePath() + { + $this->grantWindow(self::$presentation); + + $reloaded = $this->reload(self::$presentation->getId()); + $this->assertTrue($reloaded->isSubmissionReopened(), 'grant did not persist'); + $this->assertFalse($reloaded->isSubmissionClosed(), 'the fold did not treat the grant as re-opening'); + + $id = self::$presentation->getId(); + $this->action( + "DELETE", "OAuth2SummitEventsApiController@deleteEvent", + ['id' => self::$summit->getId(), 'event_id' => $id], + [], [], [], $this->getAuthHeaders() + ); + // a 403 here would mean the auth.user middleware refused before SummitService ran + $this->assertResponseStatus(204); + $this->assertNull($this->reload($id), '204 returned but the presentation is still there'); + } + + public function testMerelyClosedPresentationCannotBeDeletedViaTheEventDeletePath() + { + $reloaded = $this->reload(self::$presentation->getId()); + $this->assertNull($reloaded->getSubmissionReopenedUntil()); + $this->assertTrue($reloaded->isSubmissionClosed(), 'fixture window is not actually closed'); + + $id = self::$presentation->getId(); + $response = $this->action( + "DELETE", "OAuth2SummitEventsApiController@deleteEvent", + ['id' => self::$summit->getId(), 'event_id' => $id], + [], [], [], $this->getAuthHeaders() + ); + $this->assertRefusedByDeleteGuard($response, $id); + $this->assertNotNull($this->reload($id), 'refused with 412 but the presentation was deleted anyway'); + } + + // --------------------------------------------------------------------------------------------- + // Step 5: reopen relaxes TIMING, never WHO. + // + // updatePresentationSubmission checks canEdit() (PresentationService.php:530-534) BEFORE the + // window gate (:543-548), so the refusal message discriminates authorship from timing: this + // must be the canEdit message, NOT "Submission Period is Closed." -- otherwise the grant would + // not actually be active and the test would prove nothing about authorship. + // --------------------------------------------------------------------------------------------- + + public function testNonEditorStillCannotUpdateAReopenedPresentation() + { + $this->grantWindow(self::$foreign_presentation); + + $reloaded = $this->reload(self::$foreign_presentation->getId()); + $this->assertTrue($reloaded->isSubmissionReopened(), 'grant did not persist; timing would refuse first'); + $this->assertFalse( + $reloaded->memberCanEdit(self::$member), + 'fixture member CAN edit this presentation; the test is not exercising the authorship gate' + ); + + $response = $this->action( + "PUT", "OAuth2PresentationApiController@updatePresentationSubmission", + ['id' => self::$summit->getId(), 'presentation_id' => self::$foreign_presentation->getId()], + [], [], [], $this->getAuthHeaders(), json_encode($this->validUpdatePayload()) + ); + $this->assertResponseStatus(412); + $this->assertErrorsContain( + $response, + sprintf('Current Speaker can not edit %s presentation', self::$foreign_presentation->getId()) + ); + + $this->assertEquals( + 'OWNED BY SOMEONE ELSE', + $this->reload(self::$foreign_presentation->getId())->getTitle(), + 'refused with 412 but the edit landed anyway' + ); + } + + // --------------------------------------------------------------------------------------------- + // Step 6: creation is untouched by a reopen. + // + // submitPresentation gates on the PLAN only (PresentationService.php:277-306) and never consults + // any presentation, so a live grant on a sibling presentation must not leak into it. + // --------------------------------------------------------------------------------------------- + + public function testCreateIsStillBlockedWhileAnotherPresentationOnThePlanHasAnActiveGrant() + { + $this->grantWindow(self::$presentation); + + $reloaded = $this->reload(self::$presentation->getId()); + $this->assertTrue($reloaded->isSubmissionReopened(), 'grant did not persist; nothing could leak'); + $this->assertTrue($reloaded->getSelectionPlan()->IsEnabled()); + $this->assertFalse($reloaded->getSelectionPlan()->isSubmissionOpen(), 'plan window is not actually closed'); + + $before = self::$em->getRepository(Presentation::class) + ->count(['summit' => self::$summit->getId()]); + + $response = $this->action( + "POST", "OAuth2PresentationApiController@submitPresentation", + ['id' => self::$summit->getId()], + [], [], [], $this->getAuthHeaders(), + json_encode([ + 'title' => 'SHOULD NOT BE CREATED', + 'type_id' => self::$defaultPresentationType->getId(), + 'track_id' => self::$defaultTrack->getId(), + 'selection_plan_id' => self::$default_selection_plan->getId(), + ]) + ); + $this->assertResponseStatus(412); + $this->assertErrorsContain($response, 'Submission Period is Closed.'); + + self::$em = Registry::getManager(SilverstripeBaseModel::EntityManager); + if (!self::$em->isOpen()) { + self::$em = Registry::resetManager(SilverstripeBaseModel::EntityManager); + } + $this->assertEquals( + $before, + self::$em->getRepository(Presentation::class)->count(['summit' => self::$summit->getId()]), + 'refused with 412 but a presentation was created anyway' + ); + } +} diff --git a/tests/PresentationReopenModelTest.php b/tests/PresentationReopenModelTest.php new file mode 100644 index 000000000..f6fb06fce --- /dev/null +++ b/tests/PresentationReopenModelTest.php @@ -0,0 +1,169 @@ +setTitle("REOPEN MODEL TEST"); + self::$presentation->setType(self::$defaultPresentationType); + self::$presentation->setSelectionPlan(self::$default_selection_plan); + self::$summit->addEvent(self::$presentation); + + self::$em->persist(self::$summit); + self::$em->flush(); + } + + protected function tearDown(): void + { + self::clearSummitTestData(); + self::clearMemberTestData(); + parent::tearDown(); + } + + private function endSubmissionWindow(): void + { + $plan = self::$default_selection_plan; + $plan->setIsEnabled(true); + $plan->setSubmissionBeginDate((new \DateTime('now', new \DateTimeZone('UTC')))->sub(new \DateInterval('P10D'))); + $plan->setSubmissionEndDate((new \DateTime('now', new \DateTimeZone('UTC')))->sub(new \DateInterval('P1D'))); + } + + public function testNoGrantMeansNotReopenedAndDeadlineIsNull() + { + $this->endSubmissionWindow(); + + $this->assertNull(self::$presentation->getSubmissionReopenedUntil()); + $this->assertFalse(self::$presentation->isSubmissionReopened()); + $this->assertTrue(self::$presentation->isSubmissionClosed()); + } + + public function testActiveGrantOnEndedPlanFoldsIntoIsSubmissionClosed() + { + $this->endSubmissionWindow(); + self::$presentation->reopenSubmission(24, self::$member); + + $this->assertNotNull(self::$presentation->getSubmissionReopenedUntil()); + $this->assertTrue(self::$presentation->isSubmissionReopened()); + $this->assertFalse(self::$presentation->isSubmissionClosed()); + $this->assertEquals(24, self::$presentation->getSubmissionReopenedHours()); + $this->assertEquals(self::$member->getId(), self::$presentation->getSubmissionReopenedById()); + $this->assertStringContainsString(self::$member->getEmail(), self::$presentation->getSubmissionReopenedByNice()); + } + + public function testExpiredGrantIsNotReopened() + { + $this->endSubmissionWindow(); + self::$presentation->reopenSubmission(1, self::$member); + self::$presentation->setSubmissionReopenedDate( + (new \DateTime('now', new \DateTimeZone('UTC')))->sub(new \DateInterval('PT2H')) + ); + + $this->assertFalse(self::$presentation->isSubmissionReopened()); + $this->assertTrue(self::$presentation->isSubmissionClosed()); + } + + public function testGrantIsIgnoredWhilePlanWindowHasNotEndedYet() + { + $plan = self::$default_selection_plan; + $plan->setIsEnabled(true); + $plan->setSubmissionBeginDate((new \DateTime('now', new \DateTimeZone('UTC')))->sub(new \DateInterval('P1D'))); + $plan->setSubmissionEndDate((new \DateTime('now', new \DateTimeZone('UTC')))->add(new \DateInterval('P1D'))); + + self::$presentation->reopenSubmission(24, self::$member); + + $this->assertFalse(self::$presentation->isSubmissionReopened()); + } + + public function testGrantIsIgnoredOnDisabledPlan() + { + $this->endSubmissionWindow(); + self::$presentation->reopenSubmission(24, self::$member); + $this->assertTrue(self::$presentation->isSubmissionReopened()); + + self::$default_selection_plan->setIsEnabled(false); + + $this->assertFalse(self::$presentation->isSubmissionReopened()); + // and the delete guard closes again, which is the whole point of the invariant + $this->assertTrue(self::$presentation->isSubmissionClosed()); + } + + public function testCloseSubmissionNowClearsTheGrantRegardlessOfPlanState() + { + $this->endSubmissionWindow(); + self::$presentation->reopenSubmission(24, self::$member); + self::$default_selection_plan->setIsEnabled(false); + + self::$presentation->closeSubmissionNow(); + + $this->assertNull(self::$presentation->getSubmissionReopenedHours()); + $this->assertNull(self::$presentation->getSubmissionReopenedDate()); + $this->assertNull(self::$presentation->getSubmissionReopenedUntil()); + $this->assertFalse(self::$presentation->isSubmissionReopened()); + } + + public function testReopenReStampsRatherThanAccumulating() + { + $this->endSubmissionWindow(); + + self::$presentation->reopenSubmission(24, self::$member); + $first = self::$presentation->getSubmissionReopenedUntil(); + + self::$presentation->reopenSubmission(48, self::$member); + + $this->assertEquals(48, self::$presentation->getSubmissionReopenedHours()); + $this->assertGreaterThan($first, self::$presentation->getSubmissionReopenedUntil()); + } + + public function testReviewStatusIsUnaffectedByAReopen() + { + $this->endSubmissionWindow(); + + // A PHASE_NEW presentation returns NotSubmitted either way (Presentation.php:2490-2493), + // so asserting on the default fixture would compare a constant to itself and prove + // nothing. Drive it to a status that actually flows through the date logic first. + self::$presentation->setProgress(Presentation::PHASE_COMPLETE); + self::$presentation->setStatus(Presentation::STATUS_RECEIVED); + + $before = self::$presentation->getReviewStatus(); + $this->assertNotEquals(Presentation::ReviewStatusNoSubmitted, $before, 'fixture is not exercising the date logic'); + + self::$presentation->reopenSubmission(24, self::$member); + + $this->assertEquals($before, self::$presentation->getReviewStatus()); + } +} diff --git a/tests/Unit/Models/PresentationSubmissionReopenTest.php b/tests/Unit/Models/PresentationSubmissionReopenTest.php new file mode 100644 index 000000000..19dda7666 --- /dev/null +++ b/tests/Unit/Models/PresentationSubmissionReopenTest.php @@ -0,0 +1,423 @@ +shouldReceive('IsEnabled')->andReturn($enabled); + $plan->shouldReceive('getSubmissionEndDate')->andReturn($submission_end_date); + // setSelectionPlan() reads getId() only when replacing an existing plan + $plan->shouldReceive('getId')->andReturn(1); + return $plan; + } + + private function member(int $id = 42, ?string $first = 'Jane', ?string $last = 'Doe', ?string $email = 'jane@example.com'): Member + { + $member = Mockery::mock(Member::class); + $member->shouldReceive('getId')->andReturn($id); + $member->shouldReceive('getFullName')->andReturn(trim($first . ' ' . $last)); + $member->shouldReceive('getEmail')->andReturn($email); + return $member; + } + + private function presentation(?SelectionPlan $plan = null): Presentation + { + $presentation = new Presentation(); + if (!is_null($plan)) $presentation->setSelectionPlan($plan); + return $presentation; + } + + /** + * A plan whose submission window closed an hour ago -- the only shape on which a grant + * can ever be honored. + */ + private function endedPlan(bool $enabled = true): SelectionPlan + { + return $this->plan($this->utc('-1 hour'), $enabled); + } + + /** + * Stamps a grant of $hours and then backdates the stamp to $stamp, which is the only way + * to reach an arbitrary (hours, date) pair through the public API -- reopenSubmission() + * always stamps "now" and there is no public setter for hours. + */ + private function grant(Presentation $presentation, int $hours, ?\DateTime $stamp = null): void + { + $presentation->reopenSubmission($hours, $this->member()); + if (!is_null($stamp)) $presentation->setSubmissionReopenedDate($stamp); + } + + // ------------------------------------------------------------------------- + // The state matrix + // ------------------------------------------------------------------------- + + public function testNoGrantAtAllYieldsNoDeadlineAndIsNotReopened(): void + { + $presentation = $this->presentation($this->endedPlan()); + + $this->assertNull($presentation->getSubmissionReopenedUntil()); + $this->assertNull($presentation->getSubmissionReopenedDate()); + $this->assertNull($presentation->getSubmissionReopenedHours()); + $this->assertFalse($presentation->isSubmissionReopened()); + } + + public function testLiveGrantOnEndedWindowIsReopened(): void + { + $presentation = $this->presentation($this->endedPlan()); + $this->grant($presentation, 24); + + $this->assertTrue($presentation->isSubmissionReopened()); + $this->assertGreaterThan($this->utc('now'), $presentation->getSubmissionReopenedUntil()); + } + + public function testExpiredGrantIsNotReopened(): void + { + $presentation = $this->presentation($this->endedPlan()); + // stamped 5h ago for 2h => expired 3h ago + $this->grant($presentation, 2, $this->utc('-5 hours')); + + $this->assertFalse($presentation->isSubmissionReopened()); + $this->assertLessThan($this->utc('now'), $presentation->getSubmissionReopenedUntil()); + } + + public function testGrantWhileWindowStillOpenIsNotReopened(): void + { + // window closes in an hour: nothing has ended, so there is nothing to reopen + $presentation = $this->presentation($this->plan($this->utc('+1 hour'))); + $this->grant($presentation, 24); + + $this->assertFalse($presentation->isSubmissionReopened()); + // the deadline itself is still computed -- it is the predicate that refuses + $this->assertNotNull($presentation->getSubmissionReopenedUntil()); + } + + public function testGrantBeforeWindowEverOpensIsNotReopened(): void + { + // Pre-open and still-open fold into the same guard: isSubmissionReopened() consults + // only getSubmissionEndDate(), never the begin date. Kept as its own case because it + // is the scenario the guard exists to prevent (edits granted before the CFP opened). + $presentation = $this->presentation($this->plan($this->utc('+30 days'))); + $this->grant($presentation, 24); + + $this->assertFalse($presentation->isSubmissionReopened()); + } + + public function testGrantOnDisabledPlanIsNotReopened(): void + { + $presentation = $this->presentation($this->endedPlan(false)); + $this->grant($presentation, 24); + + $this->assertFalse($presentation->isSubmissionReopened()); + } + + public function testGrantWithNoSelectionPlanAssignedIsNotReopened(): void + { + $presentation = $this->presentation(); + $this->grant($presentation, 24); + + $this->assertNull($presentation->getSelectionPlan()); + $this->assertFalse($presentation->isSubmissionReopened()); + } + + public function testGrantOnPlanWithNullSubmissionEndDateIsNotReopened(): void + { + $presentation = $this->presentation($this->plan(null)); + $this->grant($presentation, 24); + + $this->assertFalse($presentation->isSubmissionReopened()); + } + + // ------------------------------------------------------------------------- + // Half-states: hours and date must both be present or the grant does not exist + // ------------------------------------------------------------------------- + + public function testHoursSetWithNullDateYieldsNullDeadlineAndFalsePredicate(): void + { + $presentation = $this->presentation($this->endedPlan()); + $this->grant($presentation, 24, null); + $presentation->setSubmissionReopenedDate(null); + + $this->assertSame(24, $presentation->getSubmissionReopenedHours()); + $this->assertNull($presentation->getSubmissionReopenedDate()); + $this->assertNull($presentation->getSubmissionReopenedUntil()); + $this->assertFalse($presentation->isSubmissionReopened()); + } + + public function testDateSetWithNullHoursYieldsNullDeadlineAndFalsePredicate(): void + { + $presentation = $this->presentation($this->endedPlan()); + $this->grant($presentation, 24); + $presentation->closeSubmissionNow(); + $presentation->setSubmissionReopenedDate($this->utc('now')); + + $this->assertNull($presentation->getSubmissionReopenedHours()); + $this->assertNotNull($presentation->getSubmissionReopenedDate()); + $this->assertNull($presentation->getSubmissionReopenedUntil()); + $this->assertFalse($presentation->isSubmissionReopened()); + } + + // ------------------------------------------------------------------------- + // Deadline arithmetic and boundaries + // ------------------------------------------------------------------------- + + public function testDeadlineIsExactlyStampPlusGrantedHours(): void + { + $presentation = $this->presentation($this->endedPlan()); + $this->grant($presentation, 24, new \DateTime('2026-01-01 10:00:00', new \DateTimeZone('UTC'))); + + $this->assertEquals( + new \DateTime('2026-01-02 10:00:00', new \DateTimeZone('UTC')), + $presentation->getSubmissionReopenedUntil() + ); + } + + public function testDeadlineArithmeticCrossesDstAndMonthBoundariesInUtc(): void + { + $presentation = $this->presentation($this->endedPlan()); + // 168h == the shipped ceiling; stamped so the window crosses a month end + $this->grant($presentation, 168, new \DateTime('2026-01-28 23:30:00', new \DateTimeZone('UTC'))); + + $this->assertEquals( + new \DateTime('2026-02-04 23:30:00', new \DateTimeZone('UTC')), + $presentation->getSubmissionReopenedUntil() + ); + } + + public function testGetSubmissionReopenedUntilDoesNotMutateTheStoredStamp(): void + { + $stamp = new \DateTime('2026-03-10 08:00:00', new \DateTimeZone('UTC')); + $presentation = $this->presentation($this->endedPlan()); + $this->grant($presentation, 6, $stamp); + + $first = $presentation->getSubmissionReopenedUntil(); + $second = $presentation->getSubmissionReopenedUntil(); + + // idempotent: it clones on purpose, so repeated reads agree + $this->assertEquals($first, $second); + // and the stamp itself is untouched, both as stored and as the caller's own object + $this->assertEquals( + new \DateTime('2026-03-10 08:00:00', new \DateTimeZone('UTC')), + $presentation->getSubmissionReopenedDate() + ); + $this->assertEquals(new \DateTime('2026-03-10 08:00:00', new \DateTimeZone('UTC')), $stamp); + } + + public function testGrantExpiringExactlyNowIsNotReopened(): void + { + $presentation = $this->presentation($this->endedPlan()); + // stamped exactly one hour ago for one hour => until == the instant of setup, which is + // strictly before the clock isSubmissionReopened() reads. Pins the `now < until` bound. + $this->grant($presentation, 1, $this->utc('-1 hour')); + + $this->assertFalse($presentation->isSubmissionReopened()); + } + + public function testGrantOneMinuteBeforeExpiryIsStillReopened(): void + { + $presentation = $this->presentation($this->endedPlan()); + $this->grant($presentation, 1, $this->utc('-59 minutes')); + + $this->assertTrue($presentation->isSubmissionReopened()); + } + + // ------------------------------------------------------------------------- + // Mutators + // ------------------------------------------------------------------------- + + public function testReopenSubmissionRestampsRatherThanAccumulating(): void + { + $presentation = $this->presentation($this->endedPlan()); + + $presentation->reopenSubmission(2, $this->member()); + $presentation->setSubmissionReopenedDate($this->utc('-30 minutes')); + $first_until = $presentation->getSubmissionReopenedUntil(); + + $second_actor = $this->member(99, 'Ada', 'Lovelace', 'ada@example.com'); + $presentation->reopenSubmission(5, $second_actor); + + // hours replaced, not summed + $this->assertSame(5, $presentation->getSubmissionReopenedHours()); + // stamp re-taken, so the deadline moved rather than extending from the old stamp + $this->assertGreaterThan($first_until, $presentation->getSubmissionReopenedUntil()); + $this->assertEquals( + (clone $presentation->getSubmissionReopenedDate())->add(new \DateInterval('PT5H')), + $presentation->getSubmissionReopenedUntil() + ); + $this->assertSame(99, $presentation->getSubmissionReopenedById()); + } + + public function testCloseSubmissionNowClearsAllThreeColumnsOnAnEnabledPlan(): void + { + $presentation = $this->presentation($this->endedPlan()); + $this->grant($presentation, 24); + $this->assertTrue($presentation->isSubmissionReopened()); + + $presentation->closeSubmissionNow(); + + $this->assertNull($presentation->getSubmissionReopenedHours()); + $this->assertNull($presentation->getSubmissionReopenedDate()); + $this->assertNull($presentation->getSubmissionReopenedBy()); + $this->assertNull($presentation->getSubmissionReopenedUntil()); + $this->assertFalse($presentation->isSubmissionReopened()); + } + + public function testCloseSubmissionNowClearsTheGrantOnADisabledPlan(): void + { + $presentation = $this->presentation($this->endedPlan(false)); + $this->grant($presentation, 24); + + $presentation->closeSubmissionNow(); + + $this->assertNull($presentation->getSubmissionReopenedHours()); + $this->assertNull($presentation->getSubmissionReopenedDate()); + $this->assertNull($presentation->getSubmissionReopenedBy()); + } + + public function testCloseSubmissionNowClearsTheGrantWithNoSelectionPlan(): void + { + $presentation = $this->presentation(); + $this->grant($presentation, 24); + + $presentation->closeSubmissionNow(); + + $this->assertNull($presentation->getSubmissionReopenedHours()); + $this->assertNull($presentation->getSubmissionReopenedDate()); + $this->assertNull($presentation->getSubmissionReopenedBy()); + } + + // ------------------------------------------------------------------------- + // Show Admin display getters + // ------------------------------------------------------------------------- + + public function testGetSubmissionReopenedByIdReturnsZeroWithoutAnActor(): void + { + $this->assertSame(0, $this->presentation()->getSubmissionReopenedById()); + } + + public function testGetSubmissionReopenedByIdReturnsTheActorId(): void + { + $presentation = $this->presentation(); + $presentation->reopenSubmission(24, $this->member(7)); + + $this->assertSame(7, $presentation->getSubmissionReopenedById()); + } + + public function testGetSubmissionReopenedByNiceReturnsNullWithoutAnActor(): void + { + $this->assertNull($this->presentation()->getSubmissionReopenedByNice()); + } + + public function testGetSubmissionReopenedByNiceFormatsFullNameAndEmail(): void + { + $presentation = $this->presentation(); + $presentation->reopenSubmission(24, $this->member(7, 'Ada', 'Lovelace', 'ada@example.com')); + + $this->assertSame('Ada Lovelace (ada@example.com)', $presentation->getSubmissionReopenedByNice()); + } + + // ------------------------------------------------------------------------- + // The isSubmissionClosed() fold, both directions + // ------------------------------------------------------------------------- + + public function testIsSubmissionClosedIsFalseWhileReopened(): void + { + $presentation = $this->presentation($this->endedPlan()); + $this->grant($presentation, 24); + + $this->assertTrue($presentation->isSubmissionReopened()); + $this->assertFalse($presentation->isSubmissionClosed()); + } + + public function testIsSubmissionClosedIsTrueWhenMerelyClosed(): void + { + $presentation = $this->presentation($this->endedPlan()); + + $this->assertFalse($presentation->isSubmissionReopened()); + $this->assertTrue($presentation->isSubmissionClosed()); + } + + public function testIsSubmissionClosedIsTrueOnceTheGrantExpires(): void + { + $presentation = $this->presentation($this->endedPlan()); + $this->grant($presentation, 2, $this->utc('-5 hours')); + + $this->assertTrue($presentation->isSubmissionClosed()); + } + + public function testIsSubmissionClosedIsFalseWithNoSelectionPlan(): void + { + $this->assertFalse($this->presentation()->isSubmissionClosed()); + } + + public function testIsSubmissionClosedIsFalseWhileTheWindowIsStillOpen(): void + { + $presentation = $this->presentation($this->plan($this->utc('+1 hour'))); + + $this->assertFalse($presentation->isSubmissionClosed()); + } + + public function testIsSubmissionClosedIsTrueOnADisabledPlanWithAnEndedWindow(): void + { + // a grant does NOT rescue a disabled plan: isSubmissionReopened() refuses, so the fold + // falls through to the plain closed check, which does not consult IsEnabled() + $presentation = $this->presentation($this->endedPlan(false)); + $this->grant($presentation, 24); + + $this->assertFalse($presentation->isSubmissionReopened()); + $this->assertTrue($presentation->isSubmissionClosed()); + } +} diff --git a/tests/Unit/Services/PresentationSubmissionReopenServiceTest.php b/tests/Unit/Services/PresentationSubmissionReopenServiceTest.php new file mode 100644 index 000000000..315615c83 --- /dev/null +++ b/tests/Unit/Services/PresentationSubmissionReopenServiceTest.php @@ -0,0 +1,482 @@ + 'sometimes|integer|min:1' and refuses first -- so this file is the only coverage + * it will ever have. + * + * @package Tests\Unit\Services + */ +class PresentationSubmissionReopenServiceTest extends TestCase +{ + private Container $app; + + private Repository $config; + + /** + * Number of times the mocked transaction closure actually executed. + */ + private int $tx_invocations = 0; + + protected function setUp(): void + { + parent::setUp(); + Facade::clearResolvedInstances(); + $this->app = new Container(); + $this->config = new Repository([ + 'cfp' => [ + 'max_reopen_hours' => 48, + 'default_reopen_hours' => 5, + ], + ]); + $this->app->instance('config', $this->config); + Container::setInstance($this->app); + Facade::setFacadeApplication($this->app); + $this->tx_invocations = 0; + } + + protected function tearDown(): void + { + Facade::setFacadeApplication(null); + Facade::clearResolvedInstances(); + Container::setInstance(null); + Mockery::close(); + parent::tearDown(); + } + + // ------------------------------------------------------------------------- + // Helpers + // ------------------------------------------------------------------------- + + private function utc(string $modifier): \DateTime + { + return new \DateTime($modifier, new \DateTimeZone('UTC')); + } + + /** + * Transaction service that really runs the closure, so the code under test executes. + */ + private function makeService(): PresentationSubmissionReopenService + { + $tx_service = Mockery::mock(ITransactionService::class); + $tx_service->shouldReceive('transaction') + ->once() + ->andReturnUsing(function (\Closure $callback) { + $this->tx_invocations++; + return $callback(); + }); + + return new PresentationSubmissionReopenService($tx_service); + } + + private function plan(?\DateTime $submission_end_date, bool $enabled = true): SelectionPlan + { + $plan = Mockery::mock(SelectionPlan::class); + $plan->shouldReceive('IsEnabled')->andReturn($enabled); + $plan->shouldReceive('getSubmissionEndDate')->andReturn($submission_end_date); + $plan->shouldReceive('getId')->andReturn(1); + return $plan; + } + + private function member(int $id = 42): Member + { + $member = Mockery::mock(Member::class); + $member->shouldReceive('getId')->andReturn($id); + $member->shouldReceive('getFullName')->andReturn('Jane Doe'); + $member->shouldReceive('getEmail')->andReturn('jane@example.com'); + return $member; + } + + private function presentation(?SelectionPlan $plan = null): Presentation + { + $presentation = new Presentation(); + if (!is_null($plan)) $presentation->setSelectionPlan($plan); + return $presentation; + } + + /** + * @param Presentation|SummitEvent|null $event what getEvent() hands back + */ + private function summit($event): Summit + { + $summit = Mockery::mock(Summit::class); + $summit->shouldReceive('getEvent')->with(1234)->andReturn($event); + return $summit; + } + + private function assertClosureRan(): void + { + $this->assertSame(1, $this->tx_invocations, 'the transaction closure never executed'); + } + + // ------------------------------------------------------------------------- + // reopen(): entity resolution + // ------------------------------------------------------------------------- + + public function testReopenThrowsEntityNotFoundWhenTheEventDoesNotExistInTheSummit(): void + { + $service = $this->makeService(); + + $this->expectException(EntityNotFoundException::class); + $this->expectExceptionMessage('Presentation 1234 not found.'); + + try { + $service->reopen($this->summit(null), 1234, 24, $this->member()); + } finally { + $this->assertClosureRan(); + } + } + + public function testReopenThrowsEntityNotFoundWhenTheEventIsNotAPresentation(): void + { + $service = $this->makeService(); + $event = Mockery::mock(SummitEvent::class); + + $this->expectException(EntityNotFoundException::class); + $this->expectExceptionMessage('Presentation 1234 not found.'); + + try { + $service->reopen($this->summit($event), 1234, 24, $this->member()); + } finally { + $this->assertClosureRan(); + } + } + + // ------------------------------------------------------------------------- + // reopen(): the hours guard + // ------------------------------------------------------------------------- + + public function testReopenWithNullHoursResolvesTheConfiguredDefaultAndStampsIt(): void + { + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('-1 hour'))); + $actor = $this->member(); + + $result = $service->reopen($this->summit($presentation), 1234, null, $actor); + + // 5, not the hardcoded 24 fallback -- proof the default comes from config + $this->assertSame(5, $result->getSubmissionReopenedHours()); + $this->assertClosureRan(); + } + + public function testReopenAboveTheConfiguredMaxThrowsValidation(): void + { + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('-1 hour'))); + + $this->expectException(ValidationException::class); + // 48 is the configured ceiling, not the hardcoded 168 fallback + $this->expectExceptionMessage('hours must be between 1 and 48.'); + + try { + $service->reopen($this->summit($presentation), 1234, 49, $this->member()); + } finally { + $this->assertClosureRan(); + // nothing stamped + $this->assertNull($presentation->getSubmissionReopenedHours()); + $this->assertNull($presentation->getSubmissionReopenedDate()); + } + } + + public function testReopenAtExactlyTheConfiguredMaxIsAccepted(): void + { + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('-1 hour'))); + + $result = $service->reopen($this->summit($presentation), 1234, 48, $this->member()); + + $this->assertSame(48, $result->getSubmissionReopenedHours()); + $this->assertClosureRan(); + } + + /** + * The lower bound. Unreachable over HTTP (the endpoint validates min:1 first), so these are + * the only assertions that will ever cover it. + */ + #[DataProvider('belowMinimumHoursProvider')] + public function testReopenBelowOneHourThrowsValidation(int $hours): void + { + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('-1 hour'))); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('hours must be between 1 and 48.'); + + try { + $service->reopen($this->summit($presentation), 1234, $hours, $this->member()); + } finally { + $this->assertClosureRan(); + $this->assertNull($presentation->getSubmissionReopenedHours()); + } + } + + public static function belowMinimumHoursProvider(): array + { + return [ + 'zero' => [0], + 'negative' => [-1], + // a negative interval would make getSubmissionReopenedUntil() throw on every read + 'large negative' => [-168], + ]; + } + + public function testReopenAtExactlyOneHourIsAccepted(): void + { + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('-1 hour'))); + + $result = $service->reopen($this->summit($presentation), 1234, 1, $this->member()); + + $this->assertSame(1, $result->getSubmissionReopenedHours()); + $this->assertClosureRan(); + } + + // ------------------------------------------------------------------------- + // reopen(): plan-state guards + // ------------------------------------------------------------------------- + + public function testReopenWithNoSelectionPlanAssignedThrowsValidation(): void + { + $service = $this->makeService(); + $presentation = $this->presentation(); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Presentation is not assigned to any selection plan.'); + + try { + $service->reopen($this->summit($presentation), 1234, 24, $this->member()); + } finally { + $this->assertClosureRan(); + $this->assertNull($presentation->getSubmissionReopenedHours()); + } + } + + public function testReopenOnADisabledPlanThrowsValidation(): void + { + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('-1 hour'), false)); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Selection plan is not enabled.'); + + try { + $service->reopen($this->summit($presentation), 1234, 24, $this->member()); + } finally { + $this->assertClosureRan(); + $this->assertNull($presentation->getSubmissionReopenedHours()); + } + } + + public function testReopenOnAPlanWithNoSubmissionEndDateThrowsValidation(): void + { + $service = $this->makeService(); + $presentation = $this->presentation($this->plan(null)); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Selection plan has no submission end date.'); + + try { + $service->reopen($this->summit($presentation), 1234, 24, $this->member()); + } finally { + $this->assertClosureRan(); + $this->assertNull($presentation->getSubmissionReopenedHours()); + } + } + + public function testReopenWhenTheSubmissionWindowHasNotEndedThrowsValidation(): void + { + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('+1 hour'))); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Submission period has not ended yet; nothing to reopen.'); + + try { + $service->reopen($this->summit($presentation), 1234, 24, $this->member()); + } finally { + $this->assertClosureRan(); + $this->assertNull($presentation->getSubmissionReopenedHours()); + } + } + + // ------------------------------------------------------------------------- + // reopen(): happy path + // ------------------------------------------------------------------------- + + public function testReopenStampsHoursDateAndActorAndReturnsThePresentation(): void + { + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('-1 hour'))); + $actor = $this->member(77); + $before = $this->utc('now'); + + $result = $service->reopen($this->summit($presentation), 1234, 12, $actor); + + $this->assertSame($presentation, $result); + $this->assertSame(12, $result->getSubmissionReopenedHours()); + $this->assertSame($actor, $result->getSubmissionReopenedBy()); + $this->assertSame(77, $result->getSubmissionReopenedById()); + $this->assertGreaterThanOrEqual($before, $result->getSubmissionReopenedDate()); + $this->assertEquals( + (clone $result->getSubmissionReopenedDate())->add(new \DateInterval('PT12H')), + $result->getSubmissionReopenedUntil() + ); + $this->assertTrue($result->isSubmissionReopened()); + $this->assertClosureRan(); + } + + // ------------------------------------------------------------------------- + // closeNow(): clears the grant, and deliberately applies no plan-state validation + // ------------------------------------------------------------------------- + + public function testCloseNowClearsTheGrant(): void + { + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('-1 hour'))); + $presentation->reopenSubmission(24, $this->member()); + + $service->closeNow($this->summit($presentation), 1234, $this->member()); + + $this->assertNull($presentation->getSubmissionReopenedHours()); + $this->assertNull($presentation->getSubmissionReopenedDate()); + $this->assertNull($presentation->getSubmissionReopenedBy()); + $this->assertClosureRan(); + } + + public function testCloseNowSucceedsOnADisabledPlan(): void + { + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('-1 hour'), false)); + $presentation->reopenSubmission(24, $this->member()); + + $service->closeNow($this->summit($presentation), 1234, $this->member()); + + $this->assertNull($presentation->getSubmissionReopenedHours()); + $this->assertClosureRan(); + } + + public function testCloseNowSucceedsWithNoSelectionPlanAssigned(): void + { + $service = $this->makeService(); + $presentation = $this->presentation(); + $presentation->reopenSubmission(24, $this->member()); + + $service->closeNow($this->summit($presentation), 1234, $this->member()); + + $this->assertNull($presentation->getSubmissionReopenedHours()); + $this->assertClosureRan(); + } + + public function testCloseNowSucceedsWhileTheSubmissionWindowIsStillOpen(): void + { + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('+1 hour'))); + $presentation->reopenSubmission(24, $this->member()); + + $service->closeNow($this->summit($presentation), 1234, $this->member()); + + $this->assertNull($presentation->getSubmissionReopenedHours()); + $this->assertClosureRan(); + } + + public function testCloseNowIsIdempotentWhenThereIsNoGrant(): void + { + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('-1 hour'))); + + $service->closeNow($this->summit($presentation), 1234, $this->member()); + + $this->assertNull($presentation->getSubmissionReopenedHours()); + $this->assertClosureRan(); + } + + public function testCloseNowThrowsEntityNotFoundWhenTheEventDoesNotExistInTheSummit(): void + { + $service = $this->makeService(); + + $this->expectException(EntityNotFoundException::class); + $this->expectExceptionMessage('Presentation 1234 not found.'); + + try { + $service->closeNow($this->summit(null), 1234, $this->member()); + } finally { + $this->assertClosureRan(); + } + } + + public function testCloseNowThrowsEntityNotFoundWhenTheEventIsNotAPresentation(): void + { + $service = $this->makeService(); + $event = Mockery::mock(SummitEvent::class); + + $this->expectException(EntityNotFoundException::class); + $this->expectExceptionMessage('Presentation 1234 not found.'); + + try { + $service->closeNow($this->summit($event), 1234, $this->member()); + } finally { + $this->assertClosureRan(); + } + } + + // ------------------------------------------------------------------------- + // Guard on the harness itself + // ------------------------------------------------------------------------- + + public function testTheMockedTransactionServiceReallyExecutesTheClosure(): void + { + // Without this, a transaction mock that returned a canned value would let every test + // above pass without running a single line of the service. + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('-1 hour'))); + + $this->assertSame(0, $this->tx_invocations); + $service->reopen($this->summit($presentation), 1234, 3, $this->member()); + $this->assertSame(1, $this->tx_invocations); + $this->assertSame(3, $presentation->getSubmissionReopenedHours()); + } +} From d673aa7db0c2f810b368aaca65a6c868f6ab9655 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Sat, 8 Aug 2026 15:58:53 -0500 Subject: [PATCH 2/3] fix: forward serialization params, clamp a misconfigured reopen default 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 --- .../OAuth2PresentationApiController.php | 6 +++++- .../PresentationSubmissionReopenService.php | 10 ++++++++-- tests/PresentationReopenApiTest.php | 7 ++++++- ...resentationSubmissionReopenServiceTest.php | 20 +++++++++++++++++++ 4 files changed, 39 insertions(+), 4 deletions(-) diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php index 1d544e065..f3e382778 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php @@ -593,7 +593,11 @@ public function reopenSubmissionPeriod($summit_id, $presentation_id) // unknown type silently falls back to Public, stripping the reopen fields. return $this->updated(SerializerRegistry::getInstance()->getSerializer( $presentation, SerializerRegistry::SerializerType_Private - )->serialize()); + )->serialize( + SerializerUtils::getExpand(), + SerializerUtils::getFields(), + SerializerUtils::getRelations() + )); }); } diff --git a/app/Services/Model/Imp/PresentationSubmissionReopenService.php b/app/Services/Model/Imp/PresentationSubmissionReopenService.php index 82ab7b7ca..82d546a90 100644 --- a/app/Services/Model/Imp/PresentationSubmissionReopenService.php +++ b/app/Services/Model/Imp/PresentationSubmissionReopenService.php @@ -38,9 +38,16 @@ public function reopen(Summit $summit, int $presentation_id, ?int $hours, Member if (!$presentation instanceof Presentation) throw new EntityNotFoundException(sprintf("Presentation %s not found.", $presentation_id)); + $max = intval(Config::get('cfp.max_reopen_hours', 168)); + // whole hours rule in one place: null means "unspecified", so the default is resolved // here rather than in the controller, right next to the ceiling it has to respect. - $hours = $hours ?? intval(Config::get('cfp.default_reopen_hours', 24)); + // The resolved default is clamped to the ceiling on purpose: a deployment that + // configures default_reopen_hours above max_reopen_hours would otherwise reject every + // request that omits hours, which is a config error the caller cannot see or fix. + // An explicitly supplied $hours is still validated strictly below, never clamped. + if (is_null($hours)) + $hours = min(intval(Config::get('cfp.default_reopen_hours', 24)), $max); // The lower bound is currently unreachable over HTTP -- the endpoint validates // 'hours' => 'sometimes|integer|min:1' and refuses first -- but it is deliberate, not @@ -48,7 +55,6 @@ public function reopen(Summit $summit, int $presentation_id, ?int $hours, Member // service), and a persisted non-positive value would make // Presentation::getSubmissionReopenedUntil() throw on every read, since // new \DateInterval('PT-1H') is invalid. Do not remove it as unused. - $max = intval(Config::get('cfp.max_reopen_hours', 168)); if ($hours < 1 || $hours > $max) throw new ValidationException(sprintf("hours must be between 1 and %s.", $max)); diff --git a/tests/PresentationReopenApiTest.php b/tests/PresentationReopenApiTest.php index 0564a25c5..d04bf8887 100644 --- a/tests/PresentationReopenApiTest.php +++ b/tests/PresentationReopenApiTest.php @@ -462,9 +462,14 @@ public function testByFieldsAreAbsentFromTheSubmissionSerializer() public function testReopenFieldsNeverAppearOnAPublicSerializedResponse() { $this->reopen(['hours' => 24]); + // without this the three absence assertions below hold vacuously: a failed reopen leaves + // no grant, so Public would omit the fields whether or not the mappings are Admin-only + $this->assertResponseStatus(201); + $reloaded = $this->reloadPresentation(); + $this->assertTrue($reloaded->isSubmissionReopened(), 'grant did not persist'); $payload = SerializerRegistry::getInstance()->getSerializer( - $this->reloadPresentation(), SerializerRegistry::SerializerType_Public + $reloaded, SerializerRegistry::SerializerType_Public )->serialize(); $this->assertArrayNotHasKey('submission_reopened_until', $payload); diff --git a/tests/Unit/Services/PresentationSubmissionReopenServiceTest.php b/tests/Unit/Services/PresentationSubmissionReopenServiceTest.php index 315615c83..692f925db 100644 --- a/tests/Unit/Services/PresentationSubmissionReopenServiceTest.php +++ b/tests/Unit/Services/PresentationSubmissionReopenServiceTest.php @@ -203,6 +203,26 @@ public function testReopenWithNullHoursResolvesTheConfiguredDefaultAndStampsIt() $this->assertClosureRan(); } + /** + * A deployment that sets default_reopen_hours above max_reopen_hours would otherwise reject + * every request that omits hours, which is a config error the caller can neither see nor fix. + * The resolved default is clamped to the ceiling; an explicit out-of-range hours is still + * refused (see testReopenAboveTheConfiguredMaxThrowsValidation). + */ + public function testMisconfiguredDefaultAboveMaxIsClampedRatherThanRefused(): void + { + $this->config->set('cfp.default_reopen_hours', 500); + $this->config->set('cfp.max_reopen_hours', 48); + + $service = $this->makeService(); + $presentation = $this->presentation($this->plan($this->utc('-1 hour'))); + + $result = $service->reopen($this->summit($presentation), 1234, null, $this->member()); + + $this->assertSame(48, $result->getSubmissionReopenedHours()); + $this->assertClosureRan(); + } + public function testReopenAboveTheConfiguredMaxThrowsValidation(): void { $service = $this->makeService(); From c91f721a9a4502ee3a41d20fdf1a66f9f46aec1b Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Mon, 10 Aug 2026 11:22:27 -0500 Subject: [PATCH 3/3] test: add open-window parity coverage for per-activity CFP reopen 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 --- tests/PresentationReopenApiTest.php | 258 ++++++++++++++++++++++++++++ 1 file changed, 258 insertions(+) diff --git a/tests/PresentationReopenApiTest.php b/tests/PresentationReopenApiTest.php index d04bf8887..01504d1db 100644 --- a/tests/PresentationReopenApiTest.php +++ b/tests/PresentationReopenApiTest.php @@ -16,6 +16,7 @@ use LaravelDoctrine\ORM\Facades\Registry; use ModelSerializers\SerializerRegistry; use models\summit\Presentation; +use models\summit\SummitEvent; use models\utils\SilverstripeBaseModel; /** @@ -703,4 +704,261 @@ public function testWithoutAGrantCompleteIsRefused() $this->assertRefusedBySubmissionWindow($this->completeSubmission()); $this->assertFalse($this->reloadPresentation()->isSubmitted()); } + + // --------------------------------------------------------------------------------------------- + // Open-window parity (SDS §8, "the acceptance bar"; D7). The SDS states the bar as: while + // reopened, the editable surface is exactly what the selection plan defines during the open + // window. These tests SAMPLE that bar the way §8 asks -- one prohibited field, one permitted + // field, plus the media-upload set -- they do not enumerate the whole surface. + // + // Parity holds by construction: the diff ORs the time gate in two places + // (PresentationService.php:547 for update, :671 for complete) and changes nothing else on those + // paths, so curatePayloadByPresentationAllowedQuestions / checkPresentationAllowedEdtiable + // Questions (:555-556) still run unconditionally afterwards on the one shared code path. These + // tests are a tripwire, so a later change that widens the surface for the reopen case + // specifically cannot pass unnoticed. + // + // Fixture direction, which is the opposite of what it looks like: SelectionPlan::__construct + // calls seedAllowedPresentationQuestions() and seedAllowedEditablePresentationQuestions() + // (SelectionPlan.php:457-458), so a fresh plan permits EVERY allowed field, both as a question + // and as editable. The restrictive case therefore has to be built by removing one, not by adding + // one -- hence makeNonEditable() below. + // + // Run these with the whole file, not as a hand-picked --filter subset. Each of the five passes + // on its own, the full file is green (25 tests) and so is the whole push.yml shard (59), which + // is what CI runs -- but certain multi-test --filter subsets make the tests that expect a + // successful update return 500 instead. Observed failure: PresentationSerializer:: + // getMediaUploadsSerializerType() (:142) calls isAdmin() on the resource-server context's Member + // and Doctrine raises EntityNotFoundException ("Unable to find Proxies\__CG__\models\main\Member + // entity identifier associated with the UnitOfWork"). Reproducible for the scalar pair below + // when the five are filtered together; not reproducible for any of them alone. The mechanism was + // not chased past that, so treat a red hand-picked subset here as unproven rather than as a + // regression, and reproduce against the full file before believing it. + // + // 'links' rather than a scalar for the enforcement pair, on purpose. areFieldsEqual() has two + // branches (SelectionPlan.php:1564-1571): the array branch is correct, and the scalar branch + // compares html_entity_decode($field1) to itself, so it always reports equal and the guard is + // dead for every scalar field. 'links' is array-typed in AllowedEditableFields and in + // getSnapshot() (FieldLinks => []), so it takes the working branch and these tests need no fix + // to that pre-existing defect. The scalar half of the SDS bullet cannot assert enforcement at + // all and is covered as parity only, by the last pair below. + // --------------------------------------------------------------------------------------------- + + /** + * Drop one field from the plan's allowed-editable set, leaving the rest of the seeded set intact. + * + * SelectionPlan exposes no single-question remover -- only clearAllAllowedEditablePresentation + * Questions() (:490) -- so this drops the element straight off the collection returned by + * getAllowedEditablePresentationQuestions() (:485) rather than clearing and re-adding the other + * four. Safe against the DB either way: the association is mapped cascade persist+remove with + * orphanRemoval: true (:229), so the removed row is deleted rather than left behind to reappear + * on the next read -- which the isAllowedEditablePresentationQuestion() precondition assertion in + * each caller checks against a reloaded plan. + * + * Deliberately leaves the allowed-QUESTION set alone. curatePayloadByPresentationAllowedQuestions() + * returns the curated payload by value and both call sites discard the return + * (PresentationService.php:360 and :555), so curation is a no-op today. If that discard is ever + * fixed, a field missing from the question set would be stripped from the payload, + * isset($payload[$field]) would go false, and the editable check would never run -- so + * testNonEditableFieldIsRefusedUnderReopen would get a 201 against its asserted 412 and fail, + * even though stripping is the correct behavior at that point. Keeping the field an allowed + * question keeps these tests pinned on editability, which is what they are about, instead of + * making them a spurious casualty of that fix. + */ + private function makeNonEditable(string $field): void + { + $questions = self::$default_selection_plan->getAllowedEditablePresentationQuestions(); + foreach ($questions as $question) { + if ($question->getType() === $field) { + $questions->removeElement($question); + } + } + self::$em->flush(); + } + + private function updateSubmissionWithLinks(array $links) + { + $params = [ + 'id' => self::$summit->getId(), + 'presentation_id' => self::$presentation->getId(), + ]; + $headers = $this->getAuthHeaders(); // includes CONTENT_TYPE: application/json + + return $this->action( + "PUT", "OAuth2PresentationApiController@updatePresentationSubmission", + $params, [], [], [], $headers, + json_encode(array_merge($this->validUpdatePayload(), ['links' => $links])) + ); + } + + /** + * The presentation starts with no links, so the snapshot's FieldLinks is [] -- still isset(), + * which is what checkPresentationAllowedEdtiableQuestions requires (:1594) -- and a one-element + * payload differs by count, so the array branch reports unequal and the guard fires. + */ + public function testNonEditableFieldIsRefusedUnderReopen() + { + $this->makeNonEditable(Presentation::FieldLinks); + $this->grantWindow(24); + + $reloaded = $this->reloadPresentation(); + $this->assertTrue($reloaded->isSubmissionReopened(), 'grant did not persist'); + $this->assertTrue( + $reloaded->getSelectionPlan()->isAllowedPresentationQuestion(Presentation::FieldLinks), + 'links must remain an allowed QUESTION, or this test survives a curation fix vacuously' + ); + $this->assertFalse( + $reloaded->getSelectionPlan()->isAllowedEditablePresentationQuestion(Presentation::FieldLinks), + 'fixture did not land: links is still editable, so a 412 below would prove nothing' + ); + + $response = $this->updateSubmissionWithLinks(['https://example.org/deck']); + $this->assertResponseStatus(412); + // the PLAN's message, not assertRefusedBySubmissionWindow()'s -- a window refusal here would + // mean the gate never opened, which is the opposite of what this test asserts + $this->assertErrorsContain( + $response, + sprintf( + 'Field %s is not allowed for edition on Selection Plan %s.', + Presentation::FieldLinks, + self::$default_selection_plan->getName() + ) + ); + + $this->assertCount( + 0, + $this->reloadPresentation()->getLinks(), + 'refused with 412 but the link was written anyway' + ); + } + + /** + * The other direction. No fixture change: the constructor already seeds links as editable, and + * the assertion below states that rather than assuming it. + */ + public function testEditableFieldStaysEditableUnderReopen() + { + $this->grantWindow(24); + + $reloaded = $this->reloadPresentation(); + $this->assertTrue($reloaded->isSubmissionReopened(), 'grant did not persist'); + $this->assertTrue( + $reloaded->getSelectionPlan()->isAllowedEditablePresentationQuestion(Presentation::FieldLinks), + 'links must BE an allowed editable question here' + ); + + $this->updateSubmissionWithLinks(['https://example.org/deck']); + $this->assertResponseStatus(201); + + // the edit landed -- a 201 alone would also come back if the factory ignored the field + $links = []; + foreach ($this->reloadPresentation()->getLinks() as $link) { + $links[] = $link->getLink(); + } + $this->assertEquals(['https://example.org/deck'], $links); + } + + // --------------------------------------------------------------------------------------------- + // The scalar half of the SDS bullet, as parity rather than as enforcement. + // + // A scalar change to a NON-editable field is accepted, because areFieldsEqual()'s scalar branch + // self-compares (SelectionPlan.php:1570). That defect is outside this diff and deliberately not + // fixed here, so the only honest assertion available is that reopen behaves exactly as the open + // window does -- which is what parity means. Neither test below claims the guard works. + // + // Two tests rather than one because this suite gets one SUCCESSFUL HTTP write per entity. Not a + // general BrowserKit law -- it is this app's wiring: DoctrineMiddleware closes the model entity + // manager after each request (app/Http/Middleware/DoctrineMiddleware.php:38-42) while the + // presentation service and its repositories are container singletons holding the manager they + // first resolved, so the second write mutates an untracked object and flush() persists nothing + // while still returning success (see grantWindow()). Doing both windows in one test would make + // the second arm assert nothing. + // + // Named "HandledIdentically", not "IsAccepted": the 201 each arm asserts is the comparator defect + // showing through, not a property worth preserving. When the comparator is fixed both arms move + // to 412 TOGETHER and both fail together -- that is expected, and the fix should update both. If + // only one moves, parity actually broke and that is the signal these two exist to give. + // --------------------------------------------------------------------------------------------- + + public function testNonEditableScalarHandledIdenticallyInTheOpenWindow() + { + $this->makeNonEditable(SummitEvent::FieldTitle); + self::$default_selection_plan->setSubmissionEndDate( + (new \DateTime('now', new \DateTimeZone('UTC')))->add(new \DateInterval('P1D')) + ); + self::$em->flush(); + + $reloaded = $this->reloadPresentation(); + $this->assertTrue($reloaded->getSelectionPlan()->isSubmissionOpen(), 'window did not reopen'); + $this->assertFalse($reloaded->isSubmissionReopened(), 'this arm must NOT ride on a grant'); + $this->assertFalse( + $reloaded->getSelectionPlan()->isAllowedEditablePresentationQuestion(SummitEvent::FieldTitle), + 'title must be non-editable for this to say anything' + ); + + $this->updateSubmission(); // validUpdatePayload() changes title + $this->assertResponseStatus(201); + $this->assertEquals('EDITED DURING REOPEN', $this->reloadPresentation()->getTitle()); + } + + public function testNonEditableScalarHandledIdenticallyUnderReopen() + { + $this->makeNonEditable(SummitEvent::FieldTitle); + $this->grantWindow(24); + + $reloaded = $this->reloadPresentation(); + $this->assertFalse( + $reloaded->getSelectionPlan()->isSubmissionOpen(), + 'this arm must run against a CLOSED window, or it is a copy of the one above' + ); + $this->assertTrue($reloaded->isSubmissionReopened(), 'grant did not persist'); + $this->assertFalse( + $reloaded->getSelectionPlan()->isAllowedEditablePresentationQuestion(SummitEvent::FieldTitle), + 'title must be non-editable for this to say anything' + ); + + $this->updateSubmission(); + $this->assertResponseStatus(201); + $this->assertEquals('EDITED DURING REOPEN', $this->reloadPresentation()->getTitle()); + } + + /** + * The third assertion of the SDS bullet, recorded as the structural guard it is rather than + * presented as coverage it is not. + * + * getAllowedMediaUploadTypes() (PresentationType.php:413) unconditionally returns the type's + * stored collection -- it takes no window, no selection plan and no presentation. The mutation + * path agrees: addMediaUploadTo (PresentationService.php:1083-1125) checks the media upload type, + * the presentation-type allowance and the max-qty cap, and never consults the submission window + * or the plan at all. Media and speaker subresource mutations are ungated in EVERY window -- open, + * closed, granted or not; adding that guard is ClickUp 86bba8388 and is out of scope here. + * + * So this asserts a property that cannot vary with the reopen state, and it would keep passing if + * the reopen gate broke entirely. It cannot detect a fixture change either -- both sides derive + * from the same fixture and would move together. It is kept only because the SDS bullet names + * the assertion explicitly. Do NOT read a green here as evidence that reopen leaves media uploads + * alone: what makes that true is 86bba8388 being unimplemented, not this test. It also says + * nothing about mutation behavior, only about the advertised set. + */ + public function testAllowedMediaUploadTypesAreUnchangedUnderReopen() + { + $before = []; + foreach (self::$defaultPresentationType->getAllowedMediaUploadTypes() as $type) { + $before[] = $type->getId(); + } + sort($before); + $this->assertNotEmpty($before, 'fixture attaches no media upload types; the comparison is vacuous'); + + $this->grantWindow(24); + $reloaded = $this->reloadPresentation(); + $this->assertTrue($reloaded->isSubmissionReopened(), 'grant did not persist'); + + $after = []; + foreach ($reloaded->getType()->getAllowedMediaUploadTypes() as $type) { + $after[] = $type->getId(); + } + sort($after); + + $this->assertEquals($before, $after); + } }