Skip to content

[FIX] channels.convertToTeam endpoint doesn't work when only the channelName param is provided #26858

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.

Sign up for GitHub

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

Merged
ggazzo merged 20 commits into from
Nov 28, 2022
Merged

[FIX] channels.convertToTeam endpoint doesn't work when only the channelName param is provided #26858

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
811a951
Support channelId and channelName params in channels.convertToTeam en…
matheusbsilva137 Sep 6, 2022
1d13bd4
Add tests
matheusbsilva137 Sep 12, 2022
09f0b1b
Merge branch 'develop' of https://github.com/RocketChat/Rocket.Chat i…
matheusbsilva137 Sep 12, 2022
105444f
Fix tests
matheusbsilva137 Sep 12, 2022
f8997ad
Merge branch 'develop' into fix/channels-convert-to-team-types
matheusbsilva137 Sep 13, 2022
0bf369b
Fix findChannelByIdOrName params type
matheusbsilva137 Sep 13, 2022
fb24acf
Merge branch 'develop' into fix/channels-convert-to-team-types
matheusbsilva137 Sep 13, 2022
2133c0f
Merge branch 'develop' into fix/channels-convert-to-team-types
matheusbsilva137 Sep 14, 2022
d7df939
Do not allow channel's name and id both to be sent as parameters
matheusbsilva137 Sep 15, 2022
0f1ddd6
Merge branch 'fix/channels-convert-to-team-types' of https://github.c…
matheusbsilva137 Sep 15, 2022
35605b4
Fix typing issue in findChannelByIdOrName call
matheusbsilva137 Sep 16, 2022
1879101
Merge branch 'develop' of https://github.com/RocketChat/Rocket.Chat i…
matheusbsilva137 Sep 16, 2022
1e14d94
Merge branch 'develop' of https://github.com/RocketChat/Rocket.Chat i…
matheusbsilva137 Sep 21, 2022
a52871d
Fix team name already taken test
matheusbsilva137 Sep 21, 2022
36210b0
Remove unnecessary permission updates
matheusbsilva137 Sep 21, 2022
971f37c
Improve request nesting
matheusbsilva137 Sep 21, 2022
80fa434
Fix tests order
matheusbsilva137 Sep 21, 2022
aaebe90
Use async/await in new tests
matheusbsilva137 Sep 21, 2022
7400c8b
yarn.lock from develop
sampaiodiego Nov 1, 2022
ad9a658
Merge branch 'develop' into fix/channels-convert-to-team-types
sampaiodiego Nov 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions apps/meteor/app/api/server/v1/channels.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,8 +471,7 @@ API.v1.addRoute(

const room = findChannelByIdOrName({
params: {
roomId: channelId,
roomName: channelName,
...(channelId ? { roomId: channelId } : { roomName: channelName }),

@debdutdeb debdutdeb Sep 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have to do it like this?

In this particular context, roomId in params is the same as params.roomId ? (the purpose of it) - so no matter what you do in the calling function, the called still has to check for both values any way.

This conditional and destructuring just seems useless at that point. And IMO is bad. We should keep the code simple. And inspire others (people who are gonna read your code) to do so too.

on the called, you can simply change the param type from the union to a simple all-partial and chain the truthness checks instead of membership checks as it was before roomId ? .. : roomName ? ... : undefined

it's late for me, hopefully i was able to explain myself.

@matheusbsilva137 matheusbsilva137 Sep 21, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey Debdut, I'll first explain what was the issue on the way we were calling this function (that is, with params: { roomId: channelId, roomName: channelName }): this would cause one of the fields in the params object to be undefined. So checking 'roomId' in params in findChannelByIdOrName would always return true (since this field is in the object, even though its value is undefined).

Still, I see we could change the params type to params: { roomId?: string; roomName?: string } and check for params.roomId instead of 'roomId' in params (is that what you mean?). If we do so, I think we should also bring back the error check we had before (so as to throw an error when both params are undefined).

@debdutdeb debdutdeb Sep 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

throw an error when both params are undefined

Already done by the route function. So, you can safely ignore that part.

@debdutdeb debdutdeb Sep 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rocket.Chat/apps/meteor/app/api/server/v1/channels.ts

Lines 464 to 466 in aaebe90

if (!channelId && !channelName) {
return API.v1.failure('The parameter "channelId" or "channelName" is required');
}

},
userId: this.userId,
});
Expand Down
108 changes: 69 additions & 39 deletions apps/meteor/tests/end-to-end/api/02-channels.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1890,52 +1890,82 @@ describe('[Channels]', function () {
.then(() => done());
});

it('should fail to convert channel if lacking edit-room permission', (done) => {
updatePermission('create-team', []).then(() => {
updatePermission('edit-room', ['admin']).then(() => {
request
.post(api('channels.convertToTeam'))
.set(credentials)
.send({ channelId: this.newChannel._id })
.expect(403)
.expect((res) => {
expect(res.body).to.have.a.property('success', false);
})
.end(done);
it('should fail to convert channel if lacking edit-room permission', async () => {
await updatePermission('create-team', []);
await updatePermission('edit-room', ['admin']);

await request
.post(api('channels.convertToTeam'))
.set(credentials)
.send({ channelId: this.newChannel._id })
.expect(403)
.expect((res) => {
expect(res.body).to.have.a.property('success', false);
});
});
});

it('should fail to convert channel if lacking create-team permission', (done) => {
updatePermission('create-team', ['admin']).then(() => {
updatePermission('edit-room', []).then(() => {
request
.post(api('channels.convertToTeam'))
.set(credentials)
.send({ channelId: this.newChannel._id })
.expect(403)
.expect((res) => {
expect(res.body).to.have.a.property('success', false);
})
.end(done);
it('should fail to convert channel if lacking create-team permission', async () => {
await updatePermission('create-team', ['admin']);
await updatePermission('edit-room', []);

await request
.post(api('channels.convertToTeam'))
.set(credentials)
.send({ channelId: this.newChannel._id })
.expect(403)
.expect((res) => {
expect(res.body).to.have.a.property('success', false);
});
});
});

it('should successfully convert a channel to a team', (done) => {
updatePermission('create-team', ['admin']).then(() => {
updatePermission('edit-room', ['admin']).then(() => {
request
.post(api('channels.convertToTeam'))
.set(credentials)
.send({ channelId: this.newChannel._id })
.expect(200)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
})
.end(done);
it(`should return an error when the channel's name and id are sent as parameter`, (done) => {
request
.post(api('channels.convertToTeam'))
.set(credentials)
.send({
channelName: this.newChannel.name,
channelId: this.newChannel._id,
})
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body).to.have.property('error').include(`must match exactly one schema in oneOf`);
})
.end(done);
});

it(`should successfully convert a channel to a team when the channel's id is sent as parameter`, async () => {
await updatePermission('create-team', ['admin']);
await updatePermission('edit-room', ['admin']);

await request
.post(api('channels.convertToTeam'))
.set(credentials)
.send({ channelId: this.newChannel._id })
.expect(200)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
});
});

it(`should successfully convert a channel to a team when the channel's name is sent as parameter`, async () => {
await request
.post(api('teams.convertToChannel'))
.set(credentials)
.send({ teamName: this.newChannel.name })
.expect(200)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
});

await request
.post(api('channels.convertToTeam'))
.set(credentials)
.send({ channelName: this.newChannel.name })
.expect(200)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
});
});
});

it('should fail to convert channel without the required parameters', (done) => {
Expand Down
4 changes: 2 additions & 2 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24433,7 +24433,7 @@ __metadata:
optional: true
bin:
lessc: ./bin/lessc
checksum: 61568b56b5289fdcfe3d51baf3c13e7db7140022c0a37ef0ae343169f0de927a4b4f4272bc10c20101796e8ee79e934e024051321bba93b3ae071f734309bd98
checksum: c9b8c0e865427112c48a9cac36f14964e130577743c29d56a6d93b5812b70846b04ccaa364acf1e8d75cee3855215ec0a2d8d9de569c80e774f10b6245f39b7d
languageName: node
linkType: hard

Expand Down Expand Up @@ -36132,7 +36132,7 @@ __metadata:
languageName: node
linkType: hard

"vm2@npm:^3.9.11":
"vm2@npm:^3.9.10":
version: 3.9.11
resolution: "vm2@npm:3.9.11"
dependencies:
Expand Down