Talk:Gallery2:Modules:rating - Gallery Codex
Personal tools

Talk:Gallery2:Modules:rating

From Gallery Codex

Stars

We want to be able to show both the user's vote (gold) and the average (red) at the same time. Showing them both in the same set of stars (ie, with stars that are both red and gold) at the same time is bad. Using a border around the stars seems like it would be bad because we'd wind up with a combination of different star types. Instead, I propose that we introduce a new, very thin bar that shows the average and have reusable star images.

This means that we have the following stars:

  • empty (E)
  • half red (r)
  • full red (R)
  • full gold (G)

If the user hasn't voted, all they see is stars. If they've voted, they see their vote (in gold stars) plus the average in a bar underneath:

Apologies for crappy ascii art:

Nobody has voted:          E E E E E

1 other vote, avg is 2.5:  R R r E E

avg is 3.5, my vote is 3:  G G G E E
                           ------


HTML for the 3rd one above could be:

<div class="rating">
<div class="stars">
<span class="gold"/><span class="gold"/><span class="gold"/><span class="empty"/><span class="empty"/>
</div>
<div class="average" width="70%"> </div>
</div>

The theme controls all the output here. Or we can use the iconpacks for the stars.

--Bharat


These could be the possible classes on the spans:
"empty" "half" "full" "empty userno" "half userno" "full userno" "empty useryes" "half useryes" "full useryes"
Then the theme can render different approaches..

to get the one above:
.empty and .half and .full have the expected css.
.useryes has gold star and overrides others.
.average sets something.

to get red stars with yellow border to indicate user vote:
.empty and .half and .full have the expected css.
.empty.useryes and .half.useryes and .full.useryes have yellow border.
.average has display:none;

to show user vote when possible with red border to indicate average vote:
.empty and .half and .full have the expected css.
.half.useryes and .full.useryes are gold star with half or full red border
.half.userno and .full.userno are empty star with half or full red border
.empty.useryes is gold star
.average has display:none;

--Mindless 17:24, 10 November 2005 (PST)

Where to get stars

http://2tbsp.gotdns.org/g2rating/

--Rizzo 18:54, 10 November 2005 (PST)

Sweetness

mindless I just got my head around your proposal and it makes my pants tight.

I still need the full yellow star with half and empty red border from thumb and I'll be able to implement your third proposal, leaving average with display: none.

I'm prefixing my CSS classes with .giRating to avoid namespace collisions, like .giRatingEmpty, .giRatingHalfUserYes, etc.

--Rizzo 19:04, 10 November 2005 (PST)

Code review

module.inc

  • setGroup in constructor.. put module in "Extra Data" group getItemSummaries
    • Done --Rizzo 08:37, 28 November 2005 (PST)
  • set $itemId at top of foreach to avoid repeated getId calls
    • Done --Rizzo 08:37, 28 November 2005 (PST)
  • comment "Check if this group can view permissions", s/group/user/ and s/permissions/ratings/
    • Done --Rizzo 08:37, 28 November 2005 (PST)
  • currently it is true that all $items passed to getItemSummaries have the same parent.. however, if we later have keyword albums, search results displayed as an album, etc the items may have different parents. Perhaps keep an array of parentId=>isEnabled and lookup the plugin param each time a new parent is encountered. You'll still only want your css/js, etc if some item uses ratings.
    • Done --Rizzo 14:21, 28 November 2005 (PST)

SiteAdmin

  • Add some text to explain how it works.. turn on/off ratings in each album, permissions. This site-wide setting determines if users can give ratings to entire albums in addition to individual items.
  • instead of 2 setPluginParameter calls use ? : for the value
    • Done --Rizzo 14:27, 28 November 2005 (PST)
  • update author line in header
    • Done. --Rizzo 08:39, 28 November 2005 (PST)
  • in tpl, remove msgs for status.reset and status.deactivated and change action for Reset button from undo to reset; in controller, remove "else if" block for reset. you don't need a $status for this.
    • Done. --Rizzo 18:28, 28 November 2005 (PST)

ItemEditOption

  • Is permission + on/off param really both needed?
If nobody has view or add permission, it is "off" for that album...
I think the ItemEditOption should be removed unless there's is some reason to keep it.

comments on this file if kept:

  • An ItemEditOption is sharing $form with the ItemEditPlugin and other options.. you can only put data in $form['youroptionname'].. don't set formName or other keys outside your namespace.
  • $rootAlbumId is unused
  • $module is unused
  • what is 'rating.enabled' value placed in $session for?
  • usually the form varname is the same in template data and POST data (rating.enabled vs enabledChecked)

Removed ItemEditOption code --Rizzo 22:50, 29 November 2005 (PST)

rating.js

  • handleRatingResponse, add "var" for results
    • Done. --Rizzo 08:42, 28 November 2005 (PST)

RatingImagePreload

  • rather than repeating that style on each item, how about just
 <div id="RatingImagePreload">
   <div class="giRatingEmpty"></div>......
</div> and then in rating.css define
 #RatingImagePreload div { that css.. }
    • Done. --Rizzo 18:40, 28 November 2005 (PST)

block

  • rather than pass isBlock=1 how about passing firstCall=true and then you can remove include for preload from the block tpl and the !$isBlock from the "if" in Interface tpl.
    • Done --Rizzo 09:07, 28 November 2005 (PST)

Callbacks.inc, Preloads.inc

  • add phpdoc header
    • Done --Rizzo 08:47, 28 November 2005 (PST)

RatingCallback

  • update description of this class in top phpdoc
    • Done --Rizzo 08:47, 28 November 2005 (PST)
  • i think you can omit $gallery->getActiveUserId param in hasPermission call.. isn't current user used by default?
    • Done --Rizzo 08:47, 28 November 2005 (PST)

RatingInterface

  • would be great if rating.add permission is included in template data so you can omit mouseover/onclick stuff if the user has only view permission, but not add. (it appears now you can click but then nothing will update if you don't have add perm?)
    • Done. omitting entire href tag. --Rizzo 12:24, 28 November 2005 (PST)

RatingMap, RatingCacheMap

  • update phpdoc (This is your map!) (in cachemap description mention how avgRating is encoded, value * 1000 or whatever you used)
    • Done --Rizzo 08:54, 28 November 2005 (PST)
  • are indexes on avgRating and voteCount used/needed?
    • Done (not sure why I put those there :p) --Rizzo 08:54, 28 November 2005 (PST)
  • if you're changing cachemap anyway, maybe spell out averageRating as column name
    • Done (and helper class changed too) --Rizzo 08:54, 28 November 2005 (PST)

RatingHelper

  • sessionid vs remoteid in ratingmap.. so you want to use both, correct? go ahead and put your code in place calling _getRemoteIdentifier (even though "private") and I'll put it on my list to make that public. (but do note that _getRemoteIdentifier returns an array which you may want to implode)
    • Done --Rizzo 11:42, 28 November 2005 (PST)
  • if you do the delete before the add couldn't you use deleteMapEntry instead of storage->execute ?
    • Done --Rizzo 11:42, 28 November 2005 (PST)
  • do you need a fetchRatings call at end of rateItem? don't you have all the needed info already?
    • Ha I'm dumb. good call. Done. --Rizzo 09:00, 28 November 2005 (PST)

All tests

  • move _markForCleanup up, right after _album is created (for the tiny chance album is created ok but item create fails)
    • Done. --Rizzo 18:49, 28 November 2005 (PST)
  • only add print $ret->getAsHtml in setUp(). phpunit will print out the error detail elsewhere.
    • Done. --Rizzo 18:49, 28 November 2005 (PST)
  • some missing $ret checks in tests
    • Done (think I got them all). --Rizzo 18:49, 28 November 2005 (PST)
  • remove _markPluginParametersForCleanup from tests that do not modify no-itemId parameters (those are the only ones restored by this).. in your case only the enabled-for-albums param.
    • Done. --Rizzo 07:29, 1 December 2005 (PST)

Callback test

  • add tests to verify allowAlbumRating and permissions are checked and handled correctly

Callback view test

  • s/split/explode/ (use explode when splitting on simple string, not a regexp)
    • Done. --Rizzo 18:49, 28 November 2005 (PST)
  • add test to verify rating.add permission is checked
  • maybe a test with bogus itemId? (like someone voted on an item that just got deleted...)
    • Done. --Rizzo 19:41, 28 November 2005 (PST)


--Mindless 16:14, 26 November 2005 (PST)


Additional

  • Could you try updateMapEntry to update cachemap, then do addMapEntry if that doesn't update any rows? This saves one query for all but the first rating on an item.
    • Looks like this will be more trouble than it's worth. --Rizzo 09:03, 1 December 2005 (PST)
  • Consider adding locking when adding a new rating.. potential db integrity issues with multiple concurrent ratings for same item? (could writelock the rated item)
    • I had been doing this originally. See note on article page regarding bharat saying it was more trouble than it's worth. --Rizzo 09:03, 1 December 2005 (PST)
  • Add an event handler for GalleryEntity::delete and remove all ratingmap/cachemap data for a GalleryItem or GalleryUser that is deleted.
    • Done. --Rizzo 13:05, 30 November 2005 (PST)
  • If you're up for it, look into importing rating data from G1 in migrate module.

--Mindless 08:23, 27 November 2005 (PST)

2nd review

module.inc

  • empty($enabled) ? false : true --- empty() returns boolean, so !empty($enabled) is simpler.
    • Done. --[[User:Rizzo|--Rizzo 11:36, 5 December 2005 (PST)Rizzo]] 11:19, 5 December 2005 (PST)
  • require for GalleryTemplate.class not needed now; you've got $template
    • Done. --Rizzo 11:19, 5 December 2005 (PST)
  • build the $itemIds array in the top foreach loop (before the continue, of course)
    • Done. --Rizzo 11:19, 5 December 2005 (PST)
  • handleEvent: it's GalleryAlbumItem, not GalleryAlbum.. but a GalleryAlbumItem is a GalleryItem, so just the first part of the if is sufficient.
    • Done. --Rizzo 11:19, 5 December 2005 (PST)

SiteAdmin

  • might as well assign value directly to $form['allowAlbumRating']
    • Done. --Rizzo 11:15, 5 December 2005 (PST)

ItemEdit

  • is the check in isAppropriate redundant? you registered your plugin for ItemEditAlbum.
    • Done. --Rizzo 11:34, 5 December 2005 (PST)
  • assign enabled val directly to $form[..... ; add missing $ret->isError check for that.
    • Done. --Rizzo 11:34, 5 December 2005 (PST)
  • could use a simple ?: when saving value.. shouldn't you use empty() to check instead of isset, like you did in site admin controller?
    • Done. --Rizzo 11:34, 5 December 2005 (PST)
  • varname still differs from tpl and POST.. use {g->formVar var="form[rating][enabled]"} in the tpl. this will slightly change your error check in loadTemplate (put the getPluginParameter inside if (empty($form['error'])), i guess)
    • Done. --Rizzo 11:34, 5 December 2005 (PST)

Tests-- still need these tests added:

  • Callbacks: add tests to verify allowAlbumRating and permissions are checked and handled correctly
    • Done. --Rizzo 11:28, 7 December 2005 (PST)
  • Callback view: add test to verify rating.add permission is checked
    • Done. --Rizzo 11:28, 7 December 2005 (PST)

--Mindless 21:13, 3 December 2005 (PST)

  • Callbacks.inc -- one mismatched error return (array instead of just error obj) --Mindless 11:03, 5 December 2005 (PST)
    • Done. --Rizzo 11:36, 5 December 2005 (PST)

Final review

  • assertEquals params are expectedValue then actualValue; a couple new Callbacks tests have this backwards, maybe check for other cases.
    • Done. --Rizzo 09:06, 9 December 2005 (PST)
  • remove setVariable for controller in ItemEditOption (options should not do this; ItemEditPlugins can).
    • Done. --Rizzo 09:06, 9 December 2005 (PST)
  • you load RatingHelper in 2 places in ItemEditOption, but don't use it.
    • Done. --Rizzo 09:06, 9 December 2005 (PST)

What's next

When you finish the last couple changes from code review then you'll need to prepare the module for inclusion into main gallery CVS.. it will be joining in BRANCH_API_DEV so you'll need to review API changes listed for Core API 7.0 and Module API 3.0 and make necessary updates to your module. Don't actually set the required api versions in your module constructor yet.. we'll bump those when we're ready to merge the branch back to CVS HEAD. Let us know when you've got the code ready!

  • Made BRANCH_API_DEV of rating module in gallery-contrib, ran convert.pl script there. Also updated use of now-properly-exposed getRemoteIdentifier() function. mindless finished the rest. --Rizzo 11:07, 9 December 2005 (PST)

Top x rated block

see file attached in forum at http://galleryproject.org/node/51381#comment-272507 --Lrkwz 11 Apr 2008

advertisements