current reviews-stats is confusing to users

[Michael Vogt]

Hi,

the current review-stats API is confusing to the user. Because we provide global stats only the user may see:
- 10 reviews for foo, but only 5 of them are against ubuntu, the other against random PPAs
- 5 reviews for bar, but none against 10.10 versions of bar, all against 11.04

The result is that in the UI he sees e.g. 10 reviews, but clicking on the details he only sees some (or none).

Ideally we should only provide stats for the reviews the user can see. This is a problem of course because
it needs to be both fast on the server and the client and it needs to factor in multiple origin.

This is really a "discuss how we can do it" bugreport at this point :)

QA Notes:
Server functionality QA'd with:
1) http://184.82.116.62/reviews/api/1.0/review-stats/ubuntu/natty/ (verify stats for lots packages, but not 'lightyears')
2) http://184.82.116.62/reviews/api/1.0/review-stats/ubuntu/maverick/ (verify stats for 7 packages only, including lightyears)
3) http://184.82.116.62/reviews/api/1.0/review-stats/ubuntu/lucid/ (verify no stats)
4) http://184.82.116.62/reviews/api/1.0/review-stats/ubuntu/any/ (verify lots of stats, including lightyears)

mvo: can you pls add QA notes when this is qa'able with the USC client? Thanks!

[Michael Vogt (mvo)]

An additional topic that came up was that we may want to weight differently depending on the version. I.e. reviews for old versions contribute differently to the average (that is no longer a average then ;)

[Matthew Paul Thomas (mpt)]

Straw-man proposal: The average rating for a software item, as shown to someone running a particular Ubuntu release, should be the mean of the latest 50 ratings for versions of the software that are less than or equal to the latest version available for that Ubuntu release.

From there, probably the biggest improvement would be to weight ratings for newer versions more heavily than ratings for older versions. If we did that, maybe it would no longer be necessary to limit it to the latest N ratings. Or maybe that would still be desirable for performance reasons.

[Michael Nelson (michael.nelson)]

Just some thoughts looking at possible implementations while between bugs: regarding review-stats per distroseries, it seems we just need to add a model ReviewStatsForRepository that aggregates the stats per repository (and possibly remove the stats on SoftwareItem itself if they're not useful to us).

The urls would go back to:
/reviewsapp/1.0/review-stats/ubuntu/natty/
/reviewsapp/1.0/review-stats/ubuntu/natty/updates-last-[1,3.7]-days/

That should fix the initial confusion (ensuring that the overall stats match the reviews seen for an app).

We can then look at improving the actual stats for each based on the above.

[Michael Nelson (michael.nelson)]

Note: see the discussion on bug 686553 also. I understand from there and mpt's comment above that the ideal solution have:

/reviewsapp/1.0/review-stats/ubuntu/natty/

returning the stats for the reviews of all apps based on their version number - "ratings for versions of the software that are less than or equal to the latest version available for that Ubuntu release."

We don't currently have that information (something like SoftwareItemInRepository - which would include a current_version attribute). Adding this was discussed as part of another bug: https://bugs.launchpad.net/rnr-server/+bug/675495/comments/3 but not included as yet.

[Michael Nelson (michael.nelson)]

If we do add a SoftwareItemInRepository with a current_version attribute, we'd need to check with LP to update the current_version whenever we receive a review for a version greater than (what we expect) the current to be. Also, doing database queries based on deb-version comparisons (different to string comparisons) might be hairy (there is a postgres plugin for this, but I've no idea how well it performs).

[Michael Vogt (mvo)]

As discussed in the call we will have a API like this:

/reviewsapp/1.0/review-stats/ubuntu/natty/
/reviewsapp/1.0/review-stats/any/any/

[Anthony Lenton (elachuni)]

The best model for supporting this would seem to be to have a new model, SoftwareItemInRepository or something, which would basically have a software_item, a repository, and review stats aggregation info. I think it would be best if Reviews then drops the 'repository' foreign key and replace 'softwareitem' for a foreign key to SoftwareItemInRepository (or whatever it's called :) ). We can then aggregate stats on both SoftwareItemInRepo and SoftwareItem (for when stats are requested across all origins and distroseries).

Thinking this over, this will still cause us to have server-side processing when all reviews for a certain origin but all distroseries are requested, for instance. To avoid that completely we could add *two* new models, one that adds the origin to SoftwareItemIn, the other that adds the distroseries, and aggregate stats on all three of these. Also we'd need to only allow stats to be requested for all distroseries in an origin, not the other way around (I don't even know if requesting stats for all origins with a certain distroseries makes sense). Then again, this is a model change that could be implemented after we've settled on the api we need, if we find the stats-processing load is too much.

[Michael Nelson (michael.nelson)]

QA steps (for server only) passed on daily server.

I've added the USC client as an affected project for this bug after chatting with mvo, as the client has not yet been updated to use this new functionality:

14:52 < noodles> mvo: have you updated the client to use the new API at https://bugs.launchpad.net/rnr-server/+bug/7
09172 (I've just updated the description with examples from our daily server)
14:52 < _mup_> Bug #709172: current reviews-stats is confusing to users <kb-improvement> <Ratings and Reviews server
:Fix Committed by michael.nelson> < https://launchpad.net/bugs/709172 >
14:52 < noodles> If not, I'll just add USC as an Also affects project?
14:55 < mvo> noodles: not yet
14:55 < noodles> mvo: cool, I'll add the target.
14:56 < mvo> ok
14:56 < mvo> thnx