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: Deletion of 1st message in a thread causes notification to be up without showing why. #34165

Merged
merged 16 commits into from
Dec 20, 2024

Conversation

Gustrb
Copy link
Contributor

@Gustrb Gustrb commented Dec 11, 2024

Proposed changes (including videos or screenshots)

This PR fixes the issue that would arise all the times the first (and only) message of a thread would get deleted. What was happening is that, we were not checking, on deletion, if all the messages of a thread had been deleted, which was making the UI feel unresponsive, since the 'Following' bell would show as if there were unread messages even if the message was deleted.
So, now, we do a couple checks to ensure consistency:

  1. We decrease the counter of the number of thread messages in the parent message
  2. We remove unread from the subscription of the deleted message, and, if some message was updated, we notify the client to refresh that data.
  3. If the number of thread messages of the parent message is 0, we also notify the client to update the parent message, so we avoid showing that there are 1 thread message even if it was deleted.

Issue(s)

Steps to test or reproduce

You are going to need two users, let's call them user A and B, they both must be at the same room for you to be able to test.

  1. As user A send a message in the room
  2. As user B add a thread message in the room
  3. As user B remove the thread message
  4. Click on the user A browser so the websocket update takes place

Expected:

  • The thread should show the "View thread" button, and it show there is 0 replies, and there should be no visual representation of an unread message

Further comments

CONN-261

Now, everytime a message has a tmid(is a thread), and it gets deleted, we decrease the replyCount of the parent message
then remove the unread thread unread, to make sure all the subscriptions are up to date (and notify to the client)
At the end, we check if the parent message had all of its thread messages deleted, if so, we notify the change in the parent message itself
Copy link
Contributor

dionisio-bot bot commented Dec 11, 2024

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented Dec 11, 2024

🦋 Changeset detected

Latest commit: 09754f6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 35 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/network-broker Patch
@rocket.chat/models Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/ui-voip Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Gustrb Gustrb added this to the 7.2.0 milestone Dec 11, 2024
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.27%. Comparing base (ff04c19) to head (09754f6).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop   #34165       +/-   ##
============================================
+ Coverage    59.28%   75.27%   +15.99%     
============================================
  Files         2820      516     -2304     
  Lines        67932    22540    -45392     
  Branches     15102     5485     -9617     
============================================
- Hits         40275    16968    -23307     
+ Misses       24831     4911    -19920     
+ Partials      2826      661     -2165     
Flag Coverage Δ
e2e ?
e2e-api ?
unit 75.27% <ø> (ø)

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

Copy link
Contributor

github-actions bot commented Dec 11, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://RocketChat.github.io/Rocket.Chat/pr-preview/pr-34165/
on branch gh-pages at 2024-12-20 20:46 UTC

@Gustrb Gustrb requested a review from a team as a code owner December 12, 2024 13:30
.changeset/ninety-bulldogs-dream.md Outdated Show resolved Hide resolved
.changeset/ninety-bulldogs-dream.md Outdated Show resolved Hide resolved
apps/meteor/app/lib/server/functions/deleteMessage.ts Outdated Show resolved Hide resolved
apps/meteor/app/lib/server/functions/deleteMessage.ts Outdated Show resolved Hide resolved
apps/meteor/app/lib/server/functions/deleteMessage.ts Outdated Show resolved Hide resolved
apps/meteor/app/lib/server/functions/deleteMessage.ts Outdated Show resolved Hide resolved
apps/meteor/app/lib/server/functions/deleteMessage.ts Outdated Show resolved Hide resolved
apps/meteor/app/lib/server/functions/deleteMessage.ts Outdated Show resolved Hide resolved
@Gustrb Gustrb requested a review from a team as a code owner December 13, 2024 14:22
@Gustrb Gustrb added the stat: QA assured Means it has been tested and approved by a company insider label Dec 19, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Dec 19, 2024
@Gustrb Gustrb removed the stat: QA assured Means it has been tested and approved by a company insider label Dec 19, 2024
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Dec 19, 2024
@Gustrb Gustrb added stat: ready to merge PR tested and approved waiting for merge stat: QA assured Means it has been tested and approved by a company insider labels Dec 19, 2024
@Gustrb Gustrb removed stat: ready to merge PR tested and approved waiting for merge stat: QA assured Means it has been tested and approved by a company insider labels Dec 20, 2024
@Gustrb Gustrb added the stat: QA assured Means it has been tested and approved by a company insider label Dec 20, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Dec 20, 2024
@kodiakhq kodiakhq bot merged commit c84543f into develop Dec 20, 2024
49 checks passed
@kodiakhq kodiakhq bot deleted the fix/not-notifying-on-thread-message-removal branch December 20, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants