Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Grading methods
    • Labels:

      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

        Gliffy Diagrams

        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
            danmarsden Dan Marsden added a comment -

            first version of this should be ready for peer review

            Show
            danmarsden Dan Marsden added a comment - first version of this should be ready for peer review
            Hide
            samhemelryk 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk 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
            danmarsden 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
            danmarsden 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
            danmarsden 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
            danmarsden 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
            danmarsden 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
            danmarsden 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
            samhemelryk 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
            samhemelryk 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
            danmarsden 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
            danmarsden 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
            poltawski 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
            poltawski 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
            danmarsden Dan Marsden added a comment -

            rebased

            Show
            danmarsden Dan Marsden added a comment - rebased
            Hide
            poltawski 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
            poltawski 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
            poltawski 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
            poltawski 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
            poltawski Dan Poltawski added a comment -

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

            thanks
            dan

            Show
            poltawski Dan Poltawski added a comment - Sorry another comment - are you able to write some testing instructions for this new functionality? thanks dan
            Hide
            danmarsden 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
            danmarsden 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
            danmarsden Dan Marsden added a comment -

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

            Show
            danmarsden Dan Marsden added a comment - have created a bug for the core issue mentioned (linked)
            Hide
            danmarsden 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
            danmarsden 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
            poltawski 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
            poltawski 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
            danmarsden 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
            danmarsden 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
            stronk7 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
            stronk7 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
            danmarsden Dan Marsden added a comment -

            rebased.

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

            Thanks Dan, i've integrated this now!

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

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

            Show
            phalacee Jason Fowler added a comment - testing postponed until new assignment module is also integrated as testing instructions indicate it as a pre-requisite.
            Hide
            danmarsden 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
            danmarsden 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
            phalacee Jason Fowler added a comment -

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

            Show
            phalacee Jason Fowler added a comment - Failed at 6.2 test 5 - the checkbox options are not remembered between saves
            Hide
            phalacee 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
            phalacee 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
            phalacee 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
            phalacee 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
            danmarsden 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
            danmarsden 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
            danmarsden 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
            danmarsden 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
            poltawski Dan Poltawski added a comment -

            Integrated that now, ready for testing again - thanks!

            Show
            poltawski Dan Poltawski added a comment - Integrated that now, ready for testing again - thanks!
            Hide
            danmarsden 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
            danmarsden 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
            danmarsden 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
            danmarsden 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
            poltawski Dan Poltawski added a comment -

            Thanks Dan, i've pulled that now.

            Show
            poltawski Dan Poltawski added a comment - Thanks Dan, i've pulled that now.
            Hide
            phalacee 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
            phalacee 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
            poltawski 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
            poltawski 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
            danmarsden 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
            danmarsden 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
            phalacee Jason Fowler added a comment -

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

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

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

            Show
            phalacee Jason Fowler added a comment - 6.4 test 8 fails, there is nothing there about previous feedback
            Hide
            danmarsden 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
            danmarsden 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
            danmarsden 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
            danmarsden 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
            phalacee 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
            phalacee 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
            danmarsden 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
            danmarsden 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
            danmarsden 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
            danmarsden 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
            danmarsden 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
            danmarsden 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
            poltawski Dan Poltawski added a comment -

            OK, i've integrated the latest fixes.

            Show
            poltawski Dan Poltawski added a comment - OK, i've integrated the latest fixes.
            Hide
            poltawski 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
            poltawski 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
            stronk7 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
            stronk7 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
            drex 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
            drex 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
            tsala 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
            tsala 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...
            Hide
            marycooch Mary Cooch added a comment -

            Removing the qa_test_required label as it seems several tests were created for this at the time, the first one being MDLQA-1755

            Show
            marycooch Mary Cooch added a comment - Removing the qa_test_required label as it seems several tests were created for this at the time, the first one being MDLQA-1755

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12