605499 - Add-ons manager doesn't display EULA for available add-ons that have one

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

After searching AMO for add-ons if the user tries to install one with an EULA then we should display it for them to accept.

The XUL for the UI to do this still exists but a couple of strings got removed along the way so we'd need to add these back a.s.a.p.

There is probably also sadly a small API change (additive only) to support this.

Comment 1

[Dave Townsend [:mossop]]

Blocking b7 for the strings at least.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Going to work on an automated test for this but I think we should try to get this reviewed and landed on trunk and relbranch a.s.a.p.

Turns out no API change was necessary, just needed to resurrect some strings from the 1.9.2 branch. I have not renamed the strings as they are exactly the same string in exactly the same context.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Ignore the logging in the last patch

Comment 4

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/550280c5f30b
Relbranch: http://hg.mozilla.org/mozilla-central/rev/22a3c0fcb8f1

Comment 5

[Francesco Lodolo [:flod]]

eulaHeader=%S requires that you accept the following End User License Agreement before installation can proceed:

What is %S? The add-on's name you're installing? Please add l10n comments ;-)

Comment 6

[Dave Townsend [:mossop]]

Pushed a quick l10n note
http://hg.mozilla.org/mozilla-central/rev/abd46d6b3534
http://hg.mozilla.org/mozilla-central/rev/051b5bc46443

Comment 7

[Dave Townsend [:mossop]]

Attachment: testcase rev 1

Simple testcase, installs an extension from the results that requires accepting the EULA.

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 484811 [details] [diff] [review]
testcase rev 1

This isn't passing in conjunction with other tests right now

Comment 9

[Dave Townsend [:mossop]]

Attachment: testcase rev 2

Added some errors and cleanups at the end of every test when there are unexpected installs available and made the tests that violate this clean themselves up. This will help some of our intermittent failures not blow up the following tests quite so much.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 485059 [details] [diff] [review]
testcase rev 2

> function end_test() {
>   Services.prefs.clearUserPref("extensions.update.url");
> 
>-  finish();
>+  // Test generates a lot of available installs so just cancel them all
>+  AddonManager.getAllInstalls(function(aInstalls) {
>+    aInstalls.forEach(function(aInstall) {
>+      aInstall.cancel();
>+    });
>+
>+    finish();
>+  });
> }

Pity we can't have have async cleanup functions yet.

>+// Tests tha installs and undoing installs show up correctly

Typo.

Comment 11

[Dave Townsend [:mossop]]

Landed the test: http://hg.mozilla.org/mozilla-central/rev/28675757affa

Comment 12

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

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101027 Firefox/4.0b8pre and the "Buzz It!" extension.

Btw. the monospace font we are using in this dialog is somewhat really ugly.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #12)
> Btw. the monospace font we are using in this dialog is somewhat really ugly.

Going to fix that in bug 601022.