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

VERIFIED FIXED in mozilla2.0b9

Status

()

Toolkit
Add-ons Manager
P2
major
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: osunick, Assigned: mossop)

Tracking

(Depends on: 1 bug)

Trunk
mozilla2.0b9
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus ?

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [strings][d?])

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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

7 years ago
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.
(Assignee)

Updated

7 years ago
Priority: -- → P2
(Reporter)

Comment 2

7 years ago
(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.
(Reporter)

Comment 3

7 years ago
Additional note for spec:

* The changes apply mainly to search results returned from the AMO API.
OS: Mac OS X → All
Hardware: x86 → All
(Reporter)

Comment 4

7 years ago
Pinging- it will be sad if we don't have support for this in Fx4.
Severity: normal → major
Erm, this is the first I've heard of this... and string freeze is in one day.
(Assignee)

Comment 6

7 years ago
Created attachment 475590 [details] [diff] [review]
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.
Attachment #475590 - Flags: review?(bmcbride)
(Assignee)

Updated

7 years ago
blocking2.0: --- → betaN+
Whiteboard: [strings]
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 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
Attachment #475590 - Flags: review?(bmcbride) → review+
cc'ing l10n - pre-landing strings here, we will include a good localization note

(Dave/Blair: insert a good localization note!)
blocking2.0: betaN+ → beta7+
(Assignee)

Comment 10

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

Landed the strings http://hg.mozilla.org/mozilla-central/rev/7676e2b0fea0
Attachment #475590 - Attachment description: strings → strings [checked in]
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?
(Assignee)

Comment 12

7 years ago
Created attachment 475749 [details] [diff] [review]
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.
Attachment #475749 - Flags: review?(bmcbride)
Attachment #475749 - Flags: review?(bmcbride) → review+
Can we get the followup strings landed and this moved to a betaN+ blocker, please?
Whiteboard: [strings] → [strings][strings patch can land]
(Assignee)

Comment 14

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

Already landed: http://hg.mozilla.org/mozilla-central/rev/bcca2c0c4805
Attachment #475749 - Attachment description: follow-up strings patch → follow-up strings patch [checked in]
(Assignee)

Updated

7 years ago
blocking2.0: beta7+ → betaN+
Created attachment 476965 [details]
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
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.
(Assignee)

Comment 17

7 years ago
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.
Created attachment 477234 [details]
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)
(Reporter)

Comment 19

7 years ago
Yeah for 4.0 I think the checkbox as henrik suggests would be best.
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings patch can land] → [strings][strings landed]
(Assignee)

Updated

7 years ago
Assignee: jboriss → dtownsend
(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.
(Assignee)

Comment 21

7 years ago
(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.
(Assignee)

Updated

7 years ago
Depends on: 610505

Comment 22

7 years ago
Dave, any progress on putting these strings into action?
(Assignee)

Comment 23

7 years ago
(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
(Assignee)

Comment 24

7 years ago
Created attachment 492694 [details] [diff] [review]
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.
Attachment #492694 - Flags: review?(bmcbride)
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed] → [strings][strings landed][has patch][needs review Unfocused]
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>purchase3@tests.mozilla.org</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.
Attachment #492694 - Flags: review?(bmcbride) → review-
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][has patch][needs review Unfocused] → [strings][strings landed][has patch]
(Assignee)

Comment 26

7 years ago
(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.
Whiteboard: [strings][strings landed][has patch] → [strings][strings landed][has patch][waiting on try]

Comment 27

7 years ago
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.
(Assignee)

Comment 28

7 years ago
Justin, any comments?
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][has patch][waiting on try] → [strings][strings landed][has patch]
(Assignee)

Comment 29

7 years ago
Created attachment 496190 [details] [diff] [review]
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.
Attachment #492694 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][has patch] → [strings][strings landed][has patch][needs review Unfocused]
(Assignee)

Updated

7 years ago
Attachment #496190 - Flags: review?(bmcbride)
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 on attachment 496190 [details] [diff] [review]
patch rev 2

DENIED!

(See comment 30)
Attachment #496190 - Flags: review?(bmcbride) → review-
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][has patch][needs review Unfocused] → [strings][strings landed][has patch]
Whiteboard: [strings][strings landed][has patch] → [strings][strings landed][has patch][d?]
(Assignee)

Comment 32

7 years ago
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
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][has patch][d?] → [strings][strings landed][has patch][d?][waiting on try]
(Assignee)

Comment 33

7 years ago
Created attachment 501695 [details] [diff] [review]
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.
Attachment #496190 - Attachment is obsolete: true
Attachment #501695 - Flags: review?(bmcbride)
(Assignee)

Updated

7 years ago
Whiteboard: [strings][strings landed][has patch][d?][waiting on try] → [strings][strings landed][has patch][d?][needs review Unfocused]
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
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.
Attachment #501695 - Flags: review?(bmcbride) → review+
Whiteboard: [strings][strings landed][has patch][d?][needs review Unfocused] → [strings][strings landed][has patch][d?]
(Assignee)

Comment 35

7 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus?
Resolution: --- → FIXED
Whiteboard: [strings][strings landed][has patch][d?] → [strings][d?]
Target Milestone: --- → mozilla2.0b9
(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?
(Assignee)

Comment 37

7 years ago
Created attachment 502549 [details]
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.
Blocks: 630462
Version: unspecified → Trunk
Depends on: 683105
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.