Skip to content

fix: "Reply in direct message" should always appear if you have create-d permission#36978

Open
nmagedman wants to merge 1 commit intoRocketChat:developfrom
nmagedman:noach_seekingalpha_AR-3664_show_Reply_in_direct_message
Open

fix: "Reply in direct message" should always appear if you have create-d permission#36978
nmagedman wants to merge 1 commit intoRocketChat:developfrom
nmagedman:noach_seekingalpha_AR-3664_show_Reply_in_direct_message

Conversation

@nmagedman
Copy link
Copy Markdown
Contributor

@nmagedman nmagedman commented Sep 18, 2025

Proposed changes (including videos or screenshots)

When PR #36217 refactored useReplyInDMAction, it introduced a regression. We previously checked for !canCreateDM (with a bang!) but that PR accidentally negated it to canCreateDM.

Additionally, the conditional that includes canCreateDM was duplicated. DRY it out.

Steps to test or reproduce

  • Log in as a user who has create-d permission. (By default, the user role does have this permission.)
  • Enter a group room that has a message posted by another user with whom you have not already started a DM conversation
  • Click 3-dots on the popup toolbar for that message

Expected Result:
The first menu item should be "Reply in direct message". Clicking that should create a new DM room.

Actual Result:
The "Reply in direct message" menu item is not shown.

…e-d permission

When PR RocketChat#36217 refactored useReplyInDMAction, it introduced a regression.
We previously checked for `!canCreateDM` (with a bang!) but that PR accidentally
negated it to `canCreateDM`.

Additionally, the conditional that includes `canCreateDM` was duplicated.
DRY it out.
@nmagedman nmagedman requested a review from a team as a code owner September 18, 2025 12:57
@dionisio-bot
Copy link
Copy Markdown
Contributor

dionisio-bot bot commented Sep 18, 2025

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

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Sep 18, 2025

⚠️ No Changeset found

Latest commit: 5c0a0ba

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

@nmagedman
Copy link
Copy Markdown
Contributor Author

@tassoevan @juliajforesti Please comment

Copy link
Copy Markdown
Contributor

@juliajforesti juliajforesti left a comment

Choose a reason for hiding this comment

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

Hey @nmagedman ! Thanks for your contribution.
But I don't think the behavior is 100% yet - now even if I remove the permission to create dm from user role, Reply in direct message still appears, can you please verify?

@nmagedman
Copy link
Copy Markdown
Contributor Author

Hey @nmagedman ! Thanks for your contribution. But I don't think the behavior is 100% yet - now even if I remove the permission to create dm from user role, Reply in direct message still appears, can you please verify?

Yes, @juliajforesti, you are correct; there are still problems with the logic for users without create-d permission. Since I don't understand the optimizations that were implemented in 36217, I will leave it to its original authors to decide the best solution. Consider this PR an issue having the repro instruction listed in the OP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants