Repository navigation
Migrate the channel members and member events to the generated model - #6698
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 (18)
💤 Files with no reviewable changes (7)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe change replaces ChangesChannel member model migration
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The member-model migration retains parsing, custom data, and user fallback behavior across the migrated paths. No actionable merge-blocking risk remains. 🚥 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 member fields, Comment |
d5919dc to
1d9311a
Compare
andremion
left a comment
andremion
left a comment
There was a problem hiding this comment.
Nothing blocking. Two fixtures outside the diff stop parsing with this change, and CI does not catch either.
The mock server's http_member.json has no notifications_muted and its user has no language, and helpers/members.rb sends that object as the member.added payload. The event throws and gets dropped, so test_deliveryStatusHidden_whenNewParticipantAddedAndReadEventsIsDisabled is passing without it now. Would you mind a small PR on stream-chat-test-mock-server for that?
Same two keys are missing in stream-chat-android-ui-uitests/src/androidTest/resources/http_channel.json and http_channels.json. The snapshot workflow is dispatch-only so CI is unaffected, but run_snapshot_test will fail on the next run.
Not blocking, just on the description: MembersResponse.members and UpdateMemberPartialResponse.channelMember already declare ChannelMemberResponse on develop, so queryMembers and partialUpdateMember were already on that path.
|
|
Thanks, all three were real. The uitests fixtures are fixed in c57923d: all four members in each file gained The mock server turned out to be a bit more involved: Fixed the description too, you are right that queryMembers and partialUpdateMember were already on that path. I also added a note there that the mock server PR should land first, since nothing turns red if it does not. |
|
🚀 Available in v7.12.0 |
3 participants
Goal
Parse channel members and the member events with the generated
ChannelMemberResponse, retiringDownstreamMemberDto.Part of AND-1291
Implementation
memberfield on the eight member events, andmembers/membershipon bothDownstreamChannelDtoand the hand-writtenChannelResponse, at the generated model.queryMembersandpartialUpdateMemberalready returned it throughMembersResponseandUpdateMemberPartialResponse;this adds the channel payloads and the events.
DownstreamMemberDtowith its adapter, its registration and its mapper.MemberDtos.ktandMemberDtoAdapters.ktbecomeDownstreamMemberInfoDto.ktandDownstreamMemberInfoDtoAdapter.kt, sincethat is all they still hold.
randomChannelMemberResponsewith a value that differs from its domain default. Sixof them were previously left unset, which made their assertions in the mapper test vacuous.
stream-chat-android-ui-uitestschannel fixtures thenotifications_mutedanduser.languagekeys the generated models require. The snapshot workflow is dispatch-only, so nothing ranthem.
NON_CUSTOM_MEMBER_KEYSfrom the adapter's compatibility set rather than repeating it, since mostof those keys are now declared on
ChannelMemberResponseand only stay inextraDatabecause that setholds them there.
Notes
This swap makes a member without a
userparseable. The wire declaresuser,omitemptyand falls back touser_id, butDownstreamMemberDto.userwas non-null, so such a payload would have thrown and taken thewhole response or event with it. The generated model is nullable there and the mapper already falls back to
User(id = userId).Six fields go the other way, from optional to required:
created_at,updated_at,banned,shadow_banned,channel_roleandnotifications_muted. All six are plain non-omitemptytags onpayload.ChannelMemberResponse, the struct the events and channel responses actually serialize, so thewire always sends them. Every lean member fixture had to gain them.
membershipis populated only byget_channelandget_or_create_channel;update_channelleaves itout, so an
addMembersresponse carries members without a membership. The field was already nullable andstill is.
The E2E mock server needs GetStream/stream-chat-test-mock-server#68 before this merges. Its
member.addedpayload was seeded from a fixture that predates the sync script and omits
notifications_mutedand theuser's
language, so once members parse through the generated model that event fails to decode and isdropped. Nothing turns red either way, since the one test that adds a member asserts a delivery status that
is hidden whether or not the event arrives, but without it the test stops exercising its scenario.
Testing
ChannelMemberResponse.toDomain: zero survivors across all 15 fields, where sixsurvived before the fixture was seeded.
membersandmembershipfromqueryChannels, theaddMembersresponse,partialUpdateMember, and themember.added,member.updatedandmember.removedevents. The compatibility keysuser_idandrolestill reachMember.extraDataalongside real custom data, which round-tripped on both the response and the event.