Add a ratingarea field to the ratings API

Ok there was a bit more to this than I first expected.

The component which is now a required field is still not being saved to the database, so while it is possible to use the component for validation and within code when processing comments you can't refer to or limit it within queries. This means essentially you still cannot rate more than one thing within a context, trying to do so leads to many bugs, this is a big problem when trying to use ratings within local plugins.

I'll shortly post details of a patch that adds both component and ratingarea to the database table and implements these throughout the rating API, in the case of component it will be properly limited and dealt with when fetching ratings, and in the case or ratingarea throughout the rating API where ever applicable.
The patch will also make other small changes along the way such as moving logic out of the renderers to make the ratings more code friendly/usable.

Cheers
Sam

Hi Andrew,

Could I please get you to peer review this for me when you have a free moment.
I still have more testing to do to make sure that it is 100% not going to cause any regressions however it is passing the initial tests.
The main changes I've made are:

• Added the ratingarea field and implemented it throughout the rating API as an optional
  field that defaults to null.
• Added a component field to the database and implemented its use throughout the rating
  API in order to allow proper selection based upon it.
• Added a get_rating_manager function to allow a single instance of rating_manager to be
  used which in turn allows static object caching.
• Moved the logic in the render_rating method to methods of the rating object.
• Added a unique index to the rating table that represents a user rating an item with
  the fields contextid, itemid, component, ratingarea, and userid.
• Cleaned up rating/index.php to use html_table object and moved inline styles to CSS.
• Added missing properties of the rating object that were being set throughout the rating
  API.

Cheers
Sam

What is the purpose of rating area? Is context id, item id and component not guaranteed to be unique? Can you have multiple instances of a component in a single context? Either way can you please change the following comment to explain why it exists. Its currently not clear what rating area is for.

/**
 Teh rating area to associate this rating with
 @var string
*/
public $ratingarea = null;

Feel free to leave the "teh" in

Maybe change "Returns a URL to view the ratings for this rating object" to something like "Returns the URl to view the ratings for a single item". A rating object represents a single rating by a single user. Its not a collection.

Just looking at the code it looks great otherwise. Ill do some testing to make sure it all still roughly works.

Something is wrong. When using ajax ratings when i submit a rating in the forum as a user who have the capability to see the aggregated rating result the aggregate isnt being updated. If I refresh the page I see the new aggregate but its no longer being updated via ajax.

Non-ajax ratings seem fine (they load the new aggregate as part of the next page load so dont use the fancy ajaxy aggregation updating).

Cool thanks for looking at that Andrew.

The purpose of the ratingarea is to allow a component to rate more than one item. In the case of the contrib plugin I want users to be able to rate plugin versions and plugins in general. The context will always be the system context, the component always local_contrib, and depending upon what is being rated the the item id will either be the version id or the plugin id, and the ratingarea will be either plugin or pluginversion.

I'll fix up the comments shortly and then test the bug you mentioned about the aggregate not being updated.

Cheers
Sam

Hi Andrew,

Thanks for the peer-review I've tidied up the comments and fixed the aggregate view bug you found and this is ready for peer-review again.
Any chance I could get you to give it a quick test as well?

Cheers
Sam

I think its all working fine now although I noticed two bugs while doing this. I'm 99% confident that both of these bugs predate your changes and have in fact existed since 2.0.0

MDL-27502 activity using ratings formats grade incorrectly in gradebook if using custom scale and rating aggregation is set to count
MDL-27503 Changing an activity aggregation type doesnt update student grades

Cool thanks Andrew, this is up for integration now.

It appears that the rating area parameter isnt checked at all. I can put whatever I like there and it gets put in the database. Seems like we should be doing something there but Im not sure what...

For example this works
http://andrew.moodle.local/review/master/rating/rate.php?contextid=23&component=mod_glossary&itemid=1&scaleid=-1&rating=1&rateduserid=3&returnurl=http://andrew.moodle.local/moodle20stable/mod/forum/discuss.php?d=382&sesskey=xT8XlbSRsL&ratingarea=blam

and blam gets inserted into the database.

edit: "whatever I like" is an overstatement. The url param is limited to PARAM_ALPHANUMEXT. Numbers, letters and _-.

Hmmm good spotting - time for a new callback to fetch possible rating areas so that we can cross check things.
I'll do that quickly now that should be nice and easy.

Alright I've pushed up a commit that introduces a callback to get rating areas from a plugin to cross check.
Implementing the checks within the rating uses would have broken backwards compatibility so I think this is a better idea.
Before this issue gets closed I need to update the rating docs with information about a) the ratingarea option b) this new callback.

Cheers
Sam

Hi,

I've not been able to review this completely, anyway, I've found some things missing/to improve:

1) The unique index, currently (contextid, itemid, component, ratingarea, userid). IMO it should go from max multiplicity down to min, i.e: (component, ratingarea, contextid, itemid, userid). Also note that perhaps doing that index unique is not the best idea in case dupes exist in the table. We haven't been enforcing that until now. What about not including the userid? After all it can continue being enforced by the application and the most indexing benefit will apply to getting all the ratings (together) for a given itemid.

2) The nullable component and ratingarea columns. Why not, instead, define them as mandatory and upgrade all current ratings (data, forum, glossary). And put things in place one and forever. Instead of special null statues?

3) backup & restore (harcoded right now in forum/data/glossary) must support new columns (on backup and restore) and apply good defaults for old versions.

4) Does this mix ok / complements with current security issue related to component/itemid... validations? At the end all the columns are properly checked? (component, area, context, itemid, userid?). No "holes" remaining? As said I haven't reviewed the whole patch.

5) I don't like to "unbalance", once more the comments and the ratings APIs. Immediate work should be done in comments to get the same structure. Of course, unrelated to the integration of this issue, but should be done IMO.

6) Of course, we continue having the pending problem of what happens with aggregations generating results out of scale (COUNT/SUM) and the "locking" of scale if there are ratings already (or to guarantee proper aggregation in case we allow changes on scales along the life of one item). It is separated from this but should be fixed ASAP too.

7) As commented above by developer, this needs documentation and note in the 2.1 release notes about changes in API. Is it available already?

And these are my thoughts, sorry for mixing directly related with this issue (1, 2, 3 and 4), with general ratings concerns (5, 6 and 7), but I think we cannot forget about the later(s) while, of course, the formers are the required ones to get this integrated.

I think we should hold this for 1-week coz it needs some more love and also clarify if 5, 6 and 7 are under control and planned for some release point.

Ciao

Hi Eloy,

Thanks for having a look at it, looking over your comments I think it is fine for this to wait a week.

In regards to the points that you raised:

1) Good point about the order of the fields in the index, I hadn't thought of that but it make good sense. As for it being a unique index and potentially causing trouble in the database, would changing that index to be not unique be an option? If not no worries I'll remove userid (I'm no expert on databases + index's so I'll leave that up to you)

2) The component and ratingarea columns are null simply at the moment because I didn't want to break backwards compatibility again in 2.1 (after it was broken in 2.0.3) however I will talk to Martin - it would make it so much easier if it wern't null and we upgraded the modules. Also nice to have everything sorted once and for all - or at least until the next round of changes.

3) If we make the component and ratingarea not null then this will need to be done - in which case I'll do it at the same time.

4) Yes, as part of these changes a new callback was added that retrieves a components ratingarea's which we use to validate the rating area. Again if component and rating area arn't null then I'll remove that callback and upgrade the component validation routines to check it (as they will now have a ratingarea).

— Just pointing out at this point that I will look to talk to Martin about the ratingarea + component not null thing today so that I can start work on this immediately if need be.

5) Hmmm yes I had in my mind that comments already had component within the database however just checking now I see that it doesn't. It already has commentarea which is good however it should certainly store and select upon component as well. On the upside component is a required field now so we should be able to do it without breaking backwards compatibility.

5) Hehe this issue brings rating's up to speed with comments which already has comments area and component implemented.

6) Yip - its bit of a shame all of these ratings issues came out so close to 2.1's deadline it would be much nicer to fix them all up for a single release however I think unfortunately we may now be out of time for 2.1 and that issue plus the other rating issues may end up being in the release after.

7) Haha I had intentionally left the documentation until this went through thinking there was a reasonable chance something will need to be changed. Once the final issues are decided and the work complete and set for integration I will update the docs.

Cheers
Sam

rejecting (reopening) after discussing this with Sam. More changes coming next week. Thanks!

Alright - I've just created a revised branch for the noted changes which are as follows:

