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

[matheusbsilva137]

Proposed changes (including videos or screenshots)

• Fix the channels.convertToTeam endpoint not working when the channelName is sent as parameter.

Issue(s)

Steps to test or reproduce

Example request (using cURL):

curl 'http://localhost:3000/api/v1/channels.convertToTeam' \
  -H 'X-Auth-Token: GNNn5trk37eeK2sZvamOHRy1PMRlz_rfMUCwCE6UBlO' \
  -H 'X-User-Id: HZcEBodsm3gFxg2Jt' \
  --data-raw '{"channelName":"channel-example"}'

Further comments

TC-11

[codecov]

Codecov Report

Merging #26858 (ad9a658) into develop (daa6aae) will increase coverage by 0.84%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop   #26858      +/-   ##
===========================================
+ Coverage    40.37%   41.21%   +0.84%     
===========================================
  Files          827      802      -25     
  Lines        18246    17806     -440     
  Branches      2033     1971      -62     
===========================================
- Hits          7367     7339      -28     
+ Misses       10583    10174     -409     
+ Partials       296      293       -3     

Flag	Coverage Δ
e2e	41.21% <ø> (+0.84%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[debdutdeb]

I see the problem. All you need to do is instead of checking for membership, check for value truthness, i.e. params.roomId ? findbyid : findbyname

[debdutdeb]

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]

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

[sampaiodiego]

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

[sampaiodiego]

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.

[debdutdeb]

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]

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]

throw an error when both params are undefined

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

[debdutdeb]

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');
	}