Skip to content
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.

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

Remove /jira settings command #1072

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Remove /jira settings command #1072

wants to merge 4 commits into from

Conversation

ayusht2810
Copy link
Contributor

Summary

  • Removed /jira settings from slash commands as the actual command for settings is /jira instance settings [setting] [value] as stated in the help command

Ticket Link

Fixes #1037

@ayusht2810 ayusht2810 self-assigned this Apr 30, 2024
@ayusht2810 ayusht2810 added the 2: Dev Review Requires review by a core committer label Apr 30, 2024
@raghavaggarwal2308
Copy link
Contributor

raghavaggarwal2308 commented May 6, 2024

@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

@ayusht2810
Copy link
Contributor Author

@mickmister Regarding the issue #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)?

@mickmister
Copy link
Member

Regarding the issue #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?

@ayusht2810
Copy link
Contributor Author

@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):
image
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.

@mickmister
Copy link
Member

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?

@ayusht2810
Copy link
Contributor Author

@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.

@ayusht2810
Copy link
Contributor Author

@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.

@@ -157,47 +157,47 @@ func TestPlugin_ExecuteCommand_Settings(t *testing.T) {
expectedMsg string
}{
"no storage": {
commandArgs: &model.CommandArgs{Command: "/jira instance settings", UserId: mockUserIDUnknown},
commandArgs: &model.CommandArgs{Command: "/jira settings", UserId: mockUserIDUnknown},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/jira instance settings is still a valid command right? Can we just add some tests for /jira settings instead of changing these tests?

@mickmister mickmister added the 3: QA Review Requires review by a QA tester label May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/jira settings command is not present in command autocomplete
3 participants