562790 - Design support for paid add-ons in add-ons manager

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Nick Nguyen [:osunick]]

Quick spec:

1. Some add-ons will be pay-only as part of the marketplace initiative, so when the API returns them, they will return a price (in USD for now) and a flag indicating their 'paid only' status.  
2. Paid add-ons in search results will not have an 'Add to Firefox' button but instead have a 'Learn More' button that goes to the add-on's page on AMO.  The price of the add-on should also appear.
3. Many if not most add-ons will have both paid and free versions- in that case both should appear, as they will have slightly different names and descriptions.  It should be clear that the free add-on is totally free
4. Users should be able to filter the results for free/paid/both.  Default setting should be both.

Comment 1

[Gubment Cheez]

opinion: if you need money from Firefox add-ons, you're in the wrong market stream
I would never pay for an add-on, just as I would never pay for a browser.

Comment 2

[Nick Nguyen [:osunick]]

(In reply to comment #1)
> opinion: if you need money from Firefox add-ons, you're in the wrong market
> stream
> I would never pay for an add-on, just as I would never pay for a browser.

It's more about creating opportunity for developers- it's up to them to convince people to pay for their add-ons with something worthwhile.  Some people will never pay for an add-on, and that's fine with us- we want to make both paid and free add-ons better.  Free add-ons raise the quality bar for any add-on that wants revenue.

Comment 3

[Nick Nguyen [:osunick]]

Additional note for spec:

* The changes apply mainly to search results returned from the AMO API.

Comment 4

[Nick Nguyen [:osunick]]

Pinging- it will be sad if we don't have support for this in Fx4.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Erm, this is the first I've heard of this... and string freeze is in one day.

Comment 6

[Dave Townsend [:mossop]]

Attachment: strings [checked in]

We're late to the game here, my fault for overlooking this. I have come up with a simple plan with shorlander for how we can still get this in for Firefox 4 though, we're just going to have to pre-land a couple of strings today and then follow-up with the implementation.

These strings allow us to change the "Install" button to "Purchase for x..." as well as adding a sort by price option. Filtering between pay and free add-ons is probably not going to happen for this first release but the sorting at least allows you to separate them.

Comment 7

[Alex Limi (:limi) — Firefox UX Team]

A label like "Buy ($2.99)" might be a bit shorter — since we have the price on the button already, it'll certainly be wide enough already. :)

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 475590 [details] [diff] [review]
strings [checked in]

>+<!ENTITY cmd.purchaseAddon.accesskey          "u">

Probably want to add a localization note here, linking it to the entity in extensions.properties

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

cc'ing l10n - pre-landing strings here, we will include a good localization note

(Dave/Blair: insert a good localization note!)

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 475590 [details] [diff] [review]
strings [checked in]

Landed the strings http://hg.mozilla.org/mozilla-central/rev/7676e2b0fea0

Comment 11

[Francesco Lodolo [:flod]]

I'd also add a note in the .properties file explaining that the accesskey is in the .dtd

I think it's the first time I see that happening, is there a specific reason?

Comment 12

[Dave Townsend [:mossop]]

Attachment: follow-up strings patch [checked in]

Yeah I mainly put them in the dtd because there is no need to mess around with formatting the strings, but this was is going to be simpler for the localisers, plus I mucked up the localization note in the dtd.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get the followup strings landed and this moved to a betaN+ blocker, please?

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 475749 [details] [diff] [review]
follow-up strings patch [checked in]

Already landed: http://hg.mozilla.org/mozilla-central/rev/bcca2c0c4805

Comment 15

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: How free and paid add-ons could display in the add-ons manager

Attached is one way we could show free and paid add-ons.  Obviously clicking on the price of an add-on would not, in Firefox 4.0, directly purchase the add-on.  In a future version, if the user is logged into AMO, it may after a quick in-content confirmation step

Comment 16

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

Do we really want to separate the search results into 3 different groups (installed, free, and paid)? I assume that people will not find the extensions they are searching for. Right now, I even trapped a couple of times into the situation that the wrong list was chosen and no extension have been found, because "My Add-ons" was selected. A checkbox which states to include paid add-ons would probably better. Also do we have strings for your proposed mockup? The patch attached to this bug doesn't list them.

Comment 17

[Dave Townsend [:mossop]]

No we can't do the mockup shown here, perhaps it is intended for post-Firefox 4. Me and Stephen did discuss this kind of thing but for now we're just going with the sorting option as a way to separate them out.

Comment 18

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: free and paid add-ons in one search category with different labels

(In reply to comment #17)
> No we can't do the mockup shown here, perhaps it is intended for post-Firefox
> 4. Me and Stephen did discuss this kind of thing but for now we're just going
> with the sorting option as a way to separate them out.

Yes, that would be Firefox 4.n.  Just changing the labels would be enough for 4.0, but we probably need strings that distinguish free and paid rather than paid and "normal" (see attached)

Comment 19

[Nick Nguyen [:osunick]]

Yeah for 4.0 I think the checkbox as henrik suggests would be best.

Comment 20

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #19)
> Yeah for 4.0 I think the checkbox as henrik suggests would be best.

Yeah, this sounds like a good solution.

Comment 21

[Dave Townsend [:mossop]]

(In reply to comment #20)
> (In reply to comment #19)
> > Yeah for 4.0 I think the checkbox as henrik suggests would be best.
> 
> Yeah, this sounds like a good solution.

Well it isn't the solution that we landed strings for so I'm just going to proceed with what we do have strings for, if we decide that it is definitely wrong then we can consider changing it and incurring l10n change then.

Comment 22

[Axel Hecht [:Pike]]

Dave, any progress on putting these strings into action?

Comment 23

[Dave Townsend [:mossop]]

(In reply to comment #22)
> Dave, any progress on putting these strings into action?

The code work is complete I'm just writing some automated tests. I expect this to be up for review before the end of the week

Comment 24

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Feel like taking this Blair? Implements the additional properties returned from AddonRepository searches (this is kind of an API change but not one that can break extensions in anyway I can see) and the frontend changes to display the new buttons. Both sides are tested.

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 492694 [details] [diff] [review]
patch rev 1

(In reply to comment #24)
> Feel like taking this Blair?

By "take" I assume you mean "review" :)


>+  // Also convert any purchase cost into cents
>+
>+  var price = 0;

Nit: move that comment one line down.

>+  if (aObj.purchaseAmount) {
>+    // Strip off any starting currency symbols
>+    var stripped = PRICE_REGEXP.exec(aObj.purchaseAmount);
>+    if (stripped && stripped[0]) {
>+      price = parseFloat(stripped[0]);
>+      if (isNaN(price))
>+        price = 0;
>+    }
>+  }
>+
>+  item.setAttribute("purchaseAmount", price * 100);

I assume AMO be giving the amount as a localized string, in which case parsing the value isn't going to work. eg:
* Not all locales use "." to signify the decimal place (many locales use ",").
* Different locales/currencies group numbers differently. Its not inconveievable for an addon to cost 1000 Yen (which is only USD$12) - depending on the locale, that number could be grouped as "1 000.00", "1,000.00", "1.000,00", etc.
* Not all locales/currenies have the currency symbol at the start.

Either way, this will conflict with bug 614416. Or at least, the change to INTEGER_FIELDS will conflict - but I wonder if the new method for sorting will make this easier (eg, by not needing to mess with the decimal place).

>         let item = createItem(aObj, aIsInstall, aIsRemote);
>         item.setAttribute("relevancescore", score);
>-        if (aIsRemote)
>+        if (aIsRemote) {
>           gCachedAddons[aObj.id] = aObj;
>+          if (aObj.purchaseURL)
>+            self._sorters.showprice = true;

Should also be hiding the price sorter when viewing only installed addons (its not any use then). Bonus points for remembering the sort state dependant on whether the list is filtered on remote/local (although that'll mess with the visual hierachy a bit).

>+    var purchase = document.getElementById("detail-purchase-btn");
>+    if ("purchaseURL" in aAddon && aAddon.purchaseURL) {

Nit: swap these two lines.

>+          } else if (this.mControl.mAddon.purchaseURL) {
>+            this._progress.hidden = true;
>+            showPurchase = true;
>+            this._purchaseRemote.label = gStrings.ext.formatStringFromName("addon.purchase.label", [this.mControl.mAddon.purchaseAmount], 1)

Nit: Missing semicolon. And wrap this line so it's not so long.


>diff --git a/toolkit/mozapps/extensions/test/browser/browser_purchase.js b/toolkit/mozapps/extensions/test/browser/browser_purchase.js
...
>+// Tests that linking through to buy add-ons works

Nit: This tests more than just linking.

>+    gCategoryUtilities = new CategoryUtilities(gManagerWindow);

Nit: This isn't used anywhere in this test.

>+  is(get_node(items[0], "name").value, "Ludicrously Expensive Add-on", "Add-on 0 should be correct");
>+  is_element_visible(get_purchase_btn(items[0]), "Add-on 0 purchase button should be visible");
>+  is(get_node(items[1], "name").value, "Cheap Add-on", "Add-on 1 should be correct");
>+  is_element_visible(get_purchase_btn(items[1]), "Add-on 1 purchase button should be visible");
>+  is(get_node(items[2], "name").value, "Reasonable Add-on", "Add-on 2 should be correct");
>+  is_element_visible(get_purchase_btn(items[2]), "Add-on 2 purchase button should be visible");
>+  is(get_node(items[3], "name").value, "Free Add-on", "Add-on 3 should be correct");
>+  is_element_visible(get_install_btn(items[3]), "Add-on 3 install button should be visible");
>+  is(get_node(items[4], "name").value, "More Expensive Add-on", "Add-on 4 should be correct");
>+  is_element_visible(get_purchase_btn(items[4]), "Add-on 4 purchase button should be visible");

Would be nice to have this testing the text of the purchase buttons too.

>+  is(get_node(items[0], "name").value, "Free Add-on", "Add-on 0 should be correct");
>+  is(get_node(items[1], "name").value, "Cheap Add-on", "Add-on 1 should be correct");
>+  is(get_node(items[2], "name").value, "Reasonable Add-on", "Add-on 2 should be correct");
>+  is(get_node(items[3], "name").value, "More Expensive Add-on", "Add-on 3 should be correct");
>+  is(get_node(items[4], "name").value, "Ludicrously Expensive Add-on", "Add-on 4 should be correct");

s/should be correct/should be in expected position/

>+  EventUtils.synthesizeMouseAtCenter(priceSorter, { }, gManagerWindow);

Nit: Output something saying that the sorting order was changed. Its a pain to debug tests without output like that.

>+  <!-- Fails because the add-on doesn't match the platform -->
>+  <addon>
>+    <name>FAIL</name>
>+    <type id="1">Extension</type>
>+    <guid>[e-mail]</guid>
>+    <version>2.0</version>
>+    <authors>
>+      <author>
>+        <name>Test Creator - Last Passing</name>
>+        <link>http://localhost:4444/creatorLastPassing.html</link>
>+      </author>
>+    </authors>
>+    <status id="4">Public</status>
>+    <all_compatible_os>
>+      <os>WINNT</os>

I'm guessing the OS will always be "XPCShell" for these tests? Might want to add a comment to that effect (or change the OS value to be something that's never used anywhere) - at first glance, it looks like that addon will be included in the results on Windows.

Comment 26

[Dave Townsend [:mossop]]

(In reply to comment #25)
> I assume AMO be giving the amount as a localized string, in which case parsing
> the value isn't going to work. eg:
> * Not all locales use "." to signify the decimal place (many locales use ",").
> * Different locales/currencies group numbers differently. Its not
> inconveievable for an addon to cost 1000 Yen (which is only USD$12) - depending
> on the locale, that number could be grouped as "1 000.00", "1,000.00",
> "1.000,00", etc.
> * Not all locales/currenies have the currency symbol at the start.

Given that AMO only uses $s for contributions and has no plans that I know of to extend to supporting other currencies for now I don't think it is worth spending much more time on making this perfect for all locales right now. Unless there is a quick fix I think this is probably good enough until AMO changes at which point if we have to do a fix to support then we can.

> Either way, this will conflict with bug 614416. Or at least, the change to
> INTEGER_FIELDS will conflict - but I wonder if the new method for sorting will
> make this easier (eg, by not needing to mess with the decimal place).

Updated to work but still doing the multiplication for now

> >         let item = createItem(aObj, aIsInstall, aIsRemote);
> >         item.setAttribute("relevancescore", score);
> >-        if (aIsRemote)
> >+        if (aIsRemote) {
> >           gCachedAddons[aObj.id] = aObj;
> >+          if (aObj.purchaseURL)
> >+            self._sorters.showprice = true;
> 
> Should also be hiding the price sorter when viewing only installed addons (its
> not any use then). Bonus points for remembering the sort state dependant on
> whether the list is filtered on remote/local (although that'll mess with the
> visual hierachy a bit).

The price sorter is already hidden for local searches, we only show it in that block which is only for remote add-ons.

Comment 27

[Axel Hecht [:Pike]]

FWIW, I think Nick mentioned going beyond dollars when I was last in MV, and he still with us. Doesn't have that many implications on what plans are today.

Comment 28

[Dave Townsend [:mossop]]

Justin, any comments?

Comment 29

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Updated from comments, we'll see what Justin has to say to decide if we need to put more effort into the sorting issue.

Comment 30

[Justin Scott [:fligtar]]

We plan to support multiple currencies with marketplace and will probably have some sort of mapping of locales to default currencies that our logged-in users should be able to change.

For the Add-ons Manager, presumably the default currency for the locale would have to suffice.

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 496190 [details] [diff] [review]
patch rev 2

DENIED!

(See comment 30)

Comment 32

[Dave Townsend [:mossop]]

Clarified with Justin that all add-ons in the returned list should have the same currency type so we don't need to worry about currency conversion, just the numeric value

Comment 33

[Dave Townsend [:mossop]]

Attachment: patch rev 3

This fixes the sorting problem by also passing out the clean payment amount from the source XML. I've noted in the spec for this that this needs to be a JS interpretable number.

Comment 34

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 501695 [details] [diff] [review]
patch rev 3

Looks good. Just one small thing:

>+const PRICE_REGEXP = /\d*(?:\.\d*)?$/;

This is unused now.

Comment 35

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/54b7f0bf58ff

We should probably get litmus tests for this dome at some point, but realistically we can't do that until the marketplace is actually a reality.

Comment 36

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

(In reply to comment #35)
> We should probably get litmus tests for this dome at some point, but
> realistically we can't do that until the marketplace is actually a reality.

Does it mean there is no way for me to verify this fix for the moment? Without checking the patch, which parts of the attached mockups have been finally implemented? Do we need follow-up bugs?

Comment 37

[Dave Townsend [:mossop]]

Attachment: Implemented screenshot

(In reply to comment #36)
> (In reply to comment #35)
> > We should probably get litmus tests for this dome at some point, but
> > realistically we can't do that until the marketplace is actually a reality.
> 
> Does it mean there is no way for me to verify this fix for the moment? Without
> checking the patch, which parts of the attached mockups have been finally
> implemented? Do we need follow-up bugs?

You could point extensions.getAddons.search.url to a custom XML file for testing, the testcase includes such an XML file.

The attached mockups were attached after we landed the strings for this bug so they don't really match what we have so I've attached a shot of what was actually implemented. I don't believe follow-up bugs are worthwhile at this point, I think we should wait until the marketplace actually launches to figure out how this really works out and make any necessary UI tweaks at that point.

Comment 38

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

Verified fixed the button functionality and the correct label by using the following XML file with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0.1) Gecko/20100101 Firefox/4.0.1:

http://hg.mozilla.org/mozilla-central/raw-file/e6591ea9b27b/toolkit/mozapps/extensions/test/browser/browser_purchase.xml