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.

Sign up for GitHub

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

Jump to bottom

[CIS-1724] Fix only visible for you badge not shown for ephemeral messages #1948

Merged
merged 5 commits into from
Apr 26, 2022
Merged

[CIS-1724] Fix only visible for you badge not shown for ephemeral messages #1948

merged 5 commits into from
Apr 26, 2022

Conversation

evsaev
Copy link
Contributor

@evsaev evsaev commented Apr 22, 2022 •
edited
Loading

🔗 Issue Links

Fixes: CIS-1724
Unblocks: CIS-1732

🎯 Goal

  • Fix regression when only visible for you indicator is not shown for ephemeral messages
  • Unblock CIS-1732. Handling deleted messages outside options resolver does not allow to write ChatMessageContentView's snapshot tests for deleted messages when visibility part is in ChatMessageListVC.

📝 Summary

  • Fix only visible for you indicator missing for ephemeral message
  • Fix ChatMessageListVC's lifecycle being triggered by assigning the delegate before it has a parent
  • ChatMessage.isOnlyVisibleForCurrentUser has been deprecated

🛠 Implementation

Move deleted message visibility check to ChatMessageLayoutOptionsResolver. Populate deleted messages setting on resolver from chatClient.config as the first thing on the message list.

🎨 Showcase

Before After
before after 2

🧪 Manual Testing Notes

GIVEN I'm in the channel
WHEN I use /giffy command and send the message
THEN Ephemeral message with giffy picker appears in channel
AND Ephemeral message has only visible to you badge

GIVEN I'm in thread
WHEN I use /giffy command and send the thread reply
THEN Ephemeral reply with giffy picker appears in thread
AND Ephemeral reply has only visible to you badge

☑️ Contributor Checklist

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

🎁 Meme

@evsaev evsaev added 🐞 Bug An issue or PR related to a bug 🎨 SDK: StreamChatUI Tasks related to the StreamChatUI SDK labels Apr 22, 2022
@evsaev evsaev requested a review from a team as a code owner April 22, 2022 16:01
@evsaev evsaev self-assigned this Apr 22, 2022
@evsaev evsaev force-pushed the fix/CIS-1724-only-visible-for-you branch from 6f2b61c to d81ec4e Compare April 22, 2022 16:03
@evsaev evsaev marked this pull request as draft April 22, 2022 16:03
@evsaev evsaev force-pushed the fix/CIS-1724-only-visible-for-you branch from d81ec4e to 452d0bc Compare April 22, 2022 16:20
@evsaev evsaev marked this pull request as ready for review April 22, 2022 16:21
@evsaev evsaev force-pushed the fix/CIS-1724-only-visible-for-you branch from 452d0bc to 356da2c Compare April 22, 2022 16:23
nuno-vieira
nuno-vieira reviewed Apr 22, 2022
View reviewed changes
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.

Overall looks good to me ✅ It is unfortunate we have to do this 😞 But we will improve it on v5 👍

I will officially approve it once we release 4.14.0. Otherwise, the changelog won't be in sync.

CHANGELOG.md Outdated
@@ -37,6 +38,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
- Fix audio files not rendering previews [#1907](https://github.com/GetStream/stream-chat-swift/issues/1907)
- Fix message sender name is not shown in channel with > 2 members if member identifiers were passed on channel creation [#1931](https://github.com/GetStream/stream-chat-swift/issues/1931)
- Fix incorrectly called viewWillAppear inside viewWillDissapear [#1938](https://github.com/GetStream/stream-chat-swift/pull/1938)
- Fix `onlyVisibleForYouIndicator` not being shown for ephemeral messages [#1948](https://github.com/GetStream/stream-chat-swift/pull/1948)
Copy link
Member

Choose a reason for hiding this comment

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

Friendly reminder to update the changelog after release ⏰

Sources/StreamChatUI/ChatMessageList/ChatMessage/ChatMessageLayoutOptionsResolver.swift Outdated
Comment on lines 13 to 15
// TODO: Propagate via `init` in v5.
/// The deleted messags visability. Is automatically populated from `ChatClient.config`.
public internal(set) var deletedMessagesVisibility: ChatClientConfig.DeletedMessageVisibility = .alwaysVisible
Copy link
Member

@nuno-vieira nuno-vieira Apr 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't it better to pass the whole ChatClientConfig? What if we need more stuff from it in the future? Then, we might keep adding new properties every time we need something from the chat client config.

We kinda need to rethink this in v5, the resolver probably needs to have access to the Components config as well (Example: The date separators). Or better, in v5 all UI Configs should live in the same data structure and not be mixed with ChatClientConfig + Components config.

Can we add this to our V5 Document and link this PR as well? That would be nice! So that we don't forget about this (We might forget this TODO comment)

Copy link
Contributor Author

@evsaev evsaev Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Updated to pass the entire config and added the point to v5 doc ✅

@evsaev evsaev force-pushed the fix/CIS-1724-only-visible-for-you branch 3 times, most recently from 28a81f9 to cf6f99c Compare April 25, 2022 11:39
@evsaev evsaev force-pushed the fix/CIS-1724-only-visible-for-you branch 2 times, most recently from d7cf4c9 to 46cb5df Compare April 26, 2022 11:12
@Stream-SDK-Bot
Copy link
Collaborator

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

nuno-vieira
nuno-vieira approved these changes Apr 26, 2022
View reviewed changes
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! ✅

@evsaev evsaev force-pushed the fix/CIS-1724-only-visible-for-you branch from 46cb5df to 1a52727 Compare April 26, 2022 13:18
@evsaev evsaev force-pushed the fix/CIS-1724-only-visible-for-you branch from 1a52727 to 1143439 Compare April 26, 2022 13:18
@sonarcloud
Copy link

sonarcloud bot commented Apr 26, 2022

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 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@evsaev evsaev merged commit 7c295d2 into develop Apr 26, 2022
@evsaev evsaev deleted the fix/CIS-1724-only-visible-for-you branch April 26, 2022 14:08
@b-onc b-onc mentioned this pull request May 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bielikb bielikb bielikb approved these changes

@nuno-vieira nuno-vieira nuno-vieira approved these changes

Assignees

@evsaev evsaev

Labels
🐞 Bug An issue or PR related to a bug 🎨 SDK: StreamChatUI Tasks related to the StreamChatUI SDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@evsaev @Stream-SDK-Bot @bielikb @nuno-vieira