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 6 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
12 changes: 3 additions & 9 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 @@ -32,13 +32,7 @@ function findChannelByIdOrName({
checkedArchived = true,
userId,
}: {
params:
| {
roomId: string;
}
| {
roomName: string;
};
params: { roomId?: string; roomName?: string };
userId?: string;
checkedArchived?: boolean;
}): IRoom {
Expand Down Expand Up @@ -471,8 +465,8 @@ API.v1.addRoute(

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

@debdutdeb debdutdeb Sep 13, 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.

Suggested change
...(channelId && { roomId: channelId }),
...(channelName && { roomName: channelName }),
roomId: channelId,
roomName: channelName,

The function will have to check each field any way. No need to do it again here.

@matheusbsilva137 matheusbsilva137 Sep 15, 2022 •
edited
Loading

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.

This endpoint is not working when only the channelName param is provided, so I added this change to fix it. Without these changes, the validation 'roomId' in params in findChannelByIdOrName would always return true.
Another way to fix it would be to replace 'roomId' in params by params.roomId, but this would cause some typing issues in this method 🤔
Still, it's better to change the findChannelByIdOrName method instead, though

},
userId: this.userId,
});
Expand Down
57 changes: 56 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,61 @@ describe('[Channels]', function () {
});
});

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(() => {

@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 })
.expect(200)
.expect((res) => {
expect(res.body).to.have.a.property('success', true);
})
.end(done);
});
});
});
});

it(`should successfully convert a channel to a team when the channel's name and id are 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,
channelId: this.newChannel._id,
})
.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
9 changes: 9 additions & 0 deletions packages/rest-typings/src/v1/channels/ChannelsConvertToTeamProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,15 @@ const channelsConvertToTeamPropsSchema = {
required: ['channelName'],
additionalProperties: false,
},
{
type: 'object',
properties: {
channelId: { type: 'string' },
channelName: { type: 'string' },
},
required: ['channelId', 'channelName'],
additionalProperties: false,
},
Comment thread
matheusbsilva137 marked this conversation as resolved.
Outdated
],
};

Expand Down