Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.6
    • Component/s: Events API
    • Labels:
    • Testing Instructions:
      Hide
      1. Create an assignment activity.
      2. Update the activity.
      3. Delete the activity.
      4. Create a label resource by dragging content into the course area. (Not creating manually)
      5. Edit the label.
      6. Delete the label.
      7. Create a scorm resource by dragging a scorm package into the course area.
      8. In each case above, make sure no error is encountered.
      9. Look at the site logs and make sure there is an entry with all correct details for each of the action above.
      10. Run unitests and make sure they pass (course/tests/courselib_test.php)
      Show
      Create an assignment activity. Update the activity. Delete the activity. Create a label resource by dragging content into the course area. (Not creating manually) Edit the label. Delete the label. Create a scorm resource by dragging a scorm package into the course area. In each case above, make sure no error is encountered. Look at the site logs and make sure there is an entry with all correct details for each of the action above. Run unitests and make sure they pass (course/tests/courselib_test.php)
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-39959-master
    • Story Points:
      20
    • Rank:
      51950
    • Sprint:
      BACKEND Sprint 4

      Description

      Replace all legacy events related to module.
      course/dnduploadlib.php: events_trigger('mod_created', $eventdata);
      course/lib.php: events_trigger('mod_deleted', $eventdata);

      All places where course modules are created, updated or deleted.

        Issue Links

          Activity

          Hide
          Marina Glancy added a comment -

          event mod_updated is currently triggered in update_moduleinfo() by calling edit_module_post_actions()

          Show
          Marina Glancy added a comment - event mod_updated is currently triggered in update_moduleinfo() by calling edit_module_post_actions()
          Hide
          Martin Dougiamas added a comment - - edited

          Does it make sense to consider a consistent core event here for mod_view?

          I'm thinking while it might make some sense for consistency across modules to have assignment view, lesson view, forum view etc in a reliable way, these are in the plugin code anyway soit probably makes more sense to:

          1) Keep view events together with the module, with all other operations from that plugin.
          2) Enforce some event names like "view" and "view all" as standard in activity plugins.

          ...This is pretty much what add_to_log always did.

          Show
          Martin Dougiamas added a comment - - edited Does it make sense to consider a consistent core event here for mod_view? I'm thinking while it might make some sense for consistency across modules to have assignment view, lesson view, forum view etc in a reliable way, these are in the plugin code anyway soit probably makes more sense to: 1) Keep view events together with the module, with all other operations from that plugin. 2) Enforce some event names like "view" and "view all" as standard in activity plugins. ...This is pretty much what add_to_log always did.
          Hide
          Ankit Agarwal added a comment -

          We have the following options here:-

          1.a edit_module_post_actions() triggers core event \core\event\module_created and \core\event\module_updated
          1.b edit_module_post_actions() triggers course module events, something like :- \mod_book\event\module_created and \mod_book\event\module_updated

          2.a course_delete_module() triggers core event \core\event\module_deleted
          2.b The call back 'modulename_delete_instance' triggers module event ex:- \mod_book\event\module_deleted

          3.a view.php of all module trigger core event \core\event\module_viewed
          3.b view.php of all module trigger module events ex:- \mod_book\event\module_viewed

          I vote for 1a,2a and any of 3a/3b

          Show
          Ankit Agarwal added a comment - We have the following options here:- 1.a edit_module_post_actions() triggers core event \core\event\module_created and \core\event\module_updated 1.b edit_module_post_actions() triggers course module events, something like :- \mod_book\event\module_created and \mod_book\event\module_updated 2.a course_delete_module() triggers core event \core\event\module_deleted 2.b The call back 'modulename_delete_instance' triggers module event ex:- \mod_book\event\module_deleted 3.a view.php of all module trigger core event \core\event\module_viewed 3.b view.php of all module trigger module events ex:- \mod_book\event\module_viewed I vote for 1a,2a and any of 3a/3b
          Hide
          Ankit Agarwal added a comment - - edited

          It was decided in scrum to have one core event for updated/deleted/created.
          Replacing legacy events for those action will be taken care in this issue.

          For view, it was decided to have an core abstract class, which will be extended by all plugins.
          To create all new view events I have created MDL-41353

          Thanks

          Show
          Ankit Agarwal added a comment - - edited It was decided in scrum to have one core event for updated/deleted/created. Replacing legacy events for those action will be taken care in this issue. For view, it was decided to have an core abstract class, which will be extended by all plugins. To create all new view events I have created MDL-41353 Thanks
          Hide
          Ankit Agarwal added a comment - - edited

          Note:-

          1. I have removed add_to_log() with plugin component. They don’t make sense any more. If we really want to keep it, we can just leave the add_to_log() as it is for these for the time being.
          2. I have replaced add_to_log() for component core, with the event's legacy log data.
          3. Various add_to_log() calls related to course_module actions in plugins, will be taken care of , when their respective module is cleaned up in MDL-39952

          Pushing for review.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Note:- I have removed add_to_log() with plugin component. They don’t make sense any more. If we really want to keep it, we can just leave the add_to_log() as it is for these for the time being. I have replaced add_to_log() for component core, with the event's legacy log data. Various add_to_log() calls related to course_module actions in plugins, will be taken care of , when their respective module is cleaned up in MDL-39952 Pushing for review. Thanks
          Hide
          Frédéric Massart added a comment -

          Hi Ankit,

          1. You don't need to pass 'courseid', it is guessed from the context.
          2. I thought we agreed on storing the parent context when we delete something. Not sure we actually started doing it, do you remember anything about it?
          3. instance property could be called instanceid for better understanding.
          4. Do not pass the whole record in other, add_record_snapshot() is there for that reason.
          5. I like it when descriptions start with a capital, for that you need to rephrase it a bit. "The module blah ..."
          6. Could you add some validation to your events to make sure that the other parameters are set? The rest of the class rely on it, so I think it makes sense.
          7. Please squash your commits
          8. Your arrays should be indented as per the guide lines:
          $event = \core\event\blah::create(array(
              'key' => array(
                  'key2' => '',
                  'key3' => ''
              ),
              'key2',
              'key3'
          ));
          

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Ankit, You don't need to pass 'courseid', it is guessed from the context. I thought we agreed on storing the parent context when we delete something. Not sure we actually started doing it, do you remember anything about it? instance property could be called instanceid for better understanding. Do not pass the whole record in other , add_record_snapshot() is there for that reason. I like it when descriptions start with a capital, for that you need to rephrase it a bit. "The module blah ..." Could you add some validation to your events to make sure that the other parameters are set? The rest of the class rely on it, so I think it makes sense. Please squash your commits Your arrays should be indented as per the guide lines: $event = \core\event\blah::create(array( 'key' => array( 'key2' => '', 'key3' => '' ), 'key2', 'key3' )); Cheers, Fred
          Hide
          Ankit Agarwal added a comment -

          Hi Fred,
          Thanks for the review.

          1. It is better to pass it, since we already have it. It saves us cost by not going through all the trouble of calling $context->get_course_context(), which can cost upto 1 DB query + processing.
          2. As decided on scrum, we should be passing current context here. So leaving it as it is.
          3. Updated.
          4. It is intentional. I do this only for deleted events. The loggers must store this information, since it is lost after this point.
          5. Done.
          6. Done.
          7. Done.
          8. Done.

          Pushing forward.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Fred, Thanks for the review. It is better to pass it, since we already have it. It saves us cost by not going through all the trouble of calling $context->get_course_context(), which can cost upto 1 DB query + processing. As decided on scrum, we should be passing current context here. So leaving it as it is. Updated. It is intentional. I do this only for deleted events. The loggers must store this information, since it is lost after this point. Done. Done. Done. Done. Pushing forward. Thanks
          Hide
          Dan Poltawski added a comment -

          Hi Ankit,

          There are some issues with this:

          course/modlib.php
          A1. I have concerns about passing the event name to edit_module_post_actions , yes that was the existing behaviour, but is it still what we should be doing here? I think you need to consider that further, passing $eventname when we now have a tightly defined API which might require one param for one event and not for other doesn't seem right. You are effectively coupling any events passed to this to always have the same required params. If the params ever vary between created and updated requirements then you are screwed. Please have some discussion about this and be certain you want to do it.

          course/tests/courselib_test.php:
          B1. In multiple places, you are retrieving the things to test against from the event itself. This is silly, let me show you an example why from test_course_module_updated_event():

          $cm = $DB->get_record('course_modules', array('id' => $event->objectid));
          // [...]
          $this->assertEquals($cm->id, $event->objectid);
          

          You've tested exactly nothing there. You should be getting the $cm record explicitly when you are creating things, else we'll create useless tests which validate the event data is the same as the event data. I'm afraid thats probably a symptom of using those helper methods which already existed. I advise you to make those helper methods better and return something more robust to test against.

          B2. $eventdata = new \stdClass(); doesn't seem correct to me. You are not in a namespace, no need to 'escape' it, right? (If there is, why aren't you doing it for moodle_url?)

          B3. 'modulename' => 'Somename' It would be much better to use better 'test data' here, it improves the readability and helps demonstrate better what you are actually testing. That capitlisation makes it look like a lang string, where as its actually a module name (always lowercase). So why not use the real module name?

          B4. In your get_record calls, please add MUST_EXIST's to make it clearer where the failure is in case of failure.

          B5. Why in test_course_module_deleted_event() are you capturing the course module created event? (Oh I see, its the same problem as my first point). Please add a another point to B1.

          All event files

          C1. Related to B3, we need some documentation on what the $other[] data is supposed to be. In B3 I say that $this->other['modulename'], but I am not certain, I only made that assumption because you use it in get_url() as a module name. But then, what is $this->other['name']? Its not clear what these variables are supposed to be.

          Show
          Dan Poltawski added a comment - Hi Ankit, There are some issues with this: course/modlib.php A1. I have concerns about passing the event name to edit_module_post_actions , yes that was the existing behaviour, but is it still what we should be doing here? I think you need to consider that further, passing $eventname when we now have a tightly defined API which might require one param for one event and not for other doesn't seem right. You are effectively coupling any events passed to this to always have the same required params. If the params ever vary between created and updated requirements then you are screwed. Please have some discussion about this and be certain you want to do it. course/tests/courselib_test.php: B1. In multiple places, you are retrieving the things to test against from the event itself. This is silly, let me show you an example why from test_course_module_updated_event(): $cm = $DB->get_record('course_modules', array('id' => $event->objectid)); // [...] $ this ->assertEquals($cm->id, $event->objectid); You've tested exactly nothing there. You should be getting the $cm record explicitly when you are creating things, else we'll create useless tests which validate the event data is the same as the event data. I'm afraid thats probably a symptom of using those helper methods which already existed. I advise you to make those helper methods better and return something more robust to test against. B2. $eventdata = new \stdClass(); doesn't seem correct to me. You are not in a namespace, no need to 'escape' it, right? (If there is, why aren't you doing it for moodle_url?) B3. 'modulename' => 'Somename' It would be much better to use better 'test data' here, it improves the readability and helps demonstrate better what you are actually testing. That capitlisation makes it look like a lang string, where as its actually a module name (always lowercase). So why not use the real module name? B4. In your get_record calls, please add MUST_EXIST's to make it clearer where the failure is in case of failure. B5. Why in test_course_module_deleted_event() are you capturing the course module created event? (Oh I see, its the same problem as my first point). Please add a another point to B1. All event files C1. Related to B3, we need some documentation on what the $other[] data is supposed to be. In B3 I say that $this->other ['modulename'] , but I am not certain, I only made that assumption because you use it in get_url() as a module name. But then, what is $this->other ['name'] ? Its not clear what these variables are supposed to be.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Ankit Agarwal added a comment -

          Thanks Dan for the review,
          A- I didn't really wanted to change the existing design. But anyway I have updated the patch for this. Since edit_module_post_actions() is an internal api, I have not mentioned anything in upgrade.txt.
          B- Fixed up.
          C- This is a problem with all events. I will raise this in scrum and create a issue for all events. I suggest we have a self documenting api something like get_other_param_description().

          can you please review it again?
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Dan for the review, A- I didn't really wanted to change the existing design. But anyway I have updated the patch for this. Since edit_module_post_actions() is an internal api, I have not mentioned anything in upgrade.txt. B- Fixed up. C- This is a problem with all events. I will raise this in scrum and create a issue for all events. I suggest we have a self documenting api something like get_other_param_description(). can you please review it again? Thanks
          Hide
          Dan Poltawski added a comment -

          Its worth commenting about edit_module_post_actions() in upgrade.txt. You never know what crazy things people might've done, and its just a small courtsey (and there is nothing saying DO NOT USE THIS, NOT A SUPPORTED API, is there?)

          Show
          Dan Poltawski added a comment - Its worth commenting about edit_module_post_actions() in upgrade.txt. You never know what crazy things people might've done, and its just a small courtsey (and there is nothing saying DO NOT USE THIS, NOT A SUPPORTED API, is there?)
          Hide
          Ankit Agarwal added a comment -

          Alright, done!

          Show
          Ankit Agarwal added a comment - Alright, done!
          Hide
          Marina Glancy added a comment -

          Hi Ankit,
          please rebase the branch over current master, there is a merge conflict (but simple one).

          I completely agree with Fred about his #4 - we should not be passing the $cm record twice in course_module_deleted. Especially when summary is big, it is such a waste of log storage resources. get_legacy_logdata() is perfectly able to retrieve the data from snapshot.

          Also I can see the loss of functionality: when module is created or updated there are two log entries in 2.5 and only one in 2.6. This is very wrong.

          Show
          Marina Glancy added a comment - Hi Ankit, please rebase the branch over current master, there is a merge conflict (but simple one). I completely agree with Fred about his #4 - we should not be passing the $cm record twice in course_module_deleted. Especially when summary is big, it is such a waste of log storage resources. get_legacy_logdata() is perfectly able to retrieve the data from snapshot. Also I can see the loss of functionality: when module is created or updated there are two log entries in 2.5 and only one in 2.6. This is very wrong.
          Hide
          Ankit Agarwal added a comment -

          Thanks Marina,
          For the review.

          1. Rebased and $record removed.
          2. About multiple log entries, it was discussed in scrum. All options mentioned here(https://tracker.moodle.org/browse/MDL-39959?focusedCommentId=239454&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-239454) were evaluated. We finally decided that we are not going to have module specific events for those course module actions. Hence we can either remove those logs now or wait till 2.7. I see no reason to delay it, but if you feel otherwise, we can obviously leave the second add_to_log as it is.

          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Marina, For the review. Rebased and $record removed. About multiple log entries, it was discussed in scrum. All options mentioned here( https://tracker.moodle.org/browse/MDL-39959?focusedCommentId=239454&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-239454 ) were evaluated. We finally decided that we are not going to have module specific events for those course module actions. Hence we can either remove those logs now or wait till 2.7. I see no reason to delay it, but if you feel otherwise, we can obviously leave the second add_to_log as it is. Thanks
          Hide
          Marina Glancy added a comment -

          Ankit, I'm not convinced.
          This change will break all module-specific log reports that select something like "SELECT * from mdl_log WHERE cmid=?"
          because "add" and "edit" actions no longer present in it. Besides it already changes results in /report/log/index.php when filtered by module

          Creating a module after the change produces one record in table log:

          id      time            userid 	ip 	        course 	module 	cmid 	action  	url                     	info 
          11882 	1379294906 	2 	127.0.0.1 	2 	course 	0 	add mod 	../mod/quiz/view.php?id=7 	quiz 7
          

          Before the change:

          id      time            userid 	ip 	        course 	module 	cmid 	action  	url                     	info 
          501 	1374021932 	2 	127.0.0.1 	2 	label 	70 	add             view.php?id=70                  16
          500 	1374021932 	2 	127.0.0.1 	2 	course 	0 	add mod         ../mod/label/view.php?id=70 	label 16
          

          PS Please squash commits modifying the same pieces of code.

          Show
          Marina Glancy added a comment - Ankit, I'm not convinced. This change will break all module-specific log reports that select something like "SELECT * from mdl_log WHERE cmid=?" because "add" and "edit" actions no longer present in it. Besides it already changes results in /report/log/index.php when filtered by module Creating a module after the change produces one record in table log: id time userid ip course module cmid action url info 11882 1379294906 2 127.0.0.1 2 course 0 add mod ../mod/quiz/view.php?id=7 quiz 7 Before the change: id time userid ip course module cmid action url info 501 1374021932 2 127.0.0.1 2 label 70 add view.php?id=70 16 500 1374021932 2 127.0.0.1 2 course 0 add mod ../mod/label/view.php?id=70 label 16 PS Please squash commits modifying the same pieces of code.
          Hide
          Ankit Agarwal added a comment -

          As discussed in scrum, we decided to leave the module specific add_to_log() in core till 2.7.
          update patch.
          Thanks

          Show
          Ankit Agarwal added a comment - As discussed in scrum, we decided to leave the module specific add_to_log() in core till 2.7. update patch. Thanks
          Hide
          Marina Glancy added a comment -

          Thanks Ankit, this has been integrated in master

          Show
          Marina Glancy added a comment - Thanks Ankit, this has been integrated in master
          Hide
          David Monllaó added a comment -

          It passes, for example for a label I can see the following logs when creating it:

          • course add mod - label 1
          • label add - ASDASD

          I found a problem when creating a scorm resource using drag & drop:

          500 Internal Server Error dndupload.js (line 795)
          "NetworkError: 500 Internal Server Error - http://localhost/INTEGRATION/master/course/dndupload.php"
          

          The issue is also present in the latest master weekly release but not in 25; creating an issue for it.

          Show
          David Monllaó added a comment - It passes, for example for a label I can see the following logs when creating it: course add mod - label 1 label add - ASDASD I found a problem when creating a scorm resource using drag & drop: 500 Internal Server Error dndupload.js (line 795) "NetworkError: 500 Internal Server Error - http: //localhost/INTEGRATION/master/course/dndupload.php" The issue is also present in the latest master weekly release but not in 25; creating an issue for it.
          Hide
          Sam Hemelryk added a comment -

          Yarrr me 'arties, good job done. Yer code 'as landed and the weeklies ave been released with your contributions in tow.
          The brethren thank ya for yer 'ard work and if there'd been treasure to ave ya would ave got yer cut.

          Thanks for the effort everyone, another successful weekly release has been rolled.
          Please keep in mind code freeze is just around the corner now, get your new features and improvements in ASAP.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Yarrr me 'arties, good job done. Yer code 'as landed and the weeklies ave been released with your contributions in tow. The brethren thank ya for yer 'ard work and if there'd been treasure to ave ya would ave got yer cut. Thanks for the effort everyone, another successful weekly release has been rolled. Please keep in mind code freeze is just around the corner now, get your new features and improvements in ASAP. Many thanks Sam

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Agile