Mattermost-server: Create CLI command `webhook webhook move-outgoing`

[esethna]

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

---

Notes: Jira ticket

Mattermost is improving its CLI interface and we're looking for contributors to help with that effort. This Help Wanted issue is to implement webhook webhook move-outgoing command in the CLI.

Allow moving webhooks from one team to another.

Example: webhook webhook move-outgoing newteam oldteam:w16zb5tu3n1zkqo18goqry1je

Before you start, make sure you read this short Documentation

If you need other examples you can see the PR mattermost/mattermost-server#9094

[falcon78921]

I'm new to Go, but I'd love to take this on.

[amyblais]

Thank you @falcon78921!

[hanzei]

Hey @falcon78921

Is there anything we can help you with? if you have any questions, let me know!

[falcon78921]

My apologies @hanzei for the delayed response.

Being very new to Go, I think I'll have to pass on this. I still have much to learn!

[hanzei]

No problem @falcon78921.
Feel free to grab a react related ticket if you feel more comfortable with react.

[falcon78921]

Actually @hanzei, I have some questions about this...

In Go, which modules would need to import for the database manipulations? I understand Mattermost runs on MySQL or PostgreSQL. To create a webhook move command, you would need Go to authenticate into the database backend, modify the row containing the old webhook, and insert the new webhook into that row, right?

[hanzei]

Mattermost uses a App struct defined here, which offers a loot of methods to interact with the database. These a used e.g. here. Sometimes its needed to interactive the the DB via the Store layer, e.g. here.

[hanzei]

With (a *App) CreateOutgoingWebhook you can create a new outgoing webhook.

[patniharshit]

Hi, I would like to claim this if it is still open.

[hanzei]

@patniharshit Great! If you have any questions, let us know.

[hanzei]

Hey @patniharshit, Did you had time to work on this? Do you have any questions?

[hanzei]

Making this one available for the public again.

[joshuabezaleel]

Hey @hanzei , if it is okay with you I would like to ask a question.

By "moving", does this mean that we also need to remove the webhook that we are going to move from the oldteam, or copying the webhook to the newteam while still preserving it at the oldteam?

Thank you very much. Have a good day.

[hanzei]

Hey @jonathanwiesel,

Feel free to ask as many questions ask you want!

This ticket is about moving webhook around. I don't there we have considered a copy command. But this isn't so easy to do. Incoming Webhooks use a unique URL to receive the message. Coping this URL isn't a possibility.

[patniharshit]

@hanzei apologies for not responding. I will be no longer be able to do it.

[hanzei]

Hey @patniharshit,

No problem. I'v made this available for the public again, so someone else can pick it up.

[vildapavlicek]

I will do it.

[hanzei]

Great @vildapavlicek! :+1: Let me know, if you have any questions.

[joshuabezaleel]

Hi @vildapavlicek , are you still on this issue? If not, may I take this one?

Also @hanzei , I noticed from the Mattermost Command Line Tols docs page that only the outgoing webhook that needs to be entitled to a team from the flag that needs to be supplied when creating it, and the incoming webhook doesn't have any flags to input the team id or name.
Does this mean that this move command that move webhook from oldteam to newteam only applies to outgoing webhook, and not incoming webhook?

Thank you very much :slightly_smiling_face:

[vildapavlicek]

@joshuabezaleel
Hey, yes Im still on the issues, but it will be delayed because I got sick

[joshuabezaleel]

@joshuabezaleel
Hey, yes Im still on the issues, but it will be delayed because I got sick

Got it. Get well really soon, @vildapavlicek ! 🙂

[hanzei]

Also @hanzei , I noticed from the Mattermost Command Line Tols docs page that only the outgoing webhook that needs to be entitled to a team from the flag that needs to be supplied when creating it, and the incoming webhook doesn't have any flags to input the team id or name.
Does this mean that this move command that move webhook from oldteam to newteam only applies to outgoing webhook, and not incoming webhook?

Good catch @joshuabezaleel. There is definitely some inconsistently between the two commands.

@wiersgallak mattermost webhook create-incoming mattermost webhook create-outgoing have very different (required) flags. What are the "right" flags and which are required?

