Description

Confirm design of new module that achieves the following:

Remove support for assignment sub-types
Combine 4 standard assignment sub-types into one more flexible module
Clarifies the status messages for submissions
Provides more detailed logging that can be used for audits
Upgrade workflow from the old assignment module to the new one

Hi Damyon,
I have had lots of requests to change assignment default settings (eg enable send for marking, maximum number of uploaded files etc). Could you consider including all of the assignment settings in the module settings please? Here: /admin/settings.php?section=modsettingassignment

Amanda Doughty
added a comment - 01/Feb/12 6:47 PM Hi Damyon,
I have had lots of requests to change assignment default settings (eg enable send for marking, maximum number of uploaded files etc). Could you consider including all of the assignment settings in the module settings please? Here: /admin/settings.php?section=modsettingassignment
Thankyou

Will this refactor include any work on how the "send notification email" setting works?

Currently if the gradebook column is hidden (which we recommend as best practice during marking so that all students get their grades/feedback at the same time - also gives topic coordinators a chance to perform any moderation required before "releasing" grades) a notification email is not sent to individual students when marking is finished. However there is currently nothing in the marking interface that indicates this, therefore markers may be under a false impression that students have been notified that grades and feedback are available.

This could be simplified to two options as suggested by Michael D at http://docs.moodle.org/dev/Assignment:
a. student is sent email immediately after assignment is graded, and
b. all students are sent an email at one point after marking is complete.
The visibility of the grade+feedback could be tied to this (ie, no feedback is visible until email is sent).
The content of the notification email should just be a link to direct the student to the feedback visible in Moodle. The notification email should not contain the grade or feedback as this can become out of date if the feedback is updated and does not allow feedback attention to be measured.

Grette Wilkinson
added a comment - 02/Feb/12 2:56 PM Will this refactor include any work on how the "send notification email" setting works?
Currently if the gradebook column is hidden (which we recommend as best practice during marking so that all students get their grades/feedback at the same time - also gives topic coordinators a chance to perform any moderation required before "releasing" grades) a notification email is not sent to individual students when marking is finished. However there is currently nothing in the marking interface that indicates this, therefore markers may be under a false impression that students have been notified that grades and feedback are available.
This could be simplified to two options as suggested by Michael D at http://docs.moodle.org/dev/Assignment:
a. student is sent email immediately after assignment is graded, and
b. all students are sent an email at one point after marking is complete.
The visibility of the grade+feedback could be tied to this (ie, no feedback is visible until email is sent).
The content of the notification email should just be a link to direct the student to the feedback visible in Moodle. The notification email should not contain the grade or feedback as this can become out of date if the feedback is updated and does not allow feedback attention to be measured.

Grette Wilkinson
added a comment - 02/Feb/12 2:58 PM Will this refactor include any work on how messaging to marker works?
Suggestion is for options to be available via messaging preferences - to marker upon first submission of an assignment/ upon submission of any assignment/ upon submission of late assignment.

Grette Wilkinson
added a comment - 02/Feb/12 3:00 PM Very interested in finding out more about how statuses will work in the refactor. I understand that there will be two seperate statuses - submission status and grading status.

Grette Wilkinson
added a comment - 02/Feb/12 3:03 PM - edited Also interested in how feedback logging will be handled, ie. logging student views of feedback and feedback file downloads.
Currently in the assignment log the two actions:
student uploads a file
student submits a file for marking
are both recorded as ‘assignment upload'. It would be good if they could be differentiated in the re-write.

This is a little thing but has been bugging me. In the current assignment interface for markers the column for feedback when using quick grading is called "Comment" yet in the individual student view is is called "Feedback". "Feedback" is more appropriate as "Comment" can be mis-interpreted to mean "a comment to another marker or the topic coordinator" - dangerous because this actually gets published to students.

Grette Wilkinson
added a comment - 02/Feb/12 3:06 PM This is a little thing but has been bugging me. In the current assignment interface for markers the column for feedback when using quick grading is called "Comment" yet in the individual student view is is called "Feedback". "Feedback" is more appropriate as "Comment" can be mis-interpreted to mean "a comment to another marker or the topic coordinator" - dangerous because this actually gets published to students.

In reference to the two options as suggested by Michael D at http://docs.moodle.org/dev/Assignment:
a. student is sent email immediately after assignment is graded, and
b. all students are sent an email at one point after marking is complete.

I think 'option b' should be split into two. One is that all students are sent an email when all submitted assignments after the due date are marked. A third option would be to notify all students in a Group when all marking for that Group is complete (again after the due date).

Enrico Canale
added a comment - 09/Feb/12 10:54 AM In reference to the two options as suggested by Michael D at http://docs.moodle.org/dev/Assignment:
a. student is sent email immediately after assignment is graded, and
b. all students are sent an email at one point after marking is complete.
I think 'option b' should be split into two. One is that all students are sent an email when all submitted assignments after the due date are marked. A third option would be to notify all students in a Group when all marking for that Group is complete (again after the due date).

Would someone explain the following statement, from http://docs.moodle.org/dev/Assignment:
'The framework of the assignment module should allow for other activity modules to declare themselves "Assignments" such that they can be grouped in the UI if required.' I don't understand this.

Enrico Canale
added a comment - 09/Feb/12 10:58 AM Would someone explain the following statement, from http://docs.moodle.org/dev/Assignment:
'The framework of the assignment module should allow for other activity modules to declare themselves "Assignments" such that they can be grouped in the UI if required.' I don't understand this.

In reference to:
'The assignment activity observes groups and groupings for markers: if in Separate Groups mode, the submission overview screen when viewed as a marker contains a drop-down list which will let the marker choose between showing all submissions from all participants, or any groups that the marker is part of. This way the marker can mark only a particular group at a time.'

Why should 'Separate Groups mode' be necessary for the drop-down list to appear? It is preferable that, if the marker is in one or more Groups, the list should appear regardless of whether the course is set to Separate Groups mode or not. Note that the default option selected in the drop-down list would be 'All submissions'.

Enrico Canale
added a comment - 09/Feb/12 11:02 AM In reference to:
'The assignment activity observes groups and groupings for markers: if in Separate Groups mode, the submission overview screen when viewed as a marker contains a drop-down list which will let the marker choose between showing all submissions from all participants, or any groups that the marker is part of. This way the marker can mark only a particular group at a time.'
Why should 'Separate Groups mode' be necessary for the drop-down list to appear? It is preferable that, if the marker is in one or more Groups, the list should appear regardless of whether the course is set to Separate Groups mode or not. Note that the default option selected in the drop-down list would be 'All submissions'.

And Enrico - that statement was from Martin about a an earlier idea we discussed where the assignment types would be a special type of module. The design we have changed to is that there is only one assignment type that supports plugins that can be enabled for the assignment in any combination you like. This means you can combine plugins to achieve things like "online audio with online text" etc. It is explained much better in the wiki page above.

Damyon Wiese
added a comment - 24/Feb/12 9:46 AM I have recently updated the Moodle wiki page for this tracker item. There is a lot of detail on there now about the different types of plugins with screenshots.
See: http://docs.moodle.org/dev/Assignment_Subtypes_Combined
And Enrico - that statement was from Martin about a an earlier idea we discussed where the assignment types would be a special type of module. The design we have changed to is that there is only one assignment type that supports plugins that can be enabled for the assignment in any combination you like. This means you can combine plugins to achieve things like "online audio with online text" etc. It is explained much better in the wiki page above.
Regards, Damyon

I'll start looking at this today (and the assignment refactor in general). I'll wait on your changes to the output before shifting state on this issue so please do ping when you've got it up.
I do have to say I'm very excited about getting into this review and seeing what you've done, the assignment module as it is now really needed a good overhaul and I'm glad its finally happening!

Dan after the last rush to release the integrators asked that we get included in issues well before they arrive on our doorstep, that is how I've ended up as peer-reviewer and likely integrator of this issue.
By all means if you're interested in having a look as well any help is most appreciated, for sure having two sets of eyes working over it is better than one!

Sam Hemelryk
added a comment - 01/Mar/12 7:38 AM Hi Damon,
I'll start looking at this today (and the assignment refactor in general). I'll wait on your changes to the output before shifting state on this issue so please do ping when you've got it up.
I do have to say I'm very excited about getting into this review and seeing what you've done, the assignment module as it is now really needed a good overhaul and I'm glad its finally happening!
Dan after the last rush to release the integrators asked that we get included in issues well before they arrive on our doorstep, that is how I've ended up as peer-reviewer and likely integrator of this issue.
By all means if you're interested in having a look as well any help is most appreciated, for sure having two sets of eyes working over it is better than one!
Cheers
Sam

Sam Hemelryk
added a comment - 05/Mar/12 5:46 PM Awesome thanks Damyon, I'll be focusing on integration tomorrow but will be straight onto this again Wednesday. I'll aim to have feedback to you on Friday all going well.
Cheers
Sam

I've been looking at this code this morning and have started my review.
Everything is looking really positive so far and for the most part I am liking what I see.
The following are the notes I have made from what I have read and understood. By no means is my review complete (I've a long way to go still) but I'm hoping perhaps that by sharing what I have so far we can get the discussion process started and speed up getting this in.
Please if you could have a read of the following and a little bit of a think about the things I've noted it would be much appreicated. As I'm still near the start of the review perhaps there are things I've misread and/or misunderstood, please if any of the points make no sense tell me I'm an idiot in regards to them, other than that keep in mind I'm very happy to discuss things, I'm hoping to help in building the best solution possible, not to build walls that you've got to get over.
So into it!

One of the first things that struck me when looking at this code is that the assignment class is used a whole lot. What I don't like about this is that in several instances it isn't necessary to have the assignment class. It is used for retrieving information about subplugins etc and because the assignment class is being initialised it is in turn initialising sub plugin objects as well as a the renderer.
On some pages due to the assignment class being initialised in order to get common information several instances end up being initialised and for each instance the full set of sub plugins and renderer is retrieved.
I'll pick out a couple of examples that I think highlight this issue (to get this information I added debug lines to all class constructors):

Logged in as an admin on any page where the setting block is going to be shown. The settings block is leading to the admin tree being loading, in turn mod/assign/settings.php is included which is initialising an assignment instance in order to build a menu for mod_assign_feedback_plugin_for_gradebook. Really there is no need for this, simply getting a list of plugins and using pluginname (as it is a requirement) or asking the plugin class via static method would be all that is required and would save all of that initialisation.
You do something very similar with assignment_plugin_manager::add_admin_assignment_plugin_settings.

Viewing an assignment front page (view.php) as a student of a course. This leads to two assignment instances being initialised one (the first) relates to the actual assignment being viewed. Perfectly fine. The second however is initialised by assign_extend_settings_navigation which just calls assignment::extend_settings_navigation. This function is only using the course context associated with the assignment (as well as the id but that could be retrieved through the context). If you do this as an admin three instances are initialised as the admin instance noted above is also initialised.

The reason I am bringing this up is that it presents a very clear performance win by looking at how information is being generated and where it is being used. No just in regarsds to the impact of initialising objects for basic information, but because of all of the files being included in order to gather the required code needed to initialise these objects.
Also just noting that potentially initialising an object several times through a request in order to gather the same information is wasteful. Certainly looking at static methods, caches, or refactoring the code to be independent is going to be a better solution.

The other thing I'd like to suggest in relation to this is to find a better way to initialise things that are currently being initialised as part of assignment::__construct. There are a couple of different approaches used already within Moodle. Having a methods thats called something like "ensure_renderer_available" is one such approach. Another would be to have a get_renderer method that can be used both internally and externally (and must be used internally). The advantage to the later is that you can pass the moodle_page object as an argument so in cases where there are potentially several pages in play the required page can be specified. (Block editing is one such example of several moodle_pages being in use although I don't know it has impact here)

Next on the list is the assignment::view method. I greatly dislike any class method that makes use of optional/required param. I believe they should be as close to the surface as possible. There is no telling where a class will be used and perhaps someone will without realising start using a duplicate named param for a different reason somewhere else in code before calling that method leading to unexpected mayhem. I think separating those param calls out and passing them to a method is much better. However it goes a little deeper than that, I'm not entirely sure of your plans for future extensions as I'm still only starting in this but I am a little concerned that the process_* methods being made private and exposed only through the view method presently is going to cause headaches later down the track when you start working on webservices etc. Again I can only speculate and perhaps there is more that I havn't seen, however at the moment I think it would be worth moving the view methods if..else's to view.php, and making those process_* and view_* methods public is going to be beneficial. Very Very open to discussion on that however.

Next is the sub plugin names. I've had a quick scan over existing modules with subplugins (as I'm reasonably unfamiliar with them myself) and I've noted that all plugin names start with the modules name e.g. workshopform, workshopallocation, and workshopeval. Certainly I think it is worth sticking with this convention and that the mod_assign sub plugins should be named assignsubmission and assignfeedback.

One final design specific point in relation to the assignment class. I really think that there needs to be some checking of the instance property internally. Both that it has been set before calling functions that depend upon it, and perhaps a validation that it contains the expected/required properties itself.

Some general notes:

Changing the assignment modules pluginname/modulename string is a really smart idea, however I don't know that appending "(2.2)" is the best solution. Probably best to talk to MD about what he would like to see there.

index.php ... probably should do something similar to existing index.php pages

It would be great if the new access API's could be used, the following are some examples:

style.css should be named styles.css then it will be included automatically, and won't need to be included manually (also gets properly cached, minified etc).

It would be great to make use of the MUST_EXIST option when retrieving a specific record from the database where possible. A good example of a place to convert would be view.php where $id is used to fetch the assignment record. If you specify MUST_EXIST there is no need for the print_error call it is all handled automatically.

I wonder about moving the require_capability('mod/assign:view', $context) calls out of functions and into entry files. Functions should be checking the capabilities required to perform the actions associated with them, generic read capabilities are best checked by the page to ensure that anything can be performed. This is much more maintainable as developers need not remember it and there is no chance of unnessecary duplicate calls because we can't be sure of paths.

view.php

No need to include assign.js, the renderer is including it where required. If that doesn't work then there is a bug.

lib.php:

Including locallib.php directly in lib.php negates the advantage of splitting out API callbacks and core required functions from module specific classes/methods/functions doubly so because locallib.php includes several files itself. On the upside it is a small file presently (although bound to grow) so no real issue here just noting it. Perhaps this ties in with the first thing discussed and would be solved by making the code indepenent. Food for thought I suppose

Coding style things:
These are all general things and they don't really need to be addressed until right before this is put up for integration.
Certainly it is a good idea to run Marinas and/or Tim's coding checks over them when you are ready to look at them as those tools will turn up 99% of things.
For the time being if you are aware of them all the better!

Where possible be specific about the class of the object being passed to a function, both in the function definition through type hinting and in the phpdocs for the function. The object class is vague and in fact deprecated (see lib/setuplib.php line 79)

Right, so reads like a lot at the moment but please be aware I have read over most of the code once now and while I havn't finished and there will be more what I have noted is only the negative, 95% of the code is perfect!

Sam Hemelryk
added a comment - 06/Mar/12 12:15 PM Hi Damyon,
I've been looking at this code this morning and have started my review.
Everything is looking really positive so far and for the most part I am liking what I see.
The following are the notes I have made from what I have read and understood. By no means is my review complete (I've a long way to go still) but I'm hoping perhaps that by sharing what I have so far we can get the discussion process started and speed up getting this in.
Please if you could have a read of the following and a little bit of a think about the things I've noted it would be much appreicated. As I'm still near the start of the review perhaps there are things I've misread and/or misunderstood, please if any of the points make no sense tell me I'm an idiot in regards to them, other than that keep in mind I'm very happy to discuss things, I'm hoping to help in building the best solution possible, not to build walls that you've got to get over.
So into it!
One of the first things that struck me when looking at this code is that the assignment class is used a whole lot. What I don't like about this is that in several instances it isn't necessary to have the assignment class. It is used for retrieving information about subplugins etc and because the assignment class is being initialised it is in turn initialising sub plugin objects as well as a the renderer.
On some pages due to the assignment class being initialised in order to get common information several instances end up being initialised and for each instance the full set of sub plugins and renderer is retrieved.
I'll pick out a couple of examples that I think highlight this issue (to get this information I added debug lines to all class constructors):
Logged in as an admin on any page where the setting block is going to be shown. The settings block is leading to the admin tree being loading, in turn mod/assign/settings.php is included which is initialising an assignment instance in order to build a menu for mod_assign_feedback_plugin_for_gradebook. Really there is no need for this, simply getting a list of plugins and using pluginname (as it is a requirement) or asking the plugin class via static method would be all that is required and would save all of that initialisation.
You do something very similar with assignment_plugin_manager::add_admin_assignment_plugin_settings.
Viewing an assignment front page (view.php) as a student of a course. This leads to two assignment instances being initialised one (the first) relates to the actual assignment being viewed. Perfectly fine. The second however is initialised by assign_extend_settings_navigation which just calls assignment::extend_settings_navigation. This function is only using the course context associated with the assignment (as well as the id but that could be retrieved through the context). If you do this as an admin three instances are initialised as the admin instance noted above is also initialised.
The reason I am bringing this up is that it presents a very clear performance win by looking at how information is being generated and where it is being used. No just in regarsds to the impact of initialising objects for basic information, but because of all of the files being included in order to gather the required code needed to initialise these objects.
Also just noting that potentially initialising an object several times through a request in order to gather the same information is wasteful. Certainly looking at static methods, caches, or refactoring the code to be independent is going to be a better solution.
The other thing I'd like to suggest in relation to this is to find a better way to initialise things that are currently being initialised as part of assignment::__construct. There are a couple of different approaches used already within Moodle. Having a methods thats called something like "ensure_renderer_available" is one such approach. Another would be to have a get_renderer method that can be used both internally and externally (and must be used internally). The advantage to the later is that you can pass the moodle_page object as an argument so in cases where there are potentially several pages in play the required page can be specified. (Block editing is one such example of several moodle_pages being in use although I don't know it has impact here)
Next on the list is the assignment::view method. I greatly dislike any class method that makes use of optional/required param. I believe they should be as close to the surface as possible. There is no telling where a class will be used and perhaps someone will without realising start using a duplicate named param for a different reason somewhere else in code before calling that method leading to unexpected mayhem. I think separating those param calls out and passing them to a method is much better. However it goes a little deeper than that, I'm not entirely sure of your plans for future extensions as I'm still only starting in this but I am a little concerned that the process_* methods being made private and exposed only through the view method presently is going to cause headaches later down the track when you start working on webservices etc. Again I can only speculate and perhaps there is more that I havn't seen, however at the moment I think it would be worth moving the view methods if..else's to view.php, and making those process_* and view_* methods public is going to be beneficial. Very Very open to discussion on that however.
Next is the sub plugin names. I've had a quick scan over existing modules with subplugins (as I'm reasonably unfamiliar with them myself) and I've noted that all plugin names start with the modules name e.g. workshopform, workshopallocation, and workshopeval. Certainly I think it is worth sticking with this convention and that the mod_assign sub plugins should be named assignsubmission and assignfeedback.
One final design specific point in relation to the assignment class. I really think that there needs to be some checking of the instance property internally. Both that it has been set before calling functions that depend upon it, and perhaps a validation that it contains the expected/required properties itself.
Some general notes:
Changing the assignment modules pluginname/modulename string is a really smart idea, however I don't know that appending "(2.2)" is the best solution. Probably best to talk to MD about what he would like to see there.
index.php ... probably should do something similar to existing index.php pages
It would be great if the new access API's could be used, the following are some examples:
get_context_instance(CONTEXT_COURSE,$form_data->course) ==> context_course::instance($formdata->course)
get_context_instance(CONTEXT_MODULE,$form_data->coursemodule) ==> context_module::instance($formdata->coursemodule)
style.css should be named styles.css then it will be included automatically, and won't need to be included manually (also gets properly cached, minified etc).
It would be great to make use of the MUST_EXIST option when retrieving a specific record from the database where possible. A good example of a place to convert would be view.php where $id is used to fetch the assignment record. If you specify MUST_EXIST there is no need for the print_error call it is all handled automatically.
I wonder about moving the require_capability('mod/assign:view', $context) calls out of functions and into entry files. Functions should be checking the capabilities required to perform the actions associated with them, generic read capabilities are best checked by the page to ensure that anything can be performed. This is much more maintainable as developers need not remember it and there is no chance of unnessecary duplicate calls because we can't be sure of paths.
view.php
No need to include assign.js, the renderer is including it where required. If that doesn't work then there is a bug.
lib.php:
Including locallib.php directly in lib.php negates the advantage of splitting out API callbacks and core required functions from module specific classes/methods/functions doubly so because locallib.php includes several files itself. On the upside it is a small file presently (although bound to grow) so no real issue here just noting it. Perhaps this ties in with the first thing discussed and would be solved by making the code indepenent. Food for thought I suppose
Coding style things:
These are all general things and they don't really need to be addressed until right before this is put up for integration.
Certainly it is a good idea to run Marinas and/or Tim's coding checks over them when you are ready to look at them as those tools will turn up 99% of things.
For the time being if you are aware of them all the better!
require_once calls should use absolute paths (e.g. $CFG->dirroot.'/mod/assign/mod_form.php')
variable/property names shouldn't contain underscores (only a-z0-9)
variable/property names should be descriptive and complete.. $ass ==> $assignment && $p ==> $plugins
PHP passes all objects by reference, there is no need for most (if not all) of the reference handling (&)
Lots of unused variables and globals around the place.
format_string and format_text calls should always specify the context for formatting to ensure proper adherance to filters and like.
Review language strings and remove camel casing.. `Feedback Comments` ==> `Feedback comments`
Where possible be specific about the class of the object being passed to a function, both in the function definition through type hinting and in the phpdocs for the function. The object class is vague and in fact deprecated (see lib/setuplib.php line 79)
Right, so reads like a lot at the moment but please be aware I have read over most of the code once now and while I havn't finished and there will be more what I have noted is only the negative, 95% of the code is perfect!
Cheers
Sam
P.S links to the mentioned code checking tools:
Tim's code checker: http://moodle.org/plugins/view.php?plugin=local_codechecker
Marina's code checker: https://github.com/marinaglancy/moodle-local_moodlecheck
Its worth running both as they both do slightly different checks.

This is great feedback. I have started work on some of your suggestions and have pushed some changes already.

The adminlib no longer includes the locallib and the plugin management functions have been moved out of the assignment_plugin class and into the manager class. The same with lib.php (except locallib.php is still included inside the functions that use it e.g. update_instance).

Damyon Wiese
added a comment - 06/Mar/12 4:48 PM Thanks Sam,
This is great feedback. I have started work on some of your suggestions and have pushed some changes already.
The adminlib no longer includes the locallib and the plugin management functions have been moved out of the assignment_plugin class and into the manager class. The same with lib.php (except locallib.php is still included inside the functions that use it e.g. update_instance).
I have also removed all the occurrences of & for pass by reference.
Regards, Damyon

Note - the comment about the names of the subplugin types. I agree that they should be assignsubmission and assignfeedback - there are a calls that don't use the plugin type and the subplugintype together which means that all subplugin types have to be unique across modules. I didn't realise this before I started

Damyon Wiese
added a comment - 06/Mar/12 4:50 PM Note - the comment about the names of the subplugin types. I agree that they should be assignsubmission and assignfeedback - there are a calls that don't use the plugin type and the subplugintype together which means that all subplugin types have to be unique across modules. I didn't realise this before I started
Damyon

Hi Damyon,
Thanks for looking over the earlier notes, and for starting to touch things up.
The following are the notes I have made since:

General stuff

The following files are breaking installation (fresh installation tested):

mod/assign/feedback/comments/db/install.php

mod/assign/feedback/file/db/install.php

mod/assign/submission/comments/db/install.php

mod/assign/submission/onlinetext/db/install.php

For some reason on a fresh installation I see the "mod_assign_feedback_plugin_for_gradebook" setting when setting the site name/description (should be defaulting)

When grading a submission with Feedback files enabled there is a JS error and the Feedback files interface fails to load.

locallib.php shouldn't always include mod_form.php, that is leading to moodleform_mod and formslib being loaded for every page within the module.

Leading on from the previous note forms other than mod_assign_mod_form should really be moved to a separate file leaving mod_form.php purely for instance creation/updating.

I think class naming needs to be looked at (I've generated a list of classes). Like functions classes should be prefixed with something indicative of the component they belong to. Of course the classes requiring to be named as they are for callbacks is fine, but certainly for the likes of the renderable classes they should be prefixed by something common, perhaps "mod_assign_" or "assign_widget_". In fact it is worth mentioning that for modules now we have been incouraging the use of mod_ in front of callbacks in order to reduce the likelyhood of collisions with other plugins etc. It may be worth carrying that principle over to the naming for this module.

lib.php & locallib.php

assignment::add_submissions code should really be moved through to mod_form if thats the only place it is being used. At the moment it looks like things are being split with a couple of elements being added to mod_form then calling add_settings.

assignment::get_course_context can use the context::get_course_context method to be sure of returning the course context if available.

get_courseid_from_context is deprecated

assign::list_enrolled_users_with_capability isn't overly useful at the moment, looking at uses for it perhaps it would be better to convert it to a method like get_users_who_can_submit or get_possible_submitters and have it get the current group, make the enrolment call with the correct caps and return the array of users. You could also add an argument $useridsonly so that if you only need the ids you could limit the fields get_enrolled_users returns (like the function below).

Same goes for the next function count_enrolled_users_with_capability... btw that should be using count_enrolled_users() rather than counting get_enrolled_users.

After looking at the get_userid_for_row I added a debug notice to the grading_table class and browsed the grading interfaces. When viewing the grading table things are fine, however when looking at a single users submission I see the grading table is initialised three times. Absolutely this should be looked at and a solution found, perhaps either gathering the required information early on and only once, or caching if that is safe/pertinent.

The define ASSIGN_PLUGIN_CLASS_FILE is there any need for it? Is it likely to change or can we just replace that with 'lib.php'? (I think it is 100% fine to require a hardcode lib.php and reducing defines is a positive)

assignment::load_plugins -> get_plugin_list returns an array of name => fullpath pairs, perhaps you can use the key as the name rather than calling basename (totally up to you just noting)

Also in that function the sorting method will fail if for any reason you get a sort order like 1,2,1. You'd expect to end up with 1,1,2 however you will end up with 1,2,1. Noting I havn't tested this just observing code.

assignment::add_instance should probably unset $this->instance->id just incase it is set for any reason e.g. bugs, or other people calling the API. Noting I really think this should either be static or should populate the rest of the assignment object after creation (setting context etc).

assignment::delete_instance a couple of things here:

delete_records never returns false. It returns true or it throws an exception so no need for the error handling there.

Don't rebuild the course cache there, the context and course module instance still need to be removed by the calling code (mod.php or whereever) and it will be rebuilt when the time is ready.

No need to delete events that will be handled by delete_course_module when Moodle calls it upon deleting the cm.

assign_delete_instance $id is the $cm->instance. Check out the workshop_delete_instance all you need to do is select the assign record. No need to check cm/context.

get_module_name and get_module_name_plural. Just as an interesting idea you could make those return static methods and then use a static var that gets initialised to "new lang_string()" rather than get string. That way the string object reference would be used when passing around and for all calls there would only ever be a single get_string call.

assignment::get_instance should throw a coding exceptionif $this->instance is still empty after trying the course_module (just imagining thats going to help others out who have to work on this code).

assignment::get_course_context, get_course should that be throwing a coding_exception rather than an error (can it be triggered by the user or does the object have to be set up wrong)?

assignment::can_grade fyi we have has_any_capability and has_all_capabilities methods that are useful for situations like this.

assignment::view_next_single_grade it would be preferred to have a moodle_url object passed to the redirect method.

get_submission should be two methods get_user_submission, and get_submission. If $submissionid is provided by does not exist and $create is true there is a big problem. Splitting will cleaner.

the more I look at the view methods the more I feel they should take the approach the rest of Moodle is taking and return any generated HTML content rather than echo'ing it out. This gives more control to the calling code allowing the generation and output of content to happen at different times. Perhaps not possible? What are your thoughts?

can_view_submission could use has_all_capabilities, also defaulting $userid = null and processing that as the current user would be more consistent with other areas of Moodle. Noting that is_enrolled works that was a well so you would not have to actually default it and could just do (!empty() && !has_all_capabilities()). Just double checking but can the current user always view their own submission? (thinking yes)

assignment::render_area_files Could this method be using $this->output rather than calling $PAGE->get_renderer?

Ok cool, so I've now pulled through the two lib files function by function.
I'll now wait for more changes to arrive or discussion on the points from both sets of notes so far before carrying on with the review.
Doing this will make things a bit easier as its getting harder to imagie how things may change given what I've noted so far.
I'll reopen this issue for the time being Damyon, put it back up for peer-review when you're ready again.

Sam Hemelryk
added a comment - 09/Mar/12 11:36 AM Hi Damyon,
Thanks for looking over the earlier notes, and for starting to touch things up.
The following are the notes I have made since:
General stuff
The following files are breaking installation (fresh installation tested):
mod/assign/feedback/comments/db/install.php
mod/assign/feedback/file/db/install.php
mod/assign/submission/comments/db/install.php
mod/assign/submission/onlinetext/db/install.php
For some reason on a fresh installation I see the "mod_assign_feedback_plugin_for_gradebook" setting when setting the site name/description (should be defaulting)
When grading a submission with Feedback files enabled there is a JS error and the Feedback files interface fails to load.
locallib.php shouldn't always include mod_form.php, that is leading to moodleform_mod and formslib being loaded for every page within the module.
Leading on from the previous note forms other than mod_assign_mod_form should really be moved to a separate file leaving mod_form.php purely for instance creation/updating.
I think class naming needs to be looked at (I've generated a list of classes). Like functions classes should be prefixed with something indicative of the component they belong to. Of course the classes requiring to be named as they are for callbacks is fine, but certainly for the likes of the renderable classes they should be prefixed by something common, perhaps "mod_assign_" or "assign_widget_". In fact it is worth mentioning that for modules now we have been incouraging the use of mod_ in front of callbacks in order to reduce the likelyhood of collisions with other plugins etc. It may be worth carrying that principle over to the naming for this module.
lib.php & locallib.php
assignment::add_submissions code should really be moved through to mod_form if thats the only place it is being used. At the moment it looks like things are being split with a couple of elements being added to mod_form then calling add_settings.
assignment::get_course_context can use the context::get_course_context method to be sure of returning the course context if available.
get_courseid_from_context is deprecated
assign::list_enrolled_users_with_capability isn't overly useful at the moment, looking at uses for it perhaps it would be better to convert it to a method like get_users_who_can_submit or get_possible_submitters and have it get the current group, make the enrolment call with the correct caps and return the array of users. You could also add an argument $useridsonly so that if you only need the ids you could limit the fields get_enrolled_users returns (like the function below).
Same goes for the next function count_enrolled_users_with_capability... btw that should be using count_enrolled_users() rather than counting get_enrolled_users.
After looking at the get_userid_for_row I added a debug notice to the grading_table class and browsed the grading interfaces. When viewing the grading table things are fine, however when looking at a single users submission I see the grading table is initialised three times. Absolutely this should be looked at and a solution found, perhaps either gathering the required information early on and only once, or caching if that is safe/pertinent.
The define ASSIGN_PLUGIN_CLASS_FILE is there any need for it? Is it likely to change or can we just replace that with 'lib.php'? (I think it is 100% fine to require a hardcode lib.php and reducing defines is a positive)
assignment::load_plugins -> get_plugin_list returns an array of name => fullpath pairs, perhaps you can use the key as the name rather than calling basename (totally up to you just noting)
Also in that function the sorting method will fail if for any reason you get a sort order like 1,2,1. You'd expect to end up with 1,1,2 however you will end up with 1,2,1. Noting I havn't tested this just observing code.
assignment::add_instance should probably unset $this->instance->id just incase it is set for any reason e.g. bugs, or other people calling the API. Noting I really think this should either be static or should populate the rest of the assignment object after creation (setting context etc).
assignment::delete_instance a couple of things here:
delete_records never returns false. It returns true or it throws an exception so no need for the error handling there.
Don't rebuild the course cache there, the context and course module instance still need to be removed by the calling code (mod.php or whereever) and it will be rebuilt when the time is ready.
No need to delete events that will be handled by delete_course_module when Moodle calls it upon deleting the cm.
assign_delete_instance $id is the $cm->instance. Check out the workshop_delete_instance all you need to do is select the assign record. No need to check cm/context.
get_module_name and get_module_name_plural. Just as an interesting idea you could make those return static methods and then use a static var that gets initialised to "new lang_string()" rather than get string. That way the string object reference would be used when passing around and for all calls there would only ever be a single get_string call.
assignment::get_instance should throw a coding exceptionif $this->instance is still empty after trying the course_module (just imagining thats going to help others out who have to work on this code).
assignment::get_course_context, get_course should that be throwing a coding_exception rather than an error (can it be triggered by the user or does the object have to be set up wrong)?
assignment::can_grade fyi we have has_any_capability and has_all_capabilities methods that are useful for situations like this.
assignment::view_next_single_grade it would be preferred to have a moodle_url object passed to the redirect method.
get_submission should be two methods get_user_submission, and get_submission. If $submissionid is provided by does not exist and $create is true there is a big problem. Splitting will cleaner.
the more I look at the view methods the more I feel they should take the approach the rest of Moodle is taking and return any generated HTML content rather than echo'ing it out. This gives more control to the calling code allowing the generation and output of content to happen at different times. Perhaps not possible? What are your thoughts?
can_view_submission could use has_all_capabilities, also defaulting $userid = null and processing that as the current user would be more consistent with other areas of Moodle. Noting that is_enrolled works that was a well so you would not have to actually default it and could just do (!empty() && !has_all_capabilities()). Just double checking but can the current user always view their own submission? (thinking yes)
assignment::render_area_files Could this method be using $this->output rather than calling $PAGE->get_renderer?
Ok cool, so I've now pulled through the two lib files function by function.
I'll now wait for more changes to arrive or discussion on the points from both sets of notes so far before carrying on with the review.
Doing this will make things a bit easier as its getting harder to imagie how things may change given what I've noted so far.
I'll reopen this issue for the time being Damyon, put it back up for peer-review when you're ready again.
Once more thanks for the effort!
Cheers
Sam

I have finished making changes based on all your comments below. I don't see how to send this back to peer review as you described though (it looks like it is still in peer review - the only option I have is "Finish Peer Review").

Here is a summary of the changes:

Detailed list of notes from first set of comments:

Name of old assignment module - asked MD waiting for a response

index.php now does a listing of assignments in the course

New context API is used

style.css renamed to styles.css

Now using MUST_EXIST and removed error checking

Moved view capability check to the entry page

javascript include has been removed

lib.php no longer includes locallib.php

all require_onces calls use absolute paths

no underscores in variable names

no short variable names

pass by reference removed

unused variables and globals removed

format_string and format_text always pass context as option

camel casing removed form language strings

All classes passed to a function are correctly documented in the phpdocs and the type hinting.

Detailed list of notes from second set of comments:

1. Cannot move this entirely to mod_form because it must allow the plugins to add their specific settings to the setting page (add_all_plugin_settings). I did however move all the stuff from add_settings to mod_form and then just call add_all_plugin_settings on the assignment.

2. assignment::get_course_context uses context::get_course_context

3. get_courseid_from_context has been removed

4. list_enrolled_users_with_capability has been renamed to list_participants and I added useridsonly as an parameter

5. now called count_participants and it uses count_enrolled_users

6. The constructor for the grading table only gets called once on the grading page now.

7. ASSIGN_PLUGIN_CLASS_FILE is gone

8. No longer uses basename and just gets the name from the key

9. I don't think that sorting thing will be a problem. When the plugins call move - it sets the order for all plugins sequentially (so they are self correcting). I think the worst case will be that there are holes in the order for plugins that get removed.

10. This has been changed to only load assignment->instance from the database. The form is explicitly passed as a variable for the add and updates methods now.

11. Error handling for delete_instance cleaned up, rebuild_course_cache has been removed and events are not deleted.

12. Delete still needs to work out the context as it is used to do things like cleanup files and is made available to the plugins to delete their data (which has also been added).

13. Changed to use a static variable cache for these strings

14. Now throws coding_exception

15. Now throws coding_exception

16. can_grade is now only checking one capability

17. redirect now gets a moodle_url

18. Agreed - I did the same for get_grade

19. The view functions now all return their output as a string which is written from the view page. I take your point about this allowing the calling code to chose to modify the output or choose when to write it, but this approach will generally require more memory and there is all ready an oportunity to modify the output through the renderers. This also requires ob_start/end hacks to capture the output from things like moodleform, the plagiarism api and flexible_table.

20. I can't see how this can be simplified any further - any suggestions?

21. Yes it can - that function came before $this->output and hadn't been updated.

Damyon Wiese
added a comment - 12/Mar/12 12:17 AM Hi Sam,
I have finished making changes based on all your comments below. I don't see how to send this back to peer review as you described though (it looks like it is still in peer review - the only option I have is "Finish Peer Review").
Here is a summary of the changes:
Detailed list of notes from first set of comments:
Name of old assignment module - asked MD waiting for a response
index.php now does a listing of assignments in the course
New context API is used
style.css renamed to styles.css
Now using MUST_EXIST and removed error checking
Moved view capability check to the entry page
javascript include has been removed
lib.php no longer includes locallib.php
all require_onces calls use absolute paths
no underscores in variable names
no short variable names
pass by reference removed
unused variables and globals removed
format_string and format_text always pass context as option
camel casing removed form language strings
All classes passed to a function are correctly documented in the phpdocs and the type hinting.
Detailed list of notes from second set of comments:
1. Cannot move this entirely to mod_form because it must allow the plugins to add their specific settings to the setting page (add_all_plugin_settings). I did however move all the stuff from add_settings to mod_form and then just call add_all_plugin_settings on the assignment.
2. assignment::get_course_context uses context::get_course_context
3. get_courseid_from_context has been removed
4. list_enrolled_users_with_capability has been renamed to list_participants and I added useridsonly as an parameter
5. now called count_participants and it uses count_enrolled_users
6. The constructor for the grading table only gets called once on the grading page now.
7. ASSIGN_PLUGIN_CLASS_FILE is gone
8. No longer uses basename and just gets the name from the key
9. I don't think that sorting thing will be a problem. When the plugins call move - it sets the order for all plugins sequentially (so they are self correcting). I think the worst case will be that there are holes in the order for plugins that get removed.
10. This has been changed to only load assignment->instance from the database. The form is explicitly passed as a variable for the add and updates methods now.
11. Error handling for delete_instance cleaned up, rebuild_course_cache has been removed and events are not deleted.
12. Delete still needs to work out the context as it is used to do things like cleanup files and is made available to the plugins to delete their data (which has also been added).
13. Changed to use a static variable cache for these strings
14. Now throws coding_exception
15. Now throws coding_exception
16. can_grade is now only checking one capability
17. redirect now gets a moodle_url
18. Agreed - I did the same for get_grade
19. The view functions now all return their output as a string which is written from the view page. I take your point about this allowing the calling code to chose to modify the output or choose when to write it, but this approach will generally require more memory and there is all ready an oportunity to modify the output through the renderers. This also requires ob_start/end hacks to capture the output from things like moodleform, the plagiarism api and flexible_table.
20. I can't see how this can be simplified any further - any suggestions?
21. Yes it can - that function came before $this->output and hadn't been updated.
Regards, Damyon

Awesome thanks for working on those Damyon,
I'll have a good read through in the next couple of days and continue reviewing.
The issue is still showing as in review so I must have used the wrong button when posting my review the other day sorry.

Sam Hemelryk
added a comment - 12/Mar/12 4:33 AM - edited Awesome thanks for working on those Damyon,
I'll have a good read through in the next couple of days and continue reviewing.
The issue is still showing as in review so I must have used the wrong button when posting my review the other day sorry.
Cheers
Sam

My apologies I haven't found a chance in the past week to finish off the next part of the review.
Rest assured I'm back on to it today and will aim to get the next set of feedback to you by the end of today.

Sam Hemelryk
added a comment - 26/Mar/12 9:04 AM Hi Damyon,
My apologies I haven't found a chance in the past week to finish off the next part of the review.
Rest assured I'm back on to it today and will aim to get the next set of feedback to you by the end of today.
Cheers
Sam

No need to set id param on url separately, second argument is an associative array of params.

version.php

Should be defining $module->component = 'mod_assign'. Not essential but good practice.

submission_form.php

I'm unsure whether definition should be calling set_data, it seems to me like that has the potential to throw the normal procession of form handling out of whack, however thats just a hunce and not something I know for sure. It should be investigated.

settings.php

Should be wrapping all settings content with `if ($ADMIN->fulltree) {`

Ideally should not be including locallib.php. This leads to everything being included on every page for admins or anyone else with permission to see the settings page.

Would be great if it used 'new lang_string' rather than get_string. This way string calls arn't actually made unless they the strings are going to be used. Have a look at admin/settings/* for examples.

mod_assign_feedback_plugin_for_gradebook default value is incorrect.

renderer.php

render_assign_files is producing $module but is not using it, should it be the forth argument to js_init_call below?

On a general note renderers should contain as little logic as possible, they should be purely about producing output. There are several examples of functions that need to reviewed in the renderer in order to properly separate out the logic. For instance format_grade which requests data from the database. A renderer should never need to go to the database, it should be provided with everything it needs. Keeping this logic out of the renderers is how we keep overriding the renderers a task achievable by theme designers. The goal being that they don't need to know the inner workings of whats being renderer, they're given an object, or just arguments that contain the information they will want to display. Another example of areas is capability calls. While it may be valid to make a capbility call in a renderer some circumstances usually it is something that we try to avoid. Normally this can be achieved by abstracting the capability call into an access call of the object being passed, for examples you may end up with $assignment->user_can_grade. By the way the workshop renderer is a very good example of a well written renderer.

In line with the above comment I like that you have the renderable objects in renderable.php however I think they can be GREATLY improved. For instance assignment_header, within the render_assignment_header method you make calls such as "$header->get_assignment()>get_instance()>name". The idea of having renderable objects is that you get an object specifically set up with the information you want ready at hand and easy to get to. Really in this example there should be $header->get_heading() method that does all of the rest behind the scenes. This way the render is kept simple in that you don't need to know extranious paths to the information you want, and the renderable object become a full fledged gateway to the information being renderer. I suppose you could go as far as not exposing any of the core objects within a renderable object but that would not be required. Really just simplifying the access to infromation is what I am after and woul dbe more in line with the design and best practices for renderers.

render_user_summary (ln 134) the really long line needs to be either wrapped or better yet separated out so that it is readable and not nearly so long.

render_footer I think this function should be removed and the standard footer method used instead. This going to be more consistent with other renderers in Moodle and will keep things simplier for those overriding renderers. The supporting factor to this is that the render_prefix is typically used

render_feedback_status should not be calling grade_get_grades, that should be exposed through the feedback_status object or passed into the calling method. This falls into the above general points about renderers. There are several other areas that could should be improved as well, I imagine you can spot them out so I'll leave it up to you to review the renderer methods looking for logic that can be abstracted. If you would like a hand let me know or there is something I can help with there let me know and I'll see what I can do.

render_grading_table uses deprecated method print_context_name

Looks like flexible_table and moodleform could be using ob_get_clean. These two methods are interesting, typically we havn't passed flexible table or mforms through renderers for output. Normally pages with those object use renderers before and after direct calls. I'm not against the idea of running it through buffers myself however I will have a chat to Eloy/Petr and see whether they can think of any show stoppers to that approach just in case.

renderable.php

On a general note I really think that the classes defined here should be prefixed with something that is going to make them more unique to the assign module. Classes like user_summary and grading_form are names that don't relate at all to the assign module and represent a greater chance of redifition in other components and plugins.

The notes above about abstracting content of renderable object to be direct of course apply here.

users_submissions_table::set_data php docs say $data should be an array, construct is passing a strict stdClass object. Looks like $data should be typehinted to stdClass, either way needs clarification. (get_data php docs updates as well)

assign_files::preprocess is using deprecated function file_encode_url

locallib.php
I didn't spend too much time here as I've looked over this several times in the past and made thorough comments.

There are a lot of includes here, it'd be great if we could cut that list down. Especially things such as forms, plagiarism, and portfolio as they are likely to be including other files and are typically only needed in specific scenarios and only when enabled.

lib.php

assign_print_overview queries need to be formatted. There are also several unused strings there.

index.php

Several unused variables in this file.

No need for sesskey in view.php url

Can anyone view the number of submissions for an assignment? Just wondering whether this is an issue in circumstances where separate groups are in use.

The other thing to mention is that there have been a couple of changes in the past month that impact or will require development changes for the assign module.

There is a new capability that should be implemented mod/assign:addinstance that needs to be implement. All other core modules have been updated now, it is just a case of defining the capability and the string required by it. See MDL-19125

We've started the process of increasing the developer debug to include E_STRICT. This has been done because many of the E_STRICT notifications in PHP <= 5.3 have become notices or warnings in PHP 5.4. Included in this list is ensuring overriden function definitions are identical to the parents, proper reference handling, and proper method access.
It would be a good idea to enable E_STRICT notification in PHP/Moodle and test the module as this will quickly highlight any areas requiring attention (You can set $CFG->debug = E_ALL | E_STRICT in your config.php). See MDL-12730

Thats as far as I've managed to get today.
Really at this point I hope I have picked up on many of the more important things and perhaps its time to start thinking about when this can land.
However before that happens it would be great to have it meet the coding style. Again those two tools, Tims and Marinas will be a big help for you there.

Sam Hemelryk
added a comment - 26/Mar/12 12:25 PM Alright getting down to the real nitty gritty stuff now:
view.php:
No need to set $cm, $assignment, $context to null (ln 34-36)
No need to set id param on url separately, second argument is an associative array of params.
version.php
Should be defining $module->component = 'mod_assign'. Not essential but good practice.
submission_form.php
I'm unsure whether definition should be calling set_data, it seems to me like that has the potential to throw the normal procession of form handling out of whack, however thats just a hunce and not something I know for sure. It should be investigated.
settings.php
Should be wrapping all settings content with `if ($ADMIN->fulltree) {`
Ideally should not be including locallib.php. This leads to everything being included on every page for admins or anyone else with permission to see the settings page.
Would be great if it used 'new lang_string' rather than get_string. This way string calls arn't actually made unless they the strings are going to be used. Have a look at admin/settings/* for examples.
mod_assign_feedback_plugin_for_gradebook default value is incorrect.
renderer.php
render_assign_files is producing $module but is not using it, should it be the forth argument to js_init_call below?
On a general note renderers should contain as little logic as possible, they should be purely about producing output. There are several examples of functions that need to reviewed in the renderer in order to properly separate out the logic. For instance format_grade which requests data from the database. A renderer should never need to go to the database, it should be provided with everything it needs. Keeping this logic out of the renderers is how we keep overriding the renderers a task achievable by theme designers. The goal being that they don't need to know the inner workings of whats being renderer, they're given an object, or just arguments that contain the information they will want to display. Another example of areas is capability calls. While it may be valid to make a capbility call in a renderer some circumstances usually it is something that we try to avoid. Normally this can be achieved by abstracting the capability call into an access call of the object being passed, for examples you may end up with $assignment->user_can_grade. By the way the workshop renderer is a very good example of a well written renderer.
In line with the above comment I like that you have the renderable objects in renderable.php however I think they can be GREATLY improved. For instance assignment_header, within the render_assignment_header method you make calls such as "$header->get_assignment() >get_instance() >name". The idea of having renderable objects is that you get an object specifically set up with the information you want ready at hand and easy to get to. Really in this example there should be $header->get_heading() method that does all of the rest behind the scenes. This way the render is kept simple in that you don't need to know extranious paths to the information you want, and the renderable object become a full fledged gateway to the information being renderer. I suppose you could go as far as not exposing any of the core objects within a renderable object but that would not be required. Really just simplifying the access to infromation is what I am after and woul dbe more in line with the design and best practices for renderers.
render_user_summary (ln 134) the really long line needs to be either wrapped or better yet separated out so that it is readable and not nearly so long.
render_footer I think this function should be removed and the standard footer method used instead. This going to be more consistent with other renderers in Moodle and will keep things simplier for those overriding renderers. The supporting factor to this is that the render_prefix is typically used
render_feedback_status should not be calling grade_get_grades, that should be exposed through the feedback_status object or passed into the calling method. This falls into the above general points about renderers. There are several other areas that could should be improved as well, I imagine you can spot them out so I'll leave it up to you to review the renderer methods looking for logic that can be abstracted. If you would like a hand let me know or there is something I can help with there let me know and I'll see what I can do.
render_grading_table uses deprecated method print_context_name
Looks like flexible_table and moodleform could be using ob_get_clean. These two methods are interesting, typically we havn't passed flexible table or mforms through renderers for output. Normally pages with those object use renderers before and after direct calls. I'm not against the idea of running it through buffers myself however I will have a chat to Eloy/Petr and see whether they can think of any show stoppers to that approach just in case.
renderable.php
On a general note I really think that the classes defined here should be prefixed with something that is going to make them more unique to the assign module. Classes like user_summary and grading_form are names that don't relate at all to the assign module and represent a greater chance of redifition in other components and plugins.
The notes above about abstracting content of renderable object to be direct of course apply here.
users_submissions_table::set_data php docs say $data should be an array, construct is passing a strict stdClass object. Looks like $data should be typehinted to stdClass, either way needs clarification. (get_data php docs updates as well)
assign_files::preprocess is using deprecated function file_encode_url
locallib.php
I didn't spend too much time here as I've looked over this several times in the past and made thorough comments.
There are a lot of includes here, it'd be great if we could cut that list down. Especially things such as forms, plagiarism, and portfolio as they are likely to be including other files and are typically only needed in specific scenarios and only when enabled.
lib.php
assign_print_overview queries need to be formatted. There are also several unused strings there.
index.php
Several unused variables in this file.
No need for sesskey in view.php url
Can anyone view the number of submissions for an assignment? Just wondering whether this is an issue in circumstances where separate groups are in use.
adminlib.php
Uses deprecated textlib_get_instance
assignment_plugin_manager::delete_plugin missing global
backup/moodle2/backup_assign_activity_task.class.php
backup_assign_activity_task::encode_content_links inline comments from the choice module.
The other thing to mention is that there have been a couple of changes in the past month that impact or will require development changes for the assign module.
There is a new capability that should be implemented mod/assign:addinstance that needs to be implement. All other core modules have been updated now, it is just a case of defining the capability and the string required by it. See MDL-19125
We've started the process of increasing the developer debug to include E_STRICT. This has been done because many of the E_STRICT notifications in PHP <= 5.3 have become notices or warnings in PHP 5.4. Included in this list is ensuring overriden function definitions are identical to the parents, proper reference handling, and proper method access.
It would be a good idea to enable E_STRICT notification in PHP/Moodle and test the module as this will quickly highlight any areas requiring attention (You can set $CFG->debug = E_ALL | E_STRICT in your config.php). See MDL-12730
Thats as far as I've managed to get today.
Really at this point I hope I have picked up on many of the more important things and perhaps its time to start thinking about when this can land.
However before that happens it would be great to have it meet the coding style. Again those two tools, Tims and Marinas will be a big help for you there.
Cheers
Sam

I am looking at the renderer issues generally - but one comment about the use of the ob_ functions for rendering moodleforms is that this was taken directly from the workshop module. It would be even better if this was built into moodleforms.

Damyon Wiese
added a comment - 02/Apr/12 8:50 AM Hi Sam
I am looking at the renderer issues generally - but one comment about the use of the ob_ functions for rendering moodleforms is that this was taken directly from the workshop module. It would be even better if this was built into moodleforms.
Damyon

No probs about the ob_start/ob_end, having had a bit of a think about it I can't really see a problem with it.
It would be great if moodleforms handled the buffering of its output for us, unfortunately I don't know what the chances of that happening any time soon are. Although certainly mforms is a pretty hot topic, it is rapidly reaching the limit of its potential and in situations like this becoming a burden.
Fingers crossed sometime in the near future it gets a bit of love and care to address issues like this.

Thanks for looking into the renderers, please do let me know if there is anything I can help with.

Sam Hemelryk
added a comment - 02/Apr/12 10:50 AM Hi Damyon,
No probs about the ob_start/ob_end, having had a bit of a think about it I can't really see a problem with it.
It would be great if moodleforms handled the buffering of its output for us, unfortunately I don't know what the chances of that happening any time soon are. Although certainly mforms is a pretty hot topic, it is rapidly reaching the limit of its potential and in situations like this becoming a burden.
Fingers crossed sometime in the near future it gets a bit of love and care to address issues like this.
Thanks for looking into the renderers, please do let me know if there is anything I can help with.
Cheers
Sam

How close are we to getting an "ok for core" on this from your perspective? We desperately want this to make it to 2.3 core, and we've done our best to give you time to review this over the past month - just worried that we're still getting new feedback on the code which needs to be incorporated. Will be raising this will Martin this afternoon but thought I'd ask here as well.

Mark Drechsler
added a comment - 02/Apr/12 11:00 AM Hey Sam,
How close are we to getting an "ok for core" on this from your perspective? We desperately want this to make it to 2.3 core, and we've done our best to give you time to review this over the past month - just worried that we're still getting new feedback on the code which needs to be incorporated. Will be raising this will Martin this afternoon but thought I'd ask here as well.
Thanks in advance,
Mark.

I am very confident that this will land for 2.3.
While I am still turning things up as part of my reviews that things I am noting are getting significantly smaller in scale. Really the only thing I noted that would represent significant changes from the last review was the renderers. Other things such as unused variables, use of deprecated functions etc are very small minor chages.
Certainly I think that is a good sign, only one significant and lots of minor means we've likely got through all of the major road blocks before integration and are now faced with just polishing the code, cleaning it up, and preempting any future bugs.
I look forward to seeing the next revision and will be reviewing it with an eye to it getting put up for integration.

In order to be put up for integration really we need to try to make sure that we are free of any database design issues, bugs within API's and required callbacks, and major security issues as they are the hardest to change.
However having listened in on the meeting today I am happy for coding style tasks to be put off until after this has been integrated provided they are addressed before the release of 2.3.
If nothing serious pops up in the next review then I will start talking to Eloy about getting it integrated as he will be the final eye to make sure I have not missed anything mission critical.

Once it has been integrated I will go back over all of the reviews I've made as well as the goals of the new assignment module just to make sure we havn't missed anything, and of course creating issues for anything we may have missed that we still need to look at.

Sam Hemelryk
added a comment - 02/Apr/12 1:31 PM Hi Mark,
I am very confident that this will land for 2.3.
While I am still turning things up as part of my reviews that things I am noting are getting significantly smaller in scale. Really the only thing I noted that would represent significant changes from the last review was the renderers. Other things such as unused variables, use of deprecated functions etc are very small minor chages.
Certainly I think that is a good sign, only one significant and lots of minor means we've likely got through all of the major road blocks before integration and are now faced with just polishing the code, cleaning it up, and preempting any future bugs.
I look forward to seeing the next revision and will be reviewing it with an eye to it getting put up for integration.
In order to be put up for integration really we need to try to make sure that we are free of any database design issues, bugs within API's and required callbacks, and major security issues as they are the hardest to change.
However having listened in on the meeting today I am happy for coding style tasks to be put off until after this has been integrated provided they are addressed before the release of 2.3.
If nothing serious pops up in the next review then I will start talking to Eloy about getting it integrated as he will be the final eye to make sure I have not missed anything mission critical.
Once it has been integrated I will go back over all of the reviews I've made as well as the goals of the new assignment module just to make sure we havn't missed anything, and of course creating issues for anything we may have missed that we still need to look at.
Cheers
Sam

One other thing to flag now is that there are database changes between this base set of work and the extra features that I have sitting on other branches - e.g. team assignment adds a groupid to the assign_grades table. I'm not sure if that affects anything with the integration process or not.

Damyon Wiese
added a comment - 02/Apr/12 1:48 PM Thanks Sam,
One other thing to flag now is that there are database changes between this base set of work and the extra features that I have sitting on other branches - e.g. team assignment adds a groupid to the assign_grades table. I'm not sure if that affects anything with the integration process or not.

Mark Drechsler
added a comment - 02/Apr/12 2:10 PM Cool,
Thanks Sam - and thanks for the work you've done on this as well. Hopefully the next project will be smoother now that we know what we're doing a little more!
Mark.

Damyon Wiese
added a comment - 03/Apr/12 11:38 AM Hi Sam, Raymond went through and made all of the small changes suggested and I have updated the renderer - this is now ready for review again.
Thanks - Damyon

I'm starting this review now. I am figuring it will take me at least a day as I will be going over everything again from start to finish.
I'll also start having a good play with it as part of this round, and check out the demo environment you have to see what these upcoming features are going to look like.

Sam Hemelryk
added a comment - 04/Apr/12 9:26 AM Hi guys,
I'm starting this review now. I am figuring it will take me at least a day as I will be going over everything again from start to finish.
I'll also start having a good play with it as part of this round, and check out the demo environment you have to see what these upcoming features are going to look like.
Cheers
Sam

Awesome thanks Damyon, I am currently going through things at the moment and the grading table was one of the things I made note of as needing some work after integration in regards to the UI and usability.

Let me know when you have made the other updates.

Also worth nothing while going through this time I've made a couple of commits against my review branch that tidy up minor areas.
I'll push them up to my github shortly to share them with you and detail each commit and what it does (so that you can pick and choose whether you use any of them).

Sam Hemelryk
added a comment - 05/Apr/12 11:37 AM Awesome thanks Damyon, I am currently going through things at the moment and the grading table was one of the things I made note of as needing some work after integration in regards to the UI and usability.
Let me know when you have made the other updates.
Also worth nothing while going through this time I've made a couple of commits against my review branch that tidy up minor areas.
I'll push them up to my github shortly to share them with you and detail each commit and what it does (so that you can pick and choose whether you use any of them).
Cheers
Sam

Thanks for the offer at the moment but its probably best to keep me separate.
I'll leave you to have a look through the commits I've made and cherry-pick what you would like to keep.
From there you can do what ever you want with them, squash them into a single commit, or what ever.
That way I will never get in your way with my commits as I haven't seen the code for the new features and I'd hate to get in the way with that causing conflicts and the like.

Just as a heads up I think I've just found a big change that will need to be discussed and quite likely acted upon.
I'll include it in my next review but to give you time to think about it here it is (guessing you have a public holiday tomorrow as well?

When serving files associated with a subplugin (through pluginfile.php) they should be served through the subplugin that they are associated. In this way assignfeedback_file and assignsubmission_file should be be reponsible for handling the files that are used through them.
This is significantly different from what is being gone at the moment which is simply serving them through the modules assign_pluginfile. It unfortunately will affect all file handling code there is in this module + subplugins currently. BUT it will be technically correct after this change, mean that the subplugins can be safetly removed if desired, and will provide a much better example of handling files through the subplugins.
This could be fixed after integration however doing so would potentially require upgrade code to move files into the new locations, restore code to deal with the old location perhaps, and also potentially temporary code in assing_pluginfile to redirect to the new locaiton for any old URLs. I think it would be easier to address this now. I imagine it is likely it will impact on the other improvements you've created so I would be keen to hear about what think.
This will also allow you to handle things like files within editors for the likes of feedback comments much more easily in the future.

Sam Hemelryk
added a comment - 05/Apr/12 12:01 PM - edited Hi Damyon,
Thanks for the offer at the moment but its probably best to keep me separate.
I'll leave you to have a look through the commits I've made and cherry-pick what you would like to keep.
From there you can do what ever you want with them, squash them into a single commit, or what ever.
That way I will never get in your way with my commits as I haven't seen the code for the new features and I'd hate to get in the way with that causing conflicts and the like.
Just as a heads up I think I've just found a big change that will need to be discussed and quite likely acted upon.
I'll include it in my next review but to give you time to think about it here it is (guessing you have a public holiday tomorrow as well?
When serving files associated with a subplugin (through pluginfile.php) they should be served through the subplugin that they are associated. In this way assignfeedback_file and assignsubmission_file should be be reponsible for handling the files that are used through them.
This is significantly different from what is being gone at the moment which is simply serving them through the modules assign_pluginfile. It unfortunately will affect all file handling code there is in this module + subplugins currently. BUT it will be technically correct after this change, mean that the subplugins can be safetly removed if desired, and will provide a much better example of handling files through the subplugins.
This could be fixed after integration however doing so would potentially require upgrade code to move files into the new locations, restore code to deal with the old location perhaps, and also potentially temporary code in assing_pluginfile to redirect to the new locaiton for any old URLs. I think it would be easier to address this now. I imagine it is likely it will impact on the other improvements you've created so I would be keen to hear about what think.
This will also allow you to handle things like files within editors for the likes of feedback comments much more easily in the future.
Cheers
Sam

With the file handling - I tried to do it this way but could not due to limitations in pluginfile - the problem is that for sub plugins - it does not consider the subtype and the plugin name - only the name - so if I have a plugin assignsubmission_file and assignfeedback_file (which I do) they conflict. This is a nasty bug and I do not know what the impact would be for upgrades etc if it was fixed - so I avoided it.

Damyon Wiese
added a comment - 05/Apr/12 12:39 PM Hi Sam,
With the file handling - I tried to do it this way but could not due to limitations in pluginfile - the problem is that for sub plugins - it does not consider the subtype and the plugin name - only the name - so if I have a plugin assignsubmission_file and assignfeedback_file (which I do) they conflict. This is a nasty bug and I do not know what the impact would be for upgrades etc if it was fixed - so I avoided it.

That would throw a spanner in the works!
I will have a look at that and open a bug to fix it if I can replicate it.
Certainly for the time being if it can't be done then the current approach will be fine and we will just have to create an issue to address it in the future when we've fixed subplugin issues in serving files.

I've just pushed up a set of fixes to a branch on my github repo. My appologies undoutbedly it is behind your current branch however you should still be able to cherry-pick commits fine.
To get the repo: git fetch git://github.com/samhemelryk/moodle.git mod_assign
Commits (in the order they were made):

Implemented the assign_get_extra_capabilities callback (recommened to implement this for every module where required)

Again totally up to you which you use however please do note each commit addresses an issue that would have to be addressed at some point.

I've just about finished a complete review of the new module and will move onto the admin tool after that.
Hopefully will have something to you in the very near future about that. Still lots to do unfortunately but we can look at what can be done after integration. So far the only big thing is the aforementioned file handling issue.

Sam Hemelryk
added a comment - 05/Apr/12 1:14 PM Hi Damyon,
That would throw a spanner in the works!
I will have a look at that and open a bug to fix it if I can replicate it.
Certainly for the time being if it can't be done then the current approach will be fine and we will just have to create an issue to address it in the future when we've fixed subplugin issues in serving files.
I've just pushed up a set of fixes to a branch on my github repo. My appologies undoutbedly it is behind your current branch however you should still be able to cherry-pick commits fine.
To get the repo: git fetch git://github.com/samhemelryk/moodle.git mod_assign
Commits (in the order they were made):
Commit 4c2e068 ( https://github.com/samhemelryk/moodle/commit/4c2e068 )
view.php Fixed the $PAGE->set_url call as it was missing the id param.
Commit 819f882 ( https://github.com/samhemelryk/moodle/commit/819f882 )
index.php Passed require_login the course object rather than just its id. This saves a database query.
index.php removed unused variables $coursecontext, and $sections
index.php converted course view link if there are not assignments to a moodle_url object as it is clearer thank ../../course
index.php removed sesskey from view URL
Commit 41aaee3 ( https://github.com/samhemelryk/moodle/commit/41aaee3 )
db/access.php Added mod/assign:addinstance capability definition (new requirement for all modules)
Commit 22deea2 ( https://github.com/samhemelryk/moodle/commit/22deea2 )
Renamed pix/grade_feedback.gif to pix/gradefeedback.gif as per coding style filename requirements
Refactored grading_table.php to reflect the above change
Commit c6fcca8 ( https://github.com/samhemelryk/moodle/commit/c6fcca8 )
Converted relative require_once calls to be absolute (using $CFG->dirroot) in admin_manage_plugins.php, feedback_plugin.php, locallib.php
Commit 90a4a9e ( https://github.com/samhemelryk/moodle/commit/90a4a9e )
mod_form.php Fixed deprecated call-time pass-by-reference errors
locallib.php Fixed deprecated call-time pass-by-reference errors
Commit 9c34e51 ( https://github.com/samhemelryk/moodle/commit/9c34e51 )
Fixed incompatible method definitions leading to strict warnings (all due to unconsistent type hinting). Affected files: assignment_plugin.php, feedback/comments/lib.php, grading_table.php, submission/comments/lib.php, submission/file/lib.php
Commit 36302ad ( https://github.com/samhemelryk/moodle/commit/36302ad )
feedback/comments/lib.php Removed unused private property $instance
Commit 99b67b5 ( https://github.com/samhemelryk/moodle/commit/99b67b5 )
feedback/comments/lib.php Added context option to format_text calls that were missing it.
Commit 6ce7059 ( https://github.com/samhemelryk/moodle/commit/6ce7059 )
feedback/comments/lib.php Removed unused variables from assignment_feedback_comments::get_form_elements
Commit c63238e ( https://github.com/samhemelryk/moodle/commit/c63238e )
feedback/comments/* Fixed up phpdocs as per coding style (gives you an idea what is required for the rest of the subplugins)
Commit a2099a9 ( https://github.com/samhemelryk/moodle/commit/a2099a9 )
Implemented the assign_get_extra_capabilities callback (recommened to implement this for every module where required)
Again totally up to you which you use however please do note each commit addresses an issue that would have to be addressed at some point.
I've just about finished a complete review of the new module and will move onto the admin tool after that.
Hopefully will have something to you in the very near future about that. Still lots to do unfortunately but we can look at what can be done after integration. So far the only big thing is the aforementioned file handling issue.
Cheers
Sam

I looked at the file handling again and I think when we renamed the plugins to assignsubmission and assignfeedback we resolved the issues with the pluginfile.php - so now I can implement that properly - this will mean I will have to do some rework to the plugin structure - ie I will have to move the class into a locallib.php and just have the callbacks in lib. I should get through this today and will let you know when it's done.

Damyon Wiese
added a comment - 10/Apr/12 8:50 AM Hi Sam,
I looked at the file handling again and I think when we renamed the plugins to assignsubmission and assignfeedback we resolved the issues with the pluginfile.php - so now I can implement that properly - this will mean I will have to do some rework to the plugin structure - ie I will have to move the class into a locallib.php and just have the callbacks in lib. I should get through this today and will let you know when it's done.
Damyon

Damyon Wiese
added a comment - 10/Apr/12 3:30 PM Hi Sam,
Those changes are all finished and we have done a bit of testing and found and fixed a few related to the changes. This is good for you to look at again now.
Damyon

Sam Hemelryk
added a comment - 11/Apr/12 7:31 AM Hi Damyon,
You guys are legends, I am just going through the notes I had made in the past couple of days while looking over the assign module again and I see you've fixed several of them already, awesome!
The file handling changes look real good, there is a minor security issue there at the moment but otherwise spot on! (I'll post my review in the next half hour with details)
The review I post shortly will be in regards to the module itself again, I will spend this afternoon looking at the upgrade tool.
Cheers
Sam

Changes I think are required before this gets integrated

Rename the following files removing the underscores as required by the coding style. (Note that there are exceptions to this such as forms and files that must contain underscores like portforlio_callback.php, this is a guide for new files)

Security issues in assignfeedback_file_pluginfile and assignsubmission_file_pluginfile: You must validate that the item id belongs to the assignment module the context relates to. Essentially you must verfiy the links between all the different components in play. Presently you would be able to abuse the code to retrieve a file belonging to any assign module providing you have access to a single module (of course you would need to know details about the file you were after so its not a big security risk)

Review the display of the assignment and check it is formatted correctly. I enabled the multilang filter for content and heading and then used the string "<span lang="EN" class="multilang">Pass</span><span lang="NZ" class="multilang">Fail</span>" to test and confirmed that there are several places not formatting it properly yet.

I really think the components in renderable.php need to be renamed to include something reflecting the fact that they belong to the assign module. Even just assign_ would be great (of course assign_files and assignment_header are both fine already ) You could do assignfeedback_status and assigngrading_form for instance as well. It is much easier to do this now and they do run the risk of collision because of the naming at the moment using common terms in Moodle.

integration with the gradebook needs to be looked at (sorry I am not overly knowledgeable in this area) and we need to determine what is any of the gradebook callbacks need to be implemented. Perhaps this can happen after integration however I don't know the implications of implementing gradebook callbacks after integration and what would be required so it goes here. (Looks like we need assign_grade_item_update and assign_update_grades).

Changes required before 2.3 either before or after integration.

If you do plan to make any/all of these changes after integration please create an issue to ensure they are not forgotten (this wil be required before it is integrated).

Review whether grade_form.php/gradeform.php needs to include HTML/QuickForm/input.php. If it does then this is an bug with advanced grading. If not then the require_once can be removed.

When creating a new assign module if a course max upload has not being set (it is using site limit) then file size selector shows "Course upload limit (0 bytes)" Which is very confusing.

When viewing an assignment with submissions as a teacher you are shown a "Grade assignment" button, however it doesn't take you to a grading screen it takes you to table to review submissions. I think the button text should be changed to reflect that.

In regards to the submission review table I think it is presently unclear and needs to be looked at both in regards to usability and accessibility. Things I personally think need to be reworked include the column order, the column headings, and the links to grade and lock a submission.

There is a performance win to be made in better handling feedback comments. A quick debug counter and I see that when viewing a locked+graded submission as a student assignment_feedback_comments::get_feedback_comments is called 4 times (leading to 4 database calls for the exact same content). As a teacher viewing the submission review table it leads to 2 database queries for every single assignment submission. I havn't required this to be done before integration because the query is at least a simple one so will be handled by query caching for those running DB that support it.

It appears that mod_assign_grading_actions_form (and grading_actions_form) would be a straight forward conversion to a single_select component. If so I think this would be well worth doing (unless you have future plans for that form).

assign_get_file_areas and mod_assign_get_file_info callbacks needs to be implemented in lib.php for module file browsing. Something that calls off to subplugins as well to give them the opertunity. How this is all handled will be determined by what happens with subplugin file handling.

assign_get_recent_mod_activity and assign_print_recent_mod_activity callbacks should be implement to enable recent activity support

assign_page_type_list callback should be implemented to control integration with block system and where they can be added.

assign_scale_used and assign_scale_used_anywhere callbacks should be implemented to enable proper integration with scales.

assign_user_complete and assign_user_complete callbacks should be implemented to enable interaction with the outline report.

Plagiarism interaction needs to be made conditional on plagiarism being enabled ($CFG->enableplagiarism) and inclusion of plagiarismlib.php changed to happen within each condition rather than in locallib.php.

Coding style clean up - MOST IMPORTANTLY PHPDOCS

Ideas for the future

I can turn these into issues after integration if you like.

When viewing an assignment without any submissions I think the "Grade assignment" button should be hidden.

When viewing a graded submission as a student I can click on the icon to view all the complete feedback comment. Two things here: One that icon needs to be improved for accessibility and two I think it would be great if the full feedback comment was loaded where the truncated comment is shown rather than taking you to another page.

OK so all up I would say this is looking really good now, 5 points I think need to be addressed before integration and several that can be addressed after integration but before 2.3 release.
I do want Martin/Eloy to have a look over that list in case they feel there is something in that after integration pile that should be done before integration.
If you could please have a look over that list and let me know if there are any issues there that are of concern as well that would be most appreciated.

There is still much that can be tidied up, optimised etc but I really think this module is looking good now.

Sam Hemelryk
added a comment - 11/Apr/12 7:50 AM Ok here it is:
Changes I think are required before this gets integrated
Rename the following files removing the underscores as required by the coding style. (Note that there are exceptions to this such as forms and files that must contain underscores like portforlio_callback.php, this is a guide for new files)
grading_table.php ==> gradingtable.php (or gradingtablelib.php)
admin_manage_plugins.php ==> adminmanageplugins.php
assignment_plugin.php ==> assignmentplugin.php
feedback_plugin.php ==> feedbackplugin.php
grade_form.php ==> gradeform.php
grading_actions_form.php ==> gradingactions_form.php (or gradingactionsform.php)
grading_options_forms.php ==> gradingoptions_form.php (or gradingoptionsform.php)
submission_plugin.php ==> submissionplugin.php
Security issues in assignfeedback_file_pluginfile and assignsubmission_file_pluginfile: You must validate that the item id belongs to the assignment module the context relates to. Essentially you must verfiy the links between all the different components in play. Presently you would be able to abuse the code to retrieve a file belonging to any assign module providing you have access to a single module (of course you would need to know details about the file you were after so its not a big security risk)
Review the display of the assignment and check it is formatted correctly. I enabled the multilang filter for content and heading and then used the string "<span lang="EN" class="multilang">Pass</span><span lang="NZ" class="multilang">Fail</span>" to test and confirmed that there are several places not formatting it properly yet.
I really think the components in renderable.php need to be renamed to include something reflecting the fact that they belong to the assign module. Even just assign_ would be great (of course assign_files and assignment_header are both fine already ) You could do assignfeedback_status and assigngrading_form for instance as well. It is much easier to do this now and they do run the risk of collision because of the naming at the moment using common terms in Moodle.
integration with the gradebook needs to be looked at (sorry I am not overly knowledgeable in this area) and we need to determine what is any of the gradebook callbacks need to be implemented. Perhaps this can happen after integration however I don't know the implications of implementing gradebook callbacks after integration and what would be required so it goes here. (Looks like we need assign_grade_item_update and assign_update_grades).
Changes required before 2.3 either before or after integration.
If you do plan to make any/all of these changes after integration please create an issue to ensure they are not forgotten (this wil be required before it is integrated).
Review whether grade_form.php/gradeform.php needs to include HTML/QuickForm/input.php. If it does then this is an bug with advanced grading. If not then the require_once can be removed.
When creating a new assign module if a course max upload has not being set (it is using site limit) then file size selector shows "Course upload limit (0 bytes)" Which is very confusing.
When viewing an assignment with submissions as a teacher you are shown a "Grade assignment" button, however it doesn't take you to a grading screen it takes you to table to review submissions. I think the button text should be changed to reflect that.
In regards to the submission review table I think it is presently unclear and needs to be looked at both in regards to usability and accessibility. Things I personally think need to be reworked include the column order, the column headings, and the links to grade and lock a submission.
There is a performance win to be made in better handling feedback comments. A quick debug counter and I see that when viewing a locked+graded submission as a student assignment_feedback_comments::get_feedback_comments is called 4 times (leading to 4 database calls for the exact same content). As a teacher viewing the submission review table it leads to 2 database queries for every single assignment submission. I havn't required this to be done before integration because the query is at least a simple one so will be handled by query caching for those running DB that support it.
It appears that mod_assign_grading_actions_form (and grading_actions_form) would be a straight forward conversion to a single_select component. If so I think this would be well worth doing (unless you have future plans for that form).
assign_get_file_areas and mod_assign_get_file_info callbacks needs to be implemented in lib.php for module file browsing. Something that calls off to subplugins as well to give them the opertunity. How this is all handled will be determined by what happens with subplugin file handling.
assign_get_recent_mod_activity and assign_print_recent_mod_activity callbacks should be implement to enable recent activity support
assign_page_type_list callback should be implemented to control integration with block system and where they can be added.
assign_scale_used and assign_scale_used_anywhere callbacks should be implemented to enable proper integration with scales.
assign_user_complete and assign_user_complete callbacks should be implemented to enable interaction with the outline report.
Plagiarism interaction needs to be made conditional on plagiarism being enabled ($CFG->enableplagiarism) and inclusion of plagiarismlib.php changed to happen within each condition rather than in locallib.php.
Coding style clean up - MOST IMPORTANTLY PHPDOCS
Ideas for the future
I can turn these into issues after integration if you like.
When viewing an assignment without any submissions I think the "Grade assignment" button should be hidden.
When viewing a graded submission as a student I can click on the icon to view all the complete feedback comment. Two things here: One that icon needs to be improved for accessibility and two I think it would be great if the full feedback comment was loaded where the truncated comment is shown rather than taking you to another page.
OK so all up I would say this is looking really good now, 5 points I think need to be addressed before integration and several that can be addressed after integration but before 2.3 release.
I do want Martin/Eloy to have a look over that list in case they feel there is something in that after integration pile that should be done before integration.
If you could please have a look over that list and let me know if there are any issues there that are of concern as well that would be most appreciated.
There is still much that can be tidied up, optimised etc but I really think this module is looking good now.
Excellent work guys!
Cheers
Sam

Comments on the first 5 issues listed above that are required for integration:
1. Raymond has done this one.
2. I have added an extra check to make sure the item id belongs to the assignment context passed in
3. Raymond has been looking at this one and is writing you an update now.
4. Done - they all start with assign_.
5. I have added the callbacks and tested them manually - I have no idea of how to trigger them from the gradebook though.

Damyon Wiese
added a comment - 11/Apr/12 12:45 PM Hi Sam,
Comments on the first 5 issues listed above that are required for integration:
1. Raymond has done this one.
2. I have added an extra check to make sure the item id belongs to the assignment context passed in
3. Raymond has been looking at this one and is writing you an update now.
4. Done - they all start with assign_.
5. I have added the callbacks and tested them manually - I have no idea of how to trigger them from the gradebook though.
These changes have been pushed.
Cheers
Damyon

Hi Sam,
Thanks for your feedback.
I would like quickly to share with you about what I did when trying to fix issue no. 3 in 'Changes I think are required before this gets integrated'
"3. Review the display of the assignment and check it is formatted correctly. I enabled the multilang filter for content and heading and then used the string "<span lang="EN" class="multilang">Pass</span><span lang="NZ" class="multilang">Fail</span>" to test and confirmed that there are several places not formatting it properly yet."

I am not entirely sure if I did it right on trying to reproduce what you did for no 3. What I did, I activated the multi lang content (and headings) filter and used your above code snippet and put it in all text boxes / text editors to try to find out some poorly formatted places and all of the results have been pass. Thought to double check this with you as I don't have much experience of the multilang filter and if I missed something/ didn't do when reviewing and checking the assignment format?. Thanks in advance and have an awesome day

Raymond Antonio
added a comment - 11/Apr/12 12:50 PM Hi Sam,
Thanks for your feedback.
I would like quickly to share with you about what I did when trying to fix issue no. 3 in 'Changes I think are required before this gets integrated'
"3. Review the display of the assignment and check it is formatted correctly. I enabled the multilang filter for content and heading and then used the string "<span lang="EN" class="multilang">Pass</span><span lang="NZ" class="multilang">Fail</span>" to test and confirmed that there are several places not formatting it properly yet."
I am not entirely sure if I did it right on trying to reproduce what you did for no 3. What I did, I activated the multi lang content (and headings) filter and used your above code snippet and put it in all text boxes / text editors to try to find out some poorly formatted places and all of the results have been pass. Thought to double check this with you as I don't have much experience of the multilang filter and if I missed something/ didn't do when reviewing and checking the assignment format?. Thanks in advance and have an awesome day
Cheers,
Raymond

As for no.1 "Review whether grade_form.php/gradeform.php needs to include HTML/QuickForm/input.php'. If it does then this is an bug with advanced grading. If not then the require_once can be removed." in Changes required before 2.3 either before or after integration. Yes Advance grading needs to have HTML/QuickForm/input.php in gradeform.php and it can not be removed from it. so yes It is a bug with advanced grading.

Raymond Antonio
added a comment - 11/Apr/12 1:53 PM Hi Sam,
As for no.1 "Review whether grade_form.php/gradeform.php needs to include HTML/QuickForm/input.php'. If it does then this is an bug with advanced grading. If not then the require_once can be removed." in Changes required before 2.3 either before or after integration. Yes Advance grading needs to have HTML/QuickForm/input.php in gradeform.php and it can not be removed from it. so yes It is a bug with advanced grading.
Cheers,
Raymond

It sounds like you have done everything correct in regards to the multilang filter.
I've just retested now and can confirm that I am seeing "PassFail" on several view.php pages. It is the first heading within the main content area.
I can also see it when I visit index.php for the course.
It is happening because the assignments name isn't being passed through format_string before it is being output.
Sorry I just saw that I missed saying assignment name!

No probs about the input requirement then, we can create a bug to fix advanced grading after this has been integrated and remove that include then.

Sam Hemelryk
added a comment - 11/Apr/12 2:05 PM Hi Raymond,
It sounds like you have done everything correct in regards to the multilang filter.
I've just retested now and can confirm that I am seeing "PassFail" on several view.php pages. It is the first heading within the main content area.
I can also see it when I visit index.php for the course.
It is happening because the assignments name isn't being passed through format_string before it is being output.
Sorry I just saw that I missed saying assignment name!
No probs about the input requirement then, we can create a bug to fix advanced grading after this has been integrated and remove that include then.
Cheers
Sam

Thanks guys for all the work on this. Wrt Sam's "Changes required before 2.3 either before or after integration" list above I'd like to see ALL of those done before integration. Mostly they look pretty quick to do.

Martin Dougiamas
added a comment - 12/Apr/12 10:27 AM Thanks guys for all the work on this. Wrt Sam's "Changes required before 2.3 either before or after integration" list above I'd like to see ALL of those done before integration. Mostly they look pretty quick to do.

Damyon Wiese
added a comment - 12/Apr/12 10:42 AM No problem - we are working through the list now.
With regard to 4 - I am happy to make changes to the way the grading table works but I need a bit more direction. It does at least work similarly to a lot of other tables in Moodle.

MD:
1) hitting "Save and show next" WITHOUT putting a grade in results in an error.

2) There's no help buttons on the assignment settings page. All those settings need help.

3) The only way to get to submissions is via the "Grade assignment" button on view.php. I suggest a link like "View/grade all submissions" in the settings menu, and also possibly renaming the button to match. I'd also propose a sub-menu for graders in the nav bar to make it easy to get straight there from anywhere else.

4) Student profile images are not shown in submissions list

5) The "Edit icons" are a little small and are wrapping unattractively.

6) In the submissions list, for online text, the summaries "124 words" could instead show a little of the text truncated so you get an idea, with "... (124 words)" after it

Martin Dougiamas
added a comment - 12/Apr/12 10:52 AM Some other UI things I noticed on the demo site:
MD:
1) hitting "Save and show next" WITHOUT putting a grade in results in an error.
2) There's no help buttons on the assignment settings page. All those settings need help.
3) The only way to get to submissions is via the "Grade assignment" button on view.php. I suggest a link like "View/grade all submissions" in the settings menu, and also possibly renaming the button to match. I'd also propose a sub-menu for graders in the nav bar to make it easy to get straight there from anywhere else.
4) Student profile images are not shown in submissions list
5) The "Edit icons" are a little small and are wrapping unattractively.
6) In the submissions list, for online text, the summaries "124 words" could instead show a little of the text truncated so you get an idea, with "... (124 words)" after it
7) Hitting "upload grades" takes you to http://modassign.netspot.com.au/mod/assign/view.php?id=298&action=uploadgrades . Hitting "Cancel" takes you to a blank page.
8) The template feedback file being defined site-wide surprised me ... but I guess it's OK.

With regards to 7. (assign_get_file_areas and mod_assign_get_file_info callbacks needs to be implemented in lib.php). I have made the changes and added support so they can be implemented by plugins - but I don't think it makes sense. The files are still not browsable because they don't exist independently of an itemid and so they can't be navigated to from the root. The assignment itself does not have any files attached so it might as well just return an empty list of file areas to browser and be done.

So when you load the file picker and try and navigate with server files nothing shows up. If it did allow browsing though - a marker would have a big listing of item ids for all students and then have access to all students submission files with no way to tell whose was whose. I just dont think this makes sense for the assignment at all (and I should strip it right back to returning an empty list of browsable areas).

Damyon Wiese
added a comment - 13/Apr/12 8:53 AM Hi Sam,
With regards to 7. (assign_get_file_areas and mod_assign_get_file_info callbacks needs to be implemented in lib.php). I have made the changes and added support so they can be implemented by plugins - but I don't think it makes sense. The files are still not browsable because they don't exist independently of an itemid and so they can't be navigated to from the root. The assignment itself does not have any files attached so it might as well just return an empty list of file areas to browser and be done.
Ie - the submission files layout looks like this (virtually)
assignsubmission_file/submission_files/77/.
assignsubmission_file/submission_files/77/submission.txt
assignsubmission_file/submission_files/78/.
assignsubmission_file/submission_files/78/sub4mission2.txt
assignsubmission_file/submission_files/78/4submission4.txt
assignsubmission_file/submission_files/79/.
assignsubmission_file/submission_files/79/my Files.docx.zip
assignsubmission_file/submission_files/79/related Maps.png
But there is no folder
assignsubmission_file/submission_files/.
So when you load the file picker and try and navigate with server files nothing shows up. If it did allow browsing though - a marker would have a big listing of item ids for all students and then have access to all students submission files with no way to tell whose was whose. I just dont think this makes sense for the assignment at all (and I should strip it right back to returning an empty list of browsable areas).
Damyon

Thanks for looking at the demo site. From your list of suggestions:
1) ("Save and show next" error) This is a merge bug in the demo site and does not happen with this code on master
2) (help buttons) The help buttons have not been merged with demo but are on master
3) (extra navigation to the grading page and renaming the button) This has been done
4) (Student profile images) There is a column for this - maybe you had it hidden?
5) (edit icons) I have changed these to a drop down list - but it's ugly. I'll do some more work on this over the weekend
6) (more text in online text summary) This is done
7) ( "upload grades" error) This is only on the demo site as it is an additional feature on top of master
8) The template feedback file being defined site-wide surprised me ... We have had similar feedback so we'll move it - but again this feature is not in master.

Damyon Wiese
added a comment - 13/Apr/12 3:36 PM Hi Martin,
Thanks for looking at the demo site. From your list of suggestions:
1) ("Save and show next" error) This is a merge bug in the demo site and does not happen with this code on master
2) (help buttons) The help buttons have not been merged with demo but are on master
3) (extra navigation to the grading page and renaming the button) This has been done
4) (Student profile images) There is a column for this - maybe you had it hidden?
5) (edit icons) I have changed these to a drop down list - but it's ugly. I'll do some more work on this over the weekend
6) (more text in online text summary) This is done
7) ( "upload grades" error) This is only on the demo site as it is an additional feature on top of master
8) The template feedback file being defined site-wide surprised me ... We have had similar feedback so we'll move it - but again this feature is not in master.
Damyon

1) (include HTML/QuickForm/input.php)
This is a bug with advanced grading - will leave the include in the code until this bug is fixed.
2) ("Course upload limit (0 bytes)")
Fixed
3) (Text of button Grade assignment)
Fixed
4) Usability and accessibility of grading table.
Started work here (icons replaced with drop down list) but needs more work
5) There is a performance win in feedback comments
Done
6) mod_assign_grading_actions_form -> url_select component.
We did this and it looked awful (didn't align with the grading options form) so we changed it back
7) assign_get_file_areas and mod_assign_get_file_info callbacks needs to be implemented in lib.php
Done but I think we should remove most of it as it's not useful - see comment above.
8) assign_get_recent_mod_activity and assign_print_recent_mod_activity callbacks
Done but we made the default off as we were asked to remove this specifically as noone was using it in the assignment module for privacy reasons.
9) assign_page_type_list callback
Done
10) assign_scale_used and assign_scale_used_anywhere callbacks
Done
11) assign_user_complete and assign_user_outline callbacks
Done
12) Plagiarism interaction needs to be made conditional on plagiarism being enabled
Done
13) Coding style clean up - MOST IMPORTANTLY PHPDOCS
No change here

Damyon Wiese
added a comment - 13/Apr/12 3:46 PM Hi Sam,
Just to go over your list again to make it clearer:
1) (include HTML/QuickForm/input.php)
This is a bug with advanced grading - will leave the include in the code until this bug is fixed.
2) ("Course upload limit (0 bytes)")
Fixed
3) (Text of button Grade assignment)
Fixed
4) Usability and accessibility of grading table.
Started work here (icons replaced with drop down list) but needs more work
5) There is a performance win in feedback comments
Done
6) mod_assign_grading_actions_form -> url_select component.
We did this and it looked awful (didn't align with the grading options form) so we changed it back
7) assign_get_file_areas and mod_assign_get_file_info callbacks needs to be implemented in lib.php
Done but I think we should remove most of it as it's not useful - see comment above.
8) assign_get_recent_mod_activity and assign_print_recent_mod_activity callbacks
Done but we made the default off as we were asked to remove this specifically as noone was using it in the assignment module for privacy reasons.
9) assign_page_type_list callback
Done
10) assign_scale_used and assign_scale_used_anywhere callbacks
Done
11) assign_user_complete and assign_user_outline callbacks
Done
12) Plagiarism interaction needs to be made conditional on plagiarism being enabled
Done
13) Coding style clean up - MOST IMPORTANTLY PHPDOCS
No change here

Damyon Wiese
added a comment - 16/Apr/12 3:25 PM Hi Sam,
I have pushed an update for the usability of the grading table and I think it's a bit better now (Uses a YUI menu button for the grading actions).
Regards, Damyon

This issue has my full and undivided attention today as I have been distracted with integration and other reviews for the past couple of days.
I've read over all of the changes you've made in response to my points, and Martin's (thanks for the feedback Martin!) and today I will be looking to the assignment conversion tool, and a quick review of any commits since I last looked.

Sam Hemelryk
added a comment - 17/Apr/12 5:25 AM Awesome thanks Damyon,
This issue has my full and undivided attention today as I have been distracted with integration and other reviews for the past couple of days.
I've read over all of the changes you've made in response to my points, and Martin's (thanks for the feedback Martin!) and today I will be looking to the assignment conversion tool, and a quick review of any commits since I last looked.
Cheers
Sam

Not sure if this UI is in the code being reviewed but in the last few days the Grading table UI seems to have changed to introduce an Actions drop down for the "Edit" option.

Before I'd updated the "Grade" option had been a single URL cell that could just be clicked. This is relatively minor issue but it know will annoy some of my users who already complain about too many clicks. Now 2 clicks and navigation to the item is necessary to perform what is the primary task on the page...

Could the original grade option remain and only additional options appear on a separate Actions menu?

Michael Hughes
added a comment - 17/Apr/12 5:10 PM - edited Not sure if this UI is in the code being reviewed but in the last few days the Grading table UI seems to have changed to introduce an Actions drop down for the "Edit" option.
Before I'd updated the "Grade" option had been a single URL cell that could just be clicked. This is relatively minor issue but it know will annoy some of my users who already complain about too many clicks. Now 2 clicks and navigation to the item is necessary to perform what is the primary task on the page...
Could the original grade option remain and only additional options appear on a separate Actions menu?

I've looked over the recent changes and I've been playing with the assignment upgrade tool.

A couple of things about the mod_assign plugin that have come about in the recent changes:

Use of YUI2 regretable but can be fixed later, I'm not sure that the drop down is the best approach and I see someone else has already noted that as well.

assign_print_recent_activity needs to be looked at:

get_fast_modinfo returns an object that is already being cached - no need to require it be by reference.

$modinfo->cms[$submission->cmid] is deprecated and $modinfo->get_cm($submission->cmid) shoud be used instead.

get_context_instance is deprecated.

$modinfo->groups is deprecated, there is now $modinfo->get_groups()

Getting this everywhere: Strict Standards: Declaration of assign_grading_table::col_fullname() should be compatible with that of flexible_table::col_fullname() in mod/assign/gradingtable.php on line 39

As a student in a submission I can upload an image and use it in the online text, however it is not formatted correctly when being viewed (@@PLUGINFILE@@ is still present) also broken for the teacher when grading it.

In regards to the assignment upgrade tool:

I've created a couple of commits to fix the following noted issues and pushed them to [repo] git://github.com/samhemelryk/moodle.git [branch] mod_assign

42a0170 Fixed up camel casing in strings

350a382 Coding style + phpdoc partial clean up

85afb3a Removed unneeded cap/login checks (they are made by admin_externalpage_setup)

There is a serious need for pagination in the lists of assignments that can be converted. This really needs to be done before integration as there are going to be sites out there with hundreds, and perhaps thousands of assignment instances.

Likewise there needs to be an upgrade all button/functionality before this gets integrated (I think Martin mentioned this in the meeting)

There needs to be a link back to the list of assignments requiring upgrade after successfully upgrading an assignment instance.

The whole conversion process needs to be looked at to make sure things arn't being missed out. During testing with an assignment with rubrics being used for grading I noted that it reverted to simple grading after being upgraded. Also note I didn't see anything about course/activity completion so I imagine that needs lookiung at as well (or at least testing, my appologies if it is being accounted for).

Got this when converting one assignment: Strict Standards: Creating default object from empty value in /var/www/development/mod/assign/submission/onlinetext/locallib.php on line 296

More testing required presently, I have created a test course which I can share with you if you like, however I'm not sure it is much good presently as it cannot be restored due to a bug in advanced grading. At the moment I have been testing by using a database and datadirectoy snapshot I took after creating everything. My test course has 10 old assignment modules of varying types and conditions that can be upgraded, all with submissions (not many but a few) and many of which have been graded. Advanced grading bug is MDL-32499 and contains a backup of the course if you want to check that out.

Sam Hemelryk
added a comment - 18/Apr/12 1:27 PM Hi guys,
I've looked over the recent changes and I've been playing with the assignment upgrade tool.
A couple of things about the mod_assign plugin that have come about in the recent changes:
Use of YUI2 regretable but can be fixed later, I'm not sure that the drop down is the best approach and I see someone else has already noted that as well.
assign_print_recent_activity needs to be looked at:
get_fast_modinfo returns an object that is already being cached - no need to require it be by reference.
$modinfo->cms [$submission->cmid] is deprecated and $modinfo->get_cm($submission->cmid) shoud be used instead.
get_context_instance is deprecated.
$modinfo->groups is deprecated, there is now $modinfo->get_groups()
Getting this everywhere: Strict Standards: Declaration of assign_grading_table::col_fullname() should be compatible with that of flexible_table::col_fullname() in mod/assign/gradingtable.php on line 39
As a student in a submission I can upload an image and use it in the online text, however it is not formatted correctly when being viewed (@@PLUGINFILE@@ is still present) also broken for the teacher when grading it.
In regards to the assignment upgrade tool:
I've created a couple of commits to fix the following noted issues and pushed them to [repo] git://github.com/samhemelryk/moodle.git [branch] mod_assign
42a0170 Fixed up camel casing in strings
350a382 Coding style + phpdoc partial clean up
85afb3a Removed unneeded cap/login checks (they are made by admin_externalpage_setup)
There is a serious need for pagination in the lists of assignments that can be converted. This really needs to be done before integration as there are going to be sites out there with hundreds, and perhaps thousands of assignment instances.
Likewise there needs to be an upgrade all button/functionality before this gets integrated (I think Martin mentioned this in the meeting)
There needs to be a link back to the list of assignments requiring upgrade after successfully upgrading an assignment instance.
The whole conversion process needs to be looked at to make sure things arn't being missed out. During testing with an assignment with rubrics being used for grading I noted that it reverted to simple grading after being upgraded. Also note I didn't see anything about course/activity completion so I imagine that needs lookiung at as well (or at least testing, my appologies if it is being accounted for).
Got this when converting one assignment: Strict Standards: Creating default object from empty value in /var/www/development/mod/assign/submission/onlinetext/locallib.php on line 296
More testing required presently, I have created a test course which I can share with you if you like, however I'm not sure it is much good presently as it cannot be restored due to a bug in advanced grading. At the moment I have been testing by using a database and datadirectoy snapshot I took after creating everything. My test course has 10 old assignment modules of varying types and conditions that can be upgraded, all with submissions (not many but a few) and many of which have been graded. Advanced grading bug is MDL-32499 and contains a backup of the course if you want to check that out.
Looks like there is a bit to go with the upgrade tool sorry.
Cheers
Sam

LENGTH attribute in install.xml for text fields has been removed. Those files need to be edited again with the XMLDB editor and saved so that they get updated (pretty sure it does it automatically for you).

On the same note it would be great if comments for tables and fields could be added to install.xml

One observation I made some time ago but have continually forgotten to mention was that this new module uses many strings that exist in the old assignment module. There should be a commit that uses AMOS syntax to copy identical strings between the old and new assignment module to save translators work.

Sam Hemelryk
added a comment - 19/Apr/12 6:44 AM A couple more very minor things:
LENGTH attribute in install.xml for text fields has been removed. Those files need to be edited again with the XMLDB editor and saved so that they get updated (pretty sure it does it automatically for you).
On the same note it would be great if comments for tables and fields could be added to install.xml
One observation I made some time ago but have continually forgotten to mention was that this new module uses many strings that exist in the old assignment module. There should be a commit that uses AMOS syntax to copy identical strings between the old and new assignment module to save translators work.

Hi Sam - on point 3 above - when will the AMOS commands be run - is it when the code is integrated? - I was assuming the code would be squashed into a single commit at that point and that might lose the commit message with the AMOS commands. Can you explain how this will work for me please?

Damyon Wiese
added a comment - 19/Apr/12 8:38 AM Hi Sam - on point 3 above - when will the AMOS commands be run - is it when the code is integrated? - I was assuming the code would be squashed into a single commit at that point and that might lose the commit message with the AMOS commands. Can you explain how this will work for me please?
Thanks - Damyon

Hi, I've been busy automating the Assignment acceptance tests here at HQ using the Netspot demo to obtain the locators for my assignment object repository.

I've just noticed that we have lost the ability to edit submission comments (originally called notes). Although I have modified the Acceptance Test a bit to accommodate the removal of old subtypes and make it more 2.3, the core acceptance criteria of the test have not changed. Our current acceptance test is:

DESCRIPTION:
A teacher can enable submission comments when file submissions is enabled

TEST PRE-REQUISITES:
An assignment exists with file submissions enabled and 'Allow submission comments' set to Yes.

TEST SCENARIO:
1. Login as a student and access the assignment.
2. Add a submission comment, then click the 'Save changes' button.
3. Edit the submission comment, then click the 'Save changes' button.
4. Check that the changes have been saved.

Step 3 fails!

To emphasise this loss of functionality, I have modelled the behaviour on the Netspot demo in a test if the user wants to edit the comment:
1. Login as a student and access the assignment.
2. Add a submission comment, then click the 'Save changes' button.
3. Verify that the comment cannot be edited.
4. Delete the comment.
5. Verify that the comment has been deleted.
6. Add a new submission comment that is different to the original comment (as opposed to editing the old one), then click the 'Save changes' button.
7. Verify that the comment is as entered in step 6.

Tim Barker
added a comment - 23/Apr/12 2:49 PM Hi, I've been busy automating the Assignment acceptance tests here at HQ using the Netspot demo to obtain the locators for my assignment object repository.
I've just noticed that we have lost the ability to edit submission comments (originally called notes). Although I have modified the Acceptance Test a bit to accommodate the removal of old subtypes and make it more 2.3, the core acceptance criteria of the test have not changed. Our current acceptance test is:
DESCRIPTION:
A teacher can enable submission comments when file submissions is enabled
TEST PRE-REQUISITES:
An assignment exists with file submissions enabled and 'Allow submission comments' set to Yes.
TEST SCENARIO:
1. Login as a student and access the assignment.
2. Add a submission comment, then click the 'Save changes' button.
3. Edit the submission comment, then click the 'Save changes' button.
4. Check that the changes have been saved.
Step 3 fails!
To emphasise this loss of functionality, I have modelled the behaviour on the Netspot demo in a test if the user wants to edit the comment:
1. Login as a student and access the assignment.
2. Add a submission comment, then click the 'Save changes' button.
3. Verify that the comment cannot be edited.
4. Delete the comment.
5. Verify that the comment has been deleted.
6. Add a new submission comment that is different to the original comment (as opposed to editing the old one), then click the 'Save changes' button.
7. Verify that the comment is as entered in step 6.
Will you put this functionality back?

You are correct that the comments API does not allow you to edit a submission comment, only delete it and create a new one - however changing this functionality is out of scope of the assignment module and would have to be filed against the comments api. I am not planning on making any changes to this.

Damyon Wiese
added a comment - 23/Apr/12 3:13 PM Hi Tim,
You are correct that the comments API does not allow you to edit a submission comment, only delete it and create a new one - however changing this functionality is out of scope of the assignment module and would have to be filed against the comments api. I am not planning on making any changes to this.
Regards, Damyon

David Mudrák
added a comment - 23/Apr/12 4:51 PM Hi Damyon. Re AMOS - see http://docs.moodle.org/dev/Commit_cheat_sheet#Include_AMOS_script_in_the_commit_if_needed Basically, AMOS tracks moodle.git after each weekly release. By the way, blindly squashing all the history into a single huge commit is usually discouraged. Instead, frequent commits containing logical chunks of changes and described in the commit message help to diagnose issues later using the git-blame command.

Hi, I'm automating this existing acceptance test for Assignments today, I've modified the test a bit, although not fully, to closely match the changes:

Test Description:
In an Offline activity assignment, a teacher can grade and give comments on an assignment completed offline

Test Pre-requisites:
This test requires an activity with File Submissions and Online Text disabled. i.e. a replication of the old off-line activity using the new single assignment type.

1. Login as a teacher and access the Off-line activity assignment.
2. Follow the 'View assignment grades and feedback' link and select a student to grade. (I know this has changed, it needs re-wording to take into account new pages, but essentially the workflow is the same)
3. Add a grade and comment then click the 'Save changes' button.
4. Check that the grade, comment and 'Last modified (Teacher)' date are correctly displayed for the assignment just graded and the link text in the status column has changed from 'Grade' to 'Update'.

I think I've found several bugs with offline assignments:

a) When submission comments are enabled for an offline assignment; the comments API doesn't appear anywhere.

Neither the teacher or student can enter any submission comments for Offline assignments even when submission comments are turned on.

b) When submission comments are disabled for , when the teacher navigates to the grading table, an ungraded offline assignment has a "Grade" link in the grading table "Status" column. When submission comments are enabled for offline assignments, the grading table displays a link of "No submission". This is wrong because there will never be any submissions.

I did some more exploratory testing and when I did a bit more digging into 2, I found that when a student accesses the assignment that has submission comments turned on also had access to an "Edit my submission" button. When I click on the "Edit my submission", the student has access to a plagiarism statement, checkbox and nothing else.

It looks like when submission comments are enabled for an offline activity the state of the assignment changes to expect a submission. It should not.

Tim Barker
added a comment - 24/Apr/12 1:28 PM - edited Hi, I'm automating this existing acceptance test for Assignments today, I've modified the test a bit, although not fully, to closely match the changes:
Test Description:
In an Offline activity assignment, a teacher can grade and give comments on an assignment completed offline
Test Pre-requisites:
This test requires an activity with File Submissions and Online Text disabled. i.e. a replication of the old off-line activity using the new single assignment type.
1. Login as a teacher and access the Off-line activity assignment.
2. Follow the 'View assignment grades and feedback' link and select a student to grade. (I know this has changed, it needs re-wording to take into account new pages, but essentially the workflow is the same)
3. Add a grade and comment then click the 'Save changes' button.
4. Check that the grade, comment and 'Last modified (Teacher)' date are correctly displayed for the assignment just graded and the link text in the status column has changed from 'Grade' to 'Update'.
I think I've found several bugs with offline assignments:
a) When submission comments are enabled for an offline assignment; the comments API doesn't appear anywhere.
Neither the teacher or student can enter any submission comments for Offline assignments even when submission comments are turned on.
b) When submission comments are disabled for , when the teacher navigates to the grading table, an ungraded offline assignment has a "Grade" link in the grading table "Status" column. When submission comments are enabled for offline assignments, the grading table displays a link of "No submission". This is wrong because there will never be any submissions.
I did some more exploratory testing and when I did a bit more digging into 2, I found that when a student accesses the assignment that has submission comments turned on also had access to an "Edit my submission" button. When I click on the "Edit my submission", the student has access to a plagiarism statement, checkbox and nothing else.
It looks like when submission comments are enabled for an offline activity the state of the assignment changes to expect a submission. It should not.

Damyon Wiese
added a comment - 26/Apr/12 4:19 PM Hi Martin,
Yes - I have been working on this this week - it took a while because I was trying to use the grading apis to do this but that proved impossible so I had to go back to manual database upgrades.
All the changes are done now except the AMOS commit.
Regards, Damyon

> Use of YUI2 regretable but can be fixed later, I'm not sure that the drop down is the best approach and I see someone else has already noted that as well.

This has been changed to YUI3 and I added an icon just for the grade function that is always visible so grading can be accessed with one click.

> assign_print_recent_activity needs to be looked at:
> get_fast_modinfo returns an object that is already being cached - no need to require it be by reference.
> $modinfo->cms[$submission->cmid] is deprecated and $modinfo->get_cm($submission->cmid) shoud be used instead.
> get_context_instance is deprecated.
> $modinfo->groups is deprecated, there is now $modinfo->get_groups()

These have all been fixed by Raymond

> Getting this everywhere: Strict Standards: Declaration of assign_grading_table::col_fullname() should be compatible with that of flexible_table::col_fullname() in mod/assign/gradingtable.php on line 39

Yes there was a change elsewhere recently that made Moodle start reporting these errors - we have now fixed them all.

> As a student in a submission I can upload an image and use it in the online text, however it is not formatted correctly when being viewed (@@PLUGINFILE@@ is still present) also broken for the teacher when grading it.

This is fixed.

> In regards to the assignment upgrade tool:
>
> I've created a couple of commits to fix the following noted issues and pushed them to [repo] git://github.com/samhemelryk/moodle.git [branch] mod_assign
> 42a0170 Fixed up camel casing in strings
> 350a382 Coding style + phpdoc partial clean up
> 85afb3a Removed unneeded cap/login checks (they are made by admin_externalpage_setup)

These have been merged

> There is a serious need for pagination in the lists of assignments that can be converted. This really needs to be done before integration as there are going to be sites out there with hundreds, and perhaps thousands of assignment instances.

This list is now paginated.

> Likewise there needs to be an upgrade all button/functionality before this gets integrated (I think Martin mentioned this in the meeting)

There is an upgrade all button

> There needs to be a link back to the list of assignments requiring upgrade after successfully upgrading an assignment instance.

Sorry - I missed this one but it is in the navigation bar

> The whole conversion process needs to be looked at to make sure things arn't being missed out. During testing with an assignment with rubrics being used for grading I noted that it reverted to simple grading after being upgraded. Also note I didn't see anything about course/activity completion so I imagine that needs lookiung at as well (or at least testing, my appologies if it is being accounted for).

Advanced grading and completion info is now upgraded

> Got this when converting one assignment: Strict Standards: Creating default object from empty value in /var/www/development/mod/assign/submission/onlinetext/locallib.php on line 296

This is fixed

> More testing required presently, I have created a test course which I can share with you if you like, however I'm not sure it is much good presently as it cannot be restored due to a bug in advanced grading. At the moment I have been testing by using a database and datadirectoy snapshot I took after creating everything. My test course has 10 old assignment modules of varying types and conditions that can be upgraded, all with submissions (not many but a few) and many of which have been graded. Advanced grading bug is MDL-32499 and contains a backup of the course if you want to check that out.

Damyon Wiese
added a comment - 26/Apr/12 4:25 PM Some more detailed information on what has been done:
> Use of YUI2 regretable but can be fixed later, I'm not sure that the drop down is the best approach and I see someone else has already noted that as well.
This has been changed to YUI3 and I added an icon just for the grade function that is always visible so grading can be accessed with one click.
> assign_print_recent_activity needs to be looked at:
> get_fast_modinfo returns an object that is already being cached - no need to require it be by reference.
> $modinfo->cms [$submission->cmid] is deprecated and $modinfo->get_cm($submission->cmid) shoud be used instead.
> get_context_instance is deprecated.
> $modinfo->groups is deprecated, there is now $modinfo->get_groups()
These have all been fixed by Raymond
> Getting this everywhere: Strict Standards: Declaration of assign_grading_table::col_fullname() should be compatible with that of flexible_table::col_fullname() in mod/assign/gradingtable.php on line 39
Yes there was a change elsewhere recently that made Moodle start reporting these errors - we have now fixed them all.
> As a student in a submission I can upload an image and use it in the online text, however it is not formatted correctly when being viewed (@@PLUGINFILE@@ is still present) also broken for the teacher when grading it.
This is fixed.
> In regards to the assignment upgrade tool:
>
> I've created a couple of commits to fix the following noted issues and pushed them to [repo] git://github.com/samhemelryk/moodle.git [branch] mod_assign
> 42a0170 Fixed up camel casing in strings
> 350a382 Coding style + phpdoc partial clean up
> 85afb3a Removed unneeded cap/login checks (they are made by admin_externalpage_setup)
These have been merged
> There is a serious need for pagination in the lists of assignments that can be converted. This really needs to be done before integration as there are going to be sites out there with hundreds, and perhaps thousands of assignment instances.
This list is now paginated.
> Likewise there needs to be an upgrade all button/functionality before this gets integrated (I think Martin mentioned this in the meeting)
There is an upgrade all button
> There needs to be a link back to the list of assignments requiring upgrade after successfully upgrading an assignment instance.
Sorry - I missed this one but it is in the navigation bar
> The whole conversion process needs to be looked at to make sure things arn't being missed out. During testing with an assignment with rubrics being used for grading I noted that it reverted to simple grading after being upgraded. Also note I didn't see anything about course/activity completion so I imagine that needs lookiung at as well (or at least testing, my appologies if it is being accounted for).
Advanced grading and completion info is now upgraded
> Got this when converting one assignment: Strict Standards: Creating default object from empty value in /var/www/development/mod/assign/submission/onlinetext/locallib.php on line 296
This is fixed
> More testing required presently, I have created a test course which I can share with you if you like, however I'm not sure it is much good presently as it cannot be restored due to a bug in advanced grading. At the moment I have been testing by using a database and datadirectoy snapshot I took after creating everything. My test course has 10 old assignment modules of varying types and conditions that can be upgraded, all with submissions (not many but a few) and many of which have been graded. Advanced grading bug is MDL-32499 and contains a backup of the course if you want to check that out.
This is worth retesting with the above fixes.
Regards, Damyon

> LENGTH attribute in install.xml for text fields has been removed. Those files need to be edited again with the XMLDB editor and saved so that they get updated (pretty sure it does it automatically for you).

Done
> On the same note it would be great if comments for tables and fields could be added to install.xml

Done

> One observation I made some time ago but have continually forgotten to mention was that this new module uses many strings that exist in the old assignment module. There should be a commit that uses AMOS syntax to copy identical strings between the old and new assignment module to save translators work.

Damyon Wiese
added a comment - 26/Apr/12 4:26 PM More....
> LENGTH attribute in install.xml for text fields has been removed. Those files need to be edited again with the XMLDB editor and saved so that they get updated (pretty sure it does it automatically for you).
Done
> On the same note it would be great if comments for tables and fields could be added to install.xml
Done
> One observation I made some time ago but have continually forgotten to mention was that this new module uses many strings that exist in the old assignment module. There should be a commit that uses AMOS syntax to copy identical strings between the old and new assignment module to save translators work.
Not done yet.
Regards, Damyon

Great thanks for the reply Damyon, I'll run through my testing today and/or over the weekend.
I've now made Eloy and Dan aware that this will likely be arriving to integration review in the next while so that they have time to read/think about it and prepare.

Sam Hemelryk
added a comment - 27/Apr/12 7:16 AM Great thanks for the reply Damyon, I'll run through my testing today and/or over the weekend.
I've now made Eloy and Dan aware that this will likely be arriving to integration review in the next while so that they have time to read/think about it and prepare.
Cheers
Sam

Tim Barker
added a comment - 28/Apr/12 2:12 PM Hi, I think I found another issue for you. When I graded an Online Text submission, the status of the submission in the grading table didn't update from "Submitted for grading" to anything else.
Previously to these changes to Assignment, grading an assignment would change the status from "Grade" to "Update" I was expecting to see something similar here.

Damyon Wiese
added a comment - 30/Apr/12 1:41 PM I just changed the above fix slightly as it was hiding important information (like whether the student clicked submit or not). It now show both the submission status and the grading status.
I am now just working on the AMOS commands - once that is done there is nothing outstanding from us.
Damyon

I've just been looking over this now, the code for both the module and the assignment upgrade tool looks good to me.
I've tested it now, everything appears to work fine, haha well everything you've written I've found a couple of bugs in grade book I'll report shortly.
There are only three things left to do:

Coding style: PHPdocs is a biggy, as is white space, I have end of line white space highlighting turned on in VIM and I see there is plenty of whitespace to clean up. You can do that from a regex with ease though. PHPdocs is the big one, several key things identified by Marina's code checker.

Put it up for integration.

Get it integrated.

No doubt there is going to be many more things identified in the future, really as long as there are no show stoppers identified by the integrator then I think this should go in.
We can easily report and fix minor issues, I suppose the big things are security, database correctness, and data preservation when upgrading.
This module deserves intense testing once in, and the sooner we land it the more time we will have to fix any issues.

I'll help out who ever is the integrator any way I can and sort out with them an integration plan (which will involve an AMOS commit) and fingers crossed this starts seeing some real momentum towards integration.

Sam Hemelryk
added a comment - 30/Apr/12 1:43 PM Awesome effort guys.
I've just been looking over this now, the code for both the module and the assignment upgrade tool looks good to me.
I've tested it now, everything appears to work fine, haha well everything you've written I've found a couple of bugs in grade book I'll report shortly.
There are only three things left to do:
Coding style: PHPdocs is a biggy, as is white space, I have end of line white space highlighting turned on in VIM and I see there is plenty of whitespace to clean up. You can do that from a regex with ease though. PHPdocs is the big one, several key things identified by Marina's code checker.
Put it up for integration.
Get it integrated.
No doubt there is going to be many more things identified in the future, really as long as there are no show stoppers identified by the integrator then I think this should go in.
We can easily report and fix minor issues, I suppose the big things are security, database correctness, and data preservation when upgrading.
This module deserves intense testing once in, and the sooner we land it the more time we will have to fix any issues.
I'll help out who ever is the integrator any way I can and sort out with them an integration plan (which will involve an AMOS commit) and fingers crossed this starts seeing some real momentum towards integration.
Excellent effort Damyon and Raymond, congratulations.
Cheers
Sam

Mark Drechsler
added a comment - 30/Apr/12 5:05 PM Fantastic effort Damo and Raymond, and thanks also to Sam and Martin for their guidance and support throughout the process. We'll know a lot more about what to look for next time

Damyon Wiese
added a comment - 30/Apr/12 11:06 PM Hi Sam,
I have just pushed a change that cleans up the php docs. Marinas code checker runs cleanly now.
I also pushed an update earlier today with all of the AMOS commands.
Damyon

Damyon Wiese
added a comment - 02/May/12 11:13 AM I just pushed some updates that renamed all the classes and tables, changed the SQL for the grading table and optimised the settings.php includes as suggested by Petr Škoda on MDL-26997 .
Damyon

I've just seem there are some E_STRICT warnings:
Strict standards: Declaration of assign_submission_comments::view_summary() should be compatible with that of assign_plugin::view_summary() in /Users/danp/git/moodle/mod/assign/locallib.php on line 272 Call Stack: 0.0001 640456 1.

Martin Dougiamas
added a comment - 02/May/12 11:55 AM 1. It seems the old assignment is not yet renamed to "Assignment (2.2)". I've made an issue for David to do this for 2.3, so we don't forget. MDL-32716
2. Also, we need to make sure the old assignment is turned OFF by default in new installs.
3. Also, what are we doing in the upgrade process to make sure everything is explained really well to admins? We need good strings in all languages.

Debug info: The data types ntext and nvarchar are incompatible in the equal to operator.
SELECT COUNT('x')
FROM md1_assign_submission
WHERE assignment = ? AND
status = ?
[array (
0 => '1',
1 => 'draft',
)]
Stack trace:
line 410 of /lib/dml/moodle_database.php: dml_read_exception thrown
line 260 of /lib/dml/mssql_native_moodle_database.php: call to moodle_database->query_end()
line 723 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->query_end()
line 753 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->get_recordset_sql()
line 1335 of /lib/dml/moodle_database.php: call to mssql_native_moodle_database->get_records_sql()
line 1410 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
line 1581 of /lib/dml/moodle_database.php: call to moodle_database->get_field_sql()
line 938 of /mod/assign/locallib.php: call to moodle_database->count_records_sql()
line 1901 of /mod/assign/locallib.php: call to assign->count_submissions_with_status()
line 379 of /mod/assign/locallib.php: call to assign->view_submission_page()
line 53 of /mod/assign/view.php: call to assign->view()

Dan Poltawski
added a comment - 02/May/12 12:00 PM I've just tried on mssql:
Debug info: The data types ntext and nvarchar are incompatible in the equal to operator.
SELECT COUNT('x')
FROM md1_assign_submission
WHERE assignment = ? AND
status = ?
[array (
0 => '1',
1 => 'draft',
)]
Stack trace:
line 410 of /lib/dml/moodle_database.php: dml_read_exception thrown
line 260 of /lib/dml/mssql_native_moodle_database.php: call to moodle_database->query_end()
line 723 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->query_end()
line 753 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->get_recordset_sql()
line 1335 of /lib/dml/moodle_database.php: call to mssql_native_moodle_database->get_records_sql()
line 1410 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
line 1581 of /lib/dml/moodle_database.php: call to moodle_database->get_field_sql()
line 938 of /mod/assign/locallib.php: call to moodle_database->count_records_sql()
line 1901 of /mod/assign/locallib.php: call to assign->count_submissions_with_status()
line 379 of /mod/assign/locallib.php: call to assign->view_submission_page()
line 53 of /mod/assign/view.php: call to assign->view()

Damyon Wiese
added a comment - 02/May/12 12:58 PM Hi Dan,
I just pushed a fix to the MSSQL issue. I changed the datatype for the status column to a fixed length char (which it should have been anyway).
Regards, Damyon

Damyon Wiese
added a comment - 02/May/12 1:09 PM One note about the above issue is that I don't have easy access to MSSQL server and haven't been able to confirm that the above change will fix the issue.
Damyon

Damyon Wiese
added a comment - 02/May/12 1:24 PM Hi Martin,
The old assignment module has been renamed - maybe it is a caching issue you are seeing (if it is I'm not sure how this will affect people doing upgrades generally though).
I also just pushed a change to disable the old assignment module for new installs.
Damyon

Damyon Wiese
added a comment - 02/May/12 3:10 PM Hi Martin,
I am happy to write some documentation to show during the upgrade (your point 3 above) - but I am not sure the best place to put it. Any suggestions?
Thanks - Damyon

I've starting my review of this now for integration. I will probably work on my own independent branch until I integrate this, but just early on I just wanted to draw your attention to this commit fixing a number of undefined variables:

Dan Poltawski
added a comment - 02/May/12 7:07 PM Hi Guys,
I've starting my review of this now for integration. I will probably work on my own independent branch until I integrate this, but just early on I just wanted to draw your attention to this commit fixing a number of undefined variables:
https://github.com/danpoltawski/moodle/commit/85fc0dcdc7405335d02971a52a03449fc9992708
This suggests to me that the areas in question have not been well tested recently. So it'd be great if you could look at those areas (as well as the obvious fixes).
get_user_grades_for_gradebook is a particular concern to me, but also plagiarism checks and portfolio export seem to be affected.
cheers,
dan

Dan Poltawski
added a comment - 02/May/12 11:10 PM I've been playing around with some test data with the upgrade tool, just as an admin not a developer (so i've not looked deeply into this).
But as far as I can tell comments are lost and I think gradebook items are lost too. I'm also finding that the urls on the admin tool are incorrect so I can't navigate around the list of assignments.

I will do some more testing on plagiarism/portfolio and the gradebook integration.

Comments and gradebook items should be brought across - The comments are changed to use the comments API in the process - have you got the comments API enabled? Do you think we need to do some additional checks here to make sure this is enabled before the upgrade? Not sure about the gradebook - what exactly is not brought across here?

Any additional detail you can provide on the upgrade failures would be appreciated - the upgrade process has been tested recently so I think if there are issues they should be small and quick to fix.

Damyon Wiese
added a comment - 03/May/12 9:30 AM Hi Dan,
I have merged your fix for the undefined variables.
I will do some more testing on plagiarism/portfolio and the gradebook integration.
Comments and gradebook items should be brought across - The comments are changed to use the comments API in the process - have you got the comments API enabled? Do you think we need to do some additional checks here to make sure this is enabled before the upgrade? Not sure about the gradebook - what exactly is not brought across here?
Any additional detail you can provide on the upgrade failures would be appreciated - the upgrade process has been tested recently so I think if there are issues they should be small and quick to fix.
Regards, Damyon

Sorry I wasn't very descriptive last night, it was late:
Submission comments.
=====================
1/ Create course, enrol student and teacher
2/ As teacher create 2.2 assignment - advanced uploading of files with default settings and title 'Apples'
3/ As student submit a file and send for marking
4/ As teacher grade it and add a comment "Good work bob, but maybe you could look at apple computers"
5/ As admin Go to admin > Assignment upgrade helper and select the assignment named 'Apples' to upgrade
6/ Use the tool to upgrade the assignment
7/ Visit the assignment as teacher and try and find that submission comment, the submission comments are showing as 0 and the feedback comments plugin is disabeld
8/ As student go to assignment and find your feedback, its not there (and get Comments (0))

Note: that it is in the gradebook unlike what I said above, but surely it should be added to the submission comments? I also note that the Feedback comments plugin isn't enabled.

Dan Poltawski
added a comment - 03/May/12 10:46 AM Sorry I wasn't very descriptive last night, it was late:
Submission comments.
=====================
1/ Create course, enrol student and teacher
2/ As teacher create 2.2 assignment - advanced uploading of files with default settings and title 'Apples'
3/ As student submit a file and send for marking
4/ As teacher grade it and add a comment "Good work bob, but maybe you could look at apple computers"
5/ As admin Go to admin > Assignment upgrade helper and select the assignment named 'Apples' to upgrade
6/ Use the tool to upgrade the assignment
7/ Visit the assignment as teacher and try and find that submission comment, the submission comments are showing as 0 and the feedback comments plugin is disabeld
8/ As student go to assignment and find your feedback, its not there (and get Comments (0))
Note: that it is in the gradebook unlike what I said above, but surely it should be added to the submission comments? I also note that the Feedback comments plugin isn't enabled.
Assignment upgrade table - bad url:
===================================
1/ Create course, enrol student and teacher
2/ As teacher create 2.2 assignment - advanced uploading of files with default settings
3/ Duplicate that assignment
4/ As admin Go to admin > Assignment upgrade helper
5/ List assignments which have not been upgraded
6/ Click on table header = cours
[You are taken to a non-existant url]
Fix: https://github.com/danpoltawski/moodle/commit/2ca5157087c60c1bad687c914c32cf14e98f9bfa

I found the upgrade code for the feedback plugins was missing (I must have not merged it with master). I have just added the code and pushed the change. I also found the upgrade code was not returning the error message for a failed upgrade which is also fixed. This fixes you test case 1 above.

Damyon Wiese
added a comment - 03/May/12 12:07 PM Hi Dan,
I found the upgrade code for the feedback plugins was missing (I must have not merged it with master). I have just added the code and pushed the change. I also found the upgrade code was not returning the error message for a failed upgrade which is also fixed. This fixes you test case 1 above.
Regards, Damyon

Damyon Wiese
added a comment - 03/May/12 12:58 PM Hi Dan
In looking at the get_user_grades_for_gradebook I found a bug when the grade is set to "No grade" which I have just fixed and pushed.
Regards, Damyon

admin/tool/assignmentupgrade

[MAJOR] I think batchupgrade.php should be using optional_param_array() to do proper cleaning of the params (in fact should it be required_param_array)?

tool_assignmentupgrade_any_upgradable_assignments() and tool_assignmentupgrade_load_all_upgradable_assignmentids() should be using $DB->get_in_or_equal() and probably count_records_sql()

Use of tool_assignmentupgrade_url() seems to be really inconsistent (e.g. its not used in the function just below it) and it would've fixed the bug I reported earlier if it was used.

Is tool_assignmentupgrade_process necessary?

mod/assign:

[MAJOR] In assign_extend_settings_navigation - I don't think you are using contexts quite correctly, $cm->context is deprecated (should be should call context_module::instance()) but also, shouldn't the grade capability checks be using course context?

[MAJOR] I think this should be using userdate:
$info->timeupdated = strftime('%c',$submission->timemodified);

[MAJOR] Emailing graders is using get_users_by_capability on the grade capability. I believe this is reintroducing the problem fixed in MDL-19263

PHPDoc: In a number of places there is misleading comments where your commment says you are returning void when in fact we return a string.

assign::add_instance - the course get_record call should probably be done before the instance insertion in order to prevent junk records being cerated

Phpdoc: Doing * @return None is not correct, the coding guidelines say: If function does 'return;' we use @return void. If a function uses 'return null;' thats a @return null as null is the specific data type.

Just an observation abstract class assign_plugin() is doing a lot of individual get_config calls, unfortunately at the moment that will result in a database query per call, it might be possible to reduce that impact by calling get_config once in constructor. (not important unless lots of them happening on one page, its really something we need to fix in core). Hmm, I see there is a get_config implementation for itself too.

The following functions looked like 'old-style' code to me and I had a note to try and look at in more detail: assign_print_overview assign_print_recent_activity assign_get_recent_mod_activity assign_print_recent_mod_activity

mod_assign_get_file_info() looks like it had quite a few unusused variables and looked suspciious to me

Thanks again - I really look forward to the new things which we'll see with the power of this new module

Dan Poltawski
added a comment - 03/May/12 3:15 PM Hi Guys,
I've integrated this now, congratulations and thanks for all your hard work on this!
I'm afraid i've have squashed your branch down into one commit as there 250+ commits and was probably more detail than was necessarily in the main branch.
There is some work that is still to be done, but I think its important we get this integrated and work off the integrated branch and are able to get the web services etc.
Here are my notes from review, I've tried to make the issues I believe are major clear (even if its just that they need to be checked:
admin/tool/assignmentupgrade
[MAJOR] It looks to me like all of this is missing CSRF protection (sesskey) e.g. This works without sesskey: http://dan.moodle.local/integration/admin/tool/assignmentupgrade/upgradesingle.php?id=3
[MAJOR] I think batchupgrade.php should be using optional_param_array() to do proper cleaning of the params (in fact should it be required_param_array)?
tool_assignmentupgrade_any_upgradable_assignments() and tool_assignmentupgrade_load_all_upgradable_assignmentids() should be using $DB->get_in_or_equal() and probably count_records_sql()
Use of tool_assignmentupgrade_url() seems to be really inconsistent (e.g. its not used in the function just below it) and it would've fixed the bug I reported earlier if it was used.
Is tool_assignmentupgrade_process necessary?
mod/assign:
[MAJOR] In assign_extend_settings_navigation - I don't think you are using contexts quite correctly, $cm->context is deprecated (should be should call context_module::instance()) but also, shouldn't the grade capability checks be using course context?
[MAJOR] I think this should be using userdate:
$info->timeupdated = strftime('%c',$submission->timemodified);
[MAJOR] Emailing graders is using get_users_by_capability on the grade capability. I believe this is reintroducing the problem fixed in MDL-19263
PHPDoc: In a number of places there is misleading comments where your commment says you are returning void when in fact we return a string.
assign::add_instance - the course get_record call should probably be done before the instance insertion in order to prevent junk records being cerated
Make one of the assignment types default to on
I don't think that ksort is UTF8 safe for sorting localised things.
Noticed some used strings in assign/lib:
$strgraded = get_string('graded', 'assign');
$strnotgradedyet = get_string('notgradedyet', 'assign');
$strsubmitted = get_string('submitted', 'assign');
$strreviewed = get_string('reviewed','assign');
gradeform.php: I don't understand the need for:
/** Required for advanced grading */
require_once('HTML/QuickForm/input.php');
Locallib/upgradelib: I don't understand the need for:
/** Include accesslib.php */
require_once($CFG->libdir.'/accesslib.php');
Its a shame things like this are not using moodle_url:
$info->url = $CFG->wwwroot.'/mod/assign/view.php?id='.$this->get_course_module()->id;
$o .= groups_print_activity_menu($this->get_course_module(), $CFG->wwwroot . '/mod/assign/view.php?id='
Phpdoc: Doing * @return None is not correct, the coding guidelines say: If function does 'return;' we use @return void. If a function uses 'return null;' thats a @return null as null is the specific data type.
Just an observation abstract class assign_plugin() is doing a lot of individual get_config calls, unfortunately at the moment that will result in a database query per call, it might be possible to reduce that impact by calling get_config once in constructor. (not important unless lots of them happening on one page, its really something we need to fix in core). Hmm, I see there is a get_config implementation for itself too.
The following functions looked like 'old-style' code to me and I had a note to try and look at in more detail: assign_print_overview assign_print_recent_activity assign_get_recent_mod_activity assign_print_recent_mod_activity
mod_assign_get_file_info() looks like it had quite a few unusused variables and looked suspciious to me
Thanks again - I really look forward to the new things which we'll see with the power of this new module

Damyon Wiese
added a comment - 03/May/12 3:28 PM Thanks Dan - this is great news!
To look at these outstanding issues I assume I should now create separate Moodle tickets and submit them for review?
Regards, Damyon

Dan Poltawski
added a comment - 03/May/12 5:43 PM I'm 'passing' this issue because we've got full testing going on in the QA process and i've also done quite a bit of testing on it myself.
Note this is only for the purposes of the integration, there is still lots of testing to be done!

Dan Poltawski
added a comment - 03/May/12 5:44 PM Damyon - yes, please lets create seperate issues. Also note that i'm afraid I messed up and missed off your AMOS commit message.
David is going to run this manually as discussed in MDL-32716

If you heard a noise late yesterday it was a cheer of happiness from the NetSpot office. Thanks again for all your help on this, we've learned a lot from this experience, will be much smoother next time I reckon

Mark Drechsler
added a comment - 04/May/12 7:36 AM Thanks Sam,
If you heard a noise late yesterday it was a cheer of happiness from the NetSpot office. Thanks again for all your help on this, we've learned a lot from this experience, will be much smoother next time I reckon
Mark.

We have NetSpot clients assisting with various aspects of this project including drafting user documentation. This is currently in google docs - what is the procedure for adding documentation to the user docs site for 2.3?

Damyon Wiese
added a comment - 04/May/12 1:54 PM Re: Documentation
We have NetSpot clients assisting with various aspects of this project including drafting user documentation. This is currently in google docs - what is the procedure for adding documentation to the user docs site for 2.3?
Regards, Damyon

The Moodle 2.3 documentation wiki will be created next week and then the new assignment documentation can be added to it. If you provide a link to the google doc, I can copy and paste it into the 2.3 documentation wiki, or you could do so, whichever you prefer.

Helen Foster
added a comment - 04/May/12 3:02 PM Hi Damyon,
The Moodle 2.3 documentation wiki will be created next week and then the new assignment documentation can be added to it. If you provide a link to the google doc, I can copy and paste it into the 2.3 documentation wiki, or you could do so, whichever you prefer.

FYI - I just did a fresh install of integration and it asks for the setting "assign_feedback_plugin_for_gradebook" during fresh install - needs to set default correctly and shouldn't ask during install.

Dan Marsden
added a comment - 04/May/12 3:44 PM FYI - I just did a fresh install of integration and it asks for the setting "assign_feedback_plugin_for_gradebook" during fresh install - needs to set default correctly and shouldn't ask during install.

Petr Skoda
added a comment - 06/May/12 7:52 PM Oh, the settings need to be reworked:
1/ nothing module related can appear on the new install page
2/ mod_assign can not pollute global $CFG
I am going to create separate issue and explain it there - see MDL-32813 .

As this is just a refactoring, we have two lots of assignment tests.
1) The existing ones for the old distinct assignment types.
2) The old tests re-worded to cover the 2.3 combined subtypes (as the same user scenarios should apply).

Tim Barker
added a comment - 08/May/12 10:19 AM As this is just a refactoring, we have two lots of assignment tests.
1) The existing ones for the old distinct assignment types.
2) The old tests re-worded to cover the 2.3 combined subtypes (as the same user scenarios should apply).