@ebrinkman Thanks! I did some investigation and it looks like this bug is a little bigger than just checking the contents of update_params.
The reason this bug is happening is because QuickActionsService always returns the intended command to update_params.
From my understanding, QuickActionsService, which calls QuickActions::InterpretService, does not take the status of the issue into account. For example, if an issue is already closed and you do /close on it a second time, QuickActionsService won't care that it's closed. It just converts the command into a value to be put inside update_params.
I made a WIP merge request to demonstrate what's happening: gitlab-ce!31221
Unfortunately, this is my first time trying to contribute to GitLab, so I don't have enough understanding of the codebase to go further with the investigation. I'm hoping someone can shed some light on this.
UPDATE: It doesn't. It still says: Added ~bug label even if we add the same label. It does handle other errors properly now like wrong label name, etc..
@terenceponce with gitlab-ce!26672 merged the flow here changed quite a bit. I think there are two possible failure cases here:
The user uses a quick action that isn't available.
Availability is defined by the condition blocks you see in the quick action definitions in lib/gitlab/quick_actions/*_actions.rb. These are based on permissions available and the current state of the issue / MR.
This block is also what determines what shows up in the autocomplete when you type /. But users could still type in the quick action even without the autocomplete so we should still handle this error case.
Using /close when the issue is already closed falls into this category.
The current behavior is that no message is shown at all.
The user uses an available quick action but with invalid parameters.
In this case the quick action is available to the user but the user passes invalid parameters. This is now handled by the command block and gitlab-ce!26672 allowed us to provide a message to show to the user. You can see an example here where we return an error message when moving to a project that doesn't exist.
Most quick actions I've seen have proper checks and would return the correct error. Adding a label that already exists doesn't get flagged as an error because there's some missing logic in #run_label_command to check for already existing labels so that a proper message can be returned. I think this is a separate bug and I'll create a separate issue for this (https://gitlab.com/gitlab-org/gitlab-ce/issues/65543).
Right now, it has code that checks if there are commands executed (those that passed the available? check) and the note is "commands-only", we show the message returned from the command.
I think we'd like some logic here so that if there are commands parsed and there are no commands executed, we show some error.
Or maybe we could even just change how commands_executed_count work there. Maybe rename that and include in the count those that weren't available. Those that weren't available would have a nil message anyway and then it would fall back to the Commands did not apply message there.
Hi @engwan! I see this has been laying around for a while and after trying it myself it still seems to be a problem, so I'd like to give it a try if you think that's okay.
@engwan So I did some investigation and I think the distinction you tried to make between the issues is not entirely possible. I think its best to illustrate what I mean by providing some examples, assuming the issue was open:
1. /close -> /close
Lets start with the example from the issue. Currently the result of trying this is the following:
This is because while the interpret service will extract two commands, it will only have one @update[:state_event] = close. It will identify both commands as executable? since there is no check to see if that update was already initiated. To fix this you need to change the issueable_actions.
2. /close -> /reopen
Again the interpret service will extract two commands. However, this time it it will fail the executable? check on reopen. This is because the issue is in fact still closed. However, like you said, since a message is present it won't give any failure messages. I would suggest to dedicate this issue to give errors for each command that is not executed.
However this does raise the issue of what you would like to do when you enter any combination of /close and /reopen. Either you try:
Giving error messages and success messages for each successful pair of open and close
Not allow this behaviour at all, which is what completing this issue would entail.
3. /label -> ....
Is the same idea but for different actions.
If you think that's okay, I'd like to continue to implement this issue as described in example 2.