[hanzei]

Hey @vildapavlicek,

Just want to check how the work on this issue is going. Do you have any questions?

[hanzei]

Making this one available for the public again due to inactivity. Let me know if you still want to work on this @vildapavlicek!

[checkaayush]

Hey @hanzei, I'd like to pick this up.

[hanzei]

Thanks @checkaayush. Let me know, if you have questions.

[checkaayush]

Assumption:

In webhook move newteam oldteam:w16zb5tu3n1zkqo18goqry1je, newteam and oldteam are team names and the last part is the webhookId.

Initial thoughts:

1. Use GetTeamByName to fetch the new team by its name.
2. Use GetOutgoingWebhook to fetch webhook with given webhookId.
3. Use CreateOutgoingWebhook with newteam's id in the fetched webhook.
4. Use DeleteOutgoingWebhook to delete old webhook.

@hanzei Please let me know if I missed something, especially regarding the incoming-outgoing webhook inconsistency mentioned above.

P.S.: I'm new here but am absolutely loving how this project is being maintained and managed. Kudos!

[hanzei]

Assumption:

In webhook move newteam oldteam:w16zb5tu3n1zkqo18goqry1je, newteam and oldteam are team names and the last part is the webhookId.

That's correct.

One pitfall I want to make your aware of, is that there are two types to webhook: outgoing and incoming. You need to check for both of the types. But since IDs are unique, this isn't a problem.
I propose to move the logic inside a (a *App) MoveWebhook method in app/webhook.go.

P.S.: I'm new here but am absolutely loving how this project is being maintained and managed. Kudos!

Glad you like it! If you have more feedback about the community process, I would love it hear it. (Now or later) Also, feel free to join the Mattermost Community Server.

[hanzei]

@wiersgallak mattermost webhook create-incoming mattermost webhook create-outgoing have very different (required) flags. What are the "right" flags and which are required?

@wiersgallak Could you please take a look at this questions?

[checkaayush]

Here's the progress till now: https://github.com/mattermost/mattermost-server/pull/10489.

Next steps:

1. Cater to incoming webhooks as well and move the logic inside a (a *App) MoveWebhook method in app/webhook.go, as suggested by hanzei.
2. Add tests.

[hanzei]

I would suggest to open a WIP PR to get feedback as soon as possible.

[checkaayush]

Almost every method inside app/webhook.go is specific to either an incoming or an outgoing webhook. In order to maintain consistency, I propose addition of 2 new methods: MoveIncomingWebhook and MoveOutgoingWebhook.

To decide which method to call for a given webhookId (since webhook cmd doesn't currently have an option to specify webhook type), I'll call GetOutgoingWebhook first from the command function, and if none exists with the webhookId, I'll call GetIncomingWebhook, and call the appropriate new method above.

[hanzei]

@checkaayush That's a fine approach :+1:

[checkaayush]

As I understand, moving incoming webhook to another team doesn't seem to be the same as moving an outgoing webhook (CreatIncomingWebhookForChannel exists but CreateIncomingWebhook doesn't). Also, the cli interaction webhook move doesn't clarify whether an incoming webhook is being moved to another channel or to another team.

[hanzei]

As I understand, moving incoming webhook to another team doesn't seem to be the same as moving an outgoing webhook (CreatIncomingWebhookForChannel exists but CreateIncomingWebhook doesn't).

There is a method called CreateIncomingWebhookForChannel which you can use.

Also, the cli interaction webhook move doesn't clarify whether an incoming webhook is being moved to another channel or to another team.

That's a good catch. I suppose it should move to different team and a different channel. @wiersgallak Could you help clarify this?

[checkaayush]

I suppose it should move to different team and a different channel.

@hanzei Doesn't moving a webhook to a new team by default imply moving it to a different channel (since channels are tied to teams AFAIK), even when the channel name is same?

Also, this means that then we'll take an added optional argument of channel name in the command, but it'd be useful only for incoming webhooks.

I think we should separate them into webhook move-incoming and webhook move-outgoing.

[hanzei]

Doesn't moving a webhook to a new team by default imply moving it to a different channel (since channels are tied to teams AFAIK), even when the channel name is same?

