Filtered branches and button "Delete merged branches" deletes all merged branches

@m.baur Thanks for the feedback. I can see how this is confusing when the button is available whilst search results are displayed.

@dimitrieh Please could you take a look? Should this button be hidden when displaying search results?

Hi, @dimitrieh do you have a plan for this issue?

added Create [DEPRECATED] label

So this is really confusing, i'm glad I googled this, because it's just really dangerous I think.

A "remove everything" button should not be on the same page as a filter, because that implies some correlation between the two. Especially when it's on the same line!

Either make it into "remove merged from current selection/filter" button and keep it, or move it to another page (or at the very least another location on this page, but frankly, that's sub-optimal still.

added devopscreate label

added [deprecated] Accepting merge requests label

added groupsource code label

removed [deprecated] Accepting merge requests label

added sectiondev label

added typebug label

changed milestone to %Backlog

added bugux + 1 deleted label

All SUS-impacting issues need to have a proper severity label set. Please add a severity label, remove the automation:ux-missing-labels label, and then reply to this comment briefly explaining your reasoning for providing this severity.

If you are not the DRI for this area and would like help determining the best severity, please @ the appropriate person for assistance.

added automation:ux-missing-labels label

added [deprecated] Accepting merge requests label

I can't figure out whose responsibility it is to label the severity and type of bug. This is a data-loss bug, so clearly severity1 and it's mostly likely UX. I see UX is already set. Who adds these?

Thank you @YesJustWolf. I've applied the severity1 label.

added severity1 label

removed automation:ux-missing-labels label

Hello there,

This bug has an SLO of 30 days based on the established severity SLOs for S1 bugs. It does not currently have a milestone that meets this SLO.

A due date of 30 days from now and the corresponding milestone are being set to ensure it is resolved within the target SLO.

@sean_carroll @tlinz @a_mcdonald @mle please consider pulling this bug forward into the current milestone if appropriate and capacity exists, but this bug should not be pushed past the automatically assigned milestone.

Bot policy.

changed due date to November 12, 2022

changed milestone to %15.6

added priority1 label

added workflowdesign label

@sean_carroll @tlinz @a_mcdonald This functionality was something @mvanremmerden was discussing in a separate 1:1 meeting on the general usability of the Branches page. Over next few days I will be putting an issue outlining of the current opportunities for improvements and a possible future state of the Branches page. As part of this I should be able to address a possible solution for this issue.

I will be putting an issue outlining of the current opportunities for improvements and a possible future state of the Branches page. As part of this I should be able to address a possible solution for this issue.

@mle will the solution be different than the current 2 proposals on the SSOT?

Only deleted merged branches which are shown in the filtered search

Remove "Delete merged branches" button for searches

We are currently planning %15.6. Given this is a S1/P1 we need to address it immediately. Should we assume it will be workflowready for development in %15.6

@andr3 would this be a frontend only issue or would it need backend support?

CC @tlinz

@sean_carroll from what I can see, the implementation sends a DELETE http request to /-/merged_branches. It doesn't really specify which branches to delete.

We could leverage the API, but even then we'd need to send one request per branch.

So either way, this will require some backend to allow the Frontend to send a list of branches to be deleted.

OR

We simply make the dialog a bit more explicit about which branches will be deleted (frontend only).

I think we need some UX guidance on how to procede here: make the button aware of which branches are shown or simply making the dialog clarify which branches will be deleted. @mle thoughts on this? :) Thanks in advance.

Labeling backend for the time being.

@tlinz @mle this is currently labelled %15.6 but awaiting the outcome of this question before scheduling:

make the button aware of which branches are shown or simply making the dialog clarify which branches will be deleted.

I was guilty of deleting all the merged branches at the company due to this issue. I think a warning modal or something might have stopped me from proceeding. Perhaps a text change like Delete All Merged Branches would help too. I like also the suggestion of removing the button when filtering as a simple fix.

added frontend label

