In the Cert Manager, you can select certs, or their parent container. I'd like
to see these changes:
-You should not be able to select the container by clicking on it. Perhaps when
you select a container it should turn grey rather than black.
-When you select a range of certs to view, the containers should not be selected.
-When you select a container (and no other certs), the View, Edit, and Delete
buttons should not be active. They should be gray.

Mass change "Future" target milestone to "--" on bugs that now are assigned to
nobody. Those targets reflected the prioritization of past PSM management.
Many of these should be marked invalid or wontfix, I think.

Created attachment 421904[details]
Screenshot of brokenness
I stumbled across this doing Litmus tests and thought I'd add in my 2 cents.
This was in the authorities tab (obviously), but that's where I saw the brokenness. Maybe "Delete" shouldn't be disabled if you select the container (it makes sense to be able to delete a container), but the other buttons should be disabled.
I'll also note that this works pretty much as expected on the servers tab, so I can't imagine it's that hard to at least make it better.

Comment on attachment 542981[details][diff][review]
Patch to fix this bug.
Kai really should review the final, and comment on my comments.
That being said I did see one issue with this code. It appears the code written and then copy and pasted for the other tree views. unfortunately the all copies of the code have a check 'caTreeView.isContainer(j)' in the middle of the loop, rather then userTreeView, emailTreeView, orphanTreeView, etc.
It seems that this functionality is crying out for it's own function which takes the Treeview and returns the value for 'toggle'.
Please have Kai review the results as he is more familiar with the javascript PSM ui than I am.
bob

Created attachment 560650[details][diff][review]
Patch - b78808 (v2)
(In reply to Robert Relyea from comment #15)
> That being said I did see one issue with this code. It appears the code
> written and then copy and pasted for the other tree views. unfortunately the
> all copies of the code have a check 'caTreeView.isContainer(j)' in the
> middle of the loop, rather then userTreeView, emailTreeView, orphanTreeView,
> etc.
I fixed this now.
> It seems that this functionality is crying out for it's own function which
> takes the Treeview and returns the value for 'toggle'.
Sure, I can refactor this. I'm going to wait to see what Kai has to say before I do this, so I can take any comments he has into account in my next patch.
Thanks for your review, Bob.

Created attachment 566057[details][diff][review]
Patch (v3)
I did some refactoring to this according to what the previous review mentioned. I've had this in my patch queue for a while, and just haven't had a chance to post it yet.

Comment on attachment 566057[details][diff][review]
Patch (v3)
>+function has_children(treeView)
All the callers (except one, but see below) also double-check the number of selected items before calling this function. That code could be moved here.
Unfortunately getSelectedCerts which would otherwise seem to be useful only works on the current tab, so it is no good for the cases whereby the trees get updated in the background, but you might want to refactor it in some way so that you use the same logic for disabling the buttons as you do for finding the certificates to process:
function ca_enableButtons()
{
var disabled = getSelectedCerts(caTreeView).length == 0;
var enableViewButton=document.getElementById('ca_viewButton');
enableViewButton.setAttribute("disabled",disabled);
etc.
>+ hasChildren = hasChildren || false;
No effect.
>+ toggle = has_children(caTreeView) || toggle;
Normally you would write this differently to avoid calling has_children when toggle is true.
>@@ -309,50 +336,89 @@ function websites_enableButtons()
>+ /*
>+ * This next section is to disable the buttons that aren't
>+ * relevant when on a node that isn't a child (i.e. a certificate
>+ * authority, rather than a specific certificate).
>+ */
The existing code before this comment already seems to do this?

Created attachment 8556917[details][diff][review]bug78808_certManager-disable-btns-when-container-selected_v4.patch
This is an updated version of the v3 patch.
For all the tabs, the appropriate buttons are now disabled if a container is selected.
In addition, for the "Servers" tab, the previous 1 row selected only restriction is lifted. I verified by manual testing and code inspection that selecting multiple rows and pressing the appropriate buttons works fine.
I also renamed |toggle| to |disableButtons|, since I found |toggle| to be a confusing/misleading name for this context.

Created attachment 8556925[details][diff][review]
Example certs and generation+import script (DON'T CHECK IN)
This patch contains:
- A script that generates and imports into a profile different types of certs
- Some pre-generated certs
For reference, this is what I used for testing the patch that fixes this bug (I have yet to find a way to coax the cert manager into binning something in the "Others" tab through more normal means).

I'm confused about the solution to this issue. After this patch will I be able to highlight a large number of Certificate Authorities (e.g. using Shift-DownArrow) and "delete" them? Or will the "delete" button be disabled because my selection includes "containers"?
If the latter, I would like to file a new bug to allow a way to delete/disable large numbers of the built-in CAs, because there doesn't seem to be a good way to do that at the moment.

Nevermind, I checked the current nightly (39.0a1), and it just disables the delete button if a container is selected, even if certs/authorities are part of the selection.
While this makes the UI match the actual behavior (before the button wasn't grayed out, but 'delete' did nothing if you had a container selected), I don't think this actually fixes the original request.
The OP was asking for sensible behavior for multiselect in that UI, and none of the suggestions were implemented. I suggest re-opening this bug.
I will also add a suggestion of my own as a way to solve the current issues: have a way (perhaps a config option) to just not show the "container level" at all, or at least change the way they are displayed in a way that doesn't allow them to be selected (and thus break the ability to multiselect across containers).