Details

    • Rank:
      38325

      Description

      Lightwork currently uses 2 types of rubrics which are being incorporated into Moodle.

      The first of these rubrics is very similar to the advanced grading rubric that was introduced in Moodle 2.2 and in order not to rebuild the same functionality in Moodle we have decided it would be better to modify Lightwork to use the advanced grading rubric. This will be done by using the new web service in MDL-31681 and modifying the Lightwork Java eclipse client.

      The second of these rubrics is known as a marking guide. It differs from the advanced grading rubric in that:

      • a maximum mark is defined for each criterion
      • the maximum marks defined for each criterion must add up to the maximum mark for the assignment
      • the teacher adds feedback and a mark (between 0 and the maximum allowed) for each criterion

      This marking guide can be implemented in Moodle by creating a new grading form plugin that can be called gradingform_markingguide.

      see also:
      http://moodle.org/mod/forum/discuss.php?d=195738
      http://docs.moodle.org/dev/Lightwork#Rubric_compatibility_between_the_Moodle_rubric_and_Lightwork

      Mockups of the user interface for the new marking guide can be found at
      http://docs.moodle.org/dev/Lightwork#User_Interface_-_Create_assignment.2C_define_Marking_guide_and_mark_student_work_according_to_Marking_guide

      The Lightwork rubric and marking guide have a number of features which will be included with the marking guide (and hopefully also to the existing rubric if agreed with Moodle). These are:

      • The ability to create and use a bank of frequently used comments
      • The ability for the student to generate a PDF version of the marking guide
      1. marking0guide-phpdocs.02.patch
        12 kB
        Sam Hemelryk
      2. marking-guide-phpdocs.patch
        18 kB
        Sam Hemelryk
      3. smurf.xml
        76 kB
        Dan Poltawski

        Issue Links

          Activity

          Hide
          Dan Marsden added a comment -

          first version of this should be ready for peer review

          Show
          Dan Marsden added a comment - first version of this should be ready for peer review
          Hide
          Sam Hemelryk added a comment -

          Hi Dan,

          Excellent work on this!
          The code looks really good. Sorry about the delay in the review I had integration Monday and Tuesdau steal all of my time.
          I've made the following notes while going through of things I think should be addressed before this is put up for integration:

          There is only really one (major?) design thing I noted while reviewing this, why did you call the frequently used comments table gradingform_guide_faq?
          Surely _faq is misleading and that table should be renamed to be reprentative of what it is doing. Perhaps gradingform_guide_comments or gradingform_guide_fuc (although that is getting close to you know what )

          Otherwise mostly just minor things I noted:

          Misc

          1. db/install.php There are still four uses of the word rubric in the table comments, a couple look like copy and paste could you please just double check them.
          2. There needs to be validation added somewhere for maxscore when creating/editing a guide and saving it as a draft. Try setting maxscore to "h" and you'll get a fatal error as it tried to insert that into the DB.
          3. renderer.php mentions rubrics in comments several times.
          4. preview.php still using string from gradingform_rubric

          js/guide.js

          1. Entirely optional but in regards to guide.js did you know that you can do Y.all('.blah').on('click', foo) http://yuilibrary.com/yui/docs/node/nodelist.html
          2. Unused vars collapsedwidth and shortwidth

          guideeditor.js

          1. Rather than use YAHOO.Lang.trim you should use Y.Lang.trim (given that is the only use of YUI2 there)

          lang/en/gradingform_guide.php

          1. Please order the lang strings in gradingform_guide.php alphabetically.
          2. Check camel casing in addcomment string, I don't think feedback needs to be uppercase.
          3. The guidemappingexplained string explicitly mentions assignment, surely that should be activity as advanced grading will be coming to other modules.

          lib.php

          1. gradingform_guide_controller::moduleinstance looks like it should default to false rather than array. Its not a bug as it looks like things will work perfectly still but will be clearer to anyone reading the code.
          2. gradingform_guide_controller::update_or_check_guide unused var $criterionmaxscore
          3. gradingform_guide_controller::load_definition is not going to scale well at all, certainly not if people add lots of comments. Really it looks like the grading definitions should be retrieved first, perhaps you could fetch the critieria at the same time as its unlikely people will have many critiria. However I think comments should be fetched separately. Also just noting I couldn't work out what the table aliases in that query stood for rc, and rf. Should they be gc, and gf?
            • Hmm looking more into this function it REALLY looks like it needs to be separated out, given the definition property only every gets populated once it must be assumed that there is only ever one definition. This code would be horribly prone to bugs if for any reason (bug or what have you) there was more than one definition.
            • Why are the showmarkerdesc and showstudentdesc preferences being set in this function? This caught my eye as I am very suspicious of any use of optional_param/required_param within a API. It always represents a risk of collision even if only a minor one. Could these not be moved into the pages that actually allow the preferences to be set? or into a function called by pages where they are applicable. Certianly that would be a better solution.
          4. gradingform_guide_controller::get_definition_for_editing needs to check that moduleinstance is set before using it. I had a notice because it was not.
          5. gradingform_guide_controller::get_definition_copy unused var $newlevid
          6. gradingform_guide_controller::delete_plugin_definition although untested I'd bet this function needs to be deleting comments as well. Certianly it appears the calling function is deleting the grading definition and instances
          7. gradingform_guide_controlled::get_or_create_instance The get_records with rater and item could easily be a get_record with IGNORE_MULTIPLE set for strictness (just noting is all)
          8. gradingform_guide_controlled::sql_search_from_tables/sql_search_where rc used as a table alias again?
          9. gradingform_guide_controlled::sql_search_where Worked out the rc alias. Copy+paste right? This function is adding search SQL for rl.definition but the marking guide doesn't have a levels table and sql_search_from_tables only joins to criteria table.
          10. gradingform_guide_controlled::validationerrors property is never used? Is that meant to have been defined on gradingform_guide_instance class instead (its used there but not defined).

          As well as that just noting the following:

          • Backup and restore still needs to be written, although I don't think that will be too difficult as this doesn't vary much in structure from rubrics.
          • Still requires an icon (shown in plugins overview), not really an essential so you could always copy the rubric one for the time being I suppose.

          Now I've never done this before however as I went through rather than just noted things I made minor changes as I saw required in the PHPDocs.
          Not something I would normally do and it is entirely up to you if you choose to use it.
          You can find a commit with them here:

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Dan, Excellent work on this! The code looks really good. Sorry about the delay in the review I had integration Monday and Tuesdau steal all of my time. I've made the following notes while going through of things I think should be addressed before this is put up for integration: There is only really one (major?) design thing I noted while reviewing this, why did you call the frequently used comments table gradingform_guide_faq? Surely _faq is misleading and that table should be renamed to be reprentative of what it is doing. Perhaps gradingform_guide_comments or gradingform_guide_fuc (although that is getting close to you know what ) Otherwise mostly just minor things I noted: Misc db/install.php There are still four uses of the word rubric in the table comments, a couple look like copy and paste could you please just double check them. There needs to be validation added somewhere for maxscore when creating/editing a guide and saving it as a draft. Try setting maxscore to "h" and you'll get a fatal error as it tried to insert that into the DB. renderer.php mentions rubrics in comments several times. preview.php still using string from gradingform_rubric js/guide.js Entirely optional but in regards to guide.js did you know that you can do Y.all('.blah').on('click', foo) http://yuilibrary.com/yui/docs/node/nodelist.html Unused vars collapsedwidth and shortwidth guideeditor.js Rather than use YAHOO.Lang.trim you should use Y.Lang.trim (given that is the only use of YUI2 there) lang/en/gradingform_guide.php Please order the lang strings in gradingform_guide.php alphabetically. Check camel casing in addcomment string, I don't think feedback needs to be uppercase. The guidemappingexplained string explicitly mentions assignment, surely that should be activity as advanced grading will be coming to other modules. lib.php gradingform_guide_controller::moduleinstance looks like it should default to false rather than array. Its not a bug as it looks like things will work perfectly still but will be clearer to anyone reading the code. gradingform_guide_controller::update_or_check_guide unused var $criterionmaxscore gradingform_guide_controller::load_definition is not going to scale well at all, certainly not if people add lots of comments. Really it looks like the grading definitions should be retrieved first, perhaps you could fetch the critieria at the same time as its unlikely people will have many critiria. However I think comments should be fetched separately. Also just noting I couldn't work out what the table aliases in that query stood for rc, and rf. Should they be gc, and gf? Hmm looking more into this function it REALLY looks like it needs to be separated out, given the definition property only every gets populated once it must be assumed that there is only ever one definition. This code would be horribly prone to bugs if for any reason (bug or what have you) there was more than one definition. Why are the showmarkerdesc and showstudentdesc preferences being set in this function? This caught my eye as I am very suspicious of any use of optional_param/required_param within a API. It always represents a risk of collision even if only a minor one. Could these not be moved into the pages that actually allow the preferences to be set? or into a function called by pages where they are applicable. Certianly that would be a better solution. gradingform_guide_controller::get_definition_for_editing needs to check that moduleinstance is set before using it. I had a notice because it was not. gradingform_guide_controller::get_definition_copy unused var $newlevid gradingform_guide_controller::delete_plugin_definition although untested I'd bet this function needs to be deleting comments as well. Certianly it appears the calling function is deleting the grading definition and instances gradingform_guide_controlled::get_or_create_instance The get_records with rater and item could easily be a get_record with IGNORE_MULTIPLE set for strictness (just noting is all) gradingform_guide_controlled::sql_search_from_tables/sql_search_where rc used as a table alias again? gradingform_guide_controlled::sql_search_where Worked out the rc alias. Copy+paste right? This function is adding search SQL for rl.definition but the marking guide doesn't have a levels table and sql_search_from_tables only joins to criteria table. gradingform_guide_controlled::validationerrors property is never used? Is that meant to have been defined on gradingform_guide_instance class instead (its used there but not defined). As well as that just noting the following: Backup and restore still needs to be written, although I don't think that will be too difficult as this doesn't vary much in structure from rubrics. Still requires an icon (shown in plugins overview), not really an essential so you could always copy the rubric one for the time being I suppose. Now I've never done this before however as I went through rather than just noted things I made minor changes as I saw required in the PHPDocs. Not something I would normally do and it is entirely up to you if you choose to use it. You can find a commit with them here: Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Ohh just one more note, it would be great if the CSS for the marking guide editing of criteria could be improved to make things clearer.
          On my first attempt I completely missed that I have to give the criteria a name, undoubtedly partly operator trouble but it would be nice to see if it can be visually clarified

          Show
          Sam Hemelryk added a comment - Ohh just one more note, it would be great if the CSS for the marking guide editing of criteria could be improved to make things clearer. On my first attempt I completely missed that I have to give the criteria a name, undoubtedly partly operator trouble but it would be nice to see if it can be visually clarified
          Hide
          Dan Marsden added a comment -

          Thanks Sam - that's great feedback, I've pushed changes for most of those items you've mentioned except:
          perf of gradingform_guide_controller::load_definition - separating it out (haven't looked at this yet - will look tomorrow)

          gradingform_guide_controlled::get_or_create_instance - get_record doesn't allow us to pass an order by param so it could potentially get the wrong record (have copied this from the rubric grading type btw)

          showmarkerdesc and showstudentdesc - I'm not sure if we can shift these easily - the marking guide is loaded "inside" the modules "submissions" page - it doesn't have it's own "page" which is loaded - we could modify each modules "submissions" page to manage it but this seems fragile - do you have a suggestion for a better location?

          Show
          Dan Marsden added a comment - Thanks Sam - that's great feedback, I've pushed changes for most of those items you've mentioned except: perf of gradingform_guide_controller::load_definition - separating it out (haven't looked at this yet - will look tomorrow) gradingform_guide_controlled::get_or_create_instance - get_record doesn't allow us to pass an order by param so it could potentially get the wrong record (have copied this from the rubric grading type btw) showmarkerdesc and showstudentdesc - I'm not sure if we can shift these easily - the marking guide is loaded "inside" the modules "submissions" page - it doesn't have it's own "page" which is loaded - we could modify each modules "submissions" page to manage it but this seems fragile - do you have a suggestion for a better location?
          Hide
          Dan Marsden added a comment -

          passing back for peer review, have tidied up the last bits, moved the showmarkerdesc/showstudentdesc stuff slightly and improved the comments around it - still not the ideal place but I'm unsure where else I could locate it - let me know if you spot a possible location.

          btw - issues with load_definition scaling is mostly copied from rubrics - you might want to check it out there as well. - moved from 1 single db query that may not have scaled well to 3 db calls instead.

          thanks!

          Show
          Dan Marsden added a comment - passing back for peer review, have tidied up the last bits, moved the showmarkerdesc/showstudentdesc stuff slightly and improved the comments around it - still not the ideal place but I'm unsure where else I could locate it - let me know if you spot a possible location. btw - issues with load_definition scaling is mostly copied from rubrics - you might want to check it out there as well. - moved from 1 single db query that may not have scaled well to 3 db calls instead. thanks!
          Hide
          Dan Marsden added a comment -

          Hi Sam, FYI - just pushed a couple of small fixes into the git repo to fix a couple of css issues and a bug preventing students from seeing the marks per criterion received that the Lightwork team found in testing today - you may want to update your codebase to get these - thanks!

          Show
          Dan Marsden added a comment - Hi Sam, FYI - just pushed a couple of small fixes into the git repo to fix a couple of css issues and a bug preventing students from seeing the marks per criterion received that the Lightwork team found in testing today - you may want to update your codebase to get these - thanks!
          Hide
          Sam Hemelryk added a comment -

          Hi Dan,

          Things are looking really good and it gets my +1 to put it up for integration.
          Before you do I would like you to have a look at a patch I will attach shortly marking0guide-phpdocs.02.patch.
          It contains one bug fix (gradingform_guide_controller::load_definition throws notices etc when creating a new grading form) and several phpdoc fixed.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Dan, Things are looking really good and it gets my +1 to put it up for integration. Before you do I would like you to have a look at a patch I will attach shortly marking0guide-phpdocs.02.patch. It contains one bug fix (gradingform_guide_controller::load_definition throws notices etc when creating a new grading form) and several phpdoc fixed. Cheers Sam
          Hide
          Dan Marsden added a comment -

          thanks Sam - have pushed that patch in and created a new branch based on master with a single commit (including patch to pluginlib.php to add standard plugin to list to make it easier for the integrator.

          Note to Integrator - feel free to pull this plugin from the lightwork repo here:
          https://github.com/Lightwork-Marking/moodle-gradingform_guide

          • but you will need to patch pluginlib.php yourself.
          Show
          Dan Marsden added a comment - thanks Sam - have pushed that patch in and created a new branch based on master with a single commit (including patch to pluginlib.php to add standard plugin to list to make it easier for the integrator. Note to Integrator - feel free to pull this plugin from the lightwork repo here: https://github.com/Lightwork-Marking/moodle-gradingform_guide but you will need to patch pluginlib.php yourself.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Marsden added a comment -

          rebased

          Show
          Dan Marsden added a comment - rebased
          Hide
          Dan Poltawski added a comment - - edited

          Some comments mostly from playing with the feature:

          1. Core issue - we should warn when switching between advanced grading types
          2. I think the 'click to add frequently used comment' is a bit undiscoverable, would be good to think of a way of making it clearer (maybe we could add a + icon next to each comment)
          3. I discovered an issue with JS for inserting the comment (on chrome):
            1. Click a frequently used comment to insert it into the box
            2. In the comment box use the backspace to delete a character
            3. Click to add another comment (it fails)
          Show
          Dan Poltawski added a comment - - edited Some comments mostly from playing with the feature: Core issue - we should warn when switching between advanced grading types I think the 'click to add frequently used comment' is a bit undiscoverable, would be good to think of a way of making it clearer (maybe we could add a + icon next to each comment) I discovered an issue with JS for inserting the comment (on chrome): Click a frequently used comment to insert it into the box In the comment box use the backspace to delete a character Click to add another comment (it fails)
          Hide
          Dan Poltawski added a comment -

          Hi Dan,

          The coding style checker has come up with some issues on this, mostly its just comments without punctuation (not a concern).

          But 'Inline control structures are not allowed' is a hard rule, so it'd be great if you could fix that up, cheers.
          dan

          Show
          Dan Poltawski added a comment - Hi Dan, The coding style checker has come up with some issues on this, mostly its just comments without punctuation (not a concern). But 'Inline control structures are not allowed' is a hard rule, so it'd be great if you could fix that up, cheers. dan
          Hide
          Dan Poltawski added a comment -

          Sorry another comment - are you able to write some testing instructions for this new functionality?

          thanks
          dan

          Show
          Dan Poltawski added a comment - Sorry another comment - are you able to write some testing instructions for this new functionality? thanks dan
          Hide
          Dan Marsden added a comment -

          thanks Dan - I'll take a look at this now - there are some good testing instructions already sorry I didn't add them here, have added links above.

          Show
          Dan Marsden added a comment - thanks Dan - I'll take a look at this now - there are some good testing instructions already sorry I didn't add them here, have added links above.
          Hide
          Dan Marsden added a comment -

          have created a bug for the core issue mentioned (linked)

          Show
          Dan Marsden added a comment - have created a bug for the core issue mentioned (linked)
          Hide
          Dan Marsden added a comment -

          Hi Dan - have made changes as requested/fixed the inserting bug - moved the Core issue into a new bug as mentioned above.

          I've rebased the branch to a single commit but if you want to see the individual commits you can see them in:
          https://github.com/Lightwork-Marking/moodle-gradingform_guide

          thanks!

          Show
          Dan Marsden added a comment - Hi Dan - have made changes as requested/fixed the inserting bug - moved the Core issue into a new bug as mentioned above. I've rebased the branch to a single commit but if you want to see the individual commits you can see them in: https://github.com/Lightwork-Marking/moodle-gradingform_guide thanks!
          Hide
          Dan Poltawski added a comment -

          Hi Dan,

          This is looking great now I think.

          However as I have only just managed to get back to I don't think we have the testing resource to get it in this week. I'm sorry about that - thanks for your speedy work on it/

          Show
          Dan Poltawski added a comment - Hi Dan, This is looking great now I think. However as I have only just managed to get back to I don't think we have the testing resource to get it in this week. I'm sorry about that - thanks for your speedy work on it/
          Hide
          Dan Marsden added a comment -

          Thanks Dan - makes sense to me to delay this especially as it's only for master and we've had a couple of short weeks with Anzac and Easter - thanks for the review!

          Show
          Dan Marsden added a comment - Thanks Dan - makes sense to me to delay this especially as it's only for master and we've had a couple of short weeks with Anzac and Easter - thanks for the review!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Marsden added a comment -

          rebased.

          Show
          Dan Marsden added a comment - rebased.
          Hide
          Dan Poltawski added a comment -

          Thanks Dan, i've integrated this now!

          Show
          Dan Poltawski added a comment - Thanks Dan, i've integrated this now!
          Hide
          Jason Fowler added a comment -

          testing postponed until new assignment module is also integrated as testing instructions indicate it as a pre-requisite.

          Show
          Jason Fowler added a comment - testing postponed until new assignment module is also integrated as testing instructions indicate it as a pre-requisite.
          Hide
          Dan Marsden added a comment -

          This can be tested fine with the existing assignment type - IMO it should not be held back until new assignment is integrated - it's been passed over several times already.

          The testing instructions attached include information to allow testing with new assignment mod which should be done when new assignment mod is integrated.

          Show
          Dan Marsden added a comment - This can be tested fine with the existing assignment type - IMO it should not be held back until new assignment is integrated - it's been passed over several times already. The testing instructions attached include information to allow testing with new assignment mod which should be done when new assignment mod is integrated.
          Hide
          Jason Fowler added a comment -

          Failed at 6.2 test 5 - the checkbox options are not remembered between saves

          Show
          Jason Fowler added a comment - Failed at 6.2 test 5 - the checkbox options are not remembered between saves
          Hide
          Jason Fowler added a comment - - edited

          6.3 test 2 failed too - I can set the marks criterion higher and lower than the total allowed for the assignment without a warning

          Show
          Jason Fowler added a comment - - edited 6.3 test 2 failed too - I can set the marks criterion higher and lower than the total allowed for the assignment without a warning
          Hide
          Jason Fowler added a comment -

          At this point, I have stopped testing, due to the failures. Will continue testing tomorrow to see what else is in the way of getting this through, unless someone else wants to test the remainder of the items

          Show
          Jason Fowler added a comment - At this point, I have stopped testing, due to the failures. Will continue testing tomorrow to see what else is in the way of getting this through, unless someone else wants to test the remainder of the items
          Hide
          Dan Marsden added a comment -

          Thanks Jason 6.3 might actually be an incorrect test and expected behavior but I'll take a look - an earlier version of the spec for this feature called for some different rules around the way marks were handled with the max grade setting in the assignment.

          I just took a look at 6.2 and found it's an issue if you unselect both checkboxes - should be an easy fix which I'll push through tonight.

          Show
          Dan Marsden added a comment - Thanks Jason 6.3 might actually be an incorrect test and expected behavior but I'll take a look - an earlier version of the spec for this feature called for some different rules around the way marks were handled with the max grade setting in the assignment. I just took a look at 6.2 and found it's an issue if you unselect both checkboxes - should be an easy fix which I'll push through tonight.
          Hide
          Dan Marsden added a comment -

          fix for 6.2 now in repo - 6.3 is definitely an invalid test and behavior is expected - it should scale the scores. It does show a warning on the page viewing the read-only grading method if the max grade is higher than the number of points available in the grading method as this is not required as it will scale the grade like the other Rubric Grading method.

          Show
          Dan Marsden added a comment - fix for 6.2 now in repo - 6.3 is definitely an invalid test and behavior is expected - it should scale the scores. It does show a warning on the page viewing the read-only grading method if the max grade is higher than the number of points available in the grading method as this is not required as it will scale the grade like the other Rubric Grading method.
          Hide
          Dan Poltawski added a comment -

          Integrated that now, ready for testing again - thanks!

          Show
          Dan Poltawski added a comment - Integrated that now, ready for testing again - thanks!
          Hide
          Dan Marsden added a comment -

          Sorry Dan/Jason - there was a bug there with 6.3 - the "manage.php" page displaying the read-only view of the marking guide was supposed to display the error but wasn't correctly - have just pushed through a fix into my repo - thanks!

          Show
          Dan Marsden added a comment - Sorry Dan/Jason - there was a bug there with 6.3 - the "manage.php" page displaying the read-only view of the marking guide was supposed to display the error but wasn't correctly - have just pushed through a fix into my repo - thanks!
          Hide
          Dan Marsden added a comment -

          but... the test instructions in 6.3 are still not completely correct.
          Test 6.3 should read like this - (I'll ask the lightwork team to update it)

          -START OF 6.3 test, TEST 2---
          Open a marking guide that has not yet been set ready. Modify the marks per criterion so the total is smaller than the maximum defined in the assignment specification. Select ‘Save and set ready’.
          The guide is set to ready.

          On the page describing the Marking guide a warning is displayed stating that the grades will be scaled as the max grade is higher than the number of marks possible in the guide.

          Open a marking guide that has not yet been set ready. Modify the marks per criterion so the total is larger than the maximum defined in the assignment specification. Select ‘Save and set ready’.
          The guide is set to ready.

          On the page describing the Marking guide a warning is displayed stating that the grades will be scaled as the max grade is lower than the number of marks possible in the guide.
          ---END TEST 6.3 TEST 2----

          Also, tests 6.7-> are all related to core code - so although useful to include in the test plan - it's likely that core code needs to be improved a bit - for example see MDL-32606

          Show
          Dan Marsden added a comment - but... the test instructions in 6.3 are still not completely correct. Test 6.3 should read like this - (I'll ask the lightwork team to update it) - START OF 6.3 test, TEST 2 --- Open a marking guide that has not yet been set ready. Modify the marks per criterion so the total is smaller than the maximum defined in the assignment specification. Select ‘Save and set ready’. The guide is set to ready. On the page describing the Marking guide a warning is displayed stating that the grades will be scaled as the max grade is higher than the number of marks possible in the guide. Open a marking guide that has not yet been set ready. Modify the marks per criterion so the total is larger than the maximum defined in the assignment specification. Select ‘Save and set ready’. The guide is set to ready. On the page describing the Marking guide a warning is displayed stating that the grades will be scaled as the max grade is lower than the number of marks possible in the guide. --- END TEST 6.3 TEST 2 ---- Also, tests 6.7-> are all related to core code - so although useful to include in the test plan - it's likely that core code needs to be improved a bit - for example see MDL-32606
          Hide
          Dan Poltawski added a comment -

          Thanks Dan, i've pulled that now.

          Show
          Dan Poltawski added a comment - Thanks Dan, i've pulled that now.
          Hide
          Jason Fowler added a comment -

          6.3 Test 1 generated this notice
          Notice: Trying to get property of non-object in /var/www/repos/imaster/moodle/grade/grading/form/guide/lib.php on line 342 Notice: Trying to get property of non-object in /var/www/repos/imaster/moodle/grade/grading/form/guide/lib.php on line 342

          6.3 Test 2 fails as I can set the marks lower and still save it without the warning at all

          Show
          Jason Fowler added a comment - 6.3 Test 1 generated this notice Notice: Trying to get property of non-object in /var/www/repos/imaster/moodle/grade/grading/form/guide/lib.php on line 342 Notice: Trying to get property of non-object in /var/www/repos/imaster/moodle/grade/grading/form/guide/lib.php on line 342 6.3 Test 2 fails as I can set the marks lower and still save it without the warning at all
          Hide
          Dan Poltawski added a comment -

          I should've caught that, a hardcoded assignment name which I should've caught in my pull.

          Dan: I went ahead and applied this without speaking to you in the interests of expediency.

          Show
          Dan Poltawski added a comment - I should've caught that, a hardcoded assignment name which I should've caught in my pull. Dan: I went ahead and applied this without speaking to you in the interests of expediency.
          Hide
          Dan Marsden added a comment -

          yeah - sorry - I did that too quickly and was working on other stuff at the time - stupid mistake - it should use $modulename instead - I'll try to fix it later tonight unless someone beats me to it.

          thanks!

          Show
          Dan Marsden added a comment - yeah - sorry - I did that too quickly and was working on other stuff at the time - stupid mistake - it should use $modulename instead - I'll try to fix it later tonight unless someone beats me to it. thanks!
          Hide
          Jason Fowler added a comment -

          Fails again at6.4 test 4 - negative marks do not show an error

          Show
          Jason Fowler added a comment - Fails again at6.4 test 4 - negative marks do not show an error
          Hide
          Jason Fowler added a comment -

          6.4 test 8 fails, there is nothing there about previous feedback

          Show
          Jason Fowler added a comment - 6.4 test 8 fails, there is nothing there about previous feedback
          Hide
          Dan Marsden added a comment -

          Thanks Jason - fix pushed for 6.4 T4 but I think 6.4T8 is a feature that wasn't in the original spec - It's probably something we should do for rubric grading method so I'll take a look to see how easy it would be to implement as a separate Tracker issue.

          Show
          Dan Marsden added a comment - Thanks Jason - fix pushed for 6.4 T4 but I think 6.4T8 is a feature that wasn't in the original spec - It's probably something we should do for rubric grading method so I'll take a look to see how easy it would be to implement as a separate Tracker issue.
          Hide
          Dan Marsden added a comment -

          ..but I've just tested this for the standard assignment mod and it worked fine.

          with simple grading enabled I entered some general feedback to the student.
          switched to Marking Guide and then went to grade the student and that general feedback was still there.

          is that not what you're seeing?

          Show
          Dan Marsden added a comment - ..but I've just tested this for the standard assignment mod and it worked fine. with simple grading enabled I entered some general feedback to the student. switched to Marking Guide and then went to grade the student and that general feedback was still there. is that not what you're seeing?
          Hide
          Jason Fowler added a comment -

          6.5 Test two - returning a marking guide that has been used to "not ready/draft" - this is impossible, as the system doesn't allow it (not a problem, just needs to be noted for QA testing later)

          in testing I found I could provide grades for students that had not provided a submission yet by clicked the save and next button.

          6.8 Test 1 - no warnings are displayed

          I only got as far as 6.8 Test 1 - I will continue on Monday.

          Show
          Jason Fowler added a comment - 6.5 Test two - returning a marking guide that has been used to "not ready/draft" - this is impossible, as the system doesn't allow it (not a problem, just needs to be noted for QA testing later) in testing I found I could provide grades for students that had not provided a submission yet by clicked the save and next button. 6.8 Test 1 - no warnings are displayed I only got as far as 6.8 Test 1 - I will continue on Monday.
          Hide
          Dan Marsden added a comment -

          6.4T8 seems to be a bug/issue with the way new assign handles feedback - appears to be a bug with the new assign mod - not the marking guide, I'll file a new bug for that.

          Show
          Dan Marsden added a comment - 6.4T8 seems to be a bug/issue with the way new assign handles feedback - appears to be a bug with the new assign mod - not the marking guide, I'll file a new bug for that.
          Hide
          Dan Marsden added a comment -

          in fact - there's a bigger bug there with new assignment... Feedback comments aren't editable at all. Will create a bug for that.

          if you keep simple grading method - add a comment - save, it shows the comment in the read-only view.
          edit the grade again and the comment isn't shown - if you re-save it will override the comment.

          Show
          Dan Marsden added a comment - in fact - there's a bigger bug there with new assignment... Feedback comments aren't editable at all. Will create a bug for that. if you keep simple grading method - add a comment - save, it shows the comment in the read-only view. edit the grade again and the comment isn't shown - if you re-save it will override the comment.
          Hide
          Dan Marsden added a comment -

          "in testing I found I could provide grades for students that had not provided a submission yet by clicked the save and next button." - this sounds like expected behavior to me - it's up to the module to decide - for example offline assignment doesn't have any submissions at all.

          Show
          Dan Marsden added a comment - "in testing I found I could provide grades for students that had not provided a submission yet by clicked the save and next button." - this sounds like expected behavior to me - it's up to the module to decide - for example offline assignment doesn't have any submissions at all.
          Hide
          Dan Poltawski added a comment -

          OK, i've integrated the latest fixes.

          Show
          Dan Poltawski added a comment - OK, i've integrated the latest fixes.
          Hide
          Dan Poltawski added a comment -

          I'm passing this as there are no major blockers and we will address the follow on issues in MDL-32784. We are happy enough to keep it in this weeks build.

          The testing needs to continue in MDL-32784 as per the instructions as we would've liked to have that completed for this weeks integration.

          Show
          Dan Poltawski added a comment - I'm passing this as there are no major blockers and we will address the follow on issues in MDL-32784 . We are happy enough to keep it in this weeks build. The testing needs to continue in MDL-32784 as per the instructions as we would've liked to have that completed for this weeks integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -
          UPDATE tracker_issues
             SET status = 'Closed',
                comment = 'Thanks!'
          WHEN participants = 'Did a gorgeous work'
          

          This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

          Show
          Eloy Lafuente (stronk7) added a comment - UPDATE tracker_issues SET status = 'Closed', comment = 'Thanks!' WHEN participants = 'Did a gorgeous work' This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).
          Hide
          Mark Drechsler added a comment -

          How the heck did I miss this one? Love your work - as always - Dan. Thanks to Massey (I assume) too for funding this one.

          Show
          Mark Drechsler added a comment - How the heck did I miss this one? Love your work - as always - Dan. Thanks to Massey (I assume) too for funding this one.
          Hide
          Helen Foster added a comment -

          Thanks Dan for this new feature and thanks to Mary for writing documentation for it: http://docs.moodle.org/23/en/Marking_guide

          Just removing the docs_required label...

          Show
          Helen Foster added a comment - Thanks Dan for this new feature and thanks to Mary for writing documentation for it: http://docs.moodle.org/23/en/Marking_guide Just removing the docs_required label...

            People

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

              Dates

              • Created:
                Updated:
                Resolved: