fix(presentations): resolve summit_id via owning presentation for PresentationMaterial lifecycle events - #579
fix(presentations): resolve summit_id via owning presentation for PresentationMaterial lifecycle events#579romanetar wants to merge 4 commits into
Conversation
…sentationMaterial lifecycle events Signed-off-by: romanetar <roman_ag@hotmail.com>
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds summit ID retrieval to ChangesPresentation material event propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Feature/PresentationMaterialRabbitMQIntegrationTest.php`:
- Around line 138-151: Update the message polling loop around
consumer_channel->basic_get to decode each received payload and continue
consuming until entity_type is PresentationMediaUpload and entity_id matches
$entity_id, or the timeout expires. Only assert and validate summit_id after the
matching message is found, while preserving the existing not-null failure when
no match arrives.
- Around line 59-71: Update the test setup around the RabbitMQ Config::set calls
to capture the original rabbitmq configuration before mutation, then restore
that configuration and call
app()->forgetInstance(IProcessScheduleEntityLifeCycleEventService::class) during
teardown before fixture cleanup. Ensure shared configuration and the singleton
are reset even when the test completes or fails.
- Around line 63-73: Update the RabbitMQ setup in
PresentationMaterialRabbitMQIntegrationTest to read the broker user and password
from the test environment instead of hardcoding them. Reuse those configured
values in both the rabbitmq.user/rabbitmq.password settings and the
AMQPStreamConnection construction, while preserving the existing host, port,
vhost, and lifecycle reset behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3dace2a-47db-4d6f-8033-e9d6ea32ccf2
📒 Files selected for processing (3)
app/Models/Foundation/Summit/Events/Presentations/Materials/PresentationMaterial.phptests/Feature/PresentationMaterialRabbitMQIntegrationTest.phptests/Unit/Services/PresentationMaterialEventDispatchTest.php
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-579/ This page is automatically updated on each push to this PR. |
1 similar comment
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-579/ This page is automatically updated on each push to this PR. |
Restores the original rabbitmq config and forgets the IProcessScheduleEntityLifeCycleEventService singleton in tearDown() so the real-broker override doesn't leak to later tests in the same process; sources broker credentials from existing DOMAIN_EVENTS_RABBITMQ_* / RABBITMQ_* env vars instead of hardcoding them in a tracked file; and makes the message-polling loop skip unrelated messages on the shared fanout exchange instead of asserting on the first one received. Signed-off-by: romanetar <roman_ag@hotmail.com>
3d8c06f to
af30e55
Compare
There was a problem hiding this comment.
Pull request overview
Fixes incorrect summit_id resolution in PresentationMaterial lifecycle events by adding a getSummitId() accessor that delegates to the owning Presentation, and adds regression + end-to-end coverage to ensure event payloads publish the correct summit identifier.
Changes:
- Add
PresentationMaterial::getSummitId()to allowScheduleEntity::_getSummitId()to resolve summit via reflection for materials. - Add unit coverage for insert/update/delete lifecycle dispatch behavior for
PresentationMediaUpload. - Add a RabbitMQ end-to-end integration test and wire it into the CI matrix (including provisioning a RabbitMQ service).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/Services/PresentationMaterialEventDispatchTest.php | Adds regression tests asserting lifecycle jobs carry the correct summit_id (and characterizes delete behavior). |
| tests/Feature/PresentationMaterialRabbitMQIntegrationTest.php | Adds end-to-end RabbitMQ assertion that published messages include a non-zero summit_id. |
| tests/Feature/ExampleTest.php | Removes the default scaffold example test. |
| app/Models/Foundation/Summit/Events/Presentations/Materials/PresentationMaterial.php | Adds getSummitId() delegation to Presentation to support reflection-based summit resolution. |
| .github/workflows/push.yml | Adds a CI matrix entry for the new RabbitMQ integration test and provisions a RabbitMQ service + env vars. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Restores the original rabbitmq config and forgets the IProcessScheduleEntityLifeCycleEventService singleton in tearDown() so the real-broker override doesn't leak to later tests in the same process; sources broker credentials from existing DOMAIN_EVENTS_RABBITMQ_* / RABBITMQ_* env vars instead of hardcoding them in a tracked file; and makes the message-polling loop skip unrelated messages on the shared fanout exchange instead of asserting on the first one received. Signed-off-by: romanetar <roman_ag@hotmail.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-579/ This page is automatically updated on each push to this PR. |
Restores the original rabbitmq config and forgets the IProcessScheduleEntityLifeCycleEventService singleton in tearDown() so the real-broker override doesn't leak to later tests in the same process; sources broker credentials from existing DOMAIN_EVENTS_RABBITMQ_* / RABBITMQ_* env vars instead of hardcoding them in a tracked file; and makes the message-polling loop skip unrelated messages on the shared fanout exchange instead of asserting on the first one received. Signed-off-by: romanetar <roman_ag@hotmail.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-579/ This page is automatically updated on each push to this PR. |
ref https://app.clickup.com/t/9014802374/86bb90npj
Summary by CodeRabbit
Bug Fixes
Tests