1. The component and ratingarea fields are now mandatory.
2. The new index is no longer unique and leaves out userid.
3. forum has been upgraded (including backup+restore)
4. glossary has been upgraded (including backup+restore)
5. data has been upgraded (including backup+restore)
6. Created MDL-27548 to see component stored for comments as well.

While working on this issue I found several bugs in the module code, I've fixed up the critical ones and will create issues for the remaining things.

Andrew could you please test ratings with this patch for me, and Eloy could you please review the code.

Cheers
Sam

Just updated the PULL branch - it is now wip-MDL-27471-master-revised

The commit with the latest changes: https://github.com/samhemelryk/moodle/commit/22d87fb385b398f1b56049626e6c4517a8c6093d

Ok, after a few false starts due to looking at the wrong branch and troubles with upgrade code it all seems to be working fine.

Rebased after weekly release

Some comments, all easy to fix IMO:

1) The "itemid" INDEX in the ratings table seems 100% incorrect and unnecessary. We aren't going to do itemid-only searches anymore!
2) Changes is core ratings cause the data/glossary/forum modules to require at least moodle version 2011051900[.01]. So all those modules need proper $module->requires in version.php
3) I've seen changes here and there, unrelated with the ratings thingy, like:

$row = new html_table_row(array(get_string('authorfirstname', 'data').': ', '##firstname##'));

(and others). IMO it they are proper fixes it would be worth commenting them in the testing instructions to be tested properly).
4) The xxx_get_participants() should be all them marked as deprecated. Same for glossary_count_unrated_entries() (in fact I cannot imagine why that was introduced, never had seen it before, lol). One "TODO: deprecated, to be deleted in 2.2" would be ok for later finding.
5) This is the worst (but easy fixable, lol). After looking ratings stuff more carefully, I think that your change from instantiation (new) to singleton (get + static) is completely wrong as far as is perfectly possible to handle more than one ratings_manager in the same request and the singleton really prevents that. So +1 to change back in this case.
6) The updates performed on upgrade scripts (data, forum. glossary) can spend a lot of time in one site with BIG numbers (of ratings and modules). I'd recommend one of this:

• Give more time to it (the default, if I'm not wrong is only 5 mins).
• Change it to recordset + progress bar if browser timeouts can happen
  (surely the first solution is (safe) enough if you are able to test-upgrading, say 100.000 records belonging to 10.000 different contexts in less than 5 minutes).
  7) Also the upgrades should be tested on as many DBs as possible (you know, it it works under Oracle, there are high chances it's really cross-db).

And that's all, the rest looks ok with the component + area "injected" where necessary. Tests will reveal if anything is borked.

Thanks and ciao

Hi Eloy,

Thanks for looking at that - I've just pushed up changes to that see the code related points you've raised fixed and I've updated the testing instructions.
I haven't yet tested this on a system with thousands of contexts and hundreds of thousands of ratings.
Unfortunately I haven't had time to create a script to set up such as system for me yet - I will try get through that shortly as I need this to get in sooner rather than later.

Cheers
Sam

And this has been integrated, yay! (I squashed everything into single commit, as talked).

Ciao

Got pulled away from the computer for an internal matter right after i started testing this. I can pick it up first thing in the morning if its still here. So sorry.

I have deleted a rated glossary entry without error.

I have successfully added and edited ratings in all 3 supported modules. You don't seem to be able to delete a rating. If you select "Rate..." it should delete your rating. I'm getting an error back "Invalid numeric value"

For some reason, when using ajax, forum displays the error alert twice while glossary and database only show it once.

update: Ive raised MDL-27625

Attaching one backup file containing ratings, for sure in forum + glossary (not 100% sure in data).

Note: Added one extra commit performing some whitespace cleanup I skipped on integration.

Note2: MDL-27625 has been integrated and is being tested right now.

MDL-27625 has been integrated and tested ok.

Re-esting this:

• tried rating and unrating forum/data/glossary.
• tried restoring old backup having old ratings (without component/ratingarea).
• deleted glossary entry without problems.
• search in forums producing correct (forum/discussion/context) links.
• created new database activity, added some fields, all templates (7) displayed ok without error.
• rating/unrating cause proper grades to be pushed to gradebook.

Everything looking good so far, hence passing. Thanks!

Finally, "upstreamised", yay, many thanks!

Thankyou for all your help with this Eloy