Add ability to add permissions in Data Manager

RESOLVED FIXED in seamonkey2.1b2

Status

SeaMonkey
Passwords & Permissions
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Robert Kaiser, Assigned: Robert Kaiser)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

Trunk
seamonkey2.1b2
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

7 years ago
Data Manager currently has no way to add permissions, only to change or reset/delete them. We should add a possibility to add them, so we can get e.g. the functionality of "Allow popups from this website" implemented correctly.
(Assignee)

Updated

7 years ago
Blocks: 588421

Updated

7 years ago
Blocks: 599097
(Assignee)

Comment 1

7 years ago
Created attachment 493474 [details] [diff] [review]
v1: possibility to add permissions

OK, here's the patch that allows this, adding a box at the bottom of the permissions panel that has an "Add" button, giving way for a host name entry box and a permission type selection, and adding a permission entry to the list when clicked again in that state. For any domain that has no permissions panel yet, the generic "*" domain entry enables that panel to allow adding permissions. The code for loading views gets functionality for opening that add box and prefilling values, ending up in basically the same state as what the "Allow popups from this website" menu entry does right now in SeaMonkey (that's also where this code will be used right away). Of course, a number of tests are added to make sure things work as expected.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #493474 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 2

7 years ago
This patch depends on the bug 588418 work, so marking that.
Depends on: 588418
(Assignee)

Comment 3

7 years ago
Created attachment 493544 [details] [diff] [review]
v1.1: switch view call separator to |

Here's the updated patch for the view call separator change in bug 588418.
Attachment #493474 - Attachment is obsolete: true
Attachment #493544 - Flags: review?(iann_bugzilla)
Attachment #493474 - Flags: review?(iann_bugzilla)

Comment 4

7 years ago
(In reply to comment #3)
> Created attachment 493544 [details] [diff] [review]
> v1.1: switch view call separator to |
> 
> Here's the updated patch for the view call separator change in bug 588418.

Bitrotted :(
applying bug588417-v1.1.diff
patching file suite/common/dataman/dataman.js
Hunk #2 FAILED at 94
Hunk #3 FAILED at 244
Hunk #4 FAILED at 355
3 out of 7 hunks FAILED -- saving rejects to file suite/common/dataman/dataman.js.rej
patching file suite/common/dataman/tests/browser_dataman_callviews.js
Hunk #2 FAILED at 64
1 out of 2 hunks FAILED -- saving rejects to file suite/common/dataman/tests/browser_dataman_callviews.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug588417-v1.1.diff
(Assignee)

Comment 5

7 years ago
(In reply to comment #4)
> Bitrotted :(

I don't think so. But it could very easily be needing the patch from bug 613795 to apply cleanly, as it's on top of that in my local system.
(Assignee)

Comment 6

7 years ago
Marking dependency because of how my patches are structured.
Depends on: 613795
(Assignee)

Comment 7

7 years ago
Created attachment 496221 [details] [diff] [review]
v1.2: update for test changes

In bug 588418, I added a few more test cases and that would have needed changes here anyhow, so I switched to a somewhat saner testing infrastructure as well (at first, I hadn't expected that this one would cover that many cases). This patch only contains updates for those changes.

The new patch is still on top of bug 613795, btw.
Attachment #493544 - Attachment is obsolete: true
Attachment #496221 - Flags: review?(iann_bugzilla)
Attachment #493544 - Flags: review?(iann_bugzilla)

Comment 8

7 years ago
(In reply to comment #7)
> Created attachment 496221 [details] [diff] [review]
> v1.2: update for test changes
> 
> In bug 588418, I added a few more test cases and that would have needed changes
> here anyhow, so I switched to a somewhat saner testing infrastructure as well
> (at first, I hadn't expected that this one would cover that many cases). This
> patch only contains updates for those changes.
> 
> The new patch is still on top of bug 613795, btw.

From first set of testing:
Go to permissions tab, select Add, look at type list, but don't actually add anything.
Go to another domain and permissions tab again, select add, look at type list, it has the type list repeated.
Keep going to different domains, permissions tab, click add, and look at type list, it keeps adding to the list.

Comment 9

7 years ago
Comment on attachment 496221 [details] [diff] [review]
v1.2: update for test changes

>+++ b/suite/common/dataman/dataman.js

>@@ -361,23 +364,50 @@ var gDomains = {
>           let viewdomain = gDomains.getDomainFromHost(host);
>           for (let i = 0; i < gDomains.displayedDomains.length; i++) {
>             if (gDomains.displayedDomains[i].title == viewdomain) {
>               gDomains.tree.view.selection.select(i);
>               gDomains.tree.treeBoxObject.ensureRowIsVisible(i);
>               break;
>             }
>           }
>+          if (gDomains.selectedDomain.title != viewdomain) {
>+            gDomains.tree.view.selection.select(0);
>+            gDomains.tree.treeBoxObject.ensureRowIsVisible(0);
>+          }
Here can we do something like:
let selection = 0;
for (let i = 0; i < gDomains.displayedDomains.length; i++) {
  if (gDomains.displayedDomains[i].title == viewdomain) {
    selection = i;
    break;
  }
}

then if we're adding a permission then check the flag to see if a permissions tab is enabled, if not set selection = 0 then do the {
  gDomains.tree.view.selection.select(selection);
  gDomains.tree.treeBoxObject.ensureRowIsVisible(selection);
}

yield setTimeout(nextStep, 0);

etc

Comment 10

7 years ago
If you select a domain with a permission and then change the permission to "Use Default", move to another domain and back again, the permission tab stays enabled even though there no permissions left until you close and reopen datamanager. We should be disabling the tab and, if appropriate, moving to any tab still enabled or to the next domain in the list or "*".
I've also noticed when you remove the last cookie from a domain, it removes the domain but leaves you with an empty cookie tab and no domain focussed, I'd expect it to move to either the next domain in the list or to "*".

Comment 11

7 years ago
Comment on attachment 496221 [details] [diff] [review]
v1.2: update for test changes

r- due to issue in comment 8, as for comment 9 I am okay with that either being done here or spun off into another bug.
Attachment #496221 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 12

7 years ago
Created attachment 497568 [details] [diff] [review]
v1.2.99: clear list on add, loading optimization

This should address both comment #8 and comment #9 - for the latter, I pulled a check for the add param into the domain loading, which is not completely nice, but at least it makes all domain selection be in a single place, which makes it worth that, IMHO.
Attachment #496221 - Attachment is obsolete: true
Attachment #497568 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 13

7 years ago
Comment on attachment 497568 [details] [diff] [review]
v1.2.99: clear list on add, loading optimization

This needs one more small change, coming in a minute.
Attachment #497568 - Attachment description: v1.3: clear list on add, loading optimization → v1.2.99: clear list on add, loading optimization
(Assignee)

Comment 14

7 years ago
Created attachment 497573 [details] [diff] [review]
v1.3: clear list on add, loading optimization

I only remembered to actually run the automated tests once I already had submitted the new patch, and they turned up the case of a not-yet-existing domain not having set that domain object yet. This patch covers that as well.
Attachment #497568 - Attachment is obsolete: true
Attachment #497573 - Flags: review?(iann_bugzilla)
Attachment #497568 - Flags: review?(iann_bugzilla)

Comment 15

7 years ago
Comment on attachment 497573 [details] [diff] [review]
v1.3: clear list on add, loading optimization

>+++ b/suite/common/dataman/dataman.js

How about setting:
let permAdd = (gDataman.viewToLoad[1] &&
               gDataman.viewToLoad[1] == "permissions" &&
               gDataman.viewToLoad[2] &&
               gDataman.viewToLoad[2] == "add");

and then using permAdd in the two if statements?

if (permAdd && selectIdx != 0 &&
    (!(viewdomain in gDomains.domainObjects) ||
     !gDomains.domainObjects[viewdomain].hasPermissions)) {
  selectIdx = 0; // Force * domain as we have a perm panel there.
}

if (permAdd) {
  gDataman.debugMsg("Adding permission");

r=me with that addressed/fixed
Attachment #497573 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 16

7 years ago
Applied those changes and pushed as http://hg.mozilla.org/comm-central/rev/76215aaa1e70
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b2

Updated

6 years ago
Depends on: 666102
(Assignee)

Updated

6 years ago
Duplicate of this bug: 677442

Updated

6 years ago
Blocks: 682193
You need to log in before you can comment on or make changes to this bug.