feat(events): add PUT /events/{id}/draft endpoint to save incomplete submissions#555
feat(events): add PUT /events/{id}/draft endpoint to save incomplete submissions#555romanetar wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds a PUT /api/v1/summits/{id}/events/{event_id}/draft endpoint and refactors event update logic so updates can be saved as incomplete; service signatures, presentation save behavior, DB registration, and tests are updated accordingly. ChangesDraft Event Update with Incomplete Presentation State
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.2.1)/Libs/ModelSerializers/AbstractSerializer.php ... [truncated 65750 characters] ... agGroups/TrackTagGroupAllowedTagSerializer.php Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-555/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Services/Model/Imp/SummitService.php (1)
892-935:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDemote previously complete presentations when draft edits remove required participants.
This path now allows an existing presentation to lose its speakers/moderator, but it only skips the auto-promotion on Lines 892-897; it never moves an already
STATUS_RECEIVED/PHASE_COMPLETEpresentation back out of the complete state. That leaves stored completion metadata inconsistent with the actual draft contents.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Services/Model/Imp/SummitService.php` around lines 892 - 935, Existing logic prevents auto-promotion but doesn't demote already-complete presentations when required participants are removed; after processing speakers and moderator, detect if $event->getStatus() === Presentation::STATUS_RECEIVED && $event->getProgress() === Presentation::PHASE_COMPLETE and a required participant has been removed (use the same signals: $shouldClearSpeakers or (previously had speakers but now count($speakers) == 0 for mandatory speakers, and $shouldClearModerator or $moderator_id == 0 for mandatory moderator), and !$saveAsIncomplete) and then call $event->setStatus(Presentation::STATUS_DRAFT) and $event->setProgress(Presentation::PHASE_INCOMPLETE) (or the appropriate non-complete constants) via the existing setStatus/setProgress methods to keep stored completion metadata consistent.
🧹 Nitpick comments (1)
tests/oauth2/OAuth2SummitEventsApiTest.php (1)
15-15: 💤 Low valueUnused import.
The
ISummitServiceimport appears to be unused in this test file. Consider removing it to keep imports clean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/oauth2/OAuth2SummitEventsApiTest.php` at line 15, Remove the unused import ISummitService from the OAuth2SummitEventsApiTest file: locate the "use App\Services\Model\ISummitService;" statement in OAuth2SummitEventsApiTest.php and delete it (ensuring no references to ISummitService remain in the test); run the test suite to verify no compile/test errors after removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@database/migrations/config/Version20260609175051.php`:
- Around line 54-59: The endpoint registration currently calls
insertEndpoint(self::API_NAME, self::ENDPOINT_NAME, self::ENDPOINT_ROUTE, 'GET')
but the route is a write operation and should use PUT; update the fourth
argument of the insertEndpoint call for self::ENDPOINT_ROUTE from 'GET' to 'PUT'
so the migration registers the correct HTTP method (check the
Version20260609175051 migration where insertEndpoint(...) is invoked).
In `@tests/InsertOrdersTestData.php`:
- Around line 143-150: clearOrdersTestData uses DB::table to raw-update the
SummitTicketType row, which will leave the in-memory Doctrine entity
self::$default_ticket_type and ORM caches stale; fix by using Doctrine to reset
the field on the entity (e.g. call a setter or resetQuantitySold on
self::$default_ticket_type and then self::$em->flush()) or, if keeping the raw
DB update, immediately synchronize the ORM by calling
self::$em->refresh(self::$default_ticket_type) or self::$em->clear() after the
DB::table(...) update so the entity/cache reflect QuantitySold = 0.
---
Outside diff comments:
In `@app/Services/Model/Imp/SummitService.php`:
- Around line 892-935: Existing logic prevents auto-promotion but doesn't demote
already-complete presentations when required participants are removed; after
processing speakers and moderator, detect if $event->getStatus() ===
Presentation::STATUS_RECEIVED && $event->getProgress() ===
Presentation::PHASE_COMPLETE and a required participant has been removed (use
the same signals: $shouldClearSpeakers or (previously had speakers but now
count($speakers) == 0 for mandatory speakers, and $shouldClearModerator or
$moderator_id == 0 for mandatory moderator), and !$saveAsIncomplete) and then
call $event->setStatus(Presentation::STATUS_DRAFT) and
$event->setProgress(Presentation::PHASE_INCOMPLETE) (or the appropriate
non-complete constants) via the existing setStatus/setProgress methods to keep
stored completion metadata consistent.
---
Nitpick comments:
In `@tests/oauth2/OAuth2SummitEventsApiTest.php`:
- Line 15: Remove the unused import ISummitService from the
OAuth2SummitEventsApiTest file: locate the "use
App\Services\Model\ISummitService;" statement in OAuth2SummitEventsApiTest.php
and delete it (ensuring no references to ISummitService remain in the test); run
the test suite to verify no compile/test errors after removal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24883102-d430-4aad-bec7-9396fe6bcd99
📒 Files selected for processing (8)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.phpapp/Services/Model/ISummitService.phpapp/Services/Model/Imp/SummitService.phpdatabase/migrations/config/Version20260609175051.phpdatabase/seeders/ApiEndpointsSeeder.phproutes/api_v1.phptests/InsertOrdersTestData.phptests/oauth2/OAuth2SummitEventsApiTest.php
…submissions Signed-off-by: romanetar <roman_ag@hotmail.com>
romanetar
force-pushed
the
feature/save-incomplete-submissions
branch
from
b2f5575 to
8378d59
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-555/ This page is automatically updated on each push to this PR. |
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@romanetar please review
Signed-off-by: romanetar <roman_ag@hotmail.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-555/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Services/Model/Imp/SummitService.php`:
- Around line 909-918: The current conditional (! $saveAsIncomplete ||
$event->isNew()) incorrectly prevents explicit clears for existing presentations
when saving drafts; move the mutation (calls that clear/unset speakers and
moderator like $event->clearSpeakers() and the moderator unset logic) out of
that gate so they run whenever $shouldClearSpeakers or the moderator-clear flag
is set, but keep the mandatory-speaker validation (the isAreSpeakersMandatory()
check and its throw) inside the original (!$saveAsIncomplete || $event->isNew())
block; apply the same change to the corresponding moderator handling around the
code that unsets moderator_speaker_id (the block referenced at lines ~937-944)
so explicit empty payloads persist for existing drafts.
In `@tests/oauth2/OAuth2SummitEventsApiTest.php`:
- Around line 523-542: The test
testUpdateDraftEventOnPublishedPresentationReturns412 assumes
self::$presentations[0] is published but doesn't assert it; add a precondition
assertion right after $presentation = self::$presentations[0] that verifies the
presentation is published (for example,
$this->assertTrue($presentation->isPublished(), 'precondition: presentation must
be published') or use the project’s existing published-status accessor like
getPublished()/getStatus()) so the test fails early and cannot produce false
positives if test data changes.
- Line 15: The file contains an unused import "ISummitService" in
OAuth2SummitEventsApiTest.php; remove the line "use
App\Services\Model\ISummitService;" so the test file no longer imports the
unused interface and clean up unused symbol warnings.
- Around line 519-520: Replace the negative assertions with positive checks that
verify the exact expected state: instead of asserting not equal to
Presentation::PHASE_COMPLETE and Presentation::STATUS_RECEIVED, assert that
$event->progress equals the expected constant (e.g. Presentation::PHASE_SUMMARY)
and that $event->status equals the expected status constant (replace/confirm
what the test expects) so the test fails if the state silently changes; update
the assertions referencing $event->progress and $event->status accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6d47bb6-7bbd-4659-b2dd-14a4a831a854
📒 Files selected for processing (8)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.phpapp/Services/Model/ISummitService.phpapp/Services/Model/Imp/SummitService.phpdatabase/migrations/config/Version20260609175051.phpdatabase/seeders/ApiEndpointsSeeder.phproutes/api_v1.phptests/InsertOrdersTestData.phptests/oauth2/OAuth2SummitEventsApiTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- routes/api_v1.php
- tests/InsertOrdersTestData.php
- app/Services/Model/ISummitService.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.php
| if (!$saveAsIncomplete || $event->isNew()) { | ||
| if ($event_type->isAreSpeakersMandatory()) { | ||
| if ($shouldClearSpeakers || ($event->isNew() && count($speakers) == 0)) | ||
| throw new ValidationException('Speakers are mandatory.'); | ||
| } | ||
|
|
||
| if ($shouldClearSpeakers) { | ||
| $event->clearSpeakers(); | ||
| if ($shouldClearSpeakers) { | ||
| $event->clearSpeakers(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Draft updates ignore explicit speaker/moderator clears on existing presentations.
At Line 909 and Line 937, the (!$saveAsIncomplete || $event->isNew()) gate wraps both mandatory validation and the clear/unset mutations. For existing draft saves, speakers: [] and moderator_speaker_id: 0 are silently ignored, so the payload cannot persist those incomplete states.
🔧 Proposed fix
- if (!$saveAsIncomplete || $event->isNew()) {
- if ($event_type->isAreSpeakersMandatory()) {
- if ($shouldClearSpeakers || ($event->isNew() && count($speakers) == 0))
- throw new ValidationException('Speakers are mandatory.');
- }
-
- if ($shouldClearSpeakers) {
- $event->clearSpeakers();
- }
- }
+ if ($event_type->isAreSpeakersMandatory() && (!$saveAsIncomplete || $event->isNew())) {
+ if ($shouldClearSpeakers || ($event->isNew() && count($speakers) == 0))
+ throw new ValidationException('Speakers are mandatory.');
+ }
+
+ if ($shouldClearSpeakers) {
+ $event->clearSpeakers();
+ }- if (!$saveAsIncomplete || $event->isNew()) {
- if ($event_type->isModeratorMandatory()) {
- if ($shouldClearModerator || ($event->isNew() && $moderator_id == 0))
- throw new ValidationException('moderator_speaker_id is mandatory.');
- }
-
- if ($shouldClearModerator) $event->unsetModerator();
- }
+ if ($event_type->isModeratorMandatory() && (!$saveAsIncomplete || $event->isNew())) {
+ if ($shouldClearModerator || ($event->isNew() && $moderator_id == 0))
+ throw new ValidationException('moderator_speaker_id is mandatory.');
+ }
+
+ if ($shouldClearModerator) $event->unsetModerator();Also applies to: 937-944
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Services/Model/Imp/SummitService.php` around lines 909 - 918, The current
conditional (! $saveAsIncomplete || $event->isNew()) incorrectly prevents
explicit clears for existing presentations when saving drafts; move the mutation
(calls that clear/unset speakers and moderator like $event->clearSpeakers() and
the moderator unset logic) out of that gate so they run whenever
$shouldClearSpeakers or the moderator-clear flag is set, but keep the
mandatory-speaker validation (the isAreSpeakersMandatory() check and its throw)
inside the original (!$saveAsIncomplete || $event->isNew()) block; apply the
same change to the corresponding moderator handling around the code that unsets
moderator_speaker_id (the block referenced at lines ~937-944) so explicit empty
payloads persist for existing drafts.
| * limitations under the License. | ||
| **/ | ||
| use App\Models\Foundation\Main\IGroup; | ||
| use App\Services\Model\ISummitService; |
There was a problem hiding this comment.
Remove unused import.
The ISummitService import is not referenced anywhere in this test file.
🧹 Proposed fix
-use App\Services\Model\ISummitService;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use App\Services\Model\ISummitService; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/oauth2/OAuth2SummitEventsApiTest.php` at line 15, The file contains an
unused import "ISummitService" in OAuth2SummitEventsApiTest.php; remove the line
"use App\Services\Model\ISummitService;" so the test file no longer imports the
unused interface and clean up unused symbol warnings.
| $this->assertNotEquals(Presentation::PHASE_COMPLETE, $event->progress); | ||
| $this->assertNotEquals(Presentation::STATUS_RECEIVED, $event->status); |
There was a problem hiding this comment.
Strengthen assertions to verify the expected state positively.
The assertions only check that progress is NOT PHASE_COMPLETE and status is NOT STATUS_RECEIVED. If the update silently fails or transitions to an unexpected incomplete state, the test will still pass. Assert the specific expected values instead.
🔍 Proposed fix to assert positive expected state
- $this->assertNotEquals(Presentation::PHASE_COMPLETE, $event->progress);
- $this->assertNotEquals(Presentation::STATUS_RECEIVED, $event->status);
+ $this->assertEquals(Presentation::PHASE_SUMMARY, $event->progress);
+ $this->assertNotEquals(Presentation::STATUS_RECEIVED, $event->status);If the draft update is expected to preserve PHASE_SUMMARY, use assertEquals(Presentation::PHASE_SUMMARY, ...) to explicitly verify the expected state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/oauth2/OAuth2SummitEventsApiTest.php` around lines 519 - 520, Replace
the negative assertions with positive checks that verify the exact expected
state: instead of asserting not equal to Presentation::PHASE_COMPLETE and
Presentation::STATUS_RECEIVED, assert that $event->progress equals the expected
constant (e.g. Presentation::PHASE_SUMMARY) and that $event->status equals the
expected status constant (replace/confirm what the test expects) so the test
fails if the state silently changes; update the assertions referencing
$event->progress and $event->status accordingly.
| public function testUpdateDraftEventOnPublishedPresentationReturns412() | ||
| { | ||
| $presentation = self::$presentations[0]; | ||
|
|
||
| $response = $this->action( | ||
| "PUT", | ||
| "OAuth2SummitEventsApiController@updateDraftEvent", | ||
| [ | ||
| 'id' => self::$summit->getId(), | ||
| 'event_id' => $presentation->getId(), | ||
| ], | ||
| [], | ||
| [], | ||
| [], | ||
| $this->getAuthHeaders(), | ||
| json_encode(['title' => 'Should Not Update']) | ||
| ); | ||
|
|
||
| $this->assertResponseStatus(412); | ||
| } |
There was a problem hiding this comment.
Add a precondition assertion to verify the presentation is published.
The test assumes self::$presentations[0] is published but doesn't verify this precondition. If the test data changes, the test may produce false positives or become meaningless.
🛡️ Proposed fix to add precondition check
public function testUpdateDraftEventOnPublishedPresentationReturns412()
{
$presentation = self::$presentations[0];
+ $this->assertTrue($presentation->isPublished(), "Test requires a published presentation");
$response = $this->action(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testUpdateDraftEventOnPublishedPresentationReturns412() | |
| { | |
| $presentation = self::$presentations[0]; | |
| $response = $this->action( | |
| "PUT", | |
| "OAuth2SummitEventsApiController@updateDraftEvent", | |
| [ | |
| 'id' => self::$summit->getId(), | |
| 'event_id' => $presentation->getId(), | |
| ], | |
| [], | |
| [], | |
| [], | |
| $this->getAuthHeaders(), | |
| json_encode(['title' => 'Should Not Update']) | |
| ); | |
| $this->assertResponseStatus(412); | |
| } | |
| public function testUpdateDraftEventOnPublishedPresentationReturns412() | |
| { | |
| $presentation = self::$presentations[0]; | |
| $this->assertTrue($presentation->isPublished(), "Test requires a published presentation"); | |
| $response = $this->action( | |
| "PUT", | |
| "OAuth2SummitEventsApiController@updateDraftEvent", | |
| [ | |
| 'id' => self::$summit->getId(), | |
| 'event_id' => $presentation->getId(), | |
| ], | |
| [], | |
| [], | |
| [], | |
| $this->getAuthHeaders(), | |
| json_encode(['title' => 'Should Not Update']) | |
| ); | |
| $this->assertResponseStatus(412); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 527-527: Avoid unused local variables such as '$response'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/oauth2/OAuth2SummitEventsApiTest.php` around lines 523 - 542, The test
testUpdateDraftEventOnPublishedPresentationReturns412 assumes
self::$presentations[0] is published but doesn't assert it; add a precondition
assertion right after $presentation = self::$presentations[0] that verifies the
presentation is published (for example,
$this->assertTrue($presentation->isPublished(), 'precondition: presentation must
be published') or use the project’s existing published-status accessor like
getPublished()/getStatus()) so the test fails early and cannot produce false
positives if test data changes.
2 participants
ref https://app.clickup.com/t/86ba3xg57
Summary by CodeRabbit
New Features
Behavior Changes
Tests & Data