Review Request 111401: implement ratings in the bodega-server

[Aaron J. Seigo]

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



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

    addAsset sounds like it is .. well .. adding an asset. this should probably be addRatings or even addAssetRatings



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

    may as well move this down after the if statment; no point in making the json if there's an error to report immediately after



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

    having looked at the patch, there is a much better way of doing this imho.
    
    first, ct_checkAssociationOfRatingAttributeWithAsset should be a trigger and not checked here.
    
    next, the delete and the insert should indeed go into a stored procedure and this code should call that stored procedure, something like:
    
    ct_addAssetRating(personId int, assetId int, ratings int[][])
    
    it can iterate over all the ratings
    it can do the delete and insert, if it fails the ct_checkAssociationOfRatingAttributeWithAsset trigger will reject it.
    
    the best part of this is that it means just one round trip to the database *and* we get a transaction around the whole thing for free.
    
    as handling arrays is not particularly obvious in pl/pgsql, here is an example you can use:
    
    CREATE OR REPLACE FUNCTION test(ratings int[][]) RETURNS VOID AS $$
    DECLARE
        rating  int[];
    BEGIN
        FOREACH rating SLICE 1 IN ARRAY ratings LOOP
            RAISE NOTICE 'ratings are % %', rating[1], rating[2];
        END LOOP;
    END;
    $$ LANGUAGE plpgsql;
    select test(ARRAY[[1, 2], [3, 4]]);
    



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

    when using async, you should pass the error to the callback and handle the error in the final function.
    
    the reason for this is that with queue, if there is an error there may be multiple workers going and if the all error then multiple errors will be reported back to the client.
    
    with a queue, you can push the error back to the drain (due to the concurrency) but you can have an error variable in the out function (addAsset) which they assign their error to.
    
    then each worker can check if error is set and if it is then don't do any more work. in the drain, you can check for this error and report it there.



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

    this should be 'asset/ratings/:assetId'. it is not listing anything: it is adding ratings



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

    first: appending "2" to a function name is not a good enough way to name functions. what does "2" mean?
    
    in this case, i would just use ct_checkAssociationOfRatingAttributeWithAsset as the trigger not even bother checking in the client. do the request, and if the trigger rejects it, then it fails.
    
    this means fewer round trips between the nodejs app and the ratings.


- Aaron J. Seigo


On July 25, 2013, 7:57 a.m., Giorgos Tsiapaliokas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111401/
> -----------------------------------------------------------
> 
> (Updated July 25, 2013, 7:57 a.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/20130725/95fe39a5/attachment-0001.html>