Closed Bug 435129 Opened 16 years ago Closed 16 years ago

Revise detection mechanism for "Add to Firefox" button

Categories

(addons.mozilla.org Graveyard :: Public Pages, defect, P2)

Product:
addons.mozilla.org Graveyard
Component:
Public Pages
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: baz, Assigned: clouserw)

Details

Attachments

(6 files)

Browsing IE Tab add-on using Fx2
45.67 KB, image/png
Details
Three ways to see install add-on button
32.83 KB, image/png
Details
Addon vs. Fx version compat matrix
35.06 KB, application/pdf
Details
Buttons with C#1,2,3 edge cases
212.06 KB, image/png
Details
test data that stephend requested
23.72 KB, patch
Details | Diff | Splinter Review
revised method
7.10 KB, patch
rdoherty
: review+
Details | Diff | Splinter Review 
We need to revise the behavior of AMO when displaying the "Add to Firefox" button. In particular, during the period between the availability of Firefox 2.0 and before Firefox 3.0 final is available.

Take the example of IE Tab (https://addons.mozilla.org/en-US/firefox/addon/1419).

The add-on had previous versions that are compatible with Firefox 2.0.*. As soon as they uploaded a version that has minVersion with 3.0 compatibility, it trumps the display when an Fx 2 user comes to the details page.

See attachment for what is displayed.

The link "Upgrade Firefox" is erroneous since (at this point) Firefox 3 has not been released yet. So, Fx 2 users are being directed to update their Firefox to 2.0.0.14 and that may be what they are running already.

We need the install button to be smarter and deal with the case where a compatible add-on for a previous version of Firefox is available and show that.
Target Milestone: 3.4.4 → 3.4.5
Assignee: nobody → cpollett
Chris: If you need to get the latest version compatible with a specific browser version, would making an Addon API call via AJAX do the trick?
It's not really feasible to use AJAX to solve this without extensively rewriting the existing code. The problem is the version id's are being used as part of the div tag id's within the document. We can't do firefox version detection on the server because of the netscalers and caching. Once the version id has been written into the document one is stuck with what versions of Firefox that button will be compatible with. Currently, the most recently created version id  for an addon is written. One possible solution would be to send the most recent non-alpha/beta version number for Firefox to the client. If the client has that version of Firefox, but the addon claims to be for some larger alpha or beta version, then we revise the text for some older version of the addon may work, to make it clear that the most recent version of the addon is for an alpha version of Firefox, but some older version may work.
cpollett is away, reassigning and pushing to 3.4.6
Assignee: cpollett → nobody
Target Milestone: 3.4.5 → 3.4.6
Assignee: nobody → jboriss
What about sending FF2 and FF3 buttons, setting display: none on the FF2 button, and using JS to swap out based on the useragent?  We can do this for 2 or 3 options, perhaps FF2, FF3 stable, and FF3 latest.
That would work, but I am unsure if this is flexible enough for the future: In general we'd always need to be able to replace the install button by an older version that matches your current browser version, provided the latest version of the add-on is only compatible with Firefox x+1 where x is the latest stable release.
Target Milestone: 3.4.6 → 3.4.7
Here's three cases of the user viewing and add-on and what they should see in each case.  If anyone knows of another case or thinks these should be different, please comment.
Boriss, this is good but there are some edge cases that we still need to deal with:
- I'm attaching a matrix that demonstrates the problem
- I think we need the "Upgrade Firefox" button to be a bit dynamic esp. since we may have Firefox 3.0.4 and Firefox 3.1 products alive at the same time
A few notes: 

Case C#1:

The link here should direct to page explaining the beta is for testing only and has risks, and provides a download link.  One way is for it to link to a page like http://www.mozilla.com/en-US/firefox/all-beta.html, but with this page to either describe beta briefly or provide a link to a page like http://www.mozilla.com/en-US/firefox/3.0b3/releasenotes.

Case C#2:

This would like to a download page for the latest version of Firefox, such as http://www.mozilla.com/en-US/firefox/all.html.

Case C#3:  Same as C#1.

These three cases aren't radically different from the first three, because I don't feel they need to be.  There's three basic cases that exist: upgrade, downgrade, download.  The special case introduced in 1 and 2 is for the "upgrade" to be to a version which hasn't shipped.  This is essentially still an upgrade, but should warn the user that the version is in beta and explain the risk. 

I also added a version number in C and C#2, because this answers the users' possible assumption that they already have the right version and the system just displays the same message for everyone.
I'm happy with this solution Boriss, thank you - targeting for 3.5.1 unless someone has time to pick this up for 8/18's code freeze for AMO 3.4.7.
Assignee: jboriss → nobody
Target Milestone: 3.4.7 → 3.5.1
Priority: -- → P2
Assignee: nobody → clouserw
omg
Stephen, do we have test cases to cover the matrix?
There's also a 3rd and 4th dimension (simpler, but still relevant) to the matrix: products and platforms.
(In reply to comment #11)
> Stephen, do we have test cases to cover the matrix?

We don't.  I wrote them down somewhere during my initial testing (in a notebook, I think (!)), but they didn't make it to:

https://litmus.mozilla.org/show_test.cgi?searchType=by_category&product_id=8&branch_id=18&testgroup_id=85&subgroup_id=851

Note that I don't think I can automate this easily with Selenium.

I can, however, plow through writing these testcases manually -- it'll just take some time.

By when do we need this?  Is 4.0.2 realistic?
Talked with Wil this morning and probably not, if we want to do this right.  Having proper test coverage in place first before editing code would be ideal.  We could set a goal for 4.0.2 to have the test cases written and have the development happen in the next milestone, if that is more realistic.
Comment 14 sounds good; I'll start writing them now, then, in-between my other projects' work.
http://docs.google.com/Doc?id=ddxxfpqq_162c78jvp28 is where the crop of testcases based on the attachment in comment 8 lives.

Note that it doesn't cover the products and platforms dimension as Wil noted in comment 12, but that testing can go alongside.

I'm also wondering if it will be actually possible to test _all_ of the combinations (I'm sure a few add-ons will still have left their old compat ranges, though.)

Question: Along with the proposed button changes here, are we also then expanding Advanced Search to bring back the beta/pre-version ranges?
(In reply to comment #16)
> http://docs.google.com/Doc?id=ddxxfpqq_162c78jvp28 is where the crop of
> testcases based on the attachment in comment 8 lives.

While making test cases I noticed a lot of your test cases say 2.0* (or 2.0.* in the pastebin you sent me).  For the record, I'm assuming both of those are 2.0.0.*
Here's the test data.  Designed to be loaded alongside the current remora-test-data SQL.  This will create 52 add-ons starting with add-on 100.  Which add-on is which test case is documented in the SQL.  

This should test all the cases stephend requested.
(In reply to comment #17)
> (In reply to comment #16)
> > http://docs.google.com/Doc?id=ddxxfpqq_162c78jvp28 is where the crop of
> > testcases based on the attachment in comment 8 lives.
> 
> While making test cases I noticed a lot of your test cases say 2.0* (or 2.0.*
> in the pastebin you sent me).  For the record, I'm assuming both of those are
> 2.0.0.*

Yeah; I just typed them directly from https://bugzilla.mozilla.org/attachment.cgi?id=333067.  Thanks for catching that.
Target Milestone: 4.0.2 → 4.0.3
Stepehend, is that sql good enough?  Are your test cases something I can run while I'm writing this patch?
The data looks fine; it matches what I need to test.  Once the SQL is loaded, we can start testing using http://docs.google.com/Doc?id=ddxxfpqq_162c78jvp28 (which itself is just based on the matrix in comment 8.  Testing in real-time alongside patch-writing would be awesome; happy to work with you on that.
Boriss and I agreed that Case C#1 and C#3 were the same thing so I'll just call that C#1 from now on.  I don't think this is going to be as much work as I originally thought.  Comparing to attachment 333067 [details] our current code does:

Case A:
- Is correct

Case B:
- Text is above the button instead of below.  I think we put it there because if a user is logged in we offer an option to ignore the version check below the button.  However, I can move this below.

Case C:
- We just say "Upgrade Firefox to use this add-on" and also suggest that an older version of the add-on might work.  We're abandoning the older version suggestion in this bug - is that on purpose?

Case C#1:
- This is pretty wordy; can we make it shorter and still maintain the meaning?  Something like "This add-on requires the not yet released _Firefox N_."

Case C#2:
- This is essentially what we have now with slightly different wording.

Aside from the questions in C and C#1 I noticed we offer the ignore version check option to users in Case B but not Case C.  That seems inconsistent.  Is there a reason for that or should I add it in?
+1 on the suggestion for Case C#1 (/Case C#3); I'm kind of boggling at why we would offer an override for Case B anyway?  I know we added this at one point (even when we "knew" the add-on to be incompatible), but I'll have to dig and resurrect the reasoning behind that before I spout off more.
(Oh, so users can right-click and download, hack the install.RDF and use it for testing, perhaps?)
> I know we added this at one point
> (even when we "knew" the add-on to be incompatible), but I'll have to dig and
> resurrect the reasoning behind that before I spout off more.

bug 425477
Thanks, Wil; Boriss, do you agree we should add the option to ignore the version check too?
To answer my own questions:

- I decided not to abandon the older version suggestion
- I made the text shorter
- I added the suggestion to case B and C

I don't know of a staging site using the test data.  stephend: you can use my dev site if you like. http://clouserw.khan.mozilla.org/remora/site/en-US/firefox/

Please test both logged in and out.
Attached patch revised method — — Splinter Review 
stephend is still doing the functional review but I want to get a code review in also.
Attachment #345308 - Flags: review?(rdoherty)
Comment on attachment 345308 [details] [diff] [review]
revised method

Code looks fine and works for me.

I see an un-gettext'd string:
links.parent().after(sprintf("<strong>This add-on requires the not yet released...
though.
Attachment #345308 - Flags: review?(rdoherty) → review+
There's a typo here: "This add-on requires the not yet released Firefox 3.1b1" -- that should read "not-yet released" (compound modifier).

I'm almost done testing, and I think this can land anytime...
Thanks, Landed in r19432.
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: push-needed
Resolution: --- → FIXED
Verified FIXED using the test data on Wil's server, in comment 27.
Status: RESOLVED → VERIFIED
Keywords: push-needed
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: