Details

    • Type: Task
    • Status: Closed
    • Priority: 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
    • Sprint:
      BACKEND Sprint 4
    • Story Points (Obsolete):
      20
    • 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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            marina Marina Glancy added a comment -

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

            Show
            marina Marina Glancy added a comment - event mod_updated is currently triggered in update_moduleinfo() by calling edit_module_post_actions()
            Hide
            dougiamas 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
            dougiamas 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_frenz 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_frenz 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_frenz 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_frenz 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_frenz 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_frenz 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
            fred 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
            fred 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_frenz 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_frenz 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
            poltawski 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
            poltawski 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            ankit_frenz 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_frenz 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
            poltawski 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
            poltawski 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_frenz Ankit Agarwal added a comment -

            Alright, done!

            Show
            ankit_frenz Ankit Agarwal added a comment - Alright, done!
            Hide
            marina 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 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_frenz 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_frenz 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 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 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_frenz 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_frenz 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 Marina Glancy added a comment -

            Thanks Ankit, this has been integrated in master

            Show
            marina Marina Glancy added a comment - Thanks Ankit, this has been integrated in master
            Hide
            dmonllao 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
            dmonllao 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
            samhemelryk 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
            samhemelryk 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:
                  Fix Release Date:
                  18/Nov/13

                  Agile