fix(speakers): honour payload bio on createMySpeaker instead of always overriding with member bio - #564
Conversation
…s overriding with member bio Signed-off-by: romanetar <roman_ag@hotmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesSpeaker bio precedence
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The endpoint now allows submitted values to override several member-derived speaker fields, not just bio. This is limited to the authenticated user’s own profile, but the broader authority change should receive explicit owner confirmation or follow-up before merging. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-564/ 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.
🧹 Nitpick comments (1)
tests/oauth2/OAuth2SummitSpeakersApiTest.php (1)
2084-2094: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun cleanup in a
finallyblock so a failed assertion doesn't leak the Member/Speaker.Assertions on lines 2084-2088 run before the cleanup on lines 2090-2093. If an assertion fails, the test aborts and the created member (and the speaker created by
createMySpeaker) are never removed, leaking state into subsequent tests in this class.Separately,
setUserExternalId(mt_rand())(line 2049) can in rare cases collide with the seeded members' external ids, which could make auth resolution flaky; consider a deterministic unique value derived from$prefix.♻️ Suggested structure
$this->assertResponseStatus(201); try { $speaker = json_decode($response->getContent()); $this->assertNotNull($speaker); $this->assertEquals($payloadBio, $speaker->bio, "Speaker bio must match what the user submitted, not the member/FNid bio."); } finally { $this->resetEmIfNeeded(); self::$em->remove(self::$em->find(Member::class, $newMember->getId())); self::$em->flush(); }🤖 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/OAuth2SummitSpeakersApiTest.php` around lines 2084 - 2094, Move the cleanup in the test around the response/assertion checks into a finally block so `createMySpeaker`-created state is always removed even if `assertResponseStatus`, `assertNotNull`, or `assertEquals` fails. Use the existing `resetEmIfNeeded()` and `self::$em->remove(...)`/`flush()` cleanup sequence after the assertions, and also replace the `setUserExternalId(mt_rand())` value with a deterministic unique value derived from `$prefix` to avoid rare collisions with seeded members.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@tests/oauth2/OAuth2SummitSpeakersApiTest.php`:
- Around line 2084-2094: Move the cleanup in the test around the
response/assertion checks into a finally block so `createMySpeaker`-created
state is always removed even if `assertResponseStatus`, `assertNotNull`, or
`assertEquals` fails. Use the existing `resetEmIfNeeded()` and
`self::$em->remove(...)`/`flush()` cleanup sequence after the assertions, and
also replace the `setUserExternalId(mt_rand())` value with a deterministic
unique value derived from `$prefix` to avoid rare collisions with seeded
members.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b7da1f7-fe99-4c0f-b5d1-44a013d78819
📒 Files selected for processing (2)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.phptests/oauth2/OAuth2SummitSpeakersApiTest.php
Form clients commonly serialize untouched fields as empty strings. An empty or whitespace-only value for the defaultable keys (first_name, last_name, bio, twitter, irc) won the merge against the member defaults and created a speaker with an empty field instead of falling back to the member profile data. On create there is nothing to clear yet, so such values now count as not sent. The PUT path is untouched: empty strings there still clear the field. Adds a boundary regression test: a whitespace-only submitted bio falls back to the member bio (also covering the trim).
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-564/ This page is automatically updated on each push to this PR. |
2 participants
ref https://app.clickup.com/t/9014802374/86badw3a0
Summary by CodeRabbit
Bug Fixes
Tests