760139 - Add new class to lib/ui/addons_blocklist.js to handle the Blocklist Window

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

As specified in bug 684679 comment 49, we need a new class to addons.js to handle the blocklist window.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I would say lets wait for the module refactor.

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0

Not sure what "handle" means here, but in the test bug we wanted to start this class and add a getElement method so we can also start developing a UI map for this component, so that's what this patch includes. 

Please clarify if I missed something, since those were not my bugs from the beginning there is a small chance I could understand things "my way"

Thanks

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.1

Sorry, missed to add the class in the write alphabetical order. 
Fixed that here in this follow up

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 644937 [details] [diff] [review]
patch v1.1

First thought on this patch is if we should better start with a new subfolder under libs called 'ui' and put individual window maps in there. So we have a better distinction between helper methods and ui oriented code. Dave, what are your thoughts on it?

>+function BlocklistWindow(aBrowserController) {
>+  this._controller = aBrowserController;

The internal controller has to be the one for the BlocklistWindow and not the browser. Not sure how you call this constructor but usually we want to hand over the controller of the underlying window. Also we probably need an opener method in the global scope which opens/gets the blocklist window.

>+  this._window = this._controller.window;

That we do not need.

>+      case "hardBlockedAddon":
>+        nodeCollector.queryNodes(".hardBlockedAddon");
>+        break;
>+      default:

So can you please point me to the appropriate xul file on MXR? I would be interested in a nearly complete list of elements we potentially would make use of in the near future. Would be great if you can come up with such a list at the same time.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Comment on attachment 644937 [details] [diff] [review]
> patch v1.1
> 
> First thought on this patch is if we should better start with a new
> subfolder under libs called 'ui' and put individual window maps in there. So
> we have a better distinction between helper methods and ui oriented code.
> Dave, what are your thoughts on it?

I like the idea of distinguishing these, and this suggestion makes sense to me.

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.0

I am sorry, comment 4 was not addressed in this patch. 
I think that plan will have to be moved in another refactoring bug

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

I meant comment 5

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 645682 [details] [diff] [review]
patch v2.0

This looks okay to me, but I would like Henrik to give an additional review and comment on whether the suggestion from comment 5 should be implemented in this patch.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 645682 [details] [diff] [review]
patch v2.0

Yes, please put this code in the appropriate location. I don't want to mess around with the addons module for new code which hasn't been landed yet.

Comment 10

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Comment on attachment 645682 [details] [diff] [review]
> patch v2.0
> 
> Yes, please put this code in the appropriate location. I don't want to mess
> around with the addons module for new code which hasn't been landed yet.

Henrik, just to make sure, can you please point to me the appropriate needed location? 
Judging by comment 8 this looks ok and I would not want to ping pong reviews just for guessing the location :)

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please use the window type mapped as filename. Which means put it under 'lib/ui/addons_blocklist.js'.

Comment 12

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.1

Added the class under lib/ui/addons_blocklist.js, as requested

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 646054 [details] [diff] [review]
patch v2.1

>Bug 760139 - Add new class to addons.js to handle the Blocklist Window. r=hskupin, r=dhunt

This needs an update.

>+ * @param {MozMillController} aController Controller of the window
>+ */
>+function BlocklistWindow(aController) {
>+  this._controller = aController;

My question I asked some days ago still hasn't been answered yet. Why do we have to pass in a controller?

>+   * @returns {MozMillController} Mozmill Controller

This is the 'Window controller', which should default to null.

>+  get controller() {
>+    // Get the controller for the newly opened window   
>+    return new mozmill.controller.MozMillController(this.open());

Ouch. No, please don't do those things. You will simply return this._controller.

>+    switch (type) {
>+      case "hardBlockedAddon":
>+        nodeCollector.queryNodes(".hardBlockedAddon");
>+        break;
>+      case "softBlockedAddon":
>+        nodeCollector.queryNodes(".softBlockedAddon");
>+        break;

I have asked for a list of elements and a link to MXR. I don't see those in any comments. Beside getElements() you probably also want getElement().

>+  open : function BlocklistWindow_open() {
>+    // Wait until the window has been opened
>+    mozmill.utils.waitFor(function () {
>+      window = mozmill.utils.getWindowByType("Addons:Blocklist");
>+      return !!window;
>+    }, "Window has been found.");
>+    
>+    return window;
>+  }

The open() method has to contain the code to open the window, means all the js code to trigger the blocklist ping. Also if the method succeed you want to set this._controller. I just wonder, isn't it a modal dialog?

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

The link is on the URL section in this bug - contains also the list of elements

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

You should have commented on it. I haven't gotten an email for that update.

So what we also need in this class are: moreInfo, bothMessage, and especially addonList. Without the latter it will be hard to get all the addons listed.

I would propose you create a test under lib/tests for this new class where we can test all the different elements.

Comment 16

[Maniac Vlad Florin (:vladmaniac)]

Assigning to Remus since he is back from PTO

Comment 17

[Remus Pop (:RemusPop)]

We would need to create an additional xml for blocklisting a restartless extension. Otherwise the test for the new methods would need to be a restart test.
Dave, any opinion on this one?

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Please go ahead and add the necessary addons to the blocklist XML and request review/feedback.

Comment 19

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3 (all branches)

I've updated the summary and commit message.
Regarding the questions from comment 13:
* it is not a modal dialog
* I removed the controller in the constructor, is not needed since we set the controller in the open method.

Also added getElement method and the other elements in getElements.

In the test I verified the window opening and the existence of the required elements, along with the blocked addon name.

Comment 20

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 648682 [details] [diff] [review]
patch v3 (all branches)


>+
>+const LOCAL_TEST_FOLDER = collector.addHttpResource("../../data/");
>+const ADDON = {name : "Inline Settings (Restartless)",
>+               url : LOCAL_TEST_FOLDER + "addons/extensions/restartless_inlinesettings.xpi"}
>+const BLOCKLIST_URL = LOCAL_TEST_FOLDER + "addons/blocklist/" +
>+                      "softblock_extension/blocklist.xml";
>+const PREF_BLOCKLIST = "extensions.blocklist.url";

Please separate constants which use LOCAL_TEST_FOLDER from the pref constant with a new line

>+
>+function setupModule() {
>+  controller = mozmill.getBrowserController();
>+  addonsManager = new addons.AddonsManager(controller);
>+  blocklistWindow = new blocklist.BlocklistWindow();
>+
>+  prefs.preferences.setPref(PREF_BLOCKLIST, BLOCKLIST_URL);
>+}
>+
>+function testBlocklistAPI() {

I think this naming would send to the backen API. I would suggest other 
naming here

>+  // Install the blocklisted add-on
>+  var md = new modalDialog.modalDialog(addonsManager.controller.window);
>+
>+  md.start(addons.handleInstallAddonDialog);
>+  controller.open(ADDON.url);
>+  md.waitForDialog();
>+
>+  // Open Blocklist Window
>+  blocklistWindow.open();

These comments are unnecessary as the method names are already clear. Please 
kill them

>+  controller.assert(function() {
>+    return addonList.length > 0;
>+  }, "There is at least one addon blocked");

Please use the new assertions module under lib/assertions.js
Please use it in all cases in the test

>+
>+  var bothMessage = blocklistWindow.getElement({type: "bothMessage"});
>+  controller.assertNode(bothMessage);

Please separate variable declarations from statements or callbacks with a 
new line. Do so in call cases in the test, as I seen more situations. Thanks

I think we are ok in rest, with those changes addressed please request review from 
Alex. Thanks Andreea!

Comment 21

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3.1 (all branches)

Addressed Vlad's requests, except the naming of the test which I believe reflects the whole class, seeing the big picture, there will be more methods added and tested.

Comment 22

[Alex Lakatos[:AlexLakatos]]

Comment on attachment 648688 [details] [diff] [review]
patch v3.1 (all branches)

>+// Include required modules
>+var addons = require("../addons");
>+var {expect} = require("../assertions");
>+var blocklist = require("../ui/addons_blocklist");
>+var modalDialog = require("../modal-dialog");
>+var prefs = require("../prefs");
Please keep this in alphabetical order. blocklist is above expect.

>+  prefs.preferences.setPref(PREF_BLOCKLIST, BLOCKLIST_URL);
Please close all tabs in the setupModule

Comment 23

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3.2 (all branches)

Reorganized modules and added closeAllTabs.

Comment 24

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 649199 [details] [diff] [review]
patch v3.2 (all branches)

Since the last changes are addressed successfully, moving forward with reviews to Dave. 

Sorry Alex, I would really want this to be checked in asap since its blocking a test.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 649199 [details] [diff] [review]
patch v3.2 (all branches)

>+++ b/lib/tests/testAddonsBlocklist.js
>+var addons = require("../addons");
>+var blocklist = require("../ui/addons_blocklist");
>+var {expect} = require("../assertions");

Re alphabetical order I agree, but here we have something new in our tests. It's a ui module which is not in the same folder as the other library modules. Personally I would put this into a separate block so that we do not mix up helper modules with ui modules. It could be included before all the other modules. Dave, what do you think?

>+const LOCAL_TEST_FOLDER = collector.addHttpResource("../../data/");
>+const ADDON = {name : "Inline Settings (Restartless)",
>+               url : LOCAL_TEST_FOLDER + "addons/extensions/restartless_inlinesettings.xpi"}
>+const BLOCKLIST_URL = LOCAL_TEST_FOLDER + "addons/blocklist/" +
>+                      "softblock_extension/blocklist.xml";
>+
>+const PREF_BLOCKLIST = "extensions.blocklist.url";

Splitting off the pref constant from the block is not enough. I would also separate the LOCAL_TEST_FOLDER declaration.

>+  var addonList = blocklistWindow.getElements({type: "addonList"});
>+
>+  expect.ok(addonList.length > 0, "There is at least one addon blocked");

Seeing this I think we should update our style guide to be more clear how to do this right in terms of whitespaces. In general blocks belong to each other. If multiple variables are getting declared at once they should be separated from the following code. But if you have a logical block like this there is no need to put a blank line in-between.

Also I think that this expect should be an assert here. If no blocked add-ons are available we can't test the remaining items, or?

>+  var bothMessage = blocklistWindow.getElement({type: "bothMessage"});
>+
>+  controller.assertNode(bothMessage);

Please don't use assertNode(). Replace it with expect.ok(bothMessage.getNode(), ...) and add a meaningful message to it. This also applies to other tests in this file.

>+  // Verify the name of the blocked addon
>+  var addonName = hardBlockedAddon.getNode().getAttribute("name");
>+
>+  expect.equal(addonName, ADDON.name, "Addon name should be identical - got " + 
>+               addonName + ", expected " + ADDON.name);

No need in declaring addonName. Just put everything into the equal() call. Also there is no need to use 'got', 'expected'. That's automatically done by equal().

>+++ b/lib/ui/addons_blocklist.js
>+function BlocklistWindow() { }

We should at least declare internal variables like this._controller. They shouldn't end-up to be undefined.

>+  open : function BlocklistWindow_open() {
>+    utils.updateBlocklist();
>+
>+    // Wait until the window has been opened
>+    mozmill.utils.waitFor(function () {
>+      window = mozmill.utils.getWindowByType("Addons:Blocklist");
>+      return !!window;
>+    }, "Window has been found.");

No need for the waitFor() call. We have utils.handleWindow(). See the software-update module for an example.

Comment 26

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> >+  var bothMessage = blocklistWindow.getElement({type: "bothMessage"});
> >+
> >+  controller.assertNode(bothMessage);
> 
> Please don't use assertNode(). Replace it with
> expect.ok(bothMessage.getNode(), ...) and add a meaningful message to it.
> This also applies to other tests in this file.

Regarding this suggestion, expect.ok return is "got '[object XULElement]'" when the element exists and passes, but if not the test fails with "undefined".

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andreea Matei [:AndreeaMatei] from comment #26)
> > Please don't use assertNode(). Replace it with
> > expect.ok(bothMessage.getNode(), ...) and add a meaningful message to it.
> > This also applies to other tests in this file.
> 
> Regarding this suggestion, expect.ok return is "got '[object XULElement]'"
> when the element exists and passes, but if not the test fails with
> "undefined".

What do you mean with "undefined"? Where does it appear? You should add more output. bothMessage.getNode() should return 'null' if the element is not present.

Comment 28

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3.3 (all branches)

Addressed all requests and added hardblockedMessage and softBlockedMessage to getElements.

My error with "undefined" was not at bothMessage, but when tried to check if softBlockedAddon was present.
The restartless addon blocked is hardblocked, in which case bothMessage and hardBlockedMessage are present, while softBlockedMessage is hidden.

I used an if-else block to also cover the case with softBlocked addon.
I would appreciate feedback regarding this case, if it should be treated differently, if we should install a softblocked addon as well. 

Looking into it and found this: https://wiki.mozilla.org/images/thumb/3/36/Addonblock.png/600px-Addonblock.png but is Firefox 4
From what I see there, softblocked addons are still usable if already installed (with the user accepting the risks) or can be disabled.
To install it after being blocklisted is not possible from what I've tried. 

Please correct me if I am wrong.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 650182 [details] [diff] [review]
patch v3.3 (all branches)

>+function setupModule() {
>+  prefs.preferences.setPref(PREF_BLOCKLIST, BLOCKLIST_URL);

You will have to reset the preference in teardownModule. Please place this method at the end of the file.

>+function testBlocklistAPI() {
>+  var md = new modalDialog.modalDialog(addonsManager.controller.window);
>+
>+  md.start(addons.handleInstallAddonDialog);

nit: just kill that empty line too.

>+  if(softBlockedMessage.getNode().hidden) {

For this unit test of the blocklist window we have to check all elements. It doesn't matter if an element is hidden or not. So do simply check if all the elements from the get_elements() method can be retrieved. That's really all. More logic will be in the appropriate functional tests.

nit: space after 'if'.

>+  open : function BlocklistWindow_open() {
>+    utils.updateBlocklist();
>+
>+    this._controller = utils.handleWindow("type", "Addons:Blocklist", undefined, false);
>+    return this._controller;

I don't see a reason why we should return the controller. All the code which affects the controller should stay internal to this class. If consumers need the controller they can request it via the property.

Otherwise looks way better now and I think only one more round should be necessary.

Comment 30

[Andreea Matei [:AndreeaMatei]]

I have looked today how to install a softblocked addon, finally I've found that the blocklist.xml will need to be updated with another attribute at versionRange. That is "severity", which takes values from 0 to 3 (lower value means the addon is only disabled and can still be used, 3 is blocked completly). If not set, by default is 3. 

To install another extension to be made softblocked, first we'll need to add another restartless addon and update the blocklist.xml. That will take place here or at bug 779094 where it did the first time?

Comment 31

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I would suggest opening a new bug to handle adding to or modifying the blocklist as needed. You can then set that bug as a blocker for this one.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Given that the new blocklists will be up on mozqa.com soon, please update the test for the lib module to make use of the mixed blocklist. That way we can verify that all will work as expected.

Comment 33

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v4

Updated blocklist folder and the test to get the mixed blocklist.
We now have two extensions installed, one hardblocked, the other softblocked and all elements checked.
Also added a teardown function to reset the pref, as requested.

Comment 34

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 652713 [details] [diff] [review]
patch v4

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

Looks good. Just a few minor nits with trailing whitespace.

::: lib/tests/testAddonsBlocklist.js
@@ +58,5 @@
> +  var hardBlockedAddon = blocklistWindow.getElement({type: "hardBlockedAddon"});
> +  expect.ok(hardBlockedAddon.getNode(), "Expected the addon to be hard blocked");
> +
> +  // Verify the name of the hardblocked addon
> +  expect.equal(hardBlockedAddon.getNode().getAttribute("name"), ADDONS[1].name, 

nit: trailing whitespace

@@ +65,5 @@
> +  var softBlockedAddon = blocklistWindow.getElement({type: "softBlockedAddon"});
> +  expect.ok(softBlockedAddon.getNode(), "Expected the addon to be soft blocked");
> +
> +  // Verify the name of the softblocked addon
> +  expect.equal(softBlockedAddon.getNode().getAttribute("name"), ADDONS[0].name, 

nit: trailing whitespace

::: lib/ui/addons_blocklist.js
@@ +12,5 @@
> +function BlocklistWindow() {
> +  controller = this._controller;
> +}
> +
> +BlocklistWindow.prototype = {  

nit: trailing whitespace

@@ +17,5 @@
> +  /**
> +   * Get the controller of the window
> +   *
> +   * @returns {WindowController} Window Controller
> +   */  

nit: trailing whitespace

@@ +23,5 @@
> +    return this._controller;
> +  },
> +
> +  /**
> +   * Open the Blocklist Window                 

nit: trailing whitespace

Comment 35

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v4.1 [checked-in]

Removed trailing whitespaces.

Comment 36

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/3aab7a7995a7

Comment 37

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 653692 [details] [diff] [review]
patch v4.1 [checked-in]

Somehow missed to add r+ to this.