426003 - Version History still seems to filter by OS platform

Status: VERIFIED FIXED

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

Description

[Basil Hashem [:baz]]

Using Windows, I can still can't see all the previous versions available for this addon.

Compare the output of this page with:
https://addons.mozilla.org/en-US/firefox/admin/addons?q=PicLens+%5B5579%5D

You'll notice several public MacOS versions of this add-on that do not appear on this page.

We should be doing NO filtering on this page.

Comment 1

[Reed Loden [:reed]]

Attachment: patch - v1

Something like this?

Comment 2

[Reed Loden [:reed]]

Attachment: patch - v1.1

Oops, try again without the extra semicolon.

Comment 3

[Reed Loden [:reed]]

Attachment: patch - v1.2

I'm cursed tonight, it seems. Try again with proper indentation. Sigh.

Comment 4

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

Reed, does your patch fix 425326, too?

Comment 5

[Fred Wenzel [:wenzel]]

To answer on behalf of reed, no, bug 425326 is not fixed by this. Over there, it's likely something that has to happen in JavaScript, while reed's patch is PHP magic only.

Comment 6

[Fred Wenzel [:wenzel]]

Attachment: Upper case "Addons" for controller name

Your patch looks good, Reed, however I noticed that the controller name "addons" we check for needs an upper case first letter. The attachment changes this.

That being said, r+ from my side.

Note: Bug 425326 shows pretty clearly here when this is applied: On the piclens page, this results in *two* install buttons per version, both labeled "Add to Firefox (Windows)", but that's to be fixed over there, not here.

Comment 7

[Reed Loden [:reed]]

Comment on attachment 312743 [details] [diff] [review]
Upper case "Addons" for controller name

(In reply to comment #6)
> Your patch looks good, Reed, however I noticed that the controller name
> "addons" we check for needs an upper case first letter. The attachment changes
> this.

You mean this has been broken for a while? Look at this line:

>-        if (APP_ID == APP_FIREFOX && !empty($compatible_apps)
>-            && !($this->name == 'addons' && $this->action == 'versions')) {

Note the 'addons' there... so, that's been broken?

Either way, you're way more familiar with this code than me, so if that's the only problem, then r=me for your one character change.

Comment 8

[Fred Wenzel [:wenzel]]

(In reply to comment #7)
> You mean this has been broken for a while?

Yes, so it seems. Thanks for the r+, will check this in later.

Comment 9

[Fred Wenzel [:wenzel]]

I have checked this in to r11823. Thanks for the patch, reed.

Comment 10

[Basil Hashem [:baz]]

Reed, Fred, can you verify if this has fixed bug 426793? If not, I think we should make bug 426793 a dependency since we are not listing all the available versions.

Thanks.

Comment 11

[Fred Wenzel [:wenzel]]

Yes, we are now listing all OS-specific versions no matter what platform the visitor is on.

Bug 426793 now shows sandboxed versions for sandboxed add-ons too. Note that for public add-ons, sandboxed versions (upcoming updates, for example) are not being shown. Is that the intended behavior, Basil? If not, we need to reopen bug 426793.

Comment 12

[Basil Hashem [:baz]]

That makes sense Fred. Once logged in, the complete version history can show the sandbox'ed versions as well.

Comment 13

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

Verified FIXED using both http://remora-trunk.php5stage.mozilla.com and https://addons.mozilla.org instances with their respective URLs from comment 0 and the URL field in Bugzilla.