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

chore: Improve system messages for omni-visitor abandonment feature #29724

Merged
merged 20 commits into from
Jul 13, 2023

Conversation

murtaza98
Copy link
Contributor

@murtaza98 murtaza98 commented Jul 5, 2023

Proposed changes (including videos or screenshots)

Visitor abandonment feature used to rely on room.v.lastMessageTs property to calculate the predicted time when the chat should be considered abandoned. Now when apps were updating the room properties like customFields, then the app converters were overriding lastMessageTs property ( reference ), thus making this abandonment feature not work for rooms created from apps.
So to solve this, I've moved the lastMessageTs prop out of room.v, and instead I'm storing it as room.contactLastMessageTs. This will prevent the app's engine to override this prop, thus fixing the main issue due to which app's room were not being abandoned automatically.

Update: This PR has been updated to now only include changes to system messages related to visitor abandonment feature. The rest of the changes wrt visitor abandonment feature not working has been moved to a separate PR here -> #29756

Issue(s)

Steps to test or reproduce

Further comments

@changeset-bot
Copy link

changeset-bot bot commented Jul 5, 2023

🦋 Changeset detected

Latest commit: 184a31c

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

This PR includes changesets to release 24 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/core-services Patch
@rocket.chat/cron 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/api-client Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/models Patch
@rocket.chat/uikit-playground 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

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #29724 (184a31c) into develop (1246a21) will increase coverage by 1.45%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #29724      +/-   ##
===========================================
+ Coverage    45.35%   46.81%   +1.45%     
===========================================
  Files          671      690      +19     
  Lines        12833    12981     +148     
  Branches      2236     2267      +31     
===========================================
+ Hits          5821     6077     +256     
+ Misses        6687     6572     -115     
- Partials       325      332       +7     
Flag Coverage Δ
e2e 46.79% <ø> (+1.45%) ⬆️

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

@murtaza98 murtaza98 marked this pull request as ready for review July 5, 2023 10:52
@murtaza98 murtaza98 requested review from a team as code owners July 5, 2023 10:52
Copy link
Contributor

@KevLehman KevLehman left a comment

Choose a reason for hiding this comment

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

What about compatibility with old rooms that have v.lastMessage? Since we're changing the property where we take info, we do need to make sure we keep compatibility with them.

Or we can create a single shot function that moves v.lastMessage to contactLastMessage

@murtaza98 murtaza98 requested a review from KevLehman July 5, 2023 14:59
KevLehman
KevLehman previously approved these changes Jul 5, 2023
KevLehman
KevLehman previously approved these changes Jul 6, 2023
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Jul 6, 2023
Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

if you were able to pin point the problem root (

), why not fixing the code only there? I mean, all these changes are just an alternative solution, but it doesn't fix the root cause of the problem, right?

ggazzo
ggazzo previously requested changes Jul 6, 2023
Copy link
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

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

Great explanation @murtaza98, but I would like to enforce: on patches keep your changes small as possible, if you are fixing something focus on the fix, if you are refactoring something try to refactor without change any behavior, if you are creating something new, avoid fixes and code changes that dont are directly related with the new feature

about your change in specific I think its only about change the bridge as you mentioned. that would be a great fix (2..3 lines of code)

if you are planning to replace some field or optimize any message let this to another pull request

@murtaza98 murtaza98 changed the title fix: Omnichannel Visitor Abandonment feature not working for apps refactor: Omnichannel Visitor Abandonment feature for apps Jul 7, 2023
@murtaza98 murtaza98 marked this pull request as draft July 7, 2023 07:05
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jul 11, 2023
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Jul 11, 2023
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Jul 12, 2023
@KevLehman KevLehman modified the milestones: 6.3.0, 6.4.0 Jul 12, 2023
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Jul 12, 2023
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Jul 12, 2023
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Jul 12, 2023
@kodiakhq kodiakhq bot merged commit 52a1aa9 into develop Jul 13, 2023
36 checks passed
@kodiakhq kodiakhq bot deleted the SUP-66_1 branch July 13, 2023 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants