Consider current api user when updating members in conversation #58
Conversation
When a channel has been imported and the creator may not be the same as the one make the api calls with the token, two errors may occur: - The provider may try to kick the user from the conversation, which results in an error `couldn't kick user from conversation: cant_kick_self` - The provider tries to invite the user to the conversation, which results in an error `couldn't invite users to conversation: cant_invite` caused by the actual slack error `cant_invite_self` This addresses that problems by retrieving the user that is making the api calls using the `user_id` field from the auth.test endpoint, and removing the user from the list of users to operate with.
There was a problem hiding this comment.
Thank you very much for your contribution. I have some minor comments that don't need to be addressed, I will take care of them in future refactorings. There were some lint issues that I fixed, I hope that's fine with you.
In general I would not accept PR that do not have tests for the new functionality. But given that the change is straightforward and there is no easy way for you to run the tests at the moment I will accept it as it is and I will add test in the future myself.
The test need a Slack token that is set as a secret. Secrets are not passed to workflows that are triggered by a pull request from a fork so the PR checks will fail for your PR. I run the tests locally and they are still passing so we are good.
I have to think about how others can contribute, run the tests locally, and in the GitHub actions.
| if err != nil { | ||
| return fmt.Errorf("Error authenticating with slack %w", err) | ||
| } | ||
| userIds = remove(userIds, apiUserInfo.UserID) | ||
| userIds = remove(userIds, channel.Creator) |
There was a problem hiding this comment.
Some code repetition here, not a big deal at the moment but we should keep an eye on it
| @@ -139,7 +146,7 @@ func updateChannelMembers(ctx context.Context, d *schema.ResourceData, client *s | |||
| } | |||
|
|
|||
| for _, currentMember := range channelUsers { | |||
| if currentMember != channel.Creator && !contains(userIds, currentMember) { | |||
| if currentMember != channel.Creator && currentMember != apiUserInfo.UserID && !contains(userIds, currentMember) { | |||
There was a problem hiding this comment.
We may want to extract a function with a meaningful name for this conditional to make the intent more clear
When a channel has been imported and the creator may not be the
same as the one making the api calls with the token, two errors
may occur:
The provider may try to kick the user from the conversation, which
results in an error
couldn't kick user from conversation: cant_kick_selfThe provider tries to invite the user to the conversation, which
results in an error
couldn't invite users to conversation: cant_invitecaused by the actual slack api error
cant_invite_selfThis addresses those problems by retrieving the user that is making the
api calls using the
user_idfield from the auth.test endpoint, andremoving the user from the list of users to operate with.