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 14 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
58 changes: 57 additions & 1 deletion 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 @@ -1922,7 +1922,7 @@ describe('[Channels]', function () {
});
});

it('should successfully convert a channel to a team', (done) => {
it(`should successfully convert a channel to a team when the channel's id is sent as parameter`, (done) => {
updatePermission('create-team', ['admin']).then(() => {
updatePermission('edit-room', ['admin']).then(() => {
request
Expand All @@ -1938,6 +1938,62 @@ describe('[Channels]', function () {
});
});

it(`should return an error when the channel's name and id are sent as parameter`, (done) => {
updatePermission('create-team', ['admin']).then(() => {
updatePermission('edit-room', ['admin']).then(() => {

@sampaiodiego sampaiodiego Sep 21, 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.

there is no need to update permissions.. looking the previous test, this is the current state already.

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);
})
.then(() => {
request

@sampaiodiego sampaiodiego Sep 21, 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.

this Promise nesting and the previous one with the permissions update is wrong in so many levels 🙈 I see this has been the "standard" on this particular file, but we shouldn't propagate bad code even if it is the current "standard"..

this is conceptually wrong since the Promise API was designed to actually mitigate callback hell.. so a correct usage of the Promise API should look something like:

updatePermission()
    .then(() => updatePermission())
    .then(() => request
        .post(api('teams.convertToChannel'))
        .set(credentials)
        // ....
    ).then(() => request
        .post(api('teams.convertToTeam'))
        .set(credentials)
        // ....
    )

as you can see, everything is done in a single level of nesting.

BUT, you should also not do this, since async/await is available for tests and it is the recommended way to write sequential requests, so please refactor at least the new tests to use async/await.

.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 name is sent as parameter`, (done) => {
updatePermission('create-team', ['admin']).then(() => {
updatePermission('edit-room', ['admin']).then(() => {
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);
})
.then(() => {
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);
})
.end(done);
});
});
});
});

it('should fail to convert channel without the required parameters', (done) => {
request.post(api('channels.convertToTeam')).set(credentials).send({}).expect(400).end(done);
});
Expand Down