Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mark read issues #1569

Merged
merged 13 commits into from Oct 22, 2021
Merged

Fix mark read issues #1569

merged 13 commits into from Oct 22, 2021

Conversation

tbarbugli
Copy link
Member

🎯 Goal

  • Ensure channel is marked as read when a message is added to the list and its visible (before it would only happen when the user opens the channel or when the list was scrolled)
  • Avoid calling markRead endpoint multiple times (before scrolling would cause tens of calls to markRead)

🛠 Implementation

  • Moved some db logic from middleware to database code
  • The channel updater now updates the DB when mark read is called (this is to avoid races)

🧪 Testing

  • Manual testing

🎨 Changes

Add relevant screenshots or videos showcasing the changes.

☑️ Checklist

  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Affected documentation updated (docusaurus, tutorial, CMS (task created)

@tbarbugli
Copy link
Member Author

This PR should resolve #1558

@Stream-iOS-Bot
Copy link
Collaborator

Stream-iOS-Bot commented Oct 20, 2021

6 Errors
🚫 Please start subject with capital letter.
a80ac91
🚫 Please start subject with capital letter.
c61ec5b
🚫 Please start subject with capital letter.
0652483
🚫 Please use more than one word.
1063aa7
🚫 Please start subject with capital letter.
5697fe1
🚫 Please start subject with capital letter.
8e994a8

Generated by 🚫 Danger

@tbarbugli tbarbugli requested a review from evsaev October 21, 2021 12:42
nuno-vieira
nuno-vieira previously approved these changes Oct 22, 2021
Copy link
Member

@nuno-vieira nuno-vieira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! ✅ Just added 2 small nit's, feel free to fix them 👍

Sources/StreamChatUI/ChatChannel/ChatChannelVC.swift Outdated Show resolved Hide resolved
tbarbugli and others added 2 commits October 22, 2021 15:54
Co-authored-by: Nuno Vieira <nuno.fcvieira93@gmail.com>
Co-authored-by: Nuno Vieira <nuno.fcvieira93@gmail.com>
nuno-vieira
nuno-vieira previously approved these changes Oct 22, 2021
nuno-vieira
nuno-vieira previously approved these changes Oct 22, 2021
@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #1569 (c63ad90) into main (ba3adea) will decrease coverage by 0.05%.
The diff coverage is 65.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1569      +/-   ##
==========================================
- Coverage   85.24%   85.18%   -0.06%     
==========================================
  Files         232      232              
  Lines       10773    10786      +13     
==========================================
+ Hits         9183     9188       +5     
- Misses       1590     1598       +8     
Flag Coverage Δ
llc-tests 85.18% <65.71%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
Sources/StreamChat/Database/DatabaseSession.swift 89.79% <ø> (ø)
...trollers/ChannelController/ChannelController.swift 86.70% <23.07%> (-1.53%) ⬇️
...rces/StreamChat/Database/DTOs/ChannelReadDTO.swift 96.72% <88.23%> (-3.28%) ⬇️
...ventMiddlewares/ChannelReadUpdaterMiddleware.swift 91.30% <100.00%> (+3.80%) ⬆️
Sources/StreamChat/Workers/ChannelUpdater.swift 87.07% <100.00%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 024a60a...c63ad90. Read the comment docs.

@nuno-vieira nuno-vieira merged commit 30d1323 into main Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants