Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughThe message DTO now represents mentioned groups with ChangesUser group response migration
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to The migration is covered by updated parsing and mapping paths, with no demonstrated production compatibility failure; it is mergeable. 🚥 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. A rabbit hops through groups anew Comment |
andremion
left a comment
andremion
left a comment
There was a problem hiding this comment.
Looks good. Two small things inline, both optional.
| val mentioned_here: Boolean? = null, | ||
| val mentioned_channel: Boolean? = null, | ||
| val mentioned_groups: List<DownstreamUserGroupDto> = emptyList(), | ||
| val mentioned_groups: List<UserGroupResponse> = emptyList(), |
There was a problem hiding this comment.
Small thing: UserGroupResponse makes created_at and updated_at required, but the direct parser in UserGroupAdapter still leaves both null when they are missing. The two paths now disagree on a payload without those fields, and assertBothPaths will not catch it because every fixture sends them. The backend always sends them so nothing breaks. Any reason not to require them in UserGroupAdapter too, so the paths stay in step?
| @@ -88,14 +88,6 @@ internal object MessageTestData { | |||
| "name": "engineering", | |||
| "description": "Engineering team", | |||
| "team_id": "team-1", | |||
There was a problem hiding this comment.
Any reason to drop the members block instead of adding app_pk to it? userGroupAdapter has one call site, the mentioned_groups branch in MessageAdapter, so after this no test reaches UserGroupMemberAdapter at all. And if members really cannot arrive under mentioned_groups, the members branch in UserGroupAdapter plus UserGroupMemberAdapter looks dead as well. Happy to be told otherwise.
2 participants
Goal
Parse the user groups mentioned on a message with the generated
UserGroupResponse, retiringDownstreamUserGroupDtoandDownstreamUserGroupMemberDto.Part of AND-1291
Implementation
DownstreamMessageDto.mentioned_groupsat the generated model. This was the last main-sourcein-edge on the hand-written pair: the user group endpoints already returned
UserGroupResponsethroughtheir response wrappers, so
UserGroupResponse.toDomain()already existed and was already exercised.generated model's own.
membersblock from thementioned_groupsfixture inMessageTestData.kt. Messages hydrategroups with
commonpayloads.UserGroupResponse, which declares noMembersfield at all; the endpointsuse a separate type that embeds it and adds members. The fixture described a payload the backend cannot
send, and the generated
UserGroupMemberrequiresapp_pk, so it stopped parsing.Notes
The element type now matches the end state: the generated
MessageResponsedeclaresmentioned_groupsasList<UserGroupResponse>?. The container stays non-null here.thread_participants,mentioned_groupsandmentioned_rolesshare a documented explicit-null rejection onDownstreamMessageDto, each with its own test, and loosening one of the three ahead of its siblings wouldchange null handling for a payload the wire never produces, since the Go tag is
json:"mentioned_groups,omitempty"on a slice. The three flip together whenDownstreamMessageDtobecomesMessageResponse.Three fields go from optional to required:
created_atandupdated_aton the group, which are plainnon-
omitemptytags on the payload struct the wire serializes, so they always arrive.No mutation sweep: this slice writes no mapper.
UserGroupResponse.toDomain()is already on develop, so asweep would mutate code this PR does not touch.
Testing
Device-probed every path that carries
mentioned_groups: thesendMessageresponse, thegetMessagere-read, and the message nested in a
queryChannelresponse. All three returned the group withcreated_atandupdated_atpopulated andmembersempty, confirming the slim payload. The write pathstill sends
mentioned_group_idsand is unchanged by the swap.Summary by CodeRabbit
Bug Fixes
Tests