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

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.

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

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

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

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

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

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.

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

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

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

Alright, done!

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.

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

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.

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

Thanks Ankit, this has been integrated in master

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.

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