431727 - AMO and FF3 disagree on star rating for extensions

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: API, defect)

Description

[Dave Garrett]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008050104 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008050104 Minefield/3.0pre

Using Flagfox as an example:  Right now I see a rating of 4.10 / 5 in Developer Tools.  On the AMO public page this gets rounded up to 5 stars.  When I search for "flag" in the add-on manager of Firefox 3 I see the listing for Flagfox at the top and it gets rounded down to 4 stars.

This could be filed as an AMO or Firefox bug, but I'm putting this under Firefox as AMO's rounding makes more sense.  4-5 needs to be 5 stars, otherwise the only way to get 5 stars is to have an average rating of exactly 5; no critical reviews at all. (or 4.50+, if that's the rounding cutoff here, which would still be a very small target)  Thus, the averages should go as follows:  4-5 -> 5 stars, 3-4 -> 4 stars, 2-3 -> 3 stars, 1-2 -> 2 stars, 0-1 -> 1 star, no rating -> 0 stars.  This follows what you'd normally see with a star rating; 1 star is the lowest you can get if you've been rated at all.

Though, regardless of which way you round, it needs to be the same on both ends.

Reproducible: Always

Comment 1

[Dave Townsend [:mossop]]

I'm inclined to suggest that this be fixed in the API rather than the client. This would mean that whatever rounding AMO uses in the display they can also use in the API and changes would not impact the client. What are your thoughts Laura?

Comment 2

[Laura Thomson :laura]

Easy to fix in the API.  Assigning it to myself.

Comment 3

[Laura Thomson :laura]

Attachment: Use ceil just like AMO does for stars.

Comment 4

[Chris Pollett]

Hey Laura,

this line                
if (isset($addon['Addon']['averagerating']) && $api_version < 1) {


had me a little concerned. Suppose isset($addon['Addon']['averagerating']) fails
then one hits the else clause line:
$rating = $addon['Addon']['averagerating'];
Maybe make two nested if's? Also, when testing it, it seemed I seemed to get the doubling case. Is this number we are doubling always going to be an integer?
Or do we need to round it up too?
  

Comment 5

[Laura Thomson :laura]

Attachment: Rewritten so the logic works

Comment 6

[Chris Pollett]

Comment on attachment 319853 [details] [diff] [review]
Rewritten so the logic works 

looks good, r+ing it.

Comment 7

[Laura Thomson :laura]

Committed in r12916.

Comment 8

[Stephen Donner [:stephend] Not actively reading bugmail]

This needs to be pushed live before verifying, right?  (I still see a rating of 4 stars in the Add-ons Manager window.)

(i.e., is there any way to trick out extensions.getAddons.search.url;https://services.addons.mozilla.org/%LOCALE%/%APP%/api/%API_VERSION%/search/%TERMS%/all/10/%OS%/%VERSION% to give me the patched API, or does this really just need to be landed in services before I can verify?)

Thanks!

Comment 9

[Chris Pollett]

I am guessing if you changed services.addons.mozilla.org part to previews it should have the patched version stephend. (I got sorta sick on the weekend, so sorry about being slow in replying).

Comment 10

[Chris Pollett]

I mean replace it with remora-trunk... I am getting confused on which of our staging servers actually shows trunk.

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

While verifying this, I found bug 433431, but this bug is Verified FIXED by doing the following:

* setting |extensions.getAddons.search.url| to |https://remora-trunk.php5stage.mozilla.com/%LOCALE%/%APP%/api/%API_VERSION%/search/%TERMS%/all/10/%OS%/%VERSION%| in Firefox 3 beta 5 -and-
* searching for "flagfox", which shows it at 5 stars.

Production's |https://services.addons.mozilla.org/%LOCALE%/%APP%/api/%API_VERSION%/search/%TERMS%/all/10/%OS%/%VERSION%| shows it at 4 stars.

Verified FIXED!