Review Request 111401: implement ratings in the bodega-server

Tue Jul 16 15:39:33 UTC 2013

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111401/#review36051
-----------------------------------------------------------

the apidocs are not complete; they need to include all paramters, such as the paging related parameters (offset).

server/lib/bodegadb.js
<http://git.reviewboard.kde.org/r/111401/#comment26692>

    remove: these are not used in routes.js nor are they in ratings.js.

server/lib/bodegadb.js
<http://git.reviewboard.kde.org/r/111401/#comment26691>

    ratingAsset -> assetRatings

server/lib/db/ratings.js
<http://git.reviewboard.kde.org/r/111401/#comment26702>

    left join, not inner join

server/lib/db/ratings.js
<http://git.reviewboard.kde.org/r/111401/#comment26703>

    no limit/offset

server/lib/db/ratings.js
<http://git.reviewboard.kde.org/r/111401/#comment26704>

    not necesssary; no asset will have so many attributes for rating purposes and it is not useful to the client to only receive some of the attributes.

server/lib/db/ratings.js
<http://git.reviewboard.kde.org/r/111401/#comment26701>

    this query is unnecessary; if the asset does not exist, the select will return no rows.

server/lib/db/ratings.js
<http://git.reviewboard.kde.org/r/111401/#comment26705>

    not needed

server/lib/db/ratings.js
<http://git.reviewboard.kde.org/r/111401/#comment26693>

    this is unecessary: the route requires authentication, so req.session.user.id exists and is valid.

server/lib/db/ratings.js
<http://git.reviewboard.kde.org/r/111401/#comment26699>

    this first query is unecessary; if the asset does not exist, then the database will return an error on the insert due to a referential integrity violation

server/lib/db/ratings.js
<http://git.reviewboard.kde.org/r/111401/#comment26700>

    please use async.queue instead of this. you can see an example of this in server/test/partners.js among other places

    this code also assumes that the input is well formed. sending a rating attribute twice in the same json will result in it being entered twice.

    also, there is no check to see if the user has already added a rating for this item?

    it should probably be done something like:

    * get the rating attributes for the asset
    * iterate over all ratings passed in and make sure there is only one of each, and that each attribute matches the ratings for that asset
    * delete the ratings for that user on that asset for the rating attributes passed iin
    * insert the new rating entries

server/lib/db/ratings.js
<http://git.reviewboard.kde.org/r/111401/#comment26695>

    should use utils.parseNumber here

server/lib/db/ratings.js
<http://git.reviewboard.kde.org/r/111401/#comment26694>

    should be moved to after the error checking

server/lib/db/ratings.js
<http://git.reviewboard.kde.org/r/111401/#comment26697>

    unecessary; the user is authenticated here

server/lib/db/ratings.js
<http://git.reviewboard.kde.org/r/111401/#comment26696>

    with parseNumber used, this becomes a check for > 0

server/lib/db/ratings.js
<http://git.reviewboard.kde.org/r/111401/#comment26698>

    this first query is unnecessary; it doesn't matter if the asset exists. if it doesn't, the delete will simply not do anything.

server/routes.js
<http://git.reviewboard.kde.org/r/111401/#comment26687>

    asset/ratings/list/:assetId

server/routes.js
<http://git.reviewboard.kde.org/r/111401/#comment26688>

    asset/ratings/list/:assetId

server/routes.js
<http://git.reviewboard.kde.org/r/111401/#comment26689>

    asset/ratings/delete/:assetId

server/routes.js
<http://git.reviewboard.kde.org/r/111401/#comment26690>

    asset/ratings/attributes/:assetId

sql/ratings.sql
<http://git.reviewboard.kde.org/r/111401/#comment26707>

    create indexes here on ratings(asset) and assetRatingAverages(asset)

sql/ratings.sql
<http://git.reviewboard.kde.org/r/111401/#comment26708>

    unused; remove

sql/ratings.sql
<http://git.reviewboard.kde.org/r/111401/#comment26709>

    "SELECT asset" can be "SELECT NEW.asset" and then the GROUP BY does not asset

    if you also select (and delete!) on NEW.attribute, then it will catch only the changed attribute, and so do a minimal update.

    as it is, if there are 5 attributes to be rated then each rating will add 5 rows to the ratings table. that will cause ct_sumForAssetRatings to run 5 times and it will update the ratings for ALL 5 attributes on each run!

sql/ratings.sql
<http://git.reviewboard.kde.org/r/111401/#comment26710>

    it also needs to be run on delete, as that impacts the averages.

- Aaron J. Seigo

On July 5, 2013, 1:34 p.m., Giorgos Tsiapaliokas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111401/
> -----------------------------------------------------------
> 
> (Updated July 5, 2013, 1:34 p.m.)
> 
> 
> Review request for Bodega.
> 
> 
> Description
> -------
> 
> This patch implements the ratings feature in the bodega-server.
> 
> I have also attached a screenshot of the apidocs.
> 
> 
> Diffs
> -----
> 
>   server/bodegaDbHelper ba740cb 
>   server/doc/bodega.json 59c14e3 
>   server/lib/bodegadb.js 3be8d81 
>   server/lib/db/ratings.js PRE-CREATION 
>   server/lib/errors.js 356d7c4 
>   server/routes.js 726fc80 
>   server/test/ratings.js PRE-CREATION 
>   sql/core.plsql 7b92784 
>   sql/ratings.sql PRE-CREATION 
>   sql/testdata.sql dd15d57 
> 
> Diff: http://git.reviewboard.kde.org/r/111401/diff/
> 
> 
> Testing
> -------
> 
> ./node_modules/.bin/mocha test/ratings.js --reporter spec
> WARNING: Setting up server with no ssl!
> Bodega server listening on localhost:3000 in devel mode
> 
> 
>   Ratings
>     needs to authorize first
>       ? authorize correctly. (107ms)
>     List Attributes
>       ? it should fail because the asset is invalid 
>       ? it should succeed 
>     Remove asset rate
>       ? it should fail because the asset is invalid 
>       ? it should succeed 
>     Asset Ratings
>       ? it should fail because the asset is invalid 
>       ? it should succeed 
>       ? it should be empty because there are no ratings for the asset 
>     Participant
>       ? it should succeed 
>       ? it should have no ratings 
>     Add asset rate
>       ? it should fail because the asset is invalid 
>       ? it should succeed 
> 
> 
>   12 passing (219 ms)
> 
> 
> File Attachments
> ----------------
> 
> 
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/07/05/bodega-ratings-apidocs.png
> 
> 
> Thanks,
> 
> Giorgos Tsiapaliokas
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/active/attachments/20130716/6e083f33/attachment-0001.html>