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

[Background Mapping] Fix Mark as unread #2906

Merged
merged 6 commits into from
Nov 24, 2023

Conversation

polqf
Copy link
Contributor

@polqf polqf commented Nov 21, 2023

🔗 Issue Links

Closes https://github.com/GetStream/ios-issues-tracking/issues/652

🎯 Goal

Fix Mark as unread when Background Mapping is enabled

📝 Summary

We need to use the data that the API Request returns to us, instead of waiting for the delegate calls.
The most important thing here is that we cannot use the delegate calls because we cannot know if the update is from a “mark as unread” action or a different thing. If we just update the unread banner on the delegate call, we lose all the context as the channel will be marked as read, and the views would then disappear.

🛠 Implementation

  • The completion of markUnread(from:completion:) moves from just returning the Error to returning a Result object with the data.
  • ChannelController.getFirstUnreadMessageId(for:) is now public
  • Channel exposes now a convenience function to get the lastReadMessageId based on a user id (lastReadMessageId(userId:))

🧪 Manual Testing Notes

Precondition: BG Mapping Enabled

  1. Open a channel with many messages
  2. Mark a message as unread

Expected result:

  • It should update the "Unread messages" banner to the right location

☑️ Contributor Checklist

  • I have signed the Stream CLA (required)
  • This change follows zero ⚠️ policy (required)
  • This change should be manually QAed
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Comparison screenshots added for visual changes
  • Affected documentation updated (docusaurus, tutorial, CMS)

🎁 Meme

Copy link

github-actions bot commented Nov 21, 2023

1 Warning
⚠️ The changes should be manually QAed before the Pull Request will be merged

Generated by 🚫 Danger

@polqf polqf changed the title Fix Mark As Unread in BG [Background Mapping] Fix Mark as unread Nov 22, 2023
@polqf polqf marked this pull request as ready for review November 22, 2023 15:14
@polqf polqf requested a review from a team as a code owner November 22, 2023 15:14
Copy link
Contributor

@martinmitrevski martinmitrevski left a comment

Choose a reason for hiding this comment

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

LGTM codewise ✅ Only spotted one issue while testing (shared on Slack), not sure if it's related to BM. @testableapple should run a QA as well 🙏

@martinmitrevski martinmitrevski added the 🤞 Ready For QA A PR that is Ready for QA label Nov 23, 2023
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 44 Code Smells

90.7% 90.7% Coverage
0.1% 0.1% Duplication

@testableapple testableapple added 🟢 QAed A PR that was QAed and removed 🤞 Ready For QA A PR that is Ready for QA labels Nov 23, 2023
@polqf polqf merged commit 36dc575 into develop Nov 24, 2023
17 checks passed
@polqf polqf deleted the bugfix/mark-as-unread-bg-mapping branch November 24, 2023 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🟢 QAed A PR that was QAed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants