417606 - See all search results always number 10

Status: VERIFIED FIXED

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

Description

[Laura Thomson :laura]

This is due to the API optimization we made to only return 10 results.

Comment 1

[Laura Thomson :laura]

Attachment: Temp fix for API.

This is a hackish fix for the API.  It will return the correct number of empty <addon /> elements, so that the Add-ons manager sees the correct number.  Mossop tells me ths should work.

If we push this to production on SAMO users will get the correct experience and we can change the client in b4.

Comment 2

[Michael Morgan [:morgamic]]

Comment on attachment 303379 [details] [diff] [review]
Temp fix for API.

Just promise this goes away for b4.

Comment 3

[Laura Thomson :laura]

You couldn't make me leave it in. :)

Comment 4

[Laura Thomson :laura]

Committed patch in r10355.

Comment 5

[Dave Townsend [:mossop]]

To fix this long term we want to get rid of the empty elements and just add a result count somewhere. The attributes "totalresults" and "returnedresults" or similar were suggested which seems fine by me.

Comment 6

[Laura Thomson :laura]

I'll add that attribute for b4.  Reassigning to me.

Comment 7

[Laura Thomson :laura]

This depends on the versioning scheme in 
https://bugzilla.mozilla.org/show_bug.cgi?id=418642

I have a patch up for review for that, once approved, this one is trivial to fix.

Comment 8

[Laura Thomson :laura]

Fixed in r10615.

The version number can be added directly after the API, e.g.:
https://services.addons.mozilla.org/%LOCALE%/%APP%/api/[version]/search/foo

The current version of the API to be used with Fx3 b4 is 0.9, so the config
value for extensions.getAddons.search.url should change to 
https://services.addons.mozilla.org/%LOCALE%/%APP%/api/0.9/search/%TERMS%

The XML returned looks like
<searchresults total_results="n">
...as before, minus the dummy <addon /> elements...
</searchresults>

Comment 9

[Laura Thomson :laura]

Reassigning to Mossop so he can change the Addons Mgr accordingly.

Comment 10

[Dave Townsend [:mossop]]

I have the patch for this mostly implemented, waiting on finalisation of the spec wrt multiple OS add-ons

Comment 11

[Laura Thomson :laura]

In r10671: Add translation table for outputting the platform oses the client is
looking for.

Kind of an evil hardcoded hack which I am reproducing from 
http://mxr.mozilla.org/mozilla/source/webtools/update/update/VersionCheck.php

Both of these should get fixed at the same time, and draw on the AMO
db.  Will file a bug for that separately.

Refs #418659 as well.

Comment 12

[Dave Townsend [:mossop]]

Attachment: Toolkit portion

We need to start passing the OS to AMO in the API. This extends the urlformatter a little to make that possible.

Comment 13

[Dave Townsend [:mossop]]

Attachment: EM patch rev 1

This is an update to the results parser that leaves it able to parse the current API schema (version 0) and the next major revision of the schema (version 1).

Currently AMO isn't able to provide version 1 however I have tested this against a staging server and also provided unit testing for both the old and new schemas.

The plan will be to land this code apart from the config change. Then once AMO has been updated we can just land the config change to switch to it. When that happens it will fix the search result count and the issue of OS specific extensions not being offered correctly.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 305582 [details] [diff] [review]
Toolkit portion

Please ignore the firefox.js portion of this patch.

Comment 15

[Dave Townsend [:mossop]]

Attachment: example of the new schema

Example of the new schema for reference.

Comment 16

[Dave Townsend [:mossop]]

Forgot to add that the EM patch is on top of the patch from bug 417424 so that has to land first.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 305582 [details] [diff] [review]
Toolkit portion

r=me to the non-firefox.js
changes!

Comment 18

[Dave Townsend [:mossop]]

The code can land as soon as it is reviewed, the config change has to wait for these bugs to be fixed and pushed live.

Comment 19

[Dave Townsend [:mossop]]

Laura, do you have an idea of when the new schema and url version will be going live on AMO?

Comment 20

[Dave Townsend [:mossop]]

Checked in all except the config change for now, holding open for that.

Checking in toolkit/mozapps/extensions/src/nsAddonRepository.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsAddonRepository.js,v  <--  nsAddonRepository.js
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug404024.js,v
done
Checking in toolkit/mozapps/extensions/test/unit/test_bug404024.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug404024.js,v  <--  test_bug404024.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug417606.js,v
done
Checking in toolkit/mozapps/extensions/test/unit/test_bug417606.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug417606.js,v  <--  test_bug417606.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/data/test_bug404024.xml,v
done
Checking in toolkit/mozapps/extensions/test/unit/data/test_bug404024.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/data/test_bug404024.xml,v  <--  test_bug404024.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/data/test_bug417606.xml,v
done
Checking in toolkit/mozapps/extensions/test/unit/data/test_bug417606.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/data/test_bug417606.xml,v  <--  test_bug417606.xml
initial revision: 1.1
done
Checking in toolkit/components/urlformatter/src/nsURLFormatter.js;
/cvsroot/mozilla/toolkit/components/urlformatter/src/nsURLFormatter.js,v  <--  nsURLFormatter.js
new revision: 1.8; previous revision: 1.7
done
Checking in toolkit/components/urlformatter/tests/unit/test_urlformatter.js;
/cvsroot/mozilla/toolkit/components/urlformatter/tests/unit/test_urlformatter.js,v  <--  test_urlformatter.js
new revision: 1.2; previous revision: 1.1
done

Comment 21

[Dave Townsend [:mossop]]

Will need to do the config changes for Thunderbird as well once SAMO is updated.

Comment 22

[Dave Townsend [:mossop]]

The updates to services.addons.mozilla.org are due to go live on Tuesday so I will look to committing the config change on Wednesday all being well.

Comment 23

[Dave Townsend [:mossop]]

Attachment: config changes

This is just the config change and a matching change for Thunderbird which philor agreed I could do.

Comment 24

[Dave Townsend [:mossop]]

Checked in, this is all done now.

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.308; previous revision: 1.307
done
Checking in mail/app/profile/all-thunderbird.js;
/cvsroot/mozilla/mail/app/profile/all-thunderbird.js,v  <--  all-thunderbird.js
new revision: 1.109; previous revision: 1.108
done

Comment 25

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008032704 Minefield/3.0pre

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dave, with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.4pre) Gecko/20100409 Namoroka/3.6.4pre ID:20100409033633 I still see only 10 results mentioned in the Get Addons pane. Shouldn't that have been fixed? Or do we have another regression?

Comment 27

[Dave Townsend [:mossop]]

(In reply to comment #26)
> Dave, with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
> rv:1.9.2.4pre) Gecko/20100409 Namoroka/3.6.4pre ID:20100409033633 I still see
> only 10 results mentioned in the Get Addons pane. Shouldn't that have been
> fixed? Or do we have another regression?

Perhaps you're seeing bug 554133?

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #27)
> Perhaps you're seeing bug 554133?

Looks like. While testing on other platforms and with Shiretoko and Gran Paradiso I have noticed that this bug hasn't been fixed or regressed again. Lets keep the conversation on bug 554133. Thanks Dave.