Closed ayusht2810 closed 3 days ago
@ayusht2810 I do not think we need to remove the command here. Can you revert this change and add the command to autocomplete cc: @Kshitij-Katiyar
@mickmister Regarding the issue https://github.com/mattermost/mattermost-plugin-jira/issues/1037, the command /jira settings
is used when we only have a single instance present and the command is used for that single instance (this is something like an enhancement in which we don't need to write the whole instance). The command is missing from the help command and is also not present in the readme as well. So, should we remove the command, or should we keep everything as it is, considering it is an enhancement only and not a proper feature? Also, should we add other missing commands to the autocomplete (like, connect, disconnect)?
Regarding the issue https://github.com/mattermost/mattermost-plugin-jira/issues/1037, the command
/jira settings
is used when we only have a single instance present
@ayusht2810 In almost all cases, there will be only one Jira instance installed, so I think we should cater to that scenario. I think a settings
command is expected for the plugins to implement if there are any user settings. I think we should also add autocomplete for connect
and disconnect
as you mention. Maybe we can conditionally show these based on if there are multiple Jira instances set up?
Thinking about it some more, if a command doesn't work correctly when there are multiple Jira instances installed, we can just return an error when someone tries to use it on a server with more than one Jira instance installed. Something like:
Please run this command instead: `/jira instance [jira url] settings`
If the given command is compatible with multiple instances (I think it works correctly for connect
and disconnect
), then I think we should just add them to autocomplete. If settings
is not compatible with multiple instances, we can just return an error when the user tries to run the command. What do you think?
@mickmister I checked the current flow for the code. If we have a single instance present, then the slash commands /jira connect
, /jira disconnect
and /jira settings
work fine for that single instance (no autocomplete).
But where there are multiple instances present, the user is provided with a modal in both the /jira connect
and /jira disconnect
commands (still no auto complete):
But for the
/jira settings
command, we simply get help command in the response.
I don't think we need to provide auto complete for the above commands. We can certainly add a modal for the /jira settings
command similarly to connect and disconnect commands. What are your thoughts on this? Please let me know if we should still add auto complete for the commands and return responses on the basis of the number of instances.
I don't think we need to provide auto complete for the above commands
I don't understand this though. Why not provide autocomplete for /jira connect
and /jira disconnect
? It's always a bit weird on a customer call when they run /jira connect
, but it doesn't show up in the autocomplete. Same with /jira create
(though that doesn't work on mobile in any case. We should cater to desktop though as far as autocomplete in this case).
I think /jira settings
is the outlier here, and I'm thinking we should just return an error message when the user runs it in an invalid context, which in this case is when there are multiple Jira instances connected, and just suggest to run the full multi-instance version of the command. What's the drawback of including /jira connect
etc. in the autocomplete?
@mickmister I am fine with adding /jira connect
and /jira disconnect
commands in the auto-complete. It was missing from documentation so I thought it was incorrect for those commands to be added to auto-complete. But I think what you said is correct, and we can add them to the help command as well.
The changes for the /jira settings
command also look good to me. I will start working on them.
@mickmister updated the commands and added a demo video screen-capture (6).webm
Please let me know if anything else needs to be updated here.
@mickmister Added new test cases. Please re-review
Summary
Ticket Link
Fixes #1037