fix: Deprecated filter by query in emoji-custom.all api call

[tiagoevanp]

Proposed changes (including videos or screenshots)

Fixes search by name in custom emojis list, by adding a correct parameter to the endpoint emoji-custom.all

Issue(s)

Steps to test or reproduce

Further comments

CORE-1318

[dionisio-bot]

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

• 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

[changeset-bot]

🦋 Changeset detected

Latest commit: f2be5b7

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

This PR includes changesets to release 39 packages

Name	Type
@rocket.chat/meteor	Minor
@rocket.chat/gazzodown	Major
@rocket.chat/rest-typings	Minor
@rocket.chat/fuselage-ui-kit	Major
@rocket.chat/livechat	Patch
@rocket.chat/api-client	Patch
@rocket.chat/core-services	Patch
@rocket.chat/ddp-client	Patch
@rocket.chat/http-router	Patch
@rocket.chat/models	Patch
@rocket.chat/ui-contexts	Major
@rocket.chat/web-ui-registration	Major
@rocket.chat/account-service	Patch
@rocket.chat/authorization-service	Patch
@rocket.chat/ddp-streamer	Patch
@rocket.chat/stream-hub-service	Patch
@rocket.chat/omnichannel-services	Patch
@rocket.chat/presence	Patch
rocketchat-services	Patch
@rocket.chat/uikit-playground	Patch
@rocket.chat/omnichannel-transcript	Patch
@rocket.chat/presence-service	Patch
@rocket.chat/queue-worker	Patch
@rocket.chat/network-broker	Patch
@rocket.chat/mock-providers	Patch
@rocket.chat/cron	Patch
@rocket.chat/instance-status	Patch
@rocket.chat/omni-core	Patch
@rocket.chat/omni-core-ee	Patch
@rocket.chat/ui-avatar	Major
@rocket.chat/ui-client	Major
@rocket.chat/ui-video-conf	Major
@rocket.chat/ui-voip	Major
@rocket.chat/core-typings	Minor
@rocket.chat/apps	Patch
@rocket.chat/freeswitch	Patch
@rocket.chat/model-typings	Patch
@rocket.chat/license	Patch
@rocket.chat/pdf-worker	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]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.02%. Comparing base (b450f81) to head (f2be5b7).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #36723      +/-   ##
===========================================
+ Coverage    65.97%   66.02%   +0.04%     
===========================================
  Files         3264     3268       +4     
  Lines       109712   109844     +132     
  Branches     20758    20801      +43     
===========================================
+ Hits         72385    72523     +138     
+ Misses       34672    34654      -18     
- Partials      2655     2667      +12     

Flag	Coverage Δ
e2e	57.30% <ø> (+0.16%)	⬆️
unit	71.36% <100.00%> (+0.01%)	⬆️

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sampaiodiego]

it is missing api tests.. pls also create another changeset as minor to document we now accept a new query param on the emoji-custom.list endpoint

[dougfabris]

I didn't understood how this fixes the issue attached, mainly because I'm not being able to reproduce it and the logs are pointing to somewhere else

[tiagoevanp]

I didn't understood how this fixes the issue attached, mainly because I'm not being able to reproduce it and the logs are pointing to somewhere else

I left a comment in the issue attached, this is not a fix for the problem indeed.

I’m not able to reproduce the error after tons of trials, I will let here a PR with a test file just to guarantee this will not happening again.

Tests are covering this situation

This PR started as a test: but ended as a fix:

[dougfabris]

Ok, so you can detach the task then. I will evaluate the tests. But I'm questioning if this should be a fix. For me you removed the query params in favor of name, so query was unnecessary but wasn't breaking anything. was it?

[tiagoevanp]

If I detach the task, task will not have the information of a PR adding tests to watch the intermittent problematic situation. Do you prefere that I introduce PR as a comment in task? So we should cancel that one and keep looking at the tests results.

I did not removed query, I added another parameter, name to avoid breaking changes. In future breaking changes release, the deprecated way (with query) must be removed.

[dougfabris]

If I detach the task, task will not have the information of a PR adding tests to watch the intermittent problematic situation. Do you prefere that I introduce PR as a comment in task? So we should cancel that one and keep looking at the tests results.

The way you're attaching as soon it gets merged it will close the issue as done. So yes, its better to add the PR as comment and cancel saying its not reproducible anymore and some tests were added

I did not removed query, I added another parameter, name to avoid breaking changes. In future breaking changes release, the deprecated way (with query) must be removed.

So if we're deprecating, would be nice to have a deprecation warning

[tiagoevanp]

I did not removed query, I added another parameter, name to avoid breaking changes. In future breaking changes release, the deprecated way (with query) must be removed.

So if we're deprecating, would be nice to have a deprecation warning

There is a deprecation warning already: https://github.com/RocketChat/Rocket.Chat/blob/develop/apps/meteor/app/api/server/definition.ts#L193

[dougfabris]

LGTM!