Technically you could try to find a channel with the same name in the new team. But this is kind of tricky for UX.

I think we should separate them into webhook move-incoming and webhook move-outgoing.

I agree. Maybe we should even split the command into webhook incoming move and webhook outgoing move.

[wiersgallak]

My apologies on delayed responses @checkaayush @hanzei.

I think we should separate them into webhook move-incoming and webhook move-outgoing.

I also agree. My recommendation would be to approach the webhook move-outgoing since the team is a required parameter of that command and likely is more straightforward to support. Incoming Webhooks and Outgoing Webhooks do currently have different requirements and we will need to determine what changes to make to Incoming Webhooks to support the move command.

[checkaayush]

Cool. Going ahead with just the webhook move-outgoing command for now.

[checkaayush]

@hanzei I've run into another issue: CreateOutgoingWebhook expects the webhook's channel's TeamId to be the same as the webhook's TeamId as seen here.

As per my current approach, it'll fail since I'm calling CreateOutgoingWebhook with everything but the team id from the existing webhook.

One possible approach can be to omit passing the channel id to CreateOutgoingWebhook. But then, we'll have an outgoing webhook in the target team without a channel id. Not sure if that'd break anything.

Even ModifyOutgoingWebhook can't be used since it doesn't let us override the team id. Another approach could be to make changes to ModifyOutgoingWebhook to be able to do so.

cc: @jwilander Would love your inputs here as well.

[jasonblais]

@hanzei Wondering your thoughts on the above?

[hanzei]

@checkaayush Why not:

• Get the channel name from the old channel
• Try to get a channel in the new team with the name name
• Use the id from the new channel

This would make the semantic of reusing the channel name in the new team clear.

@wiersgallak Is this the expected behaviour?

[hanzei]

@wiersgallak Gently reminder about the above questions.

[wiersgallak]

@checkaayush I would expect to provide the channel ID from the channel I want to move the command to in the new team and the Team ID. In my opinion, it is better to be explicit about where you want to move the command to.

[RajatVaryani]

@hanzei I am taking this up!

[hanzei]

Thanks @RajatVaryani :+1:

[RajatVaryani]

@wiersgallak @hanzei I have a few questions:

1. For what purpose this cli command is going to be used for?

I would expect to provide the channel ID from the channel I want to move the command to in the new team and the Team ID. In my opinion, it is better to be explicit about where you want to move the command to.

1. @wiersgallak So we should also have a channel name or id as an argument to the command?

[wiersgallak]

@RajatVaryani

1. The purpose of this is so that users can move an existing outgoing Webhooks to a new team and/or channel.
2. Per the webhook create-outgoing command accepts either channel name or id in the channel string parameter, I would expect this command to accept either as well.

[RajatVaryani]

@wiersgallak Thanks.
webhook move newteam oldteam:w16zb5tu3n1zkqo18goqry1je --channel some-channel

Does this look good? And if the channel does not exist in the new team then an error is thrown. Sounds good?

[wiersgallak]

@RajatVaryani Yes the format and the error sounds good. Thank you!

[RajatVaryani]

There is a problem. This function assumes the outgoing webhook being moved to the same team. We might need to write a new method to support moving the webhook to a different team or to delete and create a new webhook with updated values. @hanzei @wiersgallak

[RajatVaryani]

Bumping up!

[jasonblais]

@jespino wondering if you're open to taking a look at the above question?

[RajatVaryani]

Can someone please help me close this?

[jespino]

@RajatVaryani I'm sorry the delay, yes, I think delete from one team and creating it in another one can be the solution, actually, I would suggest, create it in the another team, if everything goes well, delete the old one. and if something goes wrong, try to revert the action, or at least inform about the final state of the action (if something was created and if something wasn't properly deleted).

[RajatVaryani]

@jespino Let me try that.

[BenKnigge]

@RajatVaryani is it possible to use a db transaction to ensure that the move either succeeds in full or fails in full?

[hanzei]

@RajatVaryani You want to check if you have any outstanding questions.

[RajatVaryani]

Looking to close this pretty soon. @hanzei