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

Implementation and Tests for Sorting Pinned Messages

Open CaioPacheco opened this issue 7 months ago • 4 comments

Proposed changes (including videos or screenshots)

Description of the changes made:

  • Added functionality to sort pinned messages in the GET /api/v1/chat.getPinnedMessages endpoint.
  • Implemented the sort parameter to sort messages based on the timestamp (ts) field.
  • Added automated tests to ensure that messages are correctly sorted based on the sort parameter.
  • Refactored code for better clarity and readability.

This change allows the /api/v1/chat.getPinnedMessages query to sort pinned messages according to the sort parameter value. Messages can be sorted by timestamp either in ascending or descending order depending on the parameter's value.

Issue(s)

This PR resolves the following issue:

  • Adds the ability to sort pinned messages in the chat.getPinnedMessages endpoint based on the sort parameter.

If this PR addresses a specific issue or bug, reference the issue number here:

  • Fixes the bug related to sorting pinned messages (#36087 ).

Steps to test or reproduce

To test the functionality, follow these steps:

  1. Send a GET request to the endpoint /api/v1/chat.getPinnedMessages?roomId=room123&sort={"ts":-1}.
  2. Check that the response contains messages sorted by the ts (timestamp) field in descending order (most recent first).
  3. Also verify that if the sort parameter is invalid, the error-invalid-sort error is returned.
  4. Automated tests can also be run with the command yarn mocha --require ts-node/register apps/meteor/app/api/server/test/**/*.test.ts.

Make sure sorting works correctly with other sort parameter values (e.g., {"ts":1} for ascending order).

Further comments

The solution was chosen based on the need to allow sorting of pinned messages, which will improve the user experience when querying pinned messages in channels. Considerations:

  • The sort parameter was validated to ensure only valid JSON values are accepted.
  • Other values may be added in the future to sort by other fields. Alternatives considered:
  • Using more complex database aggregations for sorting, but for simplicity and flexibility, the current implementation meets the use case.

CaioPacheco avatar Jul 11 '25 00:07 CaioPacheco

⚠️ No Changeset found

Latest commit: 8c2c48fbdcd45b383beaeb6cb7d0ad55acc61921

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

changeset-bot[bot] avatar Jul 11 '25 00:07 changeset-bot[bot]

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

  • This PR has conflicts, please resolve them before merging
  • This PR is missing the 'stat: QA assured' label
  • This PR is not mergeable
  • This PR is missing the required milestone or project
  • This PR has an invalid title

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

dionisio-bot[bot] avatar Jul 11 '25 00:07 dionisio-bot[bot]

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

:x: Juhvitoria4
:x: CaioPacheco
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Jul 11 '25 00:07 CLAassistant

Note: This review was focused on code quality, consistency, and adherence to existing codebase standards rather than technical implementation details.

jessicaschelly avatar Jul 11 '25 20:07 jessicaschelly