Last Comment Bug 588417 - Add ability to add permissions in Data Manager
: Add ability to add permissions in Data Manager
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Passwords & Permissions (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b2
Assigned To: Robert Kaiser (not working on stability any more)
:
Mentors:
: 677442 (view as bug list)
Depends on: 666102 DataManager 588418 613795
Blocks: 588421 599097 682193
  Show dependency treegraph
 
Reported: 2010-08-18 08:02 PDT by Robert Kaiser (not working on stability any more)
Modified: 2011-08-25 19:59 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1: possibility to add permissions (24.35 KB, patch)
2010-11-26 16:41 PST, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
v1.1: switch view call separator to | (24.40 KB, patch)
2010-11-27 17:55 PST, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
v1.2: update for test changes (24.36 KB, patch)
2010-12-08 11:58 PST, Robert Kaiser (not working on stability any more)
iann_bugzilla: review-
Details | Diff | Review
v1.2.99: clear list on add, loading optimization (24.76 KB, patch)
2010-12-14 12:15 PST, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
v1.3: clear list on add, loading optimization (24.82 KB, patch)
2010-12-14 12:21 PST, Robert Kaiser (not working on stability any more)
iann_bugzilla: review+
Details | Diff | Review

Description Robert Kaiser (not working on stability any more) 2010-08-18 08:02:18 PDT
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.
Comment 1 Robert Kaiser (not working on stability any more) 2010-11-26 16:41:57 PST
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.
Comment 2 Robert Kaiser (not working on stability any more) 2010-11-26 16:42:57 PST
This patch depends on the bug 588418 work, so marking that.
Comment 3 Robert Kaiser (not working on stability any more) 2010-11-27 17:55:17 PST
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.
Comment 4 Ian Neal 2010-12-07 15:12:11 PST
(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
Comment 5 Robert Kaiser (not working on stability any more) 2010-12-08 10:31:30 PST
(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.
Comment 6 Robert Kaiser (not working on stability any more) 2010-12-08 10:35:40 PST
Marking dependency because of how my patches are structured.
Comment 7 Robert Kaiser (not working on stability any more) 2010-12-08 11:58:36 PST
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.
Comment 8 Ian Neal 2010-12-13 16:03:18 PST
(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 Ian Neal 2010-12-14 08:45:40 PST
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 Ian Neal 2010-12-14 09:14:35 PST
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 Ian Neal 2010-12-14 10:09:15 PST
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.
Comment 12 Robert Kaiser (not working on stability any more) 2010-12-14 12:15:02 PST
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.
Comment 13 Robert Kaiser (not working on stability any more) 2010-12-14 12:18:21 PST
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.
Comment 14 Robert Kaiser (not working on stability any more) 2010-12-14 12:21:15 PST
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.
Comment 15 Ian Neal 2010-12-14 13:48:35 PST
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
Comment 16 Robert Kaiser (not working on stability any more) 2010-12-15 06:17:12 PST
Applied those changes and pushed as http://hg.mozilla.org/comm-central/rev/76215aaa1e70
Comment 17 Robert Kaiser (not working on stability any more) 2011-08-25 18:05:22 PDT
*** Bug 677442 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.