feat(Omnichannel): System messages in transcripts
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
π¦ 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
Looks like this PR is ready to merge! π If you have any trouble, please check the PR guidelines
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
@@ 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.
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 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?
@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 ;)
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 π
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?
This PR currently has a merge conflict. Please resolve this and then re-add the ['stat: ready to merge', 'automerge'] label.