Closed Bug 562790 Opened 14 years ago Closed 13 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: osunick, Assigned: mossop)

References

Details

(Whiteboard: [strings][d?])

Attachments

(6 files, 2 obsolete files)

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.
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.
Priority: -- → P2
(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.
Additional note for spec:

* The changes apply mainly to search results returned from the AMO API.
OS: Mac OS X → All
Hardware: x86 → All
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.
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)
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+
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?
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]
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]
blocking2.0: beta7+ → betaN+
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.
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.
(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)
Yeah for 4.0 I think the checkbox as henrik suggests would be best.
Whiteboard: [strings][strings patch can land] → [strings][strings landed]
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.
(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.
Depends on: 610505
Dave, any progress on putting these strings into action?
(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
Attached patch patch rev 1 (obsolete) — Splinter Review
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)
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-
Whiteboard: [strings][strings landed][has patch][needs review Unfocused] → [strings][strings landed][has patch]
(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]
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.
Justin, any comments?
Whiteboard: [strings][strings landed][has patch][waiting on try] → [strings][strings landed][has patch]
Attached patch patch rev 2 (obsolete) — Splinter Review
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
Whiteboard: [strings][strings landed][has patch] → [strings][strings landed][has patch][needs review Unfocused]
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-
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?]
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
Whiteboard: [strings][strings landed][has patch][d?] → [strings][strings landed][has patch][d?][waiting on try]
Attached patch patch rev 3Splinter Review
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)
Whiteboard: [strings][strings landed][has patch][d?][waiting on try] → [strings][strings landed][has patch][d?][needs review Unfocused]
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?]
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
Closed: 13 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?
Attached image 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.
Version: unspecified → Trunk
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.

Attachment

General

Created:
Updated:
Size: