Cannot filter reviews by version

Bug #688112 reported by Michael Nelson on 2010-12-09
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Ratings and Reviews server
Low
Anthony Lenton

Bug Description

Currently rnrclient.get_reviews() allows filtering by language, origin and distroseries, but not version.

According to the mockup at https://wiki.ubuntu.com/SoftwareCenter/RatingsAndReviews#Presenting%20reviews we need to filter or sort by version.

Notes to QA:

Using *the latest* rnrclient.py you should be able to test this by using just the public part of the api. First ensure that there's a reasonable spread of reviews for a certain package/app on the vps, then use a Python snippet like the following:

>>> from rnrclient import RatingsAndReviewsAPI
>>> api_public = RatingsAndReviewsAPI('http://....../reviews/api/1.0')
>>> api_public.get_reviews(language='en', origin='ubuntu', distroseries='maverick', version='1.2.3', packagename='foobar', appname='baz')

All arguments here except packagname are optional. Omitting language, origin, distroseries or version will fetch reviews for any value of that parameter, and omitting appname will fetch reviews for the package itself.

Related branches

tags: added: proj-rnr-10.12
description: updated
Michael Nelson (michael.nelson) wrote :

In fact, the above mockup leaves me wondering whether we should be filtering by distroseries for this api request at all? If we instead returned all the reviews sorted by (1) version and then (2) rating/helpfulness factor, then the distroseries might just be another attribute of the review detail like this:

Reviews of this version (1.3)
1) 5 stars
This was excellent - blah blah.
OS: Ubuntu 10.10
2) 4 stars
This was ok - blah blah.
OS: Ubuntu 10.04

Reviews of version 1.2:
1) .....

I can't see anything yet on the spec about this situation.

Matthew Paul Thomas (mpt) wrote :

I have no interest in sorting or grouping by OS version at the moment.

We could do that in future, if it becomes common that people rate the same version of an application noticably differently depending on which OS version they're using. But I don't know why they'd do that.

On Tue, Jan 4, 2011 at 2:42 PM, Matthew Paul Thomas <email address hidden> wrote:
> I have no interest in sorting or grouping by OS version at the moment.

Yes, that's what I assumed from the mockups - the problem is that the
URL for the reviews currently *forces* you to request reviews for a
certain distroseries, which obviously needs to change:

Currently it is:
/1.0/en/ubuntu/lucid/2vcard/

And based on the above, I'm assuming it should be something like:

/1.0/en/2vcard/

so that we can return the reviews sorted with most recent versions
first. Or enable an optional version: /1.0/en/2vcard/2.2/ which would
return the reviews with the specified version first, followed by the
others (which is I think what the mockup implies?)

Michael Vogt (mvo) wrote :

I think there are two seperate issues here.

One is that the sorting should be "version first, then usefulness" (currently its only sorting by usefulness
and then submission date afaict).

The other one is that our current schema forces origin and distroseries. I don't think we can omit origin
otherwise reviews for ppa:firefox-really-badly-packaged will be showing the really-well-packaged-main-ubuntu
firefox reviews (and vice versa).

If we change distroseries to version version I think we need to ensure there is a additional parameter in the
api so that the version that the user has (either installed or as candidate) is submited and no reviews for
newer versions are shown if those are not installable. Otherwise the user will see reviews about new features t
hat are not available for his/her version. Especially important for fast moving stuff (unity comes to mind).

I'm not sure we need to change the API to include a versionnumber in the query though instead of the distroseries.
For the main archive we know what the latest version for natty (we can ask LP). For launchpad PPAs we can do the
same. We need to "carry over" previous review when a new distro series opens though (that is a burden).

I have no strong feelings about versionnumber vs distroseries, I do think that for some apps it will make a difference
on what release they run because of compat issues. The removal of the notification area comes to mind in natty. So
a app relying with the nofitcation area in 10.10 will get very different reviews in 11.04. Its not a very common thing
I'm sure, but I don't think the amount of cases like this is zero.

Anthony Lenton (elachuni) wrote :

Agree that the url for getting reviews will need to change (as noodles
points out) to make specifying the distroseries optional at best.

I think the version should be added at the same level (I'd imagine a POST
variable for each) so that you can optionally specify a version.

I feel like some times we'll want to restrict reviews to *only* certain
values (only reviews in English, only reviews for i386) but other times we'd
want to just push certain reviews to the top of the list (reviews in French
first, then in English, or reviews for my current version first, then other
versions).

This *could* be done by allowing the client to specify this separately for
each argument, with something like

version_first='0.2.2-0ubuntu1', lang_only='en'

Then again, this is ugly.

Optionally, we could make the api only allow you to restrict the reviews you
retrieve (make every argument mean fetch *only* reviews matching this
parameter), and leave sorting up to the client. For this to work each review
the api returns should inform its own version, origin, language, etc., but
this is the case already.

This would allow the client to also *sort* by different attributes (not only
push certain reviews to the top of the list), which might make it an interesting
option, as well as making the api neater (we only have one argument for
each review attribute, with a clear semantic).
Then again, this is more work for the client.

Ftr, we're currently only sorting reviews explicitly by usefulness. Any further
ordering is just Django's implementation and might change.

About omitting origin, the same applies: I think we shouldn't *enforce* the
user to provide an origin, but beyond that we could provide a way to restrict
reviews to *only* a certain origin (and let the client handle showing the right
origin first in the list), or also provide a way to request certain origin to be
listed first.

I wouldn't make the version attribute special in the sense of being able to
request reviews "at this version or older, or newer only if they're
installable". I think that's hard to explain, and doesn't provide all that
much extra value. If you see a review for a version that's newer than the one
you'd install it just means you might not be able to install it. I think it
falls in the same 'ah, this might not apply as much to me' bag as any other
partial match (like reviews for other architectures or distroseries).

Changed in rnr-server:
status: New → Confirmed
importance: Undecided → Medium
tags: added: kb-task
Anthony Lenton (elachuni) wrote :

Discussed the implementation for this with noodles.

We really don't want to rely on GET arguments as it would prevent caching.
Instead we're going to:
 * Add 'version' to the current url patter for requesting reviews
 * Allow all parameters in the url (language, origin, distroseries, version) to be specified as "any", that'll retrieve reviews for any value of that parameter.
 * Any parameter that's not specified as "any" will retrieve only reviews for that value of the parameter. That is, all parameters will affect the retrieved set of reviews, not just the order in which they're returned.

With this approach, to simulate a specific ordering of the reviews, the client app can:
 * First request the specific reviews for the exact language, version, architecture and distroseries the user has.
 * Retrieve paged responses with those reviews until they run out.
 * Once those have run out, it can go relaxing the conditions one at a time (or all together) to get larger review sets. When each page of reviews is retrieved, it should check to remove any reviews that are already being shown to the user.

Some reviews will be returned in multiple result sets -- for example, reviews for the exact version, architecture and language of the user will also be returned when requesting reviews for the exact version and architecture in any language. As all reviews come with their id, the client app can avoid showing repeated reviews to the user. Note that this problem already exists with almost any paginated approach, as a new review could come in that pushes the last review from page N on to page N+1.

If reviews are going to be shown in a single growing scrolled area, then having to remove one or two reviews from a page of retrieved reviews may not be a problem. If we really want to show X new reviews every time, one way to provide this would be to retrieve additional pages until you've got at least X new reviews to show, and keep any reviews after the first X around to show on the next page, if it is eventually requested by the user.

Changed in rnr-server:
assignee: nobody → Anthony Lenton (elachuni)
status: Confirmed → In Progress
Changed in rnr-server:
importance: Medium → Low
status: In Progress → Fix Committed
description: updated
description: updated
description: updated
Dave Morley (davmor2) wrote :

The api works but has no setting in sc currently

Changed in rnr-server:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers