Thanks Mark for taking on this issue.

That's OK mate. I understand it was a bit too difficult for you so you had to pass it on to one of the top dogs.

Cheer up nub, you'll get there one day!

Adding Tim as a watcher as he is the component lead.

Mark Nelson No need to add me as watcher, since I get emails for all quiz and question issues anyway. If I understood what you were going to be doing, or why, I might comment, but I don't.

We are removing the add_to_log calls, and replacing them with events. I just added you because I have seen in the past that you got annoyed on an issue when you weren't included when it involved the quiz, maybe the component wasn't set to quiz so you weren't notified. Any ways, won't add you as a watcher when the quiz is set as one of the components.

Better safe than sorry with watchers I suppose. At least the tracker does not send email twice in that case. I just wanted to make sure you knew/had remembered that aspect of how the tracker works.

I know you are going to "removing the add_to_log calls, and replacing them with events". The summary says that already. I still don't know why. How does this make the world a better places for teachers and learners?

I think we should create some parent classes similar to course_module_viewed to reflect:
1. add/update/delete actions of module "teacher" content, such as - quiz questions, book pages, lesson pages
2. add/update/delete actions of module "student" content, such as - quiz attempts, forum discussions/posts, assignment and workshop submissions, choice and feedback responses, etc.

All those actions across all modules need to have something in common so they are easily filtered for "recent activity" block/report or similar activity reports.
I suggest to discuss it on backend meeting

Given Marina's question, which has not yet had an answer, taking this out of waiting for peer review.

oh sorry Tim, I should have commented here. We have discussed it on the meeting and I agreed that the parent class is not necessary. Event object has properties such as contextlevel, crud and level and they are enough to retrieve all add/update/delete actions inside the course and even split them between "teacher" actions and "student" actions (values for property level can be teaching/participating/other).

Also it was decided among integrators that blocker issue MDL-42962 will only go to master and therefore course_module_viewed will only exist in 2.7

Meanwhile I have a peer review comment for Mark: There should be phpdocs block about the class right above the line with class declaration (missing for all your files). Also there should be a phpdocs for the file somewhere on top of the file followed by empty line. The way you write the phpdocs blocks is that class does not have anything at all and file-level phpdocs block looks like it applies to namespace.

Also class methods that overwrite parent class methods are allowed to not have any phpdocs block at all. All IDEs and systemX will pick up the phpdocs for parent method. But this is up to you whether to add methods' phpdocs or not.

Do behat tests cover each affected file and writing to log/event triggering?

Marina, regarding the PHPDoc above each class, I did not do this because of http://docs.moodle.org/dev/Coding_style#PHPDoc where it states "The exception are files containing only one class and nothing else. In that case the class is covered by the file docblock and adding an explicit class docblock is optional." Regarding Behat, no, they do not cover all areas. However, the manual tests do. Not all event triggers that have been integrated in the pass have been covered by behat tests.

hm, all right. I don't know who wrote this. afaik doxygen and phpdocumentor require two phpdocs blocks. Oh well...\

We don't have a strict behat requirement now but I'm afraid there will be one after tomorrow's integration meeting (it's on agenda). The point of unittests and behattests is not to test the current issue but to make sure that in the future when somebody else makes seemingly unrelated change it would not break your code (or pages that use your code).

In any case I put myself as a peer reviewer. There is nothing wrong with the code, I can't stop you from submitting for integration.

Absence of any automated tests is my only concern.

Hmm, automated tests would definitely add some extra development time to this. I personally think if we were to write any behat tests it should be in a separate issue that explores the quiz functionality, not in this. However, if there is a general consensus that this requires automated tests then I will develop them, it will just take more time.

This is clearly not ready for integration. Please reopen.

Here are the things I noticed on a first pass through the code: (A more careful reading may reveal more.)

1. I just had to fix an error in the Testing instructions.
2. There seem to be some spurious extra commits in the git branch. What are they about?
3. Do we really want this integrated as 14 separate commits?
4. Are we really happy that a trivial change like this gives diff stats like "30 changed files with 1,268 additions and 34 deletions." I guess that is a fundamental flaw with the design of the events API, and not a specific problem with this patch.
5. Remind me why we have hard-coded English language strings in the get_description method.
6. Where does the attempt_previewed class name come from? I remember a long and boring argument with someone about what the event names should be, and now it seem a whole lot of other event names have been created. What is going on here? (E.g. attempt_started event already exists.)
7. question_category_created is nothign to do with the quiz. Why is it in mod_quiz.
8. With manually graded, there is a debate to be had about whether this is a quiz or question event. I think it is probably a question event. Anyway, that should be discussed before this is integrated.
9. questions_updated is not a good event name. The questions have not been updated. Questions have been added or removed from the quiz.
10. In report_viewed, where is $this->other['reportname'] set?
11. Surely the list of 'other' data that each event class requires should be documented somewhere in the PHP docs for that class.
12. A question number is not a slot. This line of code is meaningless 'questionnumber' => $slot
13. Quiz reports are a type of sub-plugin. Surely they should define their own events. The events should not be in the quiz code.
14. There was a big argument before, where people told me repeatedly that all event names shoudl be two words: subject_verb. Now suddenly there are many longer event names. What is going on?

Sending back to development upon Tim's review. Thanks

Thanks Tim, for your review.

1. It wasn't a huge error and I don't see why it was necessary to mention in your review, but thanks.
2. That is because we are introducing a new parent class to shorten the need for unnecessary code in the course_module_viewed event, so I rebased on top of that.
3. I don't care. Whatever you think. It is for peer reviewing, it makes it easy for someone to view them individually.
4. Thanks, you have mentioned this numerous times and it is not related to this patch.
5. It is simply a description of the event that is not displayed any where currently, it may be translated in the future. If we were to translate now it would mean that the translators would have to keep updating if we made changes, and since the events system is in it's infancy that could be a very real possibility.
6. That argument was with Fred, and it was agreed that the event name should end in a verb. There was no limit with two words.
7. I was simply replacing the add_to_log calls in quiz. I am not extremely knowledgeable between the differences between question and quiz tbh, so I can move this.
8. Ok, I will bring this up in scrum.
9. I am sorry, I did not look at the existing code to see what it did exactly. If questions_updated is a bad event name, why was the add_to_log call using "editquestions"?
10. Where the event is fired.
11. Sure, we haven't been doing this, but it makes sense. However, I doubt many users will look at the doc and instead will just copy the existing trigger.
12. Again, I am not familiar with the quiz code. However, "$slot = required_param('slot', PARAM_INT); // The question number in the attempt." lead me to believe that it was in fact the question number, given it actually says it is the question number. So, really you are saying the code comments are wrong.
13. Ok, sure, this can be moved.
14. See point 6.

Actually, for number 13 I completely disagree. The 'report_viewed' event is triggered in mod/quiz/report.php, and should be called regardless of the report being viewed. It isn't up to the report whether this event is triggered or not. There is no point creating an event for every single report that does the same thing. The same applies to 'attempt_deleted' for the report 'mod/quiz/report/attemptsreport.php', it is in fact deleting an attempt, so any report that has this functionality will just call the same event, rather than needing to create a new event with the same parameters again.

So really, with your list of points about this patch and claiming it wasn't worthy for integration, there are maybe two valid (non-major) points. The majority are just complaints abt the events API or are not valid.

Tim -

The things I believe need to be discussed in order for me to further work on this.

1. Isn't previewing a quiz different from starting it? I don't see why you are saying that there is already an 'attempt_started' event. I will need you to explain this to me before I make any changes. Are you suggesting I just use this event?
2. Move category_created from mod_quiz - Done.
3. Whether or not to move manually_graded outside of quiz.
4. Explain why 'editquestions' was used in the add_to_log call, and provide a better name for questions_updated. Also, should this be moved out from mod_quiz as it belongs to question?
5. Rename questionnumber to slot? Even though the php comments say that this is the question number ..
6. Maybe add the 'other' variables that are needed to the event to the docs.

Well, the fundamental issue here is that the Events API is pretty bad. For example https://github.com/markn86/moodle/commit/9e1f2a78cd3b25416f87460d199e83f9956a06aa#diff-68b0d1eefe47af1ab1ce7e0b874d703bL289.

• Code that used to take 2 lines now takes 11 lines. The old code was pretty cryptic too, but it could be understood just by reading the PHPdoc of add_to_log.
• To understand the new code, ... well there are no dev docs about how to do this yet, just the original spec. As you say, there is no PHPdoc giving the meaning of the 'other' array, ....
• There is the weird, technical $event->add_record_snapshot('quiz_attempts', $attempt); incantation.
• Event though, in almost every case, we have the same three lines of code repeated (create, add_record, trigger) there is no attempt to re-factor this common pattern into a function. We just have duplicated code everywhere.
• The API contains methods like get_description which serve no known purpose, but developers have to spend time creating them.

We are at the start of the 2.7 cycle, so there is plenty of opportunity to improve this API before it is too late, but sadly I see no desire from HQ to do that.

That is all relevant to this bug, because if the API is going to be improved, then it should be improved before this issue is finished.

The other problem is that you propose to add 1000+ lines of code to the quiz (that presumably I will end up maintaining), yet you write things like "I am not extremely knowledgeable between the differences between question and quiz tbh" and "I did not look at the existing code to see what it did exactly." It is good that you are honest about this, but it hardly inspires confidence.

I think it is normally assumed that before you try to change some code, you study it until you understand it. You can, of course, speed up the process by seeking out another developer who understands the code, and asking them to explain it, or discuss your proposed changes.

I suggest that you try to catch me online in HQ chat some time, then we can have a synchronous discussion about what to do here.

I didn't think I had to look at the whole code to see what was being logged, as the logging messages are supposed to be readable by administrators who do not even look at the code!

I have added PHPUnit tests for all the events excluding the view events.

I still see no convincing argument what benefits +1600 lines of code in mod/quiz brings.

And I still see it as a mistake that we have duplicate events for some things, like staring or submitting and attempt. Why do we need new events for these when we already have https://github.com/markn86/moodle/blob/master/mod/quiz/classes/event/attempt_submitted.php ?

When is there going to be some developer docs for this events stuff, so anyone outside HQ can understand what is going on?

Thanks Tim, that is what a peer review is for, for you to bring up questions like these. I am simply confused as to why you were passing different variables to add_to_log if they were equivalent actions? Also, regarding your criticism of the lack of 'other' information, this is being dealt with by MDL-43238.

Well, I still have no explanation of what you folks think you are doing, but I thought it went something like this:

1. We are going a build an event system, that fires for every significant action in Moodle.

2. Since every significant action in Moodle now fires an event, we can just log events, rather that having a separate log API.

Is that reasonable (obviously there is a lot more detail than that.)

OK, so for every event we want to log certain details from the event. However, that does not mean we need a separate event class for every old call to add_to_log. It just means that every event has to be logged appropriately.

It certainly does not mean we need to fire two events, one to write to the log, and one to be a real event, every time something happens. That would be terrible for performance.

In MDL-whatever it was, Fred added https://github.com/moodle/moodle/blob/master/mod/quiz/classes/event/attempt_started.php. Now you are adding another (actually two other) event started event classes.

Surely that is wrong? If it is not wrong, please explain why convincingly. Ideally give the explanation in the form of developer docs, which any add-on module developer could use to understand what they are supposed to do in their module.

(And no, MDL-43238, will not provide that sort of docs.)

Hi Tim, thanks for pointing this out. I agree 100% that we should not be adding unnecessary events if another one is applicable. These are the sort of comments I want when peer reviewing my code.

So, for the 'attempt' add_to_log call I have just replaced this with the attempt_started event, rather than creating an attempt_created event. Now, what is the other add_to_log call you feel should be using this event as well as you did say there are two events I am creating which are unnecessary. Also, if we are going to replace multiple add_to_log calls with this event then we will need a flag in the get_legacy_logdata function to determine which one to return. Do you oppose this, or do you have another solution?

Regarding the attempt_submitted event, do you think this should replace the 'close attempt' add_to_log call, rather than having attempt_closed?

Thanks!

Hey Tim Hunt, waiting for your response before I continue further with this. This is in no means a message rushing you to answer, as I suspect you may be enjoying the festive season. I hope you are/will enjoy your holidays.

Yes. I am on holiday now until 6th Jan. I sill think it would be good to try to hook up on Jabber or Google hang-out so we can discuss everything synchronously. However, that involved finding a time that is good for Whistler and UK. I guess that means UK evening which is OK.

Carrying over to next sprint.

Hey Tim, are we able to have a chat about this on Jabber this week? Is there a time that suits you? Keep in mind that you are +8 hours ahead of Whistler, Canada so it would probably have to be sometime late for me (say around midnight, so 8am your time) or early in the morning (say around 8am, so 4pm your time). Please let me know.

I don't really believe in 8:00am as a time. I normally work 10:00am to 6:30pm, so 8-10am your time = 4-6pm my time probably works best - unless that cuts into prime snow-boarding time.

Hehe, I agree with you there Tim. I worked those hours as well when I was back in Perth, even coming in at 11am. I can do 8 in the morning here, still plenty of time to get up the mountain. Which day suits you?

I am normally just at my desk working at 4:00pm, so just try it and see if I am online. Any of the remaining days this week should work. E.g. today.

Hey Tim, sorry for the delay in response. I have been working on other issues. I will be coming online at 4pm your time tomorrow (Friday 24th of Jan). Please let me know if this is (or not) going to be convenient. Thanks!

After discussing this issue on Skype with Tim we came up with these points to address -

1. Move the mod/quiz/classes/event/question_manually_graded.php to the question API (meaning we should also move the class to lib/event/classes/).
2. Remove ‘close attempt’ add_to_log call.
3. Rename ‘attempt_continued’ to ‘attempt_viewed’.
4. Move ‘override_updated’ inside ‘if (!empty($override->id)) {‘ and create an ‘override_created’ event to call in the else.

Some notes for myself.

1. Remove record_snapshots that aren’t directly taken from DB.
2. Validate other values and include PHPDocs.

Thanks Tim for taking time out of your day to discuss this issue.

Hey Tim, have addressed a fair few issues, including the ones that we discussed. If you could review again when you get time that would be much appreciated.

Results for MDL-40063

• Remote repository: https://github.com/markn86/moodle.git

• Remote branch MDL-40063_master to be integrated into upstream master
  • Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/899
  • Error: The MDL-40063_master branch at https://github.com/markn86/moodle.git exceeds the maximum number of commits ( 16 > 15)

Hey Tim, I don't mean to rush you into re-reviewing this, but do you think there would be any chance you would be able to do so this week? Thanks.

I've tried to start a few times, but, but I get as far as the diff stats, and just get too depressed.

Also, I don't see much point in me looking for things that CiBoT can find. You have not done anything about the error above "exceeds the maximum number of commits ( 16 > 15)". If you squash at least two of the commits, then CiBoT can do her thing, and you can fix all those issues, then my job will be easier.

I can deal with what CiBoT complains about later, what I require from you is an overview that the replacements of the add_to_logs was done correctly. However, just to get the CiBoT complaints out of the way I will merge the first two commits.

Results for MDL-40063

• Remote repository: https://github.com/markn86/moodle.git

• Remote branch MDL-40063_master to be integrated into upstream master
  • Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1134
  • Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1134/artifact/work/smurf.html

Mark, I will review after you have got the CiBoT issues fixed. The other thing I need to be able to understand this well enough to review it, are the developer documentation that describes what this API is for, and how Module developers should use it. I have been asking for that for more than two months now. Also, did you see the other day Eric Merrill was asking for the same thing?

Hi Tim,

99% of CiBoT's comments fall into the below two scenarios. Everything else I have addressed.

1. "Expected 1 space between asterisk and tag; 6 found" - This is because of the new doc tag for events to specify what is required in the other array. I was going to edit http://docs.moodle.org/dev/Coding_style to include this but for some reason I do not have permission.
2. "Class attempt_viewed is not documented" - There is no need to include a PHPDoc block for the class if it's the only class in the file (see http://docs.moodle.org/dev/Coding_style#Classes_3).
3. The events API can be found at http://docs.moodle.org/dev/Event_2 - which I know you have already seen. I am not aware of any other documentation. I will ask on Monday (well Sunday my time) whether we should add additional information for third party developers.

Thanks.

Hi Tim, there is currently an issue (MDL-43966) to create documentation that explains how to convert existing add_to_log calls. However, it has not been attempted yet. I do not think this is necessary for you to review this patch as you already have an understanding on how the API works.

I don't know what makes you think I understand this API. I don't. Linking MDL-43966 as a blocker.

Hi Tim, please see http://docs.moodle.org/dev/Migrating_logging_calls_in_plugins.

Tim Hunt: It would be great if you could come back to this. It's one of the few remaining areas we're trying to progress. Hopefully the doc linked by Mark will help. It will also be a test of that doc also, so your feedback is welcome there.

Michael de Raadt yes, I know, it is on the list, but I have only been back from Leipzig 2 days, and have had many other things to catch up on, and reviewing 1,600 lines is not a trivial undertaking.

Hi, Tim.

I saw your tweets from Germany. It looks like you had fun. Thanks for peer reviewing this.

Sorry, Michael de Raadt, is that sarcasm? If so, thank you for your helpful contribution.

Hi, Tim.

No sarcasm intended.

Commit 1: (view.php and index.php) OK.

Commit 2: (edit.php):

quiz_updated event. This name seems to generic to me. It used to be editquestions. However, what is really going on at this point is that the user is viewing the Edit quiz page. That is all we know. They have not necessarily changed anything. Can we change to a better event name. Does quiz_edit_page_viewed fit the rules? Also, this means that the CRUD info, and other metadata in the event, need to be updated.

and now github is being DDOSed. Yay! More later, I hope.

Please, please, please. As you address these issues, do not squash the new changes into the old changes until we have finished peer review. (It probalby woudl be helpful to do an interactive rebase to re-order things so the fixup comments come immediately after the original ones they fix, but please leave them separate until the end.)

Commit 3: (question_category_created) One problem, one thing that could be better.

The problem is that the contextid of a question cateogry is not necessarily a module-levle context. It could by a course, course category, or system context. (Although not in the one place this event is currently raised.)

The thing that could be improved is that /mod/quiz/edit.php is not the best place to see the result of this action. The best place is question/category.php (The link Question bank -> Edit categories in the admin block for either a Course or Quiz, as applicable.)

Commit 4: (attempt_deleted)

What is this extra DB query for? https://github.com/markn86/moodle/commit/c0cd8e914cd8a1716a6950e00d85ca075f1c72e2#diff-68b0d1eefe47af1ab1ce7e0b874d703bR352 If $attempt is an object, it already has the data. I though the point of ->add_record_snapshot was to improve performance, not to make it worse.

Commit 5: (report_viewed)

Another pointless DB query: https://github.com/markn86/moodle/commit/404943d46cc20a0d679e504ab723a3e3a8627724#diff-c67d1c6101f8fad0bb9e92214e7df576R86
There is no interesting informatoin in the quiz_reports table. It is about as exciting as the modules or blocks table. Do you use the record from the modules table when raising forum events? I assume not.

So, similarly $this->objectid in get_description is completely meaningless.

I am going to stop the detailed review there. You can see the sort of things I am finding. Please get back to me when there is a branch that is closer to being ready for integration. (E.g. look through it all again yourself, then perhaps get another Moodle HQ dev to look through it too, for a second pair of eyes.)

I do acutally appreciate your work on this, though my attitude migh sometimes (normally) give the opposite impression, but we can't submit this for integration until it is good enough, and I am afraid it if not there yet.

Hey Tim,

Thanks.

1. I have addressed the question_updated event (see - https://github.com/markn86/moodle/commit/78ff5ccacda2a5c0fa5bb41b1fefc6b073dd54e5)
2. I am not too sure what you mean when you refer to the question_category_created event. I do not currently trigger this in mod/quiz/edit.php as you have stated. I call this in the function add_category, which has a context available already. Am I missing something?
3. I perform the extra query because we are deleting the attempt, so we should pass a copy of it in the event. We do not use any variables in the code unless we are certain they are an exact copy of what is stored in the DB. The DB table may have defaults for certain columns which aren't specified in the object.
4. The query is performed because I need to obtain the objectid as there is an objecttable associated with the event, so an objectid is required. I have kept the query, but have altered the description (https://github.com/markn86/moodle/commit/a5b1dc40f7ba9b63450c5ec9c9337dfd14516dd9).

None of your points came up with huge flaws in the overall design. Most of my above comments are simply explanations to your points, no change was needed, so I disagree with your statement that this is not ready for review. I really don't see why you had to stop the review there.

Results for MDL-40063

• Remote repository: https://github.com/markn86/moodle.git

• Remote branch MDL-40063_master to be integrated into upstream master
  • Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2396
  • Error: The MDL-40063_master branch at https://github.com/markn86/moodle.git exceeds the maximum number of commits ( 17 > 15)

To pick just one point, please explain why setting object table to quiz_reports for that event is a good thing to do?

Why do you think it is a bad idea? We are viewing a report, the report is the object (objecttable), therefore we need an objectid. I don't understand why you are so opposed to that idea.

Because it is not useful information:

      <FIELDS>
        <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
        <FIELD NAME="name" TYPE="char" LENGTH="255" NOTNULL="false" SEQUENCE="false" COMMENT="name of the report, same as the directory name"/>
        <FIELD NAME="displayorder" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false" COMMENT="display order for report tabs"/>
        <FIELD NAME="capability" TYPE="char" LENGTH="255" NOTNULL="false" SEQUENCE="false" COMMENT="Capability required to see this report. May be blank which means use the default of mod/quiz:viewreport. This is used when
      </FIELDS>

If it was providing especially useful information to the observers then perhaps it would make sense, but this is just going to lower performance and provide nothing of value.

I understand what Tim is saying here and I think he is right.

Ok sure, I am not against removing it, I just don't see the harm. The performance issue is negligible. I think it could provide useful information as it identifies what report was viewed (I know the name is passed in 'other' but this is more obvious and allows for easy usage in SQL queries). Also, who is to say that the table won't expand in the future to include more information?

What this report is doing is showing all the attempts and a given quiz. The useful record to include is the mdl_quiz row - assuming that you can get it with a DB query, and it must already be available - just like we use the mdl_quiz row as the record for people looking at the view.php page (I assume).

Is this not obvious?

Just updating workflow state.

This is a trivial change and something I will discuss later. I am putting back for peer review so that someone else can review before we send back to Tim.

Results for MDL-40063

• Remote repository: https://github.com/markn86/moodle.git

• Remote branch MDL-40063_master to be integrated into upstream master
  • Executed job http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2494
  • Error: The MDL-40063_master branch at https://github.com/markn86/moodle.git exceeds the maximum number of commits ( 17 > 15)

Hi Mark,
Great work with events here. I am reviewing this from fresh perspective. I did notice some of the things I mentioned below were already discussed. Please feel free to ignore those:-
General

1. Integrators don't like separate commits for each event, you might want to group them together.
2. I don't really like how files are created and deleted in the history for no reason, (mod/quiz/classes/event/quiz_updated.php → mod/quiz/classes/event/quiz_edit_page_viewed)
3. Can you please update testing instructions so that the urls in the log report with standard store is verified? This will help us find any errors in the get_url(), that might be missed.
4. I personally prefer to name the variable $eventparams or $eventargs, instead of just $params.
5. Most of your unit tests validate really basic things. You might want to add more asserts, specially for url, objectid, relateduserid, snapshots etc
6. Are you aware of changes introduced by MDL-40907, that lets you log multiple legacy logs for the same event?
7. usage of s() is recommended when using text fields in get_description, for proper clean up.

course_module_viewed

1. 'level' is deprecated pls use 'edulevel'

question_category_created

1. The commit message "mod_quiz: replaced 'addcategory' add_to_log call with an event " doesn't seem correct. Shouldn't the component either be events or questions?
2. The string should be named 'eventquestioncategorycreated' and not 'eventcategorycreated' as per our conventions.
3. Seems like you have enough info here to add a snapshot, or is something missing?
4. shouldn't we pass an instance of question_category_object for observers to use? (ex : -MDL-44403)

attempt_deleted.php

1. Can you explain why you need to do this:-
   https://github.com/markn86/moodle/commit/15869ed0de86b7e3298bae9a44c7cc9e061856dc#diff-68b0d1eefe47af1ab1ce7e0b874d703bR352
   Can't we trust that the passed attempt exists? Normally we don't cross check the data in every single api, specially when it costs one extra db query.
2. Can you please add some asserts for objectid, and relateduserid in the tests?
3. You can link to quiz/report.php as the url for this event.

report_viewed

1. Do you want !isset or empty checks in validate_data() ?
2. You are triggering the event before actual action, which is wrong.

attempt_reviewed

1. There is no existing verb reviewed. Not saying that the event name is incorrect, but you need to justify why you need the new verb and that you have considered all existing ones before using a new one.
2. I haven't tested this, but can't a student review their own attempt? If they can, this shouldn't be LEVEL_TEACHING
3. Do you want !isset or empty checks in validate_data() ?
4. The event is triggered on every page of the attempt, that seems wrong to me given the name is "attempt reviewed" not something for a specific page.
5. I will pass the quiz_attempt object to the observers by setters and getters.
6. Again you are triggering the event before the actual action.

attempt_summary_viewed

1. Do you want !isset or empty checks in validate_data() ?
2. Setting courseid is not required, it is auto populated from context.
3. I will pass the quiz_attempt object to the observers by implementing setters and getters.

override_updated/deleted/created

1. Do you want !isset or empty checks in validate_data() ?
2. why is the created event linking to quiz/overrideedit.php instead of quiz/overrides.php
3. group_override_x might make more appropriate name for the events
4. why there is no call to override_deleted in overrideedit.php, even when it is doing a delete ?

attempt_viewed

1. Do you want !isset or empty checks in validate_data() ?
2. The get_url() seems incorrect, it should link to attempt.php not review.php
3. Setting courseid is not required, it is auto populated from context.

attempt_previewed

1. The place to trigger this is in quiz_attempt_save_started(). This seems conceptually wrong. We should trigger the event when the preview is finished, not when starting it.

attempt started

1. It seems strange to have a started event without an ended event.

quiz_edit_page_viewed

1. why is quiz in the event name?
2. Do you want !isset or empty checks in validate_data() ?
3. This seems a pretty useless event to me, what value does it have (legacy reasons is not enough, imo) ?

Cheers

Thanks Ankit for that huge review. To clarify some points:

question_category_created - this is still logging the wrong context in some cases, as pointed out above.

attempt_reviewed 2. - Yes, students can review their own attempts, which is learning, I guess. And teachers can review their students attempts, which is teaching. Don't now how the API copes with that

For read-only actions, does it really matter if we log the action before we render the page? If so, why? I can see that for writes, it is better to put the log call after all the other DB updates.

override_updated/deleted/created 2. overrides.php lists all the overrides that exist. This is not very helpful when the log action relates to one particular override. overrideedit.php just shows that one override. Admittedly in a form that is editable, but why is that a problem? Of course, you should not link to overrideedit.php for the deleted event, because that will just give an error.

attempt_viewed 2. No. linking to review.php, where the teacher can see the attempt in progress, is correct. This was correctly converted from the old code.

attempt_previewed 1.: No. The action we want to log is the teacher starting a preview. So, the location of the log call is correct. You may want to re-think the name. Note that this event is exactly parallel with attempt started.

attempt started 1.: There is an attempt finished event. Well, possibly called submitted. It was created when the events API was implemented, and probably has not been changed here. (But, does legacy log data need to be added there?)

quiz_edit_page_viewed 1. We should, of course, be logging every change a teacher makes to the quiz (adding/removing questions, etc.) One might have hoped that all the work on events dealt with the missing log calls, but sadly that was not done. I agree that this log call is not very good. However, some evidence of when the teacher might have edited the quiz is better than nothing.

report_viewed: Ankit seemed to have missed the problem here, which was explained at length above.

I wonder what else has still been missed?

Thanks Tim, for looking at this again.
A few comments:-

question_category_created - this is still logging the wrong context in some cases, as pointed out above.

Seems correct to me, may be I am missing something. The context for the event is set using contextid, we have the correct context object set in \core\event\base::create() using context::instance_by_id() irrespective of the context level, how can that be wrong?

For read-only actions, does it really matter if we log the action before we render the page? If so, why? I can see that for writes, it is better to put the log call after all the other DB updates.

From an user point of view, it might not make much difference. However event by definition should be triggered only after the actual action. Would be nice if we follow that everywhere. As a completion observer monitoring view events, would you really like to be notified before the user has actually viewed the activity?

override_updated/deleted/created 2. overrides.php lists all the overrides that exist. This is not very helpful when the log action relates to one particular override. overrideedit.php just shows that one override. Admittedly in a form that is editable, but why is that a problem? Of course, you should not link to overrideedit.php for the deleted event, because that will just give an error.

Neither is wrong, it is just which one is more appropriate. If you feel the edit page is more appropriate for a create action, it is fine by me.

attempt_previewed 1.: No. The action we want to log is the teacher starting a preview. So, the location of the log call is correct. You may want to re-think the name. Note that this event is exactly parallel with attempt started.

In that case the event should be renamed to attempt_preview_started, with a counter event attempt_preview_ended or something on those lines.

attempt started 1.: There is an attempt finished event. Well, possibly called submitted. It was created when the events API was implemented, and probably has not been changed here. (But, does legacy log data need to be added there?)

I guess you are referring to attempt_submitted. I did see that event, however we really need to consider renaming these events, so either we have a start/end pair or use a different verb. Again, this is just my opinion.
We are flexible about legacy log data. If you feel it doesn't fit with the current event, it can be removed.

report_viewed: Ankit seemed to have missed the problem here, which was explained at length above.

We did use different tables as object tables in some events, when the actual object tables was just a relationship table. Honestly, I don't have any strong opinions on which way to go in this case, as long as proper snapshots are added for observers.

Cheers

question_category_created:

Sorry, I over-summaraised the problem, but there is a problem here. The event object assumes that it will only ever be instantiated with a context_module, but question categories can belong to any sort of context. Look at legacy_log_data and get_url for examples of the problem.

when to log views:

Well, as a teacher, you would like to know that the student acutally read the page carefully, and remembered the key points. But you can't do that. What we are acutally logging is that Moodle responded to a GET request. You don't even know that the student's browser loaded the response. Their network connection may have dropped between issuing the request and receiving the response.

attempt_submitted:

A generic 'end' event is not very useful. The point is that there are several ways an attempt may be ended. It might be submitted, or abandoned, or becomes overdue and be submitte or abanded later. We have events that accurately report all that. To me that seems like a good thing. If you disagree, you will have to explain why.

(Drat! http://docs.moodle.org/dev/Quiz_state_diagrams is out-of-date. It needs to have the changes from http://docs.moodle.org/dev/Better_handling_of_overdue_quiz_attempts#New_state_diagram merged into it.)

question_category_created:
Right, I see the problem now. get_url() and get_legacy_logdata() assumes the event context is module level, which might not be correct.

when to log views:
If we wanted to log only GET requests, we have apache logs for that, we don't need events. The idea here is to log interesting actions with as much details as possible. When we are triggering events, there are two aspect involved, event and log. From log point of view, possibly, it doesn't make much difference if we log it before or after the actual view. But for events it is quite crucial. Consider an example from the code above, https://github.com/markn86/moodle/blob/f22a755271965d2910e06a89061eb1ef07d0be6a/mod/quiz/attempt.php#L98

If you argue the call can be anywhere in the page, would it make sense to do it before require_login() ? ofcourse not, since nothing worth logging happens if the user is not logged in, he is simply redirected. The same stands true for other checks like
https://github.com/markn86/moodle/blob/f22a755271965d2910e06a89061eb1ef07d0be6a/mod/quiz/attempt.php#L115 along with DB updates like https://github.com/markn86/moodle/blob/f22a755271965d2910e06a89061eb1ef07d0be6a/mod/quiz/attempt.php#L125 or redirects like https://github.com/markn86/moodle/blob/f22a755271965d2910e06a89061eb1ef07d0be6a/mod/quiz/attempt.php#L123
In general it is safe and logical to trigger the event after the actual action, irrespective if it is write or read action.

Whether to log views: Do apache logs contain the Moodle username? By default they don't, which makes them effectively useless for most of the investigation you need to do (Have you ever been responsible for a large Moodle site? When a student rings the help desk and claims they submitted the quiz, but the system has no record of it, you need to be able to work out what happened.) The rule should be that every page-view is logged. And, in fact that rule is in the coding guidelines: http://docs.moodle.org/dev/Security#Log_every_request. If you wish to change that guidance, you need to start a forum thread.

(Actually, because of the OU's single-sign-on system, we do have usernames in the Apache log file, which is wonderful.)

Yes, logging after the action is better. However, for view actions, I cannot get too excited about the whether the log call comes before $OUTPUT->header or after $OUTPUT->footer. All the rendering should be simple HTML output code that is unlikely to throw an exception. I don't think it is clear-cut. A typical Moodle script looks like

// All the input parsing and DB access. (What would be the C in MVC)
// All the output code. (The V in MVC)

You are suggesting we stick an extra bit of C code after the V, just because. I am suggesting that it is just as good to put it right at the end of the main block of C code. For an interesting case, consider displaying a very long forum thread.

I would hardly classify student actions as "not interesting". An example of GET request which is not interesting is, when a report does ajax requests, say every 5 seconds to see if any new information is present. And if a dev docs suggests to trigger events for each of these repeated ajax requests, imo, that doc needs to be updated. Anyway this is not related to the current issue in context and can be discussed later.

What you are proposing is for an ideal Moodle world, where renderers are pure HTML dumping methods (V), which is not true in reality. A lot of renderers are doing DB queries and other things which they should not be doing. If you can give 100% assurance that no control logic is present between the event trigger and the whole display code (which is not true for the current issue btw), you might justify triggering the event before the actual view action. You will also need to convince enough people so http://docs.moodle.org/dev/Event_2#What_are_events.3F can be updated to say events can also describe information that is going to happen in case of view actions.

Anyway I will leave it up to you and Mark to decide which is the best direction for this issue.
Cheers

Hey Ankit,

Thanks for the review.

General

1. Once the review has been done I will consider this. It is easier to review in separate commits.
2. This was done as requested by Tim, as he wanted to review the changes after the review. However, I am going to now squash these back as you have done your review, hopefully addressing many issues and Tim can have another look.
3. Done.
4. I really don’t think that matters.
5. The unit tests ensure that the event is fired and the proper data is being returned. I don’t think it’s necessary to check that the snapshots etc are working, as the legacy log data wouldn’t return the correct data if it didn’t.
6. Where should this be applied to exactly?
7. The only place I use a text field in the description is report_viewed, which I have added the usage of s() to, but can’t find other locations it should be used.

course_module_viewed.

1. Done.

question_category_created

1. I disagree. We are replacing the add_to_log call in the quiz module.
2. Done.
3. There is no data that has been retrieved directly from the DB. We do not add snapshots for data that has been inserted due to the fact the DB table could have extra fields with defaults.
4. Is this related to the above statement?

attempt_deleted

1. Because we are deleting an attempt, so we should store the row as a snapshot. The attempt being passed to the function may not contain the exact data in the DB.
2. I don’t feel this is necessary. There is no need to bloat the unit test. I have gone and added some validation to the events that use the relateduserid field but do not validate that it was passed.
3. Done.

report_viewed

1. What is wrong with “!isset”?
2. How is what I have done anything different than what we do for the ‘view’ and ‘view all’ events for activities?

attempt_reviewed.

1. I think this verb is appropriate for the action and I really don’t see any other appropriate verb in the list. The closest I saw was ‘evaluated’ - which I don’t think is a good match.
2. As Tim pointed out, a student can review their own attempt and a teacher can review someone elses. So, I am not too sure what to put here.
3. !isset is fine.
4. It is triggered in the review.php file which states - “This page prints a review of a particular quiz attempt”. I don’t see an issue here.
5. Do you mean adding a record snapshot, or something else?
6. Again - how is that different than the other pages we do this on?

attempt_summary_viewed

1. !isset is fine.
2. Sure, it’s not required, but if I set it saves the base class having to do extra functionality.
3. Again, as a snapshot or?

override_updated/deleted/created

1. !isset is fine.
2. Tim has addressed this.
3. I don’t agree. Why is the group necessary?
4. I was only replacing the add_to_log call, so missed that case. It makes sense to trigger the event there as well - done.

attempt_viewed

1. !isset is fine.
2. As Tim has pointed out - this is intentional.
3. Again, it doesn’t matter and saves the base class doing extra work.

attempt_previewed

1. As Tim has pointed out - this is intentional.

attempt_started

1. As Tim has pointed out - there are many ways an attempt can end, which is why there is no ‘ended’ event.

quiz_edit_page_viewed

1. Quiz is in the event name because it is possible that there would be multiple ‘edit_page_viewed’ for a quiz. You may not just be editing the quiz, but possibly something else. I simply left it there as it is what Tim suggested, and I considered that there might be other pages that involve editing an object. However, after more thought I agree that we can remove the ‘quiz’ term. If there are other pages then we can name them as appropriate.
2. !isset is fine.
3. I did also consider this, but I left it so that the legacy data was kept AND because Tim has a better understanding of the value of this event than I do.

Ok, quick update on the things I have changed -

1. Changed the get_url function in question_category_created so that it checks the context and returns a URL to '/question/category.php' passing either the context module id or course id.
2. Moved report_viewed, attempt_viewed and attempt_summary_viewed to the bottom of the page.
3. Renamed attempt_previewed.php to attempt_preview_started.php.
4. Removed the get_record call to the report table and removed the 'objectid' and 'objecttable' from the report_viewed event.

I am considering sending this to integration if there are no immediate complaints as code freeze is approaching.

Cheers.

Please don't send this for integration until it has been reviewed by me.

report_viewed shoul by linked to the quiz being reported on, as has already been explained at great length above.

Sorry, it is now after 9:00pm, and I am only just about to leave work. This never quite made it to the top of my todo list today. It would be great of someone from HQ could take another look before I review it again.

Tim, if you want any further comments you probably have to get them in today so this can go to integration. If you don't get to it, I will assume from the significant amount of interaction above that any remaining problems are minor anyway and could be fixed as bugs later.

Mark, apparently there is an issue about report_viewed mentioned a few times above that is not fixed yet? eg https://tracker.moodle.org/browse/MDL-40063?focusedCommentId=279458&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-279458

Latest round of code-review issues:

1. report_viewed event should use the quiz settings (mdl_quiz table) row as the object id/row/table.

2. It seems that descriptions like "User with id '4' ..." are inconsitent all over the place. Might be worth getting that right before this is integrated. See https://moodle.org/local/chatlogs/index.php?conversationid=15153 for details.

3. We also need consistency in how things are logged. For any event where the objecttable is not mdl_quiz, we shough have quizid in the other data. We should also always meantion the quiz id in the get_description output.

4. I am pretty sure https://github.com/markn86/moodle/compare/master...MDL-40063_master#diff-19 is wrong. You fixed get_url to work even if it was not module context, but this method still seems to be assuming module context.

5. What is the point of a summary like 'A question with the id of ' . $this->objectid . ' was manually graded by a user with the id ' . $this->userid . ' for the attempt with the id ' . $this->other['attemptid']? Those numeric ids are going to be completely meaningless to a user. Before, everyone was saying tha these descriptions were not used anywhere, but apparently they are used in the log report now. So, they are shown to users, but not translatable.

6. So, someone needs to update http://docs.moodle.org/dev/Migrating_logging_calls_in_plugins#get_description.28.29_and_get_url.28.29

7. Why are the question events under core\event, not under core_question\event. core_question is a recognised subsystem.

8. question_manually_graded - calling the 'other' param 'attemptid' is highly confusing. There are other things called attempt floating around in this context. Please be consistent with the existing code and call it questionattemptid. You should probably clarify the wording of get_description too.

9. Shouldn't question_manually_graded override get_url and get_legacy_log_data?

10. If view actions are meant to only be logged after the page has been output, https://github.com/markn86/moodle/compare/master...MDL-40063_master#diff-c67d1c6101f8fad0bb9e92214e7df576R98 needs to be after $OUTPUT->footer.

11. As said before, the extra get_record at https://github.com/markn86/moodle/compare/master...MDL-40063_master#diff-68b0d1eefe47af1ab1ce7e0b874d703bR369 is not acceptable. It is not necessary. Get rid of it. If you are worried about not all fields being present, valdiate that in the event validation.

12.Once all events do other['quizid'] consistently, you can stop relying on get_record_snapshot in https://github.com/markn86/moodle/compare/master...MDL-40063_master#diff-885b1f24b878a03f551c52ec6f3e4409R104.

13. https://github.com/markn86/moodle/compare/master...MDL-40063_master#diff-68b0d1eefe47af1ab1ce7e0b874d703bR294 is two large almost identical chunks of code. The only difference there should be the event name, so please refactor.

14. summary.php is not the right URL for attempt_summary_viewed. https://github.com/markn86/moodle/compare/master...MDL-40063_master#diff-b717fb74634aa874d1d910cfeafbed61R72 If you look at the top of summary.php, you will see that if a teacher tries to view the summary of someone else's attempt, they are redirected to review.php. So, the URL for this should be review.php, as it is for start_attempt and attempt_viewed. Ditto for get_legacy_logdata.

15. attempt_viewed is not teaching. This happens as a student is attempting the quiz. It is learning. (Is viewed the right verb?) Also get_description could be better here, and the lang string $string['eventattemptviewed'].

16. Why does class course_module_viewed extends \core\event\course_module_viewed { need to specify all three bits of data? Surely the only thing that varies for different modules is the $this->data['objecttable'] = 'quiz'; line? Also, if it is not necessary to override anything for course_module_instance_list_viewed, why is it necessary for course_module_viewed?

17. edit_page_viewed should use objecttable = 'quiz', like all the other quiz events that aren't related to one attempt.

18. I know the old add_to_log call used view.php for the URL, but I think edit.php would be a better URL for edit_page_viewed

19. There are two sorts of quiz override. Some overrides relate to a single user. Other overrides relate to a group. Your event only seems to handle the single user case. This needs to be fixed.

20. Lots of duplciated code where events relating to overrides are triggered. Please refactor: https://github.com/markn86/moodle/compare/master...MDL-40063_master#diff-9f8b427b72ce8555d2b07566a1edc292R160

21. Note that https://github.com/markn86/moodle/compare/master...MDL-40063_master#diff-21fd715d9c24d563b87601819281ddb9R513 is a bad place to raise this event. Note that the database is not necessarily updated here, and there is no guarantee these changes will ever be writted to the database. Really, the event should only be raised in question_engine_unit_of_work::save in question/engine/datalib.php. (The unit of work partter is descriped in Fowler "Patterns of Enterprise Application Architecture" if you are not familiar with it. I am sure there are other free sources online too.)

I'm sorry. This still seems to be a long way from being ready for integration.

Hi Martin,

Tim was complaining about using the 'quiz_reports' as the objecttable in the event, which was removed. He is now suggesting we use 'quiz' as the 'objecttable' which I think is incorrect. We are not viewing a quiz, we are viewing a report. This is how the events work. However, Tim seems adamant that we should be linking to the quiz table, if that is what it takes for this to get integrated I will do it.

We are viewing a report on all the attempts at quiz X. Just like view.php is displaying info about quiz X. The current quiz is highly relevant to all these events.

By the way, your attitude to this worries me: "if that is what it takes for this to get integrated" is not the way to think.

You should be thinking: "The right thing for the future of Moodle is ..."; "The most useful thing for researchers using the logs for learning analytics is ..."; etc.

The Moodle process, peer review, integration review, etc. Exists to help you achieve these goals better than you could on your own.

The reason my attitude seems that way is because I have offered my opinion numerous times but it seems to be ignored. The quiz may be relevant but we aren’t viewing a quiz. I agree that the quiz is an important aspect of the report, but the data we are viewing could be anything, not data directly from the quiz table. The report may be viewing the quiz attempts, so what stops the 'quiz_attempts' table from being the objecttable? Where do we draw the line?

Tim -

1. I disagree - as you already know. As discussed on jabber for other similar events there is no objecttable (eg. viewing a forums subscribers) and I think now we both agree that this should not be set.
2. I agree with the chat that the descriptions should be consistent. They are consistent among the quiz events I introduced, but there has been no agreement on what is the ‘correct’ way, so there is no point changing them now.
3. Ok, I have gone through and done this to existing events where appropriate.
4. Suggestions on how to determine the correct context in this function?
5. These are the descriptions used throughout the events. If you believe this is an issue it should be dealt in an issue that addresses the get_description function.
6. That is in another issue (see MDL-43171).
7. Again - this is what was decided in the events design. Which we have already discussed in other issues before.
8. As discussed the event was moved to mod/quiz - so the attemptid makes sense now.
9. This was moved to a location in the API where it is not possible to obtain the quiz id. After discussion on jabber it was decided to move this event (as stated above) to mod/quiz/classes/event and call it on the same page the add_to_log call it is replacing was on. This means we can now get a valid legacy log data now.
10. Ok, sure. I was triggering after the report was actually shown, but there is no harm if the whole page is displayed then it is triggered.
11. Ok, sure.
12. I did not introduce this event, it was there before and also exists in Moodle 2.6. I think this would require backporting in another issue - which I created MDL-44988 for.
13. Done. However, there is a slight difference, which is the ‘quizid’ param (for now until the above gets addressed).
14. Sorry, I am a bit confused about what you are saying. If the page get redirected the attempt_summary_viewed event will not get fired as it is as the bottom of the page.
15. Done.
16. I brought this up a long time ago, and I agree. I asked the same thing. The event course_module_instances_list_viewed was introduced after, so the base class ended up doing the rest.
17. As discussed in point 1 and on jabber I disagree and believe we agree that this should be left empty.
18. It is already using that URL - what do you mean?
19. As discussed on Jabber I have created 6 events now - user_override_created/updated/deleted and group_override_created/updated/deleted.
20. This has been done with the new design.
21. See point 9.

Thanks.

I find that a lot of the time I am adding more to what we originally discussed, or doing something completely different. One day it seems we should do it one way, then when that gets implemented, it is decided another way is preferred and so on. One example is moving the question_manually_graded event. When this was first discussed it was moved to one location, but now somewhere else is preferred. I am worried this will never reach a point where everyone is satisfied.

Request from the Integration Team: It seems this issue is the only big candidate issue which is likely to cause a request to break freeze. It would be great if we could avoid this and at least have a large part of this issue integrated in time for the start of the QA testing.

Tim and Mark, it would be great if some sort of agreement on a way forward could be made, because it will be much preferable to have 90% of this code ready for the start of testing, even if we have the last 10% post-freeze.

Dan, I agree that would be good. Can you do anything to help? Have you (or another member of the iTeam) looked at the code yet? Do you think it is ready?

Hi Tim - I am going to push this for integration, that way someone from the iTeam can give their opinion. Thanks.

Marina just made this comment on another one of my issues - "You do not need assignment id in 'other' - please use contextinstanceid". I am in two minds about this. Yes, it would be available via that context assuming the module context is always passed, but it isn't too obvious for observers. I would be interested in your opinion Tim.

It is a hassle to get assignmentid, or quizid, from the context. Get the cm from context->instanced, then look at $cm->instance (after, perhaps, first double-checking $cm->module to make sure it is the right type.

If quizid or assignmentid is available, and I would have thought it almost always is, then putting it in the events seems to be a good way to help people listening to the events, so worth doing. However, this should really be consistent for all events for all modules.

I agree 100%. Commenting on that issue.

Reading through the last round of issues, I think that Tim is raising many good points. However they apply to all the events not just the quiz issue and needs to be addressed in all of them.

2. Yep, there are lots of inconsistencies everywhere. It needs looking at for all events in all modules. Not blocking thie issue.
4. Needs addressing. Please create an new issue for it.
5. It needs looking at for all events in all modules. Not blocking thie issue.
16. Please create an new issue for it.

Thanks Mark & Tim, i've integrated this to master.

I noticed a few problems during testing.

• When creating a User override, no entry was added to the Legacy log (it was added to the standard log). Perhaps there was no legacy logging of this before.
• When I went to delete a user override, there was an error shown. I don't think this was related to events, but I don't know for sure.
• When I reviewed the events in the Standard log, I couldn't find a "view all" event. I was able to find events for creating and deleting user overrides.

I thought I should add that the prescribed unit tests (and all unit tests, in fact) passed.

Looking at this..

When I reviewed the events in the Standard log, I couldn't find a "view all" event. I was able to find events for creating and deleting user overrides.

This is caused by moving to a standardised event name for course module viewed event

I'm getting the override error too:

Debug info: SELECT id,course FROM {course_modules} WHERE id IS NULL
[array (
)]
Error code: invalidrecord
Stack trace:
line 1451 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown
line 1427 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
line 7074 of /lib/accesslib.php: call to moodle_database->get_record()
line 210 of /mod/quiz/lib.php: call to context_module::instance()
line 158 of /mod/quiz/overrideedit.php: call to quiz_delete_override()
Output buffer: <br /> <font size='1'><table class='xdebug-error xe-notice' dir='ltr' border='1' cellspacing='0' cellpadding='1'> <tr><th align='left' bgcolor='#f57900' colspan="5"><span style='background-color: #cc0000; color: #fce94f; font-size: x-large;'>( ! )</span> Notice: Undefined property: stdClass::$cmid in /Users/danp/moodles/im/moodle/mod/quiz/lib.php on line <i>210</i></th></tr> <tr><th align='left' bgcolor='#e9b96e' colspan='5'>Call Stack</th></tr> <tr><th align='center' bgcolor='#eeeeec'>#</th><th align='left' bgcolor='#eeeeec'>Time</th><th align='left' bgcolor='#eeeeec'>Memory</th><th align='left' bgcolor='#eeeeec'>Function</th><th align='left' bgcolor='#eeeeec'>Location</th></tr> <tr><td bgcolor='#eeeeec' align='center'>1</td><td bgcolor='#eeeeec' align='center'>0.0000</td><td bgcolor='#eeeeec' align='right'>239088</td><td bgcolor='#eeeeec'>{main}( )</td><td title='/Users/danp/moodles/im/moodle/mod/quiz/overrideedit.php' bgcolor='#eeeeec'>../overrideedit.php<b>:</b>0</td></tr> <tr><td bgcolor='#eeeeec' align='center'>2</td><td bgcolor='#eeeeec' align='center'>0.0533</td><td bgcolor='#eeeeec' align='right'>8002936</td><td bgcolor='#eeeeec'>quiz_delete_override( )</td><td title='/Users/danp/moodles/im/moodle/mod/quiz/overrideedit.php' bgcolor='#eeeeec'>../overrideedit.php<b>:</b>158</td></tr> </table></font>

also

You need to update your sql to include additional name fields in the user object.
line 3571 of /lib/moodlelib.php: call to debugging()
line 91 of /mod/quiz/overridedelete.php: call to fullname()

When creating a User override, no entry was added to the Legacy log (it was added to the standard log). Perhaps there was no legacy logging of this before.

There wasn't (as far as I can see).

I've created MDL-45057 in order to address the override deleting and for the meantime this code is commented out.

Yes yes, this is far from ideal, but better we get more testers on board and move forward with the release. Thanks for your testing Michael.

Created MDL-45071 after Michael's report on alternate name field warnings.

This change is now part of Moodle, thank you for your efforts.