Moodle
  1. Moodle
  2. MDL-27471

Add a ratingarea field to the ratings API

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.3
    • Fix Version/s: 2.1
    • Component/s: Ratings
    • Labels:
    • Testing Instructions:
      Hide

      These changes should be backwards compatible and there are no components within Moodle yet that implement this new field (the modules and plugins replacement will be the first).

      To test this you should make sure that it hasn't broken any areas of Moodle that use ratings so you should test Forum, Glossary, and Data.
      When testing you should test with both JavaScript enabled and disabled and just make sure you can rate, change your rating and that it is shown in the gradebook correctly.

      Test routine:

      1. Log in as a teacher and enter a course
      2. Add a new forum, set a scale and set to average ratings.
      3. Add a new glossary, set a scale and set to average ratings.
      4. Add a new database, set a scale and set to average ratings.
      5. Log out.
      6. Log back in as a student.
      7. Enter the course.
      8. Post a couple of times in the forum
      9. Create a couple of glossary entries.
      10. Create a couple of database entries.
      11. Log out.
      12. Log in as the teacher again
      13. Repeat the following with JS enabled and then JS disabled.
      14. Enter the forum and rate the students posts
      15. Change your ratings
      16. Remove your rating (choose the first option)
      17. Give them a rating again
      18. Enter the glossary and do the same for the students entries
      19. Enter the database and do the same for the students entries
      20. Go to the gradebook and make sure the studen's graded items show up correctly.

      Other required tests:

      • Restore one 2.x backup file generated before this week, containing rates and verify that those rates continue being displayed/handled properly.
      • When you create your database activity and have added fields head to the templates tab and make sure for each template type the default is loaded and that you don't get fatal errors on those pages.
      • Search some forum posts and make sure that the parent link and discussion link (see this post in context) still works.
      • Delete a glossary entry and make sure you don't get any errors about ratings.

      Cheers
      Sam

      Note: This is destined for 2.1 - you only need to test the master branch.

      Show
      These changes should be backwards compatible and there are no components within Moodle yet that implement this new field (the modules and plugins replacement will be the first). To test this you should make sure that it hasn't broken any areas of Moodle that use ratings so you should test Forum, Glossary, and Data. When testing you should test with both JavaScript enabled and disabled and just make sure you can rate, change your rating and that it is shown in the gradebook correctly. Test routine: Log in as a teacher and enter a course Add a new forum, set a scale and set to average ratings. Add a new glossary, set a scale and set to average ratings. Add a new database, set a scale and set to average ratings. Log out. Log back in as a student. Enter the course. Post a couple of times in the forum Create a couple of glossary entries. Create a couple of database entries. Log out. Log in as the teacher again Repeat the following with JS enabled and then JS disabled. Enter the forum and rate the students posts Change your ratings Remove your rating (choose the first option) Give them a rating again Enter the glossary and do the same for the students entries Enter the database and do the same for the students entries Go to the gradebook and make sure the studen's graded items show up correctly. Other required tests: Restore one 2.x backup file generated before this week, containing rates and verify that those rates continue being displayed/handled properly. When you create your database activity and have added fields head to the templates tab and make sure for each template type the default is loaded and that you don't get fatal errors on those pages. Search some forum posts and make sure that the parent link and discussion link (see this post in context) still works. Delete a glossary entry and make sure you don't get any errors about ratings. Cheers Sam Note: This is destined for 2.1 - you only need to test the master branch.
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull Master Branch:
      wip-MDL-27471-master-revised
    • Rank:
      17263

      Description

      This change is required by the modules and plugins database replacement currently in development.
      In order to be able to rate several items within the same context+component pairing I need to add a ratingarea field like the files and comments API use.
      This field will be optional NULL in order to not break backwards compatibility with existing items.

      This will be targeted for 2.1

      Cheers
      Sam

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          Andrew Davis added a comment -

          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.

          Show
          Andrew Davis added a comment - 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.
          Hide
          Andrew Davis added a comment - - edited

          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).

          Show
          Andrew Davis added a comment - - edited 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).
          Hide
          Sam Hemelryk added a comment - - edited

          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

          Show
          Sam Hemelryk added a comment - - edited 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
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          Andrew Davis added a comment -

          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

          Show
          Andrew Davis added a comment - 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
          Hide
          Sam Hemelryk added a comment -

          Cool thanks Andrew, this is up for integration now.

          Show
          Sam Hemelryk added a comment - Cool thanks Andrew, this is up for integration now.
          Hide
          Andrew Davis added a comment - - edited

          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 _-.

          Show
          Andrew Davis added a comment - - edited 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 _-.
          Hide
          Sam Hemelryk added a comment -

          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.

          Show
          Sam Hemelryk added a comment - 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.
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - rejecting (reopening) after discussing this with Sam. More changes coming next week. Thanks!
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - Alright - I've just created a revised branch for the noted changes which are as follows: The component and ratingarea fields are now mandatory. The new index is no longer unique and leaves out userid. forum has been upgraded (including backup+restore) glossary has been upgraded (including backup+restore) data has been upgraded (including backup+restore) 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
          Hide
          Sam Hemelryk added a comment -

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

          Show
          Sam Hemelryk added a comment - Just updated the PULL branch - it is now wip- MDL-27471 -master-revised
          Hide
          Sam Hemelryk added a comment -
          Show
          Sam Hemelryk added a comment - The commit with the latest changes: https://github.com/samhemelryk/moodle/commit/22d87fb385b398f1b56049626e6c4517a8c6093d
          Hide
          Andrew Davis added a comment -

          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.

          Show
          Andrew Davis added a comment - 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.
          Hide
          Sam Hemelryk added a comment -

          Rebased after weekly release

          Show
          Sam Hemelryk added a comment - Rebased after weekly release
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          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

          Show
          Eloy Lafuente (stronk7) added a comment - - edited 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
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          Eloy Lafuente (stronk7) added a comment -

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

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been integrated, yay! (I squashed everything into single commit, as talked). Ciao
          Hide
          Glenn Ansley added a comment -

          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.

          Show
          Glenn Ansley added a comment - 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.
          Hide
          Andrew Davis added a comment - - edited

          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

          Show
          Andrew Davis added a comment - - edited 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
          Hide
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - Attaching one backup file containing ratings, for sure in forum + glossary (not 100% sure in data).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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.

          Show
          Eloy Lafuente (stronk7) added a comment - 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.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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.
          Show
          Eloy Lafuente (stronk7) added a comment - 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.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Everything looking good so far, hence passing. Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Everything looking good so far, hence passing. Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Finally, "upstreamised", yay, many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Finally, "upstreamised", yay, many thanks!
          Hide
          Andrew Davis added a comment -

          Thankyou for all your help with this Eloy

          Show
          Andrew Davis added a comment - Thankyou for all your help with this Eloy

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: