Uncheck tag in CI/CD dialog doesn't work

[mkralik3]

This is a...

[ ] Feature request
[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Documentation issue or request

Description

I have some tag (e.g. myTag) in first integration. I check this tag on second integration and save the dialog. When I want to uncheck the tag from the second integration, it doesn't work. When I open CI/CD dialog after unchecking, the tag is still checked.

When I click on Remove, the tag is unchecked.

Step to reproduce:

• create two simple integrations (e.g. timer -> log )
• add some tag at first one
• check this tag in second one and save
• uncheck this tag in second one and save

[dhirajsb]

@mkralik3 unchecking the tag in that dialog simply means that you do not wish to tag for that environment when the save button is clicked. It does not mean that the integration is removed from that environment.
As a matter of fact, there is currently no way to remove an older tag from an integration. The remove button actually removes the environment from all integrations.
@gaughan @dongniwang, is this a candidate for a future use case where user might want to un-enlist an integration from an environment?

[mkralik3]

@dhirajsb I understand but my point is that it doesn't work in actual UI as described.

I see two issues (bugs) here:

1. Uncheck tag:
When I uncheck tag (because I don't want a integration for that environment anymore), in the dialog it looks unchecked, click on save button and then back to the dialog, the tag is still checked.

Expected behavior (if I understand it) :
When I uncheck tag in the integration and save dialog, the tag is not checked in the integration anymore when I open CI/CD dialog on integration again.

2. Remove tag:
When the tag is in only one integration, it is removed (as expected).
However, when the tag is in the two or more integration, after remove, the tag is not removed from all integration but only unchecked in the integration where the remove button was clicked.

Expected behavior (if I understand it) :
I have two integration and both integrations contain tag1. When I click on the Remove button in the dialog in one of these integrations, the tag1 is removed from both integrations.

[dhirajsb]

Ah, so the way that dialog works is that checks all available environments as default, whether they had been checked before.
There is no 'memory' of which environments were checked the last time, if that's what you are wondering about.

[gaughan]

So I believe you should be able to untag. That functionality exists.
Removing an environment tag completely from the list is not possible to correct?
I think the current UI is confusing as you have active links when the checkbox is not selected.

[dhirajsb]

@gashcrumb if an environment doesn't have a tag at all, I believe you get an empty value in the map. Perhaps you can use that as a hint to not select that environment by default?
So if the user never promotes an integration to a certain environment, it's unchecked by default.

[dhirajsb]

Regarding the second use case 'Remove Tag', the behavior is as expected. After removing the tag from one, it's still available to other integrations in Syndesis.

[dongniwang]

Regarding the second use case 'Remove Tag', the behavior is as expected. After removing the tag from one, it's still available to other integrations in Syndesis.

@dhirajsb - If after removing a tag from one integration, it should still show up for other integrations. The current UX is definitely confusing because the trigger action is the kebab menu for a single integration. We probably need to move this dialogue to a higher level.

[dhirajsb]

@dongniwang yeah, the single dialog is actually doing two things at the same time, managing environment names, and tagging.
We will have to push the environment management into it's own screen soon, but there are other related concepts that will tie into that screen, like Fuse parameters. Those requirements haven't been hashed out yet, so we don't know what that will look like.

[mkralik3]

So now it's possible:

• When I open CI/CD dialog the first time, I see all environments (tags) which were already created and I am able to add new if I want.

• When I check the particular tag and save dialog, this tag is "save" to the integration. After that, integration return this tag in public API (integrations//tags)

• For remove this tag from integration, I use the Remove button.

So for now, I suggest disabling checkbox in the UI dialog when it is already checked because it doesn't anything at this time. For users, it will be much clearer.

[dongniwang]

Working on a solution for this right now after a call with @dhirajsb and @gaughan. Expecting to have updated design by EOD tomorrow.

FYI - @heiko-braun @paoloantinori @gashcrumb @riccardo-forina @elvisisking Would someone has the capacity to work on the UI update once the design is ready?

[dhirajsb]

@dongniwang the behavior at present is also incorrect, because clicking remove should delete the tag across integrations. I have created a PR #4705 to fix it on the backend.
@gashcrumb clicking Remove should cause the tag to be deleted across integrations using DELETE at /public/environments/tag-name, and unchecking the tag should call the previous DELETE endpoint to delete it from the integration.

[dhirajsb]

@gashcrumb are you able to fix the UI issue I mention in the comment above?

[dhirajsb]

@gashcrumb wonder whether the UI should wait until user clicks save before calling DELETE on public/integrations/tag/<name> to avoid deleting a tag until the user clicks save?
Will that make the UI too complicated since it needs to maintain a delta with the original state?

[gashcrumb]

@dhirajsb think that's why there's the prompt, i.e. click 'remove', we prompt 'are you sure' and then delete. If we want to defer doing the actual delete until the user clicks save then yeah, we'd need to indicate that in the list in some fashion. I don't think though that it's worth further complicating the current dialog at this stage though given we've an improved design to implement that separates out the management.

[dongniwang]

Just wanted to summarize the latest discussion here:

• Updated UX design with "Manage CI/CD" in a separate area can be found in UX design tracker.
• Modal should remember user selection.
• "Remove" will remove a tag for all integrations.

cc: @gaughan @dhirajsb @gashcrumb @amysueg

[dhirajsb]

@gashcrumb I was wondering about deselecting the check box that removes the tag from the integration, not the Remove link that removes from all integrations.

[gashcrumb]

@dhirajsb Ah, gotcha. You know rather than having a separate DELETE operation that the UI has to call, you could fix the current API to accept the updated set of tags that the UI posts when the user unchecks things. For example right now on master if I create two environments "foo" and "bar", when you click save the UI posts:

["bar", "foo"]

to /public/integrations/<id>/tags. Then if I open up the dialog and uncheck "bar" and click save the UI posts:

["foo"]

Why do we need a separate DELETE endpoint for this, shouldn't the above value be what's set on the integration?

[gashcrumb]

I adjusted the URL used to delete environment names, but I suggest removing the separate delete endpoint for disassociating tags from an integration and instead work with the tag list the UI posts to sort this issue.

[heiko-braun]

@gashcrumb Can we consider it done?

[gashcrumb]

@heiko-braun no, I think we need to update the backend API to work with the data that the UI is posting in this comment. Something kinda has to be done anyways as currently when that API is used it's creating duplicate tags in #4755

[heiko-braun]

ok, thanks. Sounds like more work is needed by @dhirajsb

[dhirajsb]

@gashcrumb since this API is for external CI/CD processes as well, I want to keep it generic and fine grained. I will add a deleteOtherTags parameter to that endpoint to delete the missing tag names.
You'll have to send a json object with environments and deleteOtherTags fields.

[gashcrumb]

Yep, that works, so post should look like: { environments: [], deleteOtherTags: true } for example?

[dhirajsb]

yeah, just testing it locally on the PR, should be ready in a few

[dhirajsb]

@gashcrumb I was thinking that it would be more elegant and RESTy to use PATCH and PUT instead of a POST with an extra parameter. But then the UI might not be able to use that in a form since only GET and POST are allowed. Is that correct?

[gashcrumb]

UI is just doing an XHR request, not a form submit so it can use anything. But honestly, from the outside my assumption was that this is just a setter for the whole array. It seems weird that I need to tell the API to behave that way with an extra boolean.

And yeah, whatever method you want to use just let me know, honestly our API isn't really all that rest-y anyways. Or if you want it's just a matter of updating this code to use the relevant method function (post, patch, put) and adding the additional argument.

[gashcrumb]

adding the additional argument.

Sorry, updating the existing argument to post, i.e. change: environments to { environments, deleteOtherTags: true }.

[dhirajsb]

@gashcrumb fixed the endpoint, changed the method to PUT, and fixed the environment Remove operation

[dhirajsb]

@gashcrumb can you approve PR #4783 after reviewing the UI changes I made?

[dhirajsb]

@mkralik3 please close this issue once you have verified the fix from PR #4783