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: Omnichannel Visitor Abandonment feature not working for apps #29705

Closed
wants to merge 4 commits into from

Conversation

murtaza98
Copy link
Contributor

@murtaza98 murtaza98 commented Jul 3, 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 issue, I may have updated the app's converters to not override lastMessageTs. However I did not go that route since I didn't find any significant reference to room.v.lastMessageTs - hence I felt there was no need to store and maintain this property. There's only one reference where this property was being used and I've updated it to query the database direct, and get the lastMessage timestamp from source.

Issue(s)

https://rocketchat.atlassian.net/browse/SUP-66

Steps to test or reproduce

Further comments

@changeset-bot
Copy link

changeset-bot bot commented Jul 3, 2023

⚠️ No Changeset found

Latest commit: bbffd03

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@murtaza98 murtaza98 marked this pull request as ready for review July 3, 2023 13:54
@murtaza98 murtaza98 requested review from a team as code owners July 3, 2023 13:54
Comment on lines +1655 to +1656
rid,
token: { $exists: true },
Copy link
Contributor

Choose a reason for hiding this comment

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

:hmmm: not sure if we need a rid + token index for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I believe we'll need this index. However considering this is Messages collection which is always growing rapidly, I'd like to be cautious and not add this index because this is only needed for a single feature and majority of the application would not use this index. So let me try exploring other ways we can fix this issue

@KevLehman KevLehman added this to the 6.3.0 milestone Jul 4, 2023
@murtaza98 murtaza98 marked this pull request as draft July 5, 2023 06:14
@murtaza98
Copy link
Contributor Author

Closing this in favour of #29724 . The approach which I was taking here would require adding an sparingly used index on Messages collection which is likely the biggest collection we have on DB. This index would not be used by majority of the application, rather it would be used for for a single visitor abandonment feature. Hence I've tried to solve the underlying issue with a different approach on the new PR which involves preventing app's converters to override the lastMessageTs field

@murtaza98 murtaza98 closed this Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants