Moodle

Attaching a patch. The upgrade code creates the rating table and copies over data, forum and glossary ratings. The ratings code is largely complete (pending unforeseen module specific requirements) but the forums etc still use their own ratings implementations for the moment.

The file sandbox-ratings.php demonstrates the new ratings.

Just a quick review:
1/ I guess we should use singular in capabilities, function names, strings, etc.
2/ the risks in cap definitions are incorrect
3/ the access control is incorrect, you must find out correct $course and $cm from the current context and use it in require_login()
4/ the username === 'guest' account must not be allowed to rate

Firstly, one silly question:

Q: What happens if one teacher changes the scale to be used once there are already ratings for that activity?

Then, about the patch, mainly, upgrade script:

1) The install.xml file is missing?
2) contextid and userid must be defined as foreign key, you don't need to define them as indexes (xmldb will create them).
3) migrate_module_ratings() function is accepting an array of rates as 1st param, that doesn't scale at all (imagine one forum with thousands of discussions/zillions of rates). You must, within the function, use xxxx_recordset() functions instead. So perhaps that first param should be, simply, the sql to process instead of the ratings.
4) There are some harcoded "if($moduleid==7){//forum" that's NOT true at all. If you fulfill 3) you can aliase the query so it always will return one constant named "itemid" field in the select. (SELECT userid, post as itemid, time, rating from {forum_ratings}). That way you won't need any conditional coding within migrate_module_ratings()
5) The function itself should be in upgradelib or so, not declared in upgrade.php
6) As far as it can be a loooong process, you must insert strategic calls to upgrade_set_timeout(xx); calls to give the upgrade script more and more time continuously. Not for each record but for each, say, 500 (for example).
7) The upgrade step seems to be missing the corresponding upgrade_main_savepoint() or there is one "strange" - return $result - in the middle of the patch, not sure, plz, review.

Ciao

I think I've dealt with pretty much all of the above. version 2 of the patch attached. Note that this version doesn't actually work. It turns out I dont yet fully understand how contexts work in Moodle.

I havent yet forbidden guests from submitting ratings. Do we want to absolutely forbid guests from rating things? Surely there are public sites where visitors can rate forum posts?

"Q: What happens if one teacher changes the scale to be used once there are already ratings for that activity?"

Martin and I have had a few discussions about that but never come to a definite conclusion. Each rating stores the scale that was in use at the time the rating occurred. This allows us to rescale previous ratings, throw old ratings away or whatever we ultimately decide to do. At the moment the answer to your question is "nothing good".

Install/upgrade code looks now near perfect. Just 2 comments:

1) Change that $ratings = $DB->get_records_sql($ratingssql) by one get_recordset_sql() and iterate over it properly (closing once finished). That query returns all the ratings and we cannot load them in memory, but process them 1 by 1.

2) There is something strange in the $moduleandscalesql sql. I don't get why you are including the "xxx_ratings" table there again, as far as you already have the "itemid" (post->id, entry->id...) you don't need it anymore IMO.

3) Also, about this query... it's executed by each record in the main query... so can end being executed zillions of times. And we cannot cache anything from it easily... so perhaps... the best way to make it optimal is to DELETE it completely and just make 1 query to return everything (both the rating and the module/scale). For example, for forums:

$ratingssql = '
    SELECT fr.id, fr.post as itemid, fr.rating, fr.userid, fr.time as timecreated, fr.time as timemodified, 
           'forum' as activitytype, f.id as activityid, f.scale as scaleid
    FROM {forum_ratings} fr,
    INNER JOIN {forum_post} fp ON fp.id = fr.post
    INNER JOIN {forum_discussions} fd ON fd = fp.discussion
    INNER JOIN {forum} f ON f.id = fd.forum';

(not tested!) That way you'll end with one record already containing all the info, hence, the "individual" get_record() isn't needed any more.

4) About the contexts, I think you have a little confusion somewhere, hehe. Basically all you need to do is to "navigate" from the activity id to the context id (passing by
course_modules and modules) using the "$rs->activitytype" field above. Caching looks ok (just remember the keys in the cache must be the "$rs->activityid" values above. Or, alternatively, you can use these higher level functions (also caching results of course):

$cm = get_coursemodule_from_instance($rs->activitytype, $rs->activityid);
$context = get_context_instance(CONTEXT_MODULE, $cm->id);

Ciao

Ive merged those two queries into one. Cuts down on the trips to the database and resolves some of the other issues you pointed out.

I think I'll get someone here to go through contexts with me. Im not clear on what you mean.

I've added assesstimestart and assesstimefinish to the table 'data'. The rating's section of the configuration screen is now produced by moodleform rather than the individual module (forum etc)

Note that a few unrelated changes for the closely related MDL-19133 have been caught up in this patch. This patch is purely for discussion/review purposes.

A few unanswered questions below:

Should it be forbidden for guests to submit ratings? Petr has previously suggested this but is it necessary to make it impossible for a site to accept ratings from guests.

The allcanrate flag that is being stored in glossary.assessed. This flag was used to indicate when permissions should be ignored and everyone allowed to submit ratings. The forum and data modules stored their aggregation method in this column. glossary previously didnt support different aggregation methods. I'm suggesting removing the 'allcanrate' functionality in favour of proper permissions, using this column as it is in the other modules and defaulting aggregation to 'average' in the glossary.

Just a reminder of what we talked with Andrew:
I'm working on the community hub. I have a table named site_directory with a siteid field.
User can vote for a site, there is not really any context attached to this vote. The itemid of the rating table would be the siteid (foreign key) of the site_directory table.
So I would need to be able to retrieve rating and calculate average rating without caring about the value into contextid. Thanks

Just did an initial commit of the rating code. Attached is sandbox-ratings.php if you need a demo of how to include ratings.

Thanks for committing the rating code Andrew.
I got an error when upgrading:

migrating forum ratings
Fatal error: Call to a member function close() on a non-object in /srv/http/head/lib/db/upgradelib.php on line 439 Call Stack: 0.0010 476216 1. {main}() /srv/http/head/admin/index.php:0 0.2758 32332704 2. upgrade_core() /srv/http/head/admin/index.php:256 0.3742 36975708 3. xmldb_main_upgrade() /srv/http/head/lib/upgradelib.php:1232 0.4359 37217356 4. upgrade_module_ratings() /srv/http/head/lib/db/upgrade.php:3090

Removed $ratings->close() in lib/db/upgradelib.php, now it worked.

Another small issue is in lib/db/upgrade.php line 3079, before create table, dbman should check if the table is existed, this is useful in case the upgrade script failed, so we could start over.

if (!$dbman->table_exists($table)) { $dbman->create_table($table); }

Regards,
Dongsheng Cai

I've committed a new version that includes those fixes. Not sure how $ratings->close() slipped through. Thanks Dongsheng

1/ use the same capitalisation and whitespace in SQL code as in the rest of moodle - pretty please do not invent yet another inconsistent coding style

2/ following code is not acceptable, you must check context level, or better use the new context info fetching function from admin/roles/permissions.php

$context = get_context_instance_by_id($itemcontextid);
$cm = get_coursemodule_from_id('', $context->instanceid, 0, false, MUST_EXIST);

3/ get_rating_permissions() makes me headaches when doing security review, I do not like it at all

4/ there is no sesskey XSS protection or even require_login() in the ratings/rate.php - horrible, horrible, horrible - over and over again everybody makes the same mistake in ajax scripts

5/ $userrating = required_param('rating'.$itemid, PARAM_INT); is weird

6/ we usually do all the require_once()s right at the beginning of script

7/ the code is not finished but - do not print $OUTPUT->header(); when you want to do redirect() later, otherwise you get ugly click to continue page or inaccessible automatic redirect

8/ $PAGE->set_url() should not include the action commands itself, it should contain the normal address of the page without the actual command - in the case of rate.php it is a problem because it does not have any such address - we will have to find some solution, maybe we could put there some address derived from the context

9/ do not use CalmelCase for classes: $rating = new Rating()

10/ please fix all your PHPDOCs - the type is missing there

11/ where is the class method indentation - the ratinglib.php is confusing

12/ add access modifier to all class methods (missing public there in ratinglib.php)

13/ class rating implements renderable -oh! renderables were supposed to be simple data objects, not the full API itself, this looks really scary

..end of short review, not finished yet

"there is no sesskey XSS protection or even require_login() in the ratings/rate.php - horrible, horrible, horrible - over and over again everybody makes the same mistake in ajax scripts"

Petr, Ill go through that list properly later but just quickly, what is the correct way to check for a logged in user in a file like rate.php? Martin and I actually wrote the permission check bit of code and took out require_login together this morning. It appears that no one but maybe you knows how it should work. Can you point me to an example of where it is done correctly or provide a sample?

14/ new moodle_url() - in new code always use the parameters array instead of putting them into the first param

15/ value="'.get_string('rate', 'forum').'" is technically wrong, what if other langs need to put there string with " in it? This was one of the main reasons why we started to sue html_writer - to get rid of missing s() in forms html markup

16/ static $strrate;//holds the string "rate". Its static so we only fetch it once. - no tricks like this are needed any more, AMOS project should solve this

17/ why did you added the require_once($CFG->dirroot.'/mnet/lib.php'); ?

re #17 I've committed a version of admin/settings/mnet.php without that require_once. That was a temporary workaround for an mnet bug that should never have been committed.

18/ the capabilities use 'ratings' - it MUST be 'rating'!

19/ I could not find consistency in ratings-comments, where is it?

20/ why 'ratings' table, we have plurals only for BC reasons, reserved words, etc. there - it should be 'rating' (see course, config, forum, user, ...)

I can not help myself, +1 for complete revert asap, the code does not have production quality yet and it is imo technically not possible to fix it in the cvs so close to beta release.

re revert, these arent major pieces of work you've identified. Ill have the vast majority of them fixed by this time tomorrow.

I will go through all of your points in numerical order tomorrow morning. I've made one pass through them and have fixed many of them already. In particular the table is now singular and so are the capabilities. I have some questions.

"1/ use the same capitalisation and whitespace in SQL code as in the rest of moodle - pretty please do not invent yet another inconsistent coding style"

I need a concrete example of the problem you see and the correct formatting I should be using. I couldn't find a coding standard for SQL and can't see a significant difference between the rating sql and a handful of other queries I looked around from elsewhere in Moodle. What aren't I seeing?

"3/ get_rating_permissions() makes me headaches when doing security review, I do not like it at all"

Why so? How would you prefer it to be structured?

"4/ there is no sesskey XSS protection or even require_login() in the ratings/rate.php - horrible, horrible, horrible - over and over again everybody makes the same mistake in ajax scripts"

As said in a previous comment, I need to know the correct way to do this. Can you point me to an example?

20/ +$string['sessionexpired'] = 'Your session has expired. Please log in again.'; in rating.php file? Please add strings only when really necessary and reuse them correctly if possible - translators have to translate everything

21/ +$string['allowratings'] = 'Allow posts to be rated?'; We are going to rate other things too, right?

uhm... just detected that you are adding two new columns to the "data" table in your upgrade script. That is one module and such upgrade should go to mod/data/db/upgrade.php (and install.xml, of course!!).

Ciao

uhm... changing the table name to singular (that I strongly disagree, but I'm not going to discuss!) seems to be incomplete in any case (install.xml continues showing the plural form).

Ciao

I do not like the mixed plurals x singulars either, but the current coding style says the first work is singular and other plurals with the exception of reserved words in sql, do not ask me why

1/ just read any of my SQL code or http://docs.moodle.org/en/Development:DB_layer_2.0_examples

4/ reguire_login()/ require_course_login() is the fundamental part of our access control, this is the one of the first functions every moodle developer has to learn; the sesskey protection (oh! CSRF not XSS, sorry for the wrong name of exploit) is another fundamental moodle security feature, every action which modifies data/is stored on server must use it, no exceptions

Eloy: yes! the core upgrade must not touch mod/xxx structures or code - always do these changes in mod/xxxx/db/upgrade.php and mod/xxxx/db.install.xml

Eloy, I've fixed install.xml I'll shift/the upgrade code that modifies the data table now.

Petr, responses to your items are below. Pretty much all done.

1/ use the same capitalisation and whitespace in SQL code as in the rest of moodle - pretty please do not invent yet another inconsistent coding style
I believe I have fixed this. Are you talking about the SQL in the rating code itself or the upgrade code?

2/ following code is not acceptable, you must check context level, or better use the new context info fetching function from admin/roles/permissions.php
done. based it on what the comments submission page (comment/comment_post.php) is doing.

3/ get_rating_permissions() makes me headaches when doing security review, I do not like it at all
I'm happy to refactor this however you think is best. You'll need to give me a clue though Should I add a method like comment::_check_permissions()? Bear in mind that will result in as many calls to has_capability() as there are ratings on the page which is why I opted to check permissions once then hold onto them.

4/ there is no sesskey XSS protection or even require_login() in the ratings/rate.php - horrible, horrible, horrible - over and over again everybody makes the same mistake in ajax scripts
The problem with require_login is that it redirects which isnt what you want for ajax requests. Despite that I've added a call to require_login. Hopefully the call before it to isloggedin will handle the situation for ajax requests.

5/ $userrating = required_param('rating'.$itemid, PARAM_INT); is weird
Its that way to ensure the html elements have unique IDs so you don't get validation errors. If I just call the rating dropdown 'rating' and you put multiple rating elements on a page you wind up with a bunch of elements all with the id 'menurating'. Appending the item id makes them unique.

6/ we usually do all the require_once()s right at the beginning of script
done

7/ the code is not finished but - do not print $OUTPUT->header(); when you want to do redirect() later, otherwise you get ugly click to continue page or inaccessible automatic redirect
Can you tell me specifically where you can see this happening? It will never happen that I'm aware of.

8/ $PAGE->set_url() should not include the action commands itself, it should contain the normal address of the page without the actual command - in the case of rate.php it is a problem because it does not have any such address - we will have to find some solution, maybe we could put there some address derived from the context
Not sure what to do here.

9/ do not use CalmelCase for classes: $rating = new Rating()
Im assuming you mean the class name shouldnt start with a capital R. This was already changed in the code but not the document.

10/ please fix all your PHPDOCs - the type is missing there
done

11/ where is the class method indentation - the ratinglib.php is confusing
done

12/ add access modifier to all class methods (missing public there in ratinglib.php)"
done

13/ class rating implements renderable -oh! renderables were supposed to be simple data objects, not the full API itself, this looks really scary"
Renderable is an empty interface. Effectively a flag marking objects as having a UI. You'll have to be more specific about what you mean by scary. What risk are you concerned about?

14/ new moodle_url() - in new code always use the parameters array instead of putting them into the first param
I'm not sure where I'm not doing that.

15/ value="'.get_string('rate', 'forum').'" is technically wrong, what if other langs need to put there string with " in it? This was one of the main reasons why we started to sue html_writer - to get rid of missing s() in forms html markup
I've added the string to the ratings language file, added the call to s() and raised MDL-21836 for me to come back and make better use of html_writer.

16/ static $strrate;//holds the string "rate". Its static so we only fetch it once. - no tricks like this are needed any more, AMOS project should solve this
done

17/ why did you added the require_once($CFG->dirroot.'/mnet/lib.php'); ?
done

18/ the capabilities use 'ratings' - it MUST be 'rating'!
done

19/ I could not find consistency in ratings-comments, where is it?
Its a class representing an instance of the object and containing a small number of static methods for manipulating objects of that type.

20/ why 'ratings' table, we have plurals only for BC reasons, reserved words, etc. there - it should be 'rating' (see course, config, forum, user, ...)
done

20/ +$string['sessionexpired'] = 'Your session has expired. Please log in again.'; in rating.php file? Please add strings only when really necessary and reuse them correctly if possible - translators have to translate everything
Although this one didnt where strings previously existed elsewhere, in the forum for example, should I leave them there?

21/ +$string['allowratings'] = 'Allow posts to be rated?'; We are going to rate other things too, right?
done

1/ sql - no whitespace is pretty much random there, why use INNER, double quotes

3/ do not hide capability checks in complex logic elsewhere or other variables if possible, the only reason to not use has_capability() directly is when there are other complex conditions that would have to be repeated - for example finding out if user may post in forum depends on several other things

4/ ajax scripts have tons of problems, everybody just works around them - I have already discussed this with David and told him how to finally solve it: each ajax script would be marked with some define, the default error handler and renderer would be swapped in setuplib.php

5/ reqired_param() is accepting html element names, not ids - there are no duplicity problems there!

8/ just come up with some reasonable url based on the context id

13/ simply do not pass such a huge object into $OUTPUT, there definitely must not be one renderable interface spanning the whole ratings api - that is insane, what if you need to render more different things related to ratings; just remove the renderable interface and find some other nice way

14/ oops, me neither

19/ it does not matter how it looks inside much (I prefer them to be similar), but the point is that the public api/usage is the same, the implementation ideas are the same; ideally I should not be able to see that it was implemented by two different people - I can do that immediately; the fact that each is OOP in similarly named files means 10% similarity for me

20/ the session expiration is not a rating or forum problem, so is has no place in there, either we add new error string specific for ajax calls or reuse the string from the login page - new strings are very expensive, we really have to think a lot more before adding them

22/ separate ajax and normal rating scripts, this will be necessary once we have long term solution in pace that was described in 4/

23/ the last $result && $oldversion < 2010031700 for renaming of rating table just pollutes the upgrade, it is not necessary, just bump up version of the previous block and drop both rating and ratings tables first and rerun the upgrade

24/ at the same time get rid of the forum, data and glossary modules from there

25/ 'moodle/rating:view' should be nearly everybody I guess, at least that is the default in forum and glossary

26/ 'moodle/rating:viewall' what is this for?

27/ 'moodle/rating:rate' is not RISK_DATALOSS, it just adds new value into database, right?

28/ is it possible to change my own rating later? are we going to have some capability for that?

29/ must not use global $PAGE in renderers - always use $this->page->

30/ $scaleid is required, but in not included in page url * rating/index.php

31/ rating/index.php is using forumpostheader class

32/ class rating does not define context, itemid, scaleid, userid properties - is it public or private? phpdocs are handy in any case

33/ the method names repeat unnecessarily the "rating" word, ex. rating::rating_get_aggregation_method()

34/ load_ratings_for_item() should use the user_picture::fields() to get list of necessary fields

35/ public static function load_ratings($context, $items, $aggregate=RATING_AGGREGATE_AVERAGE, $scaleid=5, $userid = null, $returnurl = null) {

what is the magic "5" doing there?

36/ incorrect use of GROUP BY - from postgresql manual "When GROUP BY is present, it is not valid for the SELECT list expressions to refer to ungrouped columns except within aggregate functions, since there would be more than one possible value to return for an ungrouped column." - blocker in load_ratings()

37/ phpdocs is still unfinished:

• missing parameter type in some cases
• completely missing return types

Hi Petr. Thanks for your continuing feedback

1/ sql - no whitespace is pretty much random there, why use INNER, double quotes
I used inner because Im doing an inner join. If you just say join it defaults to an inner join but surely its better to be explicit. Although to be fair when I do a left outer join I typically only say left join so I suppose I am not being entirely explicit. Do you want me to remove the "inner" keyword?

White space made consistent although the only inconcistency that jumps out at me is one new line.

Are you saying you want the queries in upgrade.php in double quotes?

3/ do not hide capability checks in complex logic elsewhere or other variables if possible, the only reason to not use has_capability() directly is when there are other complex conditions that would have to be repeated - for example finding out if user may post in forum depends on several other things
done.

4/ ajax scripts have tons of problems, everybody just works around them - I have already discussed this with David and told him how to finally solve it: each ajax script would be marked with some define, the default error handler and renderer would be swapped in setuplib.php
no changes necessary at this time as I understand what you're written

5/ reqired_param() is accepting html element names, not ids - there are no duplicity problems there!
The duplicate id is when you're viewing the ratings not when you're receiving the post.
$strratings .= html_writer::select($scalearray, 'rating'.$rating->itemid, $rating->rating, false, array('class'=>'postratingmenu ratinginput'));
Without ".$rating->itemid" html_writer::select produces a select box with id menurating. If you render 4 ratings you get four select boxes with the same id.

8/ just come up with some reasonable url based on the context id
done I think. Not entirely sure what qualifies as reasonable.

13/ simply do not pass such a huge object into $OUTPUT, there definitely must not be one renderable interface spanning the whole ratings api - that is insane, what if you need to render more different things related to ratings; just remove the renderable interface and find some other nice way
To be fair the rating object isnt huge as static methods dont affect the size of the object. I could split out the static methods into a rating_manager type class? If that is preferable am I ok to declare both rating and rating_manager (or whatever) in /rating/lib.php?

19/ it does not matter how it looks inside much (I prefer them to be similar), but the point is that the public api/usage is the same, the implementation ideas are the same; ideally I should not be able to see that it was implemented by two different people - I can do that immediately; the fact that each is OOP in similarly named files means 10% similarity for me
They have certainly diverged. Whilst consistency is ideal they have diverged due to their different requirements.

20/ the session expiration is not a rating or forum problem, so is has no place in there, either we add new error string specific for ajax calls or reuse the string from the login page - new strings are very expensive, we really have to think a lot more before adding them
Went through the login page and found an appropriate string. 'sessionerroruser' in 'error'.

22/ separate ajax and normal rating scripts, this will be necessary once we have long term solution in pace that was described in 4/
This sounds like a bad idea at this time (short time to get moodle 2.0 into beta). Apart from the response that's returned to the client (json Vs html) the processing for an ajax and non-ajax rating submission are absolutely identical right now. I'm wary of spending time doing extra work and possibly introducing some level of duplicate code just to be ready for a potential future change.

23/ the last $result && $oldversion < 2010031700 for renaming of rating table just pollutes the upgrade, it is not necessary, just bump up version of the previous block and drop both rating and ratings tables first and rerun the upgrade
done.

24/ at the same time get rid of the forum, data and glossary modules from there
Do you mean the transferring of the ratings from the old table to the new one or the dropping of the old tables (yet to be added)? Is the main upgrade script always executed before the module upgrades? Is it safe to assume that the central upgrade has run and created the rating table then have the modules individually transfer over their ratings and drop their old rating table?

25/ 'moodle/rating:view' should be nearly everybody I guess, at least that is the default in forum and glossary
done. Modelled the rating permissions on those for the data, forum and glossary modules.

26/ 'moodle/rating:viewall' what is this for?
Users with that permission can click on the rating aggregate and view a listing of the individual ratings submitted. User A rated it 3, user B rated it 4 etc.

27/ 'moodle/rating:rate' is not RISK_DATALOSS, it just adds new value into database, right?
adds or updates. Can you confirm whether that is or is not RISK_DATALOSS?

28/ is it possible to change my own rating later? are we going to have some capability for that?
yes. If you have rate permission you can submit a rating then update it as often as you like within the assessed time window.

29/ must not use global $PAGE in renderers - always use $this->page->
done

30/ $scaleid is required, but in not included in page url * rating/index.php
done

31/ rating/index.php is using forumpostheader class
done

32/ class rating does not define context, itemid, scaleid, userid properties - is it public or private? phpdocs are handy in any case
done

33/ the method names repeat unnecessarily the "rating" word, ex. rating::rating_get_aggregation_method()
done

34/ load_ratings_for_item() should use the user_picture::fields() to get list of necessary fields
done

35/ public static function load_ratings($context, $items, $aggregate=RATING_AGGREGATE_AVERAGE, $scaleid=5, $userid = null, $returnurl = null) {
what is the magic "5" doing there?
We've talked about doing something like having a rating out of 5 automatically switch to being a stars out of 5 UI rather than a dropdown box. 5 was in preparation for that I suppose. Plus rating out of 5 is fairly common around the Internet so it seemed like a sensible default.

36/ incorrect use of GROUP BY - from postgresql manual "When GROUP BY is present, it is not valid for the SELECT list expressions to refer to ungrouped columns except within aggregate functions, since there would be more than one possible value to return for an ungrouped column." - blocker in load_ratings()
Is that an actual error you got? Its odd if so. It works in mysql and validates fine. http://developer.mimer.com/validator/index.htm

I'll install postgresql if necessary. For the time being would you mind trying something for me? Could you change line 246 of /rating/lib.php (the group by line) to
GROUP BY r.itemid,ur.id, ur.userid, ur.scaleid
That makes no difference really but if postgres is complaining about selecting non-grouped columns that should make it work.

37/ phpdocs is still unfinished:

• missing parameter type in some cases
• completely missing return types
  done.

has_capability(RATING_VIEW,$context); this is really not good, how am I supposed to grep this?

34/ I realised the id from user table returned by user_picture::fields() is going to override the r.id in the result set, tricky - see MDL-21851, there is a new option for this case. Sorry for the trouble.

1/ grep: JOIN (726 hits) = LEFT JOIN (140 hits), INNER JOIN (89), OUTER JOIN(26), RIGHT JOIN (1)
this means:

• our standard is JOIN (470 hits) - not INNER JOIN (only old code, outsiders and newcomers use INNER)
• right join is forbidden - there is a bug in tags library
• OUTER JOIN should not be used

This was discussed many several times before

4/ ajax - no, split it into two separate files, one for ajax callbacks and one for normal forms

5/ very simple - just pass your own $attributes with id into html_writer::select()

13/ it will only get bigger with each new feature, you never know what will happen in future - but I am 100% sure it is incorrect at present, it is like if the workshop class implemented renderable

18/ I believe it can be much more consistent, sorry

22/ no, just split them and once we have the new flag for all ajax scripts it will work with only minor modifications

24/ you must not touch plugin tables from core - never, the main upgrade runs before all plugin upgrades; please read the upgrade code internals if you do not believe me

26/ viewall: makes sense

27/ just grep for RISK_DATALOSS in main lib/db/access.php and you will see (just ignore the comments cap for now)

28/ I think in future somebody will want this feature - just rate once and prevent changes; but I agree it is not needed now

35/ if it is kind of default used in several different places please use some constant

36/ our code has to run in mysql, postgresql, mssql and oracle - did you try it in these too? I did not, but from my prev experience it should fail in postgresql, anyway you have to prove it does not or fix it

I would strongly recommend to install postgresql alongside the mysql, please do not ask me if it works or not, just try it if you do not believe me.

I defined a constant just because it seemed weird to copy and paste literal strings all over the place. Ill switch to the strings as it does seem to be the way its done throughout Moodle.

The need to unalias the user id column from user_picture::fields is unfortunate. Its potentially the source of fairly subtle bugs if you think you have the id of one thing but actually have another.

oops, the iead behind the default constant was to hide the fact that there is "5", the RATING_SCALE_OUT_OF_5 does not do that at all - it should be something like RATING_DEFAULT_SCALEOUTOF or something like that. Thinking a bit more about this, are there going to be a need to change this default? Should it be in the function interface?

The more I think about this the less I like the defaults in public static function load_ratings($context, $items, $aggregate=RATING_AGGREGATE_AVERAGE, $scaleid=RATING_SCALE_OUT_OF_5, $userid = null, $returnurl = null)
because code should not "guess" the defaults imo, the code calling load_ratings() always has to know $aggregate and $scaleid. We need some defaults for these, but maybe constants are not the best way, might be better to use admin settings. In the short term the constant is fine for defaults because it can be easily changes to admin settings without actually breaking stuff, but the changes in API should be minimised after the beta.

1/ grep: JOIN (726 hits) = LEFT JOIN (140 hits), INNER JOIN (89), OUTER JOIN(26), RIGHT JOIN (1)
this means:

• our standard is JOIN (470 hits) - not INNER JOIN (only old code, outsiders and newcomers use INNER)
• right join is forbidden - there is a bug in tags library
• OUTER JOIN should not be used

I've removed the keyword INNER.

4/ ajax - no, split it into two separate files, one for ajax callbacks and one for normal forms
done. rate.php and rate_ajax.php

5/ very simple - just pass your own $attributes with id into html_writer::select()
didnt know you could that (obviously) done.

13/ it will only get bigger with each new feature, you never know what will happen in future - but I am 100% sure it is incorrect at present, it is like if the workshop class implemented renderable

I've split the class rating into rating and rating_manager. rating_manager contains the static function for operating on sets of instances of rating. Note that I made them non-static to allow unit tests to replace the real rating_manager class with a dummy if so required. I havent updated the rating documentation to reflect this yet.

18/ I believe it can be much more consistent, sorry
ok, two key differences that I see.
Firstly, I dont think its possible to retrieve the comments for multiple items at once. Pulling all the ratings for a set of items from the database at once is where much of the static code comes from. Ive shifted that into rating_manager.
Secondly, instead of passing arguments to the comment constructor its building an object and passing that in. I've remodelled the rating code to work in a similar fashion.

22/ no, just split them and once we have the new flag for all ajax scripts it will work with only minor modifications
done

24/ you must not touch plugin tables from core - never, the main upgrade runs before all plugin upgrades; please read the upgrade code internals if you do not believe me
done. Its not that I dont believe you. Just wanted to be sure

27/ just grep for RISK_DATALOSS in main lib/db/access.php and you will see (just ignore the comments cap for now)
It looks like most write operations arent marked as RISK_DATALOSS. Its more deletions. I've removed it from moodle/rating:rate

35/ if it is kind of default used in several different places please use some constant
done (changed it then later came back and removed it)

36/ our code has to run in mysql, postgresql, mssql and oracle - did you try it in these too? I did not, but from my prev experience it should fail in postgresql, anyway you have to prove it does not or fix it. I would strongly recommend to install postgresql alongside the mysql, please do not ask me if it works or not, just try it if you do not believe me.

I'll need to install postgresql at home over the weekend and do this then.

Hi, Andrew,

I just updated, got an error said upgrade_module_ratings is undefined, found this function in lib/db/upgradelib.php, maybe it should be in lib/upgradelib.php?

I've shifted the function. It works for me but it also worked for me before I shifted it. weird.

37/ upgrade_module_ratings() is not good at all

37a/ Sql formatting yet again

'select cxt.id from {course_modules} cm inner join {modules} m on cm.module=m.id
inner join {context} cxt on cxt.instanceid=cm.id
where m.name=:modulename and cm.instance=:moduleinstanceid and cxt.contextlevel='.CONTEXT_MODULE;

$sql = "SELECT cxt.id
                      FROM {context} cxt
                      JOIN {modules} m ON cm.module = m.id
                      JOIN {course_modules} cm ON cm.id = cxt.instanceid
                     WHERE m.name = :modulename AND cm.instance = :moduleinstanceid AND cxt.contextlevel = :contextmodule";

37b/
$result = $result && $DB->insert_record('rating', $rating); is not definitely not current coding style, it never returns false!

37c/
$ratings = $DB->get_records_sql($ratingssql); will not fin into memory, use recordsets

37d/
$rating = new stdclass; should be new object() or at least stdClass

37e/
if the upgrade gets interrupted the ratings get duplicated or insert fails - looks critical

37f/ please move the upgrade_module_ratings() back into the lib/db/upgradelib.php - the lib/upgradelib.php contains only stuff that is used in each upgrade, the other one is for one time upgrade stuff only - the solution for the forum,data, etc. is to include that file manually, thanks

I fixed that query you were concerned about (the one with the group by clause) so it works in PostgreSQL. Due to MDL-21874 I haven't got a working Moodle install but was able to reshuffle the table order in install.xml to at least get a rating table so I could test the query.

As I haven't got a working Moodle install to test these changes against I've just created a patch. Unless I get more time Sunday I'll test and commit the patch Monday morning.

Note sure what to do about interrupted upgrades. If you set the versions back the rating table will get dropped, recreated and repopulated. Is it worth the effort to make the rating migration function smart enough to check whether a given rating has already been migrated?

37f/ hmm, it should be possible to replace the ratings code with just one SQL insert - it would solve many problems and would be much much faster (there are already some examples in lib/statslib.php.

Attached is a patch that switches the forum over to using the new ratings system.

It does work although rating_manager::get_user_grades() could do with being reworked. This function needs to return the aggregate of the ratings of a user's items. Not the user's ratings but ratings of the user's items. This is currently really difficult as the rating table doesnt store the user id of the user being rated and cannot currently know whether to look in the forum_post table, glossary_entry or whatever. One of the better proposed solutions is to add a rateduserid column to the rating table. That way getting the user's grade for a forum is just a case of select ratings by context and rateduserid then aggregating them.

Alternatively the calling module could supply a table name ('forum_posts') and the name of the user id column ('userid') and the rating code could build a query joining that table with rating.

I'll update the ratings 2.0 documentation tonight to reflect the current state of the ratings code. Its changed in the course of integrating it into the forum.

Should anyone else wish to attempt to do so this is the process to integrate ratings into a module:

1) Setup rating configuration. Check the following line appears in mod name_supports(). I've already done this for the forum, data and glossary although the remainder of the data and glossary integration is NOT done.

case FEATURE_RATE: return true;

2) Attach ratings to your items. Around 5298 of mod/forum/lib.php you can see the ratings being attached to the items. Forum posts in this case. After that is done an item's rating is available at $items[0]->rating, $items[1]->rating etc

2) Render your ratings. Add rating rendering to your item renderer. See line 3439 of /mod/forum/lib.php
if( !empty($post->rating) ){ echo $OUTPUT->render($post->rating); }

Be aware that the rating code will look for a method called %modulename%_update_grades($moduleinstance,$userid) when a rating is submitted. For example forum_update_grades($forum, $userid=0) This method already exists within the forum, glossary and data. It typically queries the rating code for the user's grade then updates the grade book.

I've updated http://docs.moodle.org/en/Development:Ratings_2.0 I believe its now up to date.

Below is the ratings to-do list:

Verify that you can add glossary and data activities (MDL-22065) Fixed but needs comitting. Should be possible to independently commit just this.

Theres an upgrade bug (MDL-22063). The scale id column is wrongly marked as being unsigned when it can be +ve or -ve.

Rework rating_manager::get_user_grades() as per the above comments

Test forum rating integration. ajax and non-ajax. (MDL-21995, MDL-17369)

Commit the above. This will prompt new people to both look at the integration and the rating code itself so more work may come out of that.

Integrate ratings into the glossary module (generic steps in a previous comment. detailed ones below)
Refactor the ratings retrieval at mod/glossary/view.php line 402 attaching the ratings to entries in $allentries
Remove the ratings function argument from methods like glossary_print_entry_lower_section() in mod/glossary/lib.php Ratings arrays dont need to be passed around anymore
Refactor glossary_print_entry_ratings() in mod/glossary/lib.php to use the $OUTPUT renderer
Remove any remaining references to $ratingsmenuused as its no longer used
Refactor glossary_get_user_grades() to use the ratings system instead of going direct to the database

Integrate ratings in the database module (see generic steps in a previous comment)
Is the query referencing ratings within data_get_participants() in mod/data/lib.php necessary? It doesnt look like it would return anything not already in $records.
Refactor data_print_ratings() to use the $OUTPUT renderer
Remove data_print_ratings_mean(), data_get_ratings_mean() and data_print_rating_menu()
Refactor data_get_user_grades() to use the ratings system and not go direct to the database

Remove redundant files like mod/data/report.php There are files within the 3 affected modules that are no longer used. At a minimum there's 1 to view submitted ratings and 1 to submit a rating. This is now all handled by the ratings code outside of the modules.

Once integration is complete and looks ok alter the upgrade code to drop the old module specific rating tables.

Patch attached (20100415_forum.patch) It fixes some problems with the rating system and integrates it into the forum module. Resolves MDL-21995 and MDL-17369.

Depending on the results of the review all thats left is integration into the glossary and database modules (will be much faster than the forum integration), removal of no longer required files and dropping of no longer required tables.

Slightly updated patch attached. The issue of the scale help icon being broken when using the "connected ways of learning" scale is being dealt with in MDL-22093.

New patch attached. Ignore all previous patches. This one includes forum and glossary integration. I've removed most of the now unnecessary rating code from the glossary module but have left some commented out for the moment.

1/ random whitespace, newlines, tab chars in many places
2/ the upgrade is not going to scale, can not be interrupted - why not do it in pure sql, is there any technical reason to do it one-by-one?
3/ hardcoded table prefix "FROM mdl_rating r" !!
4/ the label "Grade" in ratings section of modedit is wrong - this is not a grade, this is a rating
5/ the gradebook interaction problems with sum aggregation are not solved
6/ rate_ajax.php was not updated to use the new ajax arror handler (see comments ajax)
7/ no need to block the guest user in rating scripts - the has_capability() blocks all write caps for the guest user automatically (just remove the todos)
8/ ratings are not removed when activity deleted, right?
9/ reset of activity does not remove ratings, right?
10/ forum_ratings table dropped from install.xml but not in upgrade - this should be done at the same time, this install.xml MUST match current db schema; just tried to grep for forum_ratings, WTF? it is still used in tons of places - this means the forum would be completely borked in new installs!

Yet another patch attached.

1) Martin directed me to some spots that had some white space weirdness going on. I think I've fixed them.
2) a query something like this should work for each module that has legacy ratings. I'll need to do some testing before switching over to this approach.

INSERT mdl_rating (contextid, scaleid, itemid, rating, userid, timecreated, timemodified)
SELECT cxt.id, f.scale, r.post AS itemid, r.rating, r.userid, r.time AS timecreated, r.time AS timemodified
FROM mdl_forum_ratings r
JOIN mdl_forum_posts p on p.id=r.post
JOIN mdl_forum_discussions d on d.id=p.discussion
JOIN mdl_forum f on f.id=d.forum
JOIN mdl_course_modules cm on cm.instance=f.id
JOIN mdl_context cxt ON cxt.instanceid=cm.id
JOIN mdl_modules m ON m.id=cm.module
WHERE m.name = 'forum' AND cxt.contextlevel = 70

3) fixed
4) changed from "grade" to "scale". Its a slightly odd select box as you're either selecting a scale or the maximum possible numerical value.
5) "the gradebook interaction problems with sum aggregation are not solved"
I'm not sure what you mean.
6) I'll update the ajax error handling in the morning.
7) fixed
8) fixed
9) fixed
10) will add drop to upgrade script tomorrow. I also made a pass over references to the old table. most are removed. backup/restore of ratings hasnt been refactored yet.

mini-comment:

2) query looks ok. Just dont' use harcoded 70 but corresponding CONTEXT_XXX constant.

and one question:

are we going to continue storing scale-ids in their "signed" way in that table? With negatives meaning "real scale" and positives meaning "max value" ? Just to be 100% sure. Can be changed later. Just all we must ack about that.

Ciao

Theres a bunch of stuff that will change about that query before it actually goes into the code. The above query is my test version copied and pasted from query analyzer. Thankyou for having a look.

For the moment, scale ids are still going to be stored in their plus id numerical, minus is a PK "signed" way. That way the scale ids in the rating table match those in the forum, glossary and database tables. There are plans to refactor scales to remove that system but that refactoring isn't going to make it into 2.0. When the refactoring does happen however the rows in the rating table will need to be altered to reflect proper scale ids.

Did a commit. New ratings should be in use in the forum and glossary. Database integration isn't done.

I haven't yet added a drop to the upgrade of forum_ratings and glossary_ratings. Want to see one or two people do the upgrade before dropping the old tables.

I've opened MDL-22162 to improve the rating ajax error handling. For any new issues with ratings its up to you whether to post them here or in a new tracker item. It depends on how big the issue is I guess.

Marking this resolved. Open a separate issue for any bugs.