added backend label

mentioned in epic &6592 (closed)

@sean_carroll I'll save my bigger vision for Branches for another day and focus on this delete button behaviour as part of this issue.

Questions

@andr3 @sean_carroll Is it possible to get the count of total merged branches and the breakdown of the count of those are active and stale branches?
@andr3 @sean_carroll If we can't get a breakdown of active & stale, would it be possible to get a list of the names of the merged branches?
@aqualls Could you please review the text in this modal as you were also looking at a similar issue here #371977 (closed)

Proposal

Update the delete modal to inform the user that they are going to be deleting all of the merged branches.

Current	Proposal

The update to the modal introduces a few things to help better inform the user of the action that they are about to do:

Informs users of total number of branches to be deleted.
Highlight the number of active and stale branches to bring more attention that this is across the repository.
- If we can't get the split of active & stale, then if we had a list of branch names then we could display the list in a collapsible section. In the scenario of a large list, this list would be scrollable. The GitLab repo has +12K branches so I could imagine there would be a large enough list that would require scrolling
The use of the word "bulk" is added to hint to the fact that there could be multiple if the user has only one branch in the view.
The input of total count is required to enable the "Delete merged branches" as extra friction to this flow. This was inspired by this issue New design for merged branches deletion confirm... (#20483 - closed)

Fields

Title: Delete all merged branches?
Body:

You are about to delete [total count] branches that were merged into ‘[Default branch name]’.

- [active count] active branch
- [stale count] stale branches

This bulk action is permanent and cannot be undone or recovered.

Enter the number of total merged branches to be deleted to confirm:
`[total count]`
{text input}

Secondary CTA: Cancel
Primary CTA: Delete merged branches

Background

Why removing the delete button when filtering would NOT be the ideal fix

Since the creation of this issue, it looks like we also introduced tabs to the Branches page. The tabs that we have are "Overview", "Active", "Stale", and "All".

When you have a filter selected say "Stale" and you click on "Delete merged branches" it would delete ALL merged branches even those that are not stale. In the video below it shows me having one merged branch that is classified as Active and when I am on the Stale tab and press "Delete merged branches" it deleted all of them including the one I had in Active.

CleanShot_2022-10-14_at_20.16.01

If we remove this button when filtering, then we should remove it for tabs too. If we remove it for the tabs scenarios then there are no opportunities to surface this button.

Therefore we should keep it visible for now and handle the deletion action.

Spirit of the delete merged branches button

it would be cool if it would be possible to remove all source branches that were merged from a repository with one click

This feature gitlab-foss#21076 (closed) was intended to be a bulk action for the entire repository rather than what was on the screen. As part of this solution to resolve the severity1 issue I would say we keep this behaviour that it looks at the entire repo for merged branches rather than what is visible on the screen. We could revisit this in the future should we look at how we might handle bulk actions in the future.

Updating the modal for deleting merged branches

This comment from 2 years ago captures the anxiety from clicking this button:

Hey guys, does this show a preview of what branches will be deleted and let us mark them individually for deletion before confirming? I'm a bit afraid of clicking the button and killing stuff that I shouldn't be killing...

Extracting the key points:

Show a preview of branches to be deleted
Allow selection/deselection of branches that are to be deleted

Aside: the modal for deleting all merged branches looks less scary than deleting one branch. Both are not using the recommended Pajamas guidelines around delete modals correctly but this is an ongoing discussion:

Delete merged branches	Delete a branch

This is not the first time someone has suggested updating the delete merged branches modal:

The interesting concept with #20483 (closed) was adding an extra step of friction by inputting the number of merged branch to be deleted as an act of confirmation.

I would start out with listing the number of active & stale branches as a way to surface a quick glance that there could be a mix of different branches to delete. If we are able to get the names of the branches then we could do something like this:

1 active

branch A

3 stale

branch 1
branch 2
branch 3

I know you are looking for answers to your questions. But I just wanted to say your proposal looks good to me.

@igor.drozdov could you please answer these questions and weight this issue from a backend perspective?

@andr3 @sean_carroll Is it possible to get the count of total merged branches and the breakdown of the count of those are active and stale branches?

@andr3 @sean_carroll If we can't get a breakdown of active & stale, would it be possible to get a list of the names of the merged branches?

@mle getting the list of the names of the merged branches is easier and more performant than getting the breakdown of those branches by their activeness.

One thing to mention: if a branch is protected or associated with an open merge request it won't be deleted. I wonder if we need to reflect that in the popup?

if a branch is protected or associated with an open merge request it won't be deleted. I wonder if we need to reflect that in the popup?

A great point @igor.drozdov

Good points here @igor.drozdov

I will take this into account and update the designs

@igor.drozdov I took your input and modified the designs to look like this:

A: With input	B: Without input

With the list of branches and the extra text I am not too sure about having the extra input for confirming the delete. I'm gonna look into this.

@jerasmus Handling a long list in a modal, are we able to include a vertical scrollbar or is there another pattern to use here?

Scrolling by default	Show more

@mle Your proposed language is excellent. No changes from me.

@mle our modals do support scrolling content (example).

I am running a short solution validation with this design

@igor.drozdov @nradina @sean_carroll I have updated the description of this issue with the designs for this issue.

Findings for solution validation can be found here #20384 (comment 1146210710)

Thank you for the quick turnaround on this one @mle

@mle sorry for the back and forth, but we've encountered a potential performance issue with retrieving the whole list of the merge branch names.

While I'm looking for the optimal way to get that list, just a question: will it help to mitigate the S1/P1 if we show the message like You are about to delete ALL branches that were merged to main (without specifying any numbers/branch names)? Or does it have too little value even for the first iteration?

/cc @nradina

@igor.drozdov Assessing my options here, are we able to get the count of merged branches?

@mle I will reply on behalf of @igor.drozdov that unfortunately retrieving total of all merged branches has the same performance impact as retrieving names. So it will not solve the problem we are facing here.

@mle @nradina unfortunately, yes

My concern is that the number of branches can be too huge to retrieve performantly. Even if we compute it once and save the size of the list in cache, it won't be very efficient because the number will be invalidated too quickly by merging/removing the branch, creating a merge request or a protected branch.

I haven't managed to think of a reasonable alternative option yet.

One of the options that I was thinking about was fetching a limited number of branches from Gitaly, for example, no more than 100. And then we could display something like You are about to delete MORE THAN 100 branches if it's larger than 100. But this number may be reduced by being protected or having an open MR, so may not be accurate sometimes.

@nradina @igor.drozdov Thank you for the context and updated contraints.

Let's go with a boring solution of replacing the list of branches with the sentence: "This may include merged branches that are not visible on the current screen."

Instead of typing the number of merged branches, the modal is asking the user to input the word "delete". The spirit of doing this is to act as a speed bump to slow down the user from making a delete action that they can't undo.

@aqualls For your awareness of changing the text in the delete merged branches modal because we are unable to retrieve the list of branches to be deleted peformantly.

Great! let's go this way.

mentioned in issue #371977 (closed)

assigned to @igor.drozdov

set weight to 3

removed [deprecated] Accepting merge requests label

added Deliverable workflowready for development labels and removed Create [DEPRECATED] UX backend bugux devopscreate frontend groupsource code priority1 repository sectiondev severity1 typebug workflowdesign + 1 deleted label

@sean_carroll, thanks for adding the workflowready for development label.

This issue has been sent back to workflowrefinement because it's missing the work type classification.

Please add a type:: label and try again.

This message was generated automatically. You're welcome to improve it.

@igor.drozdov this is assigned to you as a Deliverable

added workflowrefinement label and removed workflowready for development label

added Category:Source Code Management UX backend bugfunctional devopscreate frontend groupsource code repository sectiondev labels

added typebug label

added workflowdesign label and removed workflowrefinement label

added bugux + 1 deleted label and removed bugfunctional label

All SUS-impacting issues need to have a proper severity label set. Please add a severity label, remove the automation:ux-missing-labels label, and then reply to this comment briefly explaining your reasoning for providing this severity.

If you are not the DRI for this area and would like help determining the best severity, please @ the appropriate person for assistance.

added automation:ux-missing-labels label

added frontend-weight2 label

mentioned in issue create-stage#13040 (closed)

added workflowsolution validation label and removed workflowdesign label

assigned to @nradina

added severity1 label

removed automation:ux-missing-labels label

added priority1 label

changed the description

Solution validation insights:

When people read the modal they understood what would happen when deleted
- One person glanced at the modal and found it "odd" that one branch remained
Content in modal could help identify what the user should focus on
- In the prototype the text was mainly one weight and there was not enough on the page to pull the user attention in. Suggestions included showing what would be deleted in active/stale tab and better highlighting of text in modal
Entering the number would prevent mistakes

Links

added UX solution validation label

added workflowready for development label and removed workflowsolution validation label

Future thought @sean_carroll @tlinz @andr3 - To improve this experience further in the future we should explore the idea of exploring a better way to bulk manage branches. There is an issue around some of these ideas that we could expand on Feature Proposal: Improve bulk branch managemen... (#22613)

Feature Proposal: Improve bulk branch managemen... (#22613) looks like a great new feature - have commented there.

mentioned in issue #22613

mentioned in merge request !102231 (merged)

Not sure whom is the best to ask, but what about the text in the modal if there is only one merged branch?

Are we fine with: You are about to delete 1 branches that were merged into master Or shall branches be changed into branch(es)? And how about was/were? /cc @andr3 @aqualls

@nradina Thank you for catching this! If we want to keep this format, we should plan for singular / plural work.

You are about to delete 1 branches that were merged into master.
You are about to delete 1 branch that was merged into master.

(Hopefully we're also wrapping the name of the branch in backticks, @ml3?)

I assume the ml3 is the merged persona of Andre and I

For the record, I am ok with the branch name in backticks which is the inline code styling.

@aqualls, @mle great, thank you for the suggestions, will apply!

@mle @andr3 I was not at my best last week. At least my mistake was funny!

@mle few small updates from my side -

I haven't found any examples of using backticks so I used <code></code> to wrap default branch name, as we do in similar components - ex: delete branch modal.
I suggest to keep delete button disabled up until User enters the correct amount of branches - similar as we do in Delete project modal or delete branch modal when branch is protected.
I also noticed that some similar modals have additional 'Are you ABSOLUTELY SURE?' text in the title. Shall we add it as well for consistency? I attach the delete protected branch modal screenshot as an example of all things mentioned.

@nradina

Yes happy for you to use <code> to implement that look
Yes happy for disabling the delete button until the user enters the correct number of branches
No we should not add the "ABSOLUTELY SURE" part to the title.
- The screenshot you shared is a good example of drawing attention. Eventually this modal should be changed. A few months ago the UX team documented how we should be communicating these high severity deletion scenarios https://design.gitlab.com/usability/error-prevention/#high-severity.

@mle, great thank you for your input!

@mle one more thing to mention - I simply do not show button if there is no merged branches. I think it is most logical for such use case.

@igor.drozdov I am not sure if we really need any backend work here. I am able to get needed data from .haml template and pass it to .vue component via data attribute. Unless we really want to call endpoint instead.

The main downside I see with this approach is that it will be not possible to update the delete merged branches button and modal after deleting branches without page refresh, but as the rest of the page(displayed branches) are not updated as well - not sure if it is an issue for now.

@nradina I agree with this check but how do you know if there are no merged branches?

The weird thing at the moment with this button that it doesn't take in context of the current tab/search but the entire repository.

We could create a separate issue for that functionality to show/hide "delete merged branches" button and then evaluate it against Feature Proposal: Improve bulk branch managemen... (#22613).

@mle once you delete the merged branches, there are no merged branches . Or for the fresh repos.

I just wanted to cover this use-case, it's not big of a deal. Otherwise modal would say: You are about to delete 0 branches, which is weird, isn't it? We can also adapt modal text for this case, but my idea was to just hide button.

I don't think we need to create a separate issue for it. Btw, I love the idea with bulk edit! This is something I wanted to suggest as well. /cc @andr3

I am not sure if we really need any backend work here. I am able to get needed data from .haml template and pass it to .vue component via data attribute. Unless we really want to call endpoint instead.

@nradina that's great! I agree that it's the simplest solution at the moment

I'll unassign myself then, feel free to ping me if I can be helpful

@igor.drozdov looks like I need a bit of your help! Just realised that @merged_branches_names variable is scoped to currently visible on the page branches, which is not exactly what needed. Instead I need to use all merged branches from the repo, and I am not sure how to get those. Feel free to either suggest some code or push your changes to the branch. Sorry for wrong assumption earlier, reassigning you back!

@nradina For the record I would rather hide the button rather than disabling it or showing the "you are about to delete 0 branches".

In comparison to the current implementation in production, when you have zero merged branches and press "Delete merged branches", it still goes through the same flow and modals as if you had merged branches. Thank you again for your suggestion on this edge case because it would make the UI clearer.

@mle awesome, thank you for the confirmation!

I have updated the issue description with this change in direction.

added Technical Writing label

added UI text label

added workflowin dev label and removed workflowready for development label

mentioned in merge request !102644 (merged)

unassigned @igor.drozdov

removed backend label

assigned to @igor.drozdov

added backend label

added workflowin review label and removed workflowin dev label

unassigned @igor.drozdov

marked this issue as related to #381720

mentioned in merge request !103342 (closed)

assigned to @igor.drozdov

@igor.drozdov reassigning you back, since it looks like this can't move forward without your input!

changed health status to at risk

@tlinz @sean_carroll FYI, we're adding a field to GraphQL and there are ongoing discussions that make it likely that this will slip over to %15.7. I've updated the Health Status on this issue to "At Risk".

Thanks for taking the time to ship this properly without risking sacrifice performance, @nradina @igor.drozdov!

/cc @nradina @igor.drozdov

@andr3 @nradina yes, sorry that I haven't noticed it earlier. Currently, I think about how things can be improved on backend side in order to implement it efficiently and will come back with a suggestion

mentioned in issue gitlab-org/quality/triage-reports#9917 (closed)

mentioned in issue create-stage#13078 (closed)

changed the description

This one verified on production today, looks good @mle thanks for updating the description! If there are any further improvements let's create a follow-ups.

closed

mentioned in issue #20483 (closed)

marked this issue as related to #371977 (closed)

Filtered branches and button "Delete merged branches" deletes all merged branches

Summary

Steps to reproduce

Example Project

What is the current bug behavior?

What is the expected correct behavior?

Relevant logs and/or screenshots

Output of checks

Results of GitLab environment info

Results of GitLab application Check

Direction

Designs

Child items ...

Activity

Questions

Proposal

Fields

Background

Why removing the delete button when filtering would NOT be the ideal fix

Spirit of the delete merged branches button

Updating the modal for deleting merged branches

Solution validation insights:

Links

Filtered branches and button "Delete merged branches" deletes all merged branches

Summary

Steps to reproduce

Example Project

What is the current bug behavior?

What is the expected correct behavior?

Relevant logs and/or screenshots

Output of checks

Results of GitLab environment info

Results of GitLab application Check

Direction

Relates to

Activity

Questions

Proposal

Fields

Background

Why removing the delete button when filtering would NOT be the ideal fix

Spirit of the delete merged branches button

Updating the modal for deleting merged branches

Solution validation insights:

Links