[CIS-1227] [CIS-1430] Channel.membership correct handling #1736
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
b-onc
changed the title
b-onc
added
馃悶聽Bug
evsaev
reviewed
Jan 3, 2022
Sources/StreamChat/WebSocketClient/EventMiddlewares/MemberEventMiddleware.swift
Outdated
Show resolved
Hide resolved
evsaev
reviewed
Jan 3, 2022
Sources/StreamChat/WebSocketClient/EventMiddlewares/MemberEventMiddleware.swift
Outdated
Show resolved
Hide resolved
evsaev
reviewed
Jan 3, 2022
Sources/StreamChat/WebSocketClient/EventMiddlewares/MemberEventMiddleware.swift
Show resolved
Hide resolved
evsaev
reviewed
Jan 3, 2022
b-onc
force-pushed
the
cis-1227-channel-membership
branch
from
bb5c5db
to
a8c47ca
Compare
evsaev
reviewed
Jan 4, 2022
Sources/StreamChat/WebSocketClient/EventMiddlewares/MemberEventMiddleware.swift
Outdated
Show resolved
Hide resolved
evsaev
reviewed
Jan 5, 2022
Comment on lines
135
to
137
| var channel: ChatChannel? { | ||
| database.viewContext.channel(cid: cid)?.asModel() | ||
| } | ||
|
|
||
| // Load the MemberListQueryDTO | ||
| var memberListQueryDTO: ChannelMemberListQueryDTO? { | ||
| database.viewContext.channelMemberListQuery(queryHash: memberListQuery.queryHash) | ||
| } |
There was a problem hiding this comment.
nit: we don't use async assert here so can go with stored variable
Suggested change
| var channel: ChatChannel? { | |
| database.viewContext.channel(cid: cid)?.asModel() | |
| } | |
| // Load the MemberListQueryDTO | |
| var memberListQueryDTO: ChannelMemberListQueryDTO? { | |
| database.viewContext.channelMemberListQuery(queryHash: memberListQuery.queryHash) | |
| } | |
| let channel = try XCTUnwrap( | |
| database.viewContext.channel(cid: cid)?.asModel() | |
| ) |
There was a problem hiding this comment.
hmm, are we using the channel at all? 馃
There was a problem hiding this comment.
channel was a copy/paste leftover :D we can't use let memberListQuery since we need it to be loaded again when we call it again
evsaev
reviewed
Jan 5, 2022
Comment on lines
552
to
554
| var channel: ChatChannel? { | ||
| database.viewContext.channel(cid: cid)?.asModel() | ||
| } |
There was a problem hiding this comment.
we don't access this variable, do we?
There was a problem hiding this comment.
correct, copy/paste leftover, thank you
evsaev
reviewed
Jan 5, 2022
Comment on lines
13
to
15
| "extraData" : { | ||
|
|
||
| }, |
There was a problem hiding this comment.
Suggested change
| "extraData" : { | |
| }, |
evsaev
reviewed
Jan 5, 2022
Comment on lines
13
to
15
| "extraData" : { | ||
|
|
||
| }, |
There was a problem hiding this comment.
Suggested change
| "extraData" : { | |
| }, | |
evsaev
approved these changes
Jan 5, 2022
added 7 commits
January 5, 2022 16:10
and comments to existing fields explaining that it's equal to `membership`
so they check if `membership` is correctly updated
to duplicate the channel query filtering in delegate methods
b-onc
force-pushed
the
cis-1227-channel-membership
branch
from
75f9961
to
803aa0c
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1736 +/- ##
===========================================
+ Coverage 85.33% 85.36% +0.02%
===========================================
Files 234 234
Lines 11186 11224 +38
===========================================
+ Hits 9546 9581 +35
- Misses 1640 1643 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
馃敆 Issue Link
CIS-1227
CIS-1430
馃幆 Goal
CIS-1227 :
Channel.membershipwas not parsed / removed in all cases. Some WS events were not handled correctly.CIS-1430 :
MemberListControllerdidn't reportmember.addedandnotification.added_to_channelevents correctly.馃洜 Implementation
MemberEventsMiddlewarenow correctly handles all events.We cannot add members to a
MemberListQueryif the query has a filter, since we can't verify if the member matches the filter.馃И Testing
Unit tests are implemented.
鈽戯笍 Checklist