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: custom emoji update issues #32768

Closed
wants to merge 4 commits into from

Conversation

abhinavkrin
Copy link
Member

@abhinavkrin abhinavkrin commented Jul 11, 2024

Proposed changes (including videos or screenshots)

This PR addresses two main issues with custom emoji updates in Rocket.Chat:

  1. Issue with Alias Update:

    • Users were required to upload the image again when updating a custom emoji's alias or name, leading to an error if no file was uploaded.
    • Solution: Made the file optional for the emoji-custom.update endpoint.
  2. Issue with Image Update Not Reflecting:

    • The new image was indeed updated but not reflected in browser due to browser cache. The max-age for caching is set for one year, so even if the image changed (without changing the emoji name), the URL remained the same, causing the old emoji to be loaded from the cache.
    • Solution: Created an additional hash for the emoji each time its image is changed. While rendering, attached the hash as a query to the image URL.

Additionally, while fixing the first issue, the following bugs were discovered and fixed:

  • The aliases were not saved as an array. The aliases string submitted in the endpoint should have been split using a comma and saved as an array, but this was not happening. [Fixed]
  • The client threw an error on updating the emoji list in memory after receiving the updates from the server. [Fixed]
  • The aliases were not displayed properly in the custom emoji list. [Fixed]

Also, there was a minor issue with the test related to the first issue. The test was already there but was not being executed, so we never caught the bug as it always appeared to pass. [Fixed]

Before

The aliases are not saved and displayed properly. The emoji could not be updated without a new file

Screen.Recording.2024-07-12.at.2.24.58.PM.mov

Client threw error on receiving the emoji update data and failed to update the emoji list in memory
Screenshot 2024-07-12 at 2 26 20 PM

After

Screen.Recording.2024-07-12.at.3.29.42.PM.mov

Issue(s)

closes #26980

closes #20318
closes #21821

Closes #20317
Closes #20316

Steps to test or reproduce

Further comments

SUP-622

Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
Copy link
Contributor

dionisio-bot bot commented Jul 11, 2024

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented Jul 11, 2024

🦋 Changeset detected

Latest commit: b5c984d

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

This PR includes changesets to release 32 packages
Name Type
@rocket.chat/model-typings Patch
@rocket.chat/core-typings Patch
@rocket.chat/meteor Patch
@rocket.chat/apps Patch
@rocket.chat/models 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/omnichannel-services Patch
rocketchat-services Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/rest-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
@rocket.chat/instance-status Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/ddp-client Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/web-ui-registration 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

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 56.67%. Comparing base (0e59a15) to head (b5c984d).
Report is 10 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #32768      +/-   ##
===========================================
- Coverage    56.83%   56.67%   -0.17%     
===========================================
  Files         2501     2504       +3     
  Lines        55404    55536     +132     
  Branches     11408    11444      +36     
===========================================
- Hits         31490    31475      -15     
- Misses       21243    21380     +137     
- Partials      2671     2681      +10     
Flag Coverage Δ
e2e 56.48% <40.00%> (-0.06%) ⬇️
unit 72.21% <ø> (+<0.01%) ⬆️

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

Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
@abhinavkrin abhinavkrin marked this pull request as ready for review July 12, 2024 12:36
@abhinavkrin abhinavkrin requested review from a team as code owners July 12, 2024 12:36
Copy link
Member

@MarcosSpessatto MarcosSpessatto left a comment

Choose a reason for hiding this comment

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

I saw you fixed multiple issues in this PR and also you moved/refactored/extracted some code from Meteor methods to pure functions.

I suggest we create a separate PR for each fix, I mean at least the not strictly related ones. Also, let's keep the refactor or any code extraction to a specific PR, otherwise, it's really hard to understand what indeed changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants