CertManager: buttons should deactivate when appropriate

RESOLVED FIXED in Firefox 38

Status

P3
normal
RESOLVED FIXED
18 years ago
2 years ago

People

(Reporter: lord, Assigned: Cykesiopka)

Tracking

Trunk
mozilla38

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [kerh-coz])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

18 years ago
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.
(Reporter)

Updated

18 years ago
Target Milestone: --- → 2.0

Comment 1

17 years ago
->p3
Priority: -- → P3

Comment 2

17 years ago
Mass reassigning target to 2.1
Target Milestone: 2.0 → 2.1

Updated

17 years ago
Keywords: nsenterprise

Comment 3

17 years ago
*** Bug 83535 has been marked as a duplicate of this bug. ***

Comment 4

17 years ago
Moving all P3 and P4 bugs targetted to 2.1 to future.
Target Milestone: 2.1 → Future

Comment 5

17 years ago
removing nsenterprise keyword from PSM bugs with target milestone of future.
Keywords: nsenterprise

Comment 6

17 years ago
Mass assigning QA to ckritzer.
QA Contact: junruh → ckritzer

Comment 7

17 years ago
Reassigning to ssaux.
Assignee: ian.mcgreer → ssaux
QA Contact: ckritzer → junruh

Comment 8

15 years ago
Mass reassign ssaux bugs to nobody
Assignee: ssaux → nobody
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.
Target Milestone: Future → ---

Updated

14 years ago
Component: Security: UI → Security: UI
Product: PSM → Core

Updated

13 years ago
Whiteboard: [kerh-coz]
QA Contact: junruh → ui

Updated

10 years ago
Version: psm2.0 → 1.0 Branch

Updated

10 years ago
Whiteboard: [kerh-coz] → [kerh-coz][good first bug]
Version: 1.0 Branch → Trunk
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 11

9 years ago
I looked into the code and is as simple as it sounds. I'm taking a shoot at it.
Confirmed on x86_64 Linux (Ubuntu 11.04) as well.

Updated

7 years ago
Assignee: nobody → sjohnson
Created attachment 542981 [details] [diff] [review]
Patch to fix this bug.

Forced disabling the buttons based on whether the selection was an expandable component.
Comment on attachment 542981 [details] [diff] [review]
Patch to fix this bug.

Let me know if someone else would be better to review this.
Attachment #542981 - Flags: review?(kaie)

Updated

7 years ago
OS: Windows 2000 → All
Hardware: x86 → All

Updated

7 years ago
Attachment #542981 - Flags: review?(rrelyea)

Comment 15

7 years ago
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
Attachment #542981 - Flags: review?(rrelyea) → review-
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.
Attachment #542981 - Attachment is obsolete: true
Attachment #542981 - Flags: review?(kaie)
Attachment #560650 - Flags: review?(kaie)
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.
Attachment #560650 - Attachment is obsolete: true
Attachment #560650 - Flags: review?(kaie)
Attachment #566057 - Flags: review?(kaie)
reviewer ping. Please add your comments so we can land this. Thx!
Comment on attachment 566057 [details] [diff] [review]
Patch (v3)

Review of attachment 566057 [details] [diff] [review]:
-----------------------------------------------------------------

Couple small comments, but otherwise looks fine as a change that's just JS + XUL front end code.

::: security/manager/pki/resources/content/certManager.js
@@ +241,5 @@
> +      {
> +        hasChildren = hasChildren || false;
> +      } else {
> +        hasChildren = true;
> +      }

This seems a little wonky, better as:

  if (treeView.isContainer(j))
  {
    hasChildren = true;
  }

No need to keep reassigning to hasChildren when it's not a container.

but, really, since the entire point of this is just to determine if _some_ item is a container, it would be nice to bail out immediately upon discovering one.

Not sure what PSM style prefers, either:

  if (treeView.isContainer(j))
  {
    return true; // early return
  }
  ...
  return false; // instead of |return hasChildren|

or if early returns are frowned upon, |break| and add a !hasChildren conditional to the |i| for-loop. (i.e., so finding a container immediately terminates iterations and reaches the final |return|).

@@ +405,5 @@
> +   * relevant when on a node that isn't a child (i.e. a certificate
> +   * authority, rather than a specific certificate).
> +   */
> +  var nr = items.getRangeCount();
> +  var nr = items.getRangeCount();

Probably don't need to do this twice. ;-)
Attachment #566057 - Flags: feedback+
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?
Attachment #566057 - Flags: feedback-

Comment 21

6 years ago
Comment on attachment 566057 [details] [diff] [review]
Patch (v3)

I don't have time to focus on this
Attachment #566057 - Flags: review?(kaie)
Doesn't look like this is going to get reviewed, so I'm going to unassign myself.
Assignee: sjohnson → nobody
PSM/Certs and [good first bug] is nearly an oxymoron. Clearing.
Whiteboard: [kerh-coz][good first bug] → [kerh-coz]
(Assignee)

Updated

4 years ago
Duplicate of this bug: 439631
(Assignee)

Updated

4 years ago
Duplicate of this bug: 485602
(Assignee)

Comment 26

4 years ago
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.
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Attachment #8556917 - Flags: review?(dkeeler)
(Assignee)

Comment 27

4 years ago
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).
Comment on attachment 8556917 [details] [diff] [review]
bug78808_certManager-disable-btns-when-container-selected_v4.patch

Review of attachment 8556917 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome - this looks great.

::: security/manager/pki/resources/content/certManager.js
@@ +197,5 @@
>    }
>  }
>  
> +/**
> + * Returns whether nothing in the given cert tree is selected or if the

This comment could be a bit more clear:

"Returns true if nothing... . Returns false otherwise."
Attachment #8556917 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 29

4 years ago
Created attachment 8557535 [details] [diff] [review]
bug78808_certManager-disable-btns-when-container-selected_v5.patch

+ Makes nothingOrContainerSelected() documentation more clear per Comment 28
Attachment #566057 - Attachment is obsolete: true
Attachment #8556917 - Attachment is obsolete: true
Attachment #8557535 - Flags: review+
(Assignee)

Comment 30

4 years ago
Comment on attachment 8557535 [details] [diff] [review]
bug78808_certManager-disable-btns-when-container-selected_v5.patch

Thanks for the review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=826ac030efe6
Attachment #8557535 - Flags: checkin?

Updated

4 years ago
Attachment #8557535 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/645f4595673d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Duplicate of this bug: 400093

Comment 34

4 years ago
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.

Comment 35

4 years ago
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).
Hi Matthew - I think at this point it would be best to open a new bug for each change you'd like to see and have them block bug 1029832. Thanks!
(Assignee)

Updated

3 years ago
Duplicate of this bug: 540065
(Assignee)

Updated

3 years ago
Duplicate of this bug: 434714
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.