Rocket.Chat icon indicating copy to clipboard operation
Rocket.Chat copied to clipboard

feat(Omnichannel): System messages in transcripts

Open yash-rajpal opened this issue 1 year ago β€’ 3 comments

Proposed changes (including videos or screenshots)

image

Issue(s)

Steps to test or reproduce

Further comments

CORE-494

yash-rajpal avatar Jul 10 '24 04:07 yash-rajpal

πŸ¦‹ Changeset detected

Latest commit: aeabfa96870ec92d1bb44b7fe14d11e6df7ef2af

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

This PR includes changesets to release 34 packages
Name Type
@rocket.chat/omnichannel-services Minor
@rocket.chat/pdf-worker Minor
@rocket.chat/core-services Minor
@rocket.chat/model-typings Minor
@rocket.chat/i18n Minor
@rocket.chat/meteor Minor
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/queue-worker Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/presence-service Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/apps Patch
@rocket.chat/models Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-contexts Major
@rocket.chat/web-ui-registration Major
@rocket.chat/cron Patch
@rocket.chat/instance-status Patch
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/ui-client Major
@rocket.chat/gazzodown Major
@rocket.chat/livechat Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-video-conf Major
@rocket.chat/uikit-playground Patch
@rocket.chat/core-typings Minor
@rocket.chat/rest-typings Minor
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/ddp-client 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

changeset-bot[bot] avatar Jul 10 '24 04:07 changeset-bot[bot]

Looks like this PR is ready to merge! πŸŽ‰ If you have any trouble, please check the PR guidelines

dionisio-bot[bot] avatar Jul 10 '24 04:07 dionisio-bot[bot]

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 55.47%. Comparing base (b4bbcbf) to head (aeabfa9). Report is 2 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop   #32752   +/-   ##
========================================
  Coverage    55.46%   55.47%           
========================================
  Files         2635     2635           
  Lines        57343    57343           
  Branches     11874    11876    +2     
========================================
+ Hits         31807    31811    +4     
- Misses       22842    22853   +11     
+ Partials      2694     2679   -15     
Flag Coverage Ξ”
e2e 54.02% <ΓΈ> (-0.03%) :arrow_down:
unit 72.27% <33.33%> (+0.06%) :arrow_up:

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

codecov[bot] avatar Jul 10 '24 05:07 codecov[bot]

As your pr has changes related with performance, I don't think we need a service to deal with translations, I think everything could be moved to a lib and all services could use it, that would be much faster. what do you think?

ggazzo avatar Jul 20 '24 00:07 ggazzo

@ggazzo Do you mean moving all i18n logic to the package itself, so all packages can use?

If so, then that's a great improvement and we should definitely do that, but I think it is outside the scope of this PR and should be handled separately. Currently this PR is important for this release. Maybe this can be a future overall improvement. What do you think?

yash-rajpal avatar Jul 22 '24 11:07 yash-rajpal

@ggazzo Do you mean moving all i18n logic to the package itself, so all packages can use?

yep, its not necessarily huge code. I imagine this service was created before we removed meteor dependencies, nowadays, it would make much more sense to use this as a lib

but I think it is outside the scope of this PR and should be handled separately.

you just explained exactly why I would keep your performance part in another PR, separate from the feature ;)

ggazzo avatar Jul 22 '24 12:07 ggazzo

Vote 3 for removing service. It was created when translations were handled by meteor. Now that the files are free of meteor we could move the actual i18n thing onto it.

Also vote of doing that on a separate PR πŸ‘€

KevLehman avatar Jul 22 '24 15:07 KevLehman

you just explained exactly why I would keep your performance part in another PR, separate from the feature ;)

You are right, but earlier I implemented the feature without the performance part, but the implementation created performance issues as we were having to do a lot of i18n calls for each and every message, making entire process async (as getting translations was async). The performance part is just making this process synchronous using some tricks.

So, it is kinda needed, can't remove it for now to do in future.

But for sure this translation service should be removed and should be handled by a lib, when doing this we can improve transcripts as well. What do you think?

yash-rajpal avatar Jul 22 '24 16:07 yash-rajpal

This PR currently has a merge conflict. Please resolve this and then re-add the ['stat: ready to merge', 'automerge'] label.

kodiakhq[bot] avatar Jul 24 '24 00:07 kodiakhq[bot]