Skip to content

Conversation

@KevLehman
Copy link
Member

Proposed changes (including videos or screenshots)

  • Fixed canAccessRoom validation
  • Added e2e tests
  • Removed channels that user cannot add to the team from autocomplete suggestions
  • Improved error messages

Issue(s)

Steps to test or reproduce

Further comments


if (!hasPermission(this.userId, 'add-team-channel', team.roomId)) {
return API.v1.unauthorized();
return API.v1.unauthorized('Teams_add_channel_no_permission');
Copy link
Member

Choose a reason for hiding this comment

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

please use an error code instead.. something like error-no-permission-add-channel

looks like we need to standardize these errors 🤔

Copy link
Member Author

@KevLehman KevLehman Apr 23, 2021

Choose a reason for hiding this comment

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

Ah, didn't know the - were meant to be error codes, I'll change it.

},
],
name: nameRegex,
'u._id': uid,
Copy link
Member

Choose a reason for hiding this comment

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

I know we used this filter on the other validation as well, but I don't think this is the correct way to validate what we need. this field actually stores the "creator" of the channel, which is formerly know as the owner, but we also have a owner role, which I think is more appropriate. Like when a user transfer the ownership of a channel we don't update the u field, we just give the new user the owner role.

the role is stored on Subscriptions, so here you could rely that filtered list of _ids where the user has the owner role..

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I know about the owner role of the subs, I was trying to keep both filters the same (the one here and the one in the addRooms service.

If checking the subs is the way to go, I'll change this and the other :)

Copy link
Member

Choose a reason for hiding this comment

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

yep, I think that is the expected behavior.. I didn't realize that before 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, you're right on that 👀 we should've modified that validation before 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

@sampaiodiego what should be the behavior of the autocomplete then? Currently, it fetches all public channels and private channels the user is part of. Should this be changed to only show channels where the user is the owner (disregard if it's public/private)?

Copy link
Member

Choose a reason for hiding this comment

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

Yep. I think so :)

@KevLehman KevLehman requested a review from sampaiodiego April 27, 2021 16:00
@sampaiodiego sampaiodiego merged commit 1ee1dd9 into develop Apr 27, 2021
@sampaiodiego sampaiodiego deleted the qol/teams-add-channel branch April 27, 2021 18:01
@sampaiodiego sampaiodiego mentioned this pull request Apr 28, 2021
vanhoang1107 pushed a commit to vanhoang1107/Rocket.Chat that referenced this pull request May 13, 2021
* rocketchat/master: (273 commits)
  Bump version to 3.14.0
  Bump version to 3.14.0-rc.4
  bump fuselage (RocketChat#21841)
  [FIX] Duplicated header on admin's user contextualbar (RocketChat#21810)
  Bump version to 3.14.0-rc.3
  Bump Apps-Engine version (RocketChat#21840)
  [FIX][Enterprise] Omnichannel simultaneous chat limit is not properly checking the limit by department (RocketChat#21839)
  Fix node_modules cache path again
  Regression: Reactivate direct conversations only if all involved users are active (RocketChat#21714)
  Bump version to 3.14.0-rc.2
  [FIX] Omnichannel Activity Monitor closing chats returned to the queue (RocketChat#21782)
  Fix node_modules cache path
  Regression: Problem with Importer's logs (RocketChat#21812)
  Chore: Add tests for teams.update REST endpoint (RocketChat#21653)
  QoL improvements to add channel to team flow (RocketChat#21778)
  Chore: Cache EE node_modules on CI (RocketChat#21831)
  Regression: team sync not accepting multiple teams (RocketChat#21768)
  regression: Italic being parsed with surrounding non-whitespace text (RocketChat#21815)
  Regression: Unread Threads Header and List (RocketChat#21816)
  Fix attachment previews. (RocketChat#21746)
  ...
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.

3 participants