Closed
Bug 562790
Opened 15 years ago
Closed 14 years ago
Design support for paid add-ons in add-ons manager
Categories
(Toolkit :: Add-ons Manager, defect, P2)
Toolkit
Add-ons Manager
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)
(deleted),
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details |
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•15 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•15 years ago
|
Priority: -- → P2
Reporter | ||
Comment 2•15 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•15 years ago
|
||
Additional note for spec: * The changes apply mainly to search results returned from the AMO API.
Updated•15 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Reporter | ||
Comment 4•14 years ago
|
||
Pinging- it will be sad if we don't have support for this in Fx4.
Severity: normal → major
Comment 5•14 years ago
|
||
Erm, this is the first I've heard of this... and string freeze is in one day.
Assignee | ||
Comment 6•14 years ago
|
||
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•14 years ago
|
blocking2.0: --- → betaN+
Whiteboard: [strings]
Comment 7•14 years ago
|
||
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•14 years ago
|
||
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+
Comment 9•14 years ago
|
||
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•14 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]
Comment 11•14 years ago
|
||
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•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #475749 -
Flags: review?(bmcbride) → review+
Comment 13•14 years ago
|
||
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•14 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•14 years ago
|
blocking2.0: beta7+ → betaN+
Comment 15•14 years ago
|
||
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•14 years ago
|
||
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•14 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.
Comment 18•14 years ago
|
||
(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•14 years ago
|
||
Yeah for 4.0 I think the checkbox as henrik suggests would be best.
Assignee | ||
Updated•14 years ago
|
Whiteboard: [strings][strings patch can land] → [strings][strings landed]
Assignee | ||
Updated•14 years ago
|
Assignee: jboriss → dtownsend
Comment 20•14 years ago
|
||
(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•14 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 | ||
Comment 23•14 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•14 years ago
|
||
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•14 years ago
|
Whiteboard: [strings][strings landed] → [strings][strings landed][has patch][needs review Unfocused]
Comment 25•14 years ago
|
||
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•14 years ago
|
Whiteboard: [strings][strings landed][has patch][needs review Unfocused] → [strings][strings landed][has patch]
Assignee | ||
Comment 26•14 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•14 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 | ||
Updated•14 years ago
|
Whiteboard: [strings][strings landed][has patch][waiting on try] → [strings][strings landed][has patch]
Assignee | ||
Comment 29•14 years ago
|
||
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•14 years ago
|
Whiteboard: [strings][strings landed][has patch] → [strings][strings landed][has patch][needs review Unfocused]
Assignee | ||
Updated•14 years ago
|
Attachment #496190 -
Flags: review?(bmcbride)
Comment 30•14 years ago
|
||
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•14 years ago
|
||
Comment on attachment 496190 [details] [diff] [review] patch rev 2 DENIED! (See comment 30)
Attachment #496190 -
Flags: review?(bmcbride) → review-
Assignee | ||
Updated•14 years ago
|
Whiteboard: [strings][strings landed][has patch][needs review Unfocused] → [strings][strings landed][has patch]
Updated•14 years ago
|
Whiteboard: [strings][strings landed][has patch] → [strings][strings landed][has patch][d?]
Assignee | ||
Comment 32•14 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•14 years ago
|
Whiteboard: [strings][strings landed][has patch][d?] → [strings][strings landed][has patch][d?][waiting on try]
Assignee | ||
Comment 33•14 years ago
|
||
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•14 years ago
|
Whiteboard: [strings][strings landed][has patch][d?][waiting on try] → [strings][strings landed][has patch][d?][needs review Unfocused]
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Comment 34•14 years ago
|
||
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+
Updated•14 years ago
|
Whiteboard: [strings][strings landed][has patch][d?][needs review Unfocused] → [strings][strings landed][has patch][d?]
Assignee | ||
Comment 35•14 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
Closed: 14 years ago
Flags: in-testsuite+
Flags: in-litmus?
Resolution: --- → FIXED
Whiteboard: [strings][strings landed][has patch][d?] → [strings][d?]
Target Milestone: --- → mozilla2.0b9
Comment 36•14 years ago
|
||
(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•14 years ago
|
||
(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.
Updated•14 years ago
|
Version: unspecified → Trunk
Comment 38•13 years ago
|
||
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.
Description
•