Moodle
  1. Moodle
  2. MDL-37153

Scores in new assignment are shown, even if we configure gradebook to show only Letters

    Details

    • Testing Instructions:
      Hide
      1. Create a new assignment with grade = 100
      2. In grade, select "simple view" and edit settings for assignment
      3. Click "show mode" and set "Grade display type" => Letter
      4. As teacher mark few students for that assignment
      5. As student go back and view assignment
      6. Make sure Grade shown in Feedback is letter and not number.
      • Repeat test by changing grade in "simple view' to different options and make sure expected grade is visible.
      Show
      Create a new assignment with grade = 100 In grade, select "simple view" and edit settings for assignment Click "show mode" and set "Grade display type" => Letter As teacher mark few students for that assignment As student go back and view assignment Make sure Grade shown in Feedback is letter and not number. Repeat test by changing grade in "simple view' to different options and make sure expected grade is visible.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull 2.5 Branch:
    • Pull Master Branch:
      MDL-37153_master
    • Rank:
      46725

      Description

      I have my gradebook configured so that students cannot see their grades - only the Letters (it's something that's mandatory in our schools, for younger students).

      But, in a "Simple direct grading" configured (new type of) assignment, although it shows the Letters in the gradebook (either teacher or students one), as it should, if a student goes directly to the assignment, he/she can still see the grade (should see only the letter).

      This was not happening in previous versions of Moodle (2.3.2, as far as I can remember)

      1. studentattempts.png
        112 kB
      2. student view.png
        27 kB
      3. teacher view.png
        23 kB

        Issue Links

          Activity

          Hide
          Damyon Wiese added a comment -

          Thanks for reporting this.

          I've put that on the backlog.

          In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

          Show
          Damyon Wiese added a comment - Thanks for reporting this. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
          Hide
          Nigel Pegram added a comment -

          Bug still present in version 2.4.1+

          It's a requirement of our institution that grade letters be displayed, not raw grades.

          In the short term I'm simply removing the grade from the assignment feedback display and will let the students get their grades from the gradebook.

          In the medium term will look at a fix.

          Will post each code change as they are done, in case anyone needs them.

          Show
          Nigel Pegram added a comment - Bug still present in version 2.4.1+ It's a requirement of our institution that grade letters be displayed, not raw grades. In the short term I'm simply removing the grade from the assignment feedback display and will let the students get their grades from the gradebook. In the medium term will look at a fix. Will post each code change as they are done, in case anyone needs them.
          Hide
          Nigel Pegram added a comment - - edited

          A short-term fix to hide the grade value:

          Important: you will need to ensure the gradebook is visible to students in all courses for this to work.

          Edit the file mod/assign/locallib.php

          For version 2.4.1+ only

          Paste the following on line 2944 (which should be blank):
          $gradefordisplay = "See gradebook (Settings→Course administration→Grades)" ;

          Save and test.

          This should cause the part of the display where the grade value was shown to display the message "See gradebook (Settings→Course administration→Grades)", directing the student to see the gradebook for actual grades.

          For versions other than 2.4.1+
          The change is made in the function view_student_summary.
          You should be able to find near the bottom of that function where the grade value is assigned. All the above does is reassign the value of the variable just before it is used for display purposes.

          Use at you own risk, no warranty, etc.

          I hope this helps...

          Show
          Nigel Pegram added a comment - - edited A short-term fix to hide the grade value: Important: you will need to ensure the gradebook is visible to students in all courses for this to work. Edit the file mod/assign/locallib.php For version 2.4.1+ only Paste the following on line 2944 (which should be blank): $gradefordisplay = "See gradebook (Settings→Course administration→Grades)" ; Save and test. This should cause the part of the display where the grade value was shown to display the message "See gradebook (Settings→Course administration→Grades)", directing the student to see the gradebook for actual grades. For versions other than 2.4.1+ The change is made in the function view_student_summary. You should be able to find near the bottom of that function where the grade value is assigned. All the above does is reassign the value of the variable just before it is used for display purposes. Use at you own risk, no warranty, etc. I hope this helps...
          Hide
          Nigel Pegram added a comment - - edited

          I have a patch that causes the grade letters to be displayed.

          The patch is for Moodle 2.4.1+ (Build: 20130208), for mod/assign/locallib.php

          In essence, I add a check in the view_student_summary function. Just before the $feedbackstatus variable is constructed, I add a test to see if lettergrades are meant to be shown. If this is true, then $gradefordisplay is set to the grade letter, rather than the result of $this->display_grade.

          It would have seemed neater to me to add the test to disply_grade, but I couldn't find the necessary variables to test and set the letter within that function, so I left it in view_student_summary.

          I don't believe this has any impact elsewhere, but as usual, no warranty, YMMV.

          HTH,
          Nigel

          Patch begins on line below:

          --- /tmp/locallib.php
          +++ /var/www/pegram.homelinux.net/moodle_current/mod/assign/locallib.php
          @@ -2974,7 +2974,12 @@
                                                                                $gradebookgrade->str_long_grade,
                                                                                $cangrade);
                               } else {
          -                        $gradefordisplay = $this->display_grade($gradebookgrade->grade, false);
          +						$showlettergrade = grade_get_setting($this->courseid, 'report_user_showlettergrade', !empty($CFG->grade_report_user_showlettergrade));
          +						if ($showlettergrade && !$cangrade){
          +							$gradefordisplay= $gradebookgrade->str_grade ;
          +						} else {
          +							$gradefordisplay = $this->display_grade($gradebookgrade->grade, false);
          +						}
                               }
                               $gradeddate = $gradebookgrade->dategraded;
                               if (isset($grade->grader)) {
          
          Show
          Nigel Pegram added a comment - - edited I have a patch that causes the grade letters to be displayed. The patch is for Moodle 2.4.1+ (Build: 20130208), for mod/assign/locallib.php In essence, I add a check in the view_student_summary function. Just before the $feedbackstatus variable is constructed, I add a test to see if lettergrades are meant to be shown. If this is true, then $gradefordisplay is set to the grade letter, rather than the result of $this->display_grade. It would have seemed neater to me to add the test to disply_grade, but I couldn't find the necessary variables to test and set the letter within that function, so I left it in view_student_summary. I don't believe this has any impact elsewhere, but as usual, no warranty, YMMV. HTH, Nigel Patch begins on line below: --- /tmp/locallib.php +++ /var/www/pegram.homelinux.net/moodle_current/mod/assign/locallib.php @@ -2974,7 +2974,12 @@ $gradebookgrade->str_long_grade, $cangrade); } else { - $gradefordisplay = $this->display_grade($gradebookgrade->grade, false); + $showlettergrade = grade_get_setting($this->courseid, 'report_user_showlettergrade', !empty($CFG->grade_report_user_showlettergrade)); + if ($showlettergrade && !$cangrade){ + $gradefordisplay= $gradebookgrade->str_grade ; + } else { + $gradefordisplay = $this->display_grade($gradebookgrade->grade, false); + } } $gradeddate = $gradebookgrade->dategraded; if (isset($grade->grader)) {
          Hide
          Greg added a comment - - edited

          Hi Nigel,

          Just a thought; would it be better to make this change within the display_grade function to ensure a letter grade can be returned to plugins etc? This does mean that an additional parameter is required for the function.

          This isn't a complete patch as I haven't looked at gradingtable.php - just a thought

          diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php
          index bce42d2..f779fa9 100644
          --- a/mod/assign/locallib.php
          +++ b/mod/assign/locallib.php
          @@ -1131,7 +1131,7 @@ public function get_course() {
                * @param int $modified Timestamp from when the grade was last modified
                * @return string User-friendly representation of grade
                */
          -    public function display_grade($grade, $editing, $userid=0, $modified=0) {
          +    public function display_grade($grade, $editing, $long_grade = null, $userid=0, $modified=0) {
                   global $DB;
           
                   static $scalegrades = array();
          @@ -1167,10 +1167,14 @@ class="quickgrade"/>';
                               $o .= '-';
                               return $o;
                           } else {
          -                    $o .= format_float($grade, 2) .
          -                          ' / ' .
          -                          format_float($this->get_instance()->grade, 2);
          -                    return $o;
          +                    if(empty($long_grade)){
          +	                    $o .= format_float($grade, 2) .
          +	                          ' / ' .
          +	                          format_float($this->get_instance()->grade, 2);
          +	                    return $o;
          +                    } else {
          +	                    $o .= $long_grade;	
          +                    }
                           }
                       }
           
          @@ -3399,7 +3403,7 @@ public function view_student_summary($user, $showlinks) {
                                                                                $gradebookgrade->str_long_grade,
                                                                                $cangrade);
                               } else {
          -                        $gradefordisplay = $this->display_grade($gradebookgrade->grade, false);
          +                        $gradefordisplay = $this->display_grade($gradebookgrade->grade, false, $gradebookgrade->str_long_grade);
                               }
                               $gradeddate = $gradebookgrade->dategraded;
                               if (isset($grade->grader)) {
          

          Greg

          Show
          Greg added a comment - - edited Hi Nigel, Just a thought; would it be better to make this change within the display_grade function to ensure a letter grade can be returned to plugins etc? This does mean that an additional parameter is required for the function. This isn't a complete patch as I haven't looked at gradingtable.php - just a thought diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index bce42d2..f779fa9 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -1131,7 +1131,7 @@ public function get_course() { * @param int $modified Timestamp from when the grade was last modified * @ return string User-friendly representation of grade */ - public function display_grade($grade, $editing, $userid=0, $modified=0) { + public function display_grade($grade, $editing, $long_grade = null , $userid=0, $modified=0) { global $DB; static $scalegrades = array(); @@ -1167,10 +1167,14 @@ class= "quickgrade" />'; $o .= '-'; return $o; } else { - $o .= format_float($grade, 2) . - ' / ' . - format_float($ this ->get_instance()->grade, 2); - return $o; + if (empty($long_grade)){ + $o .= format_float($grade, 2) . + ' / ' . + format_float($ this ->get_instance()->grade, 2); + return $o; + } else { + $o .= $long_grade; + } } } @@ -3399,7 +3403,7 @@ public function view_student_summary($user, $showlinks) { $gradebookgrade->str_long_grade, $cangrade); } else { - $gradefordisplay = $ this ->display_grade($gradebookgrade->grade, false ); + $gradefordisplay = $ this ->display_grade($gradebookgrade->grade, false , $gradebookgrade->str_long_grade); } $gradeddate = $gradebookgrade->dategraded; if (isset($grade->grader)) { Greg
          Hide
          Nigel Pegram added a comment -

          Hi Greg,

          Yes, to change the function would seem to be a cleaner solution. (I hadn't considered the plugin issue.)

          Since I've not been part of the Moodle effort (and was after a quick fix) I tried to make the impact as minimal as possible.

          I did consider changing functions, but was hesitant since I didn't know what impact it might have elsewhere.

          If I was going to do what you suggest I would add the extra parameter to the end of the existing ones so that existing calls elsewhere in the code do not need any alteration. If I read things correctly, with your amendments, any calls which use $userid or $modified would behave unpredictably at best or fail.

          Your thoughts?

          Nigel

          Show
          Nigel Pegram added a comment - Hi Greg, Yes, to change the function would seem to be a cleaner solution. (I hadn't considered the plugin issue.) Since I've not been part of the Moodle effort (and was after a quick fix) I tried to make the impact as minimal as possible. I did consider changing functions, but was hesitant since I didn't know what impact it might have elsewhere. If I was going to do what you suggest I would add the extra parameter to the end of the existing ones so that existing calls elsewhere in the code do not need any alteration. If I read things correctly, with your amendments, any calls which use $userid or $modified would behave unpredictably at best or fail. Your thoughts? Nigel
          Hide
          Damyon Wiese added a comment -

          This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

          For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

          Show
          Damyon Wiese added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
          Hide
          carol howells added a comment -

          I have this problem since the recent upgrade to 2.4.1.
          My organisation and awarding body insist that we give out letter grades and not percentage grades. I thought this was a bug too as it used to work fine in 2.1 and it seems inconsistent. My students really use the assignment submission feedback page to view their grades. Despite setting it at site level, since the upgrade I cannot get a letter grade to show in the submission feedback page only the actual percentage mark. I am aware that you can tweak the gradebook quite a bit to show either. For me, this is serious set back and although I read all the release notes on 2.4, I did not realise that something that worked before would be lost in an upgrade. It's inconsistent and a core necessity of moodle that online assignments work.

          Show
          carol howells added a comment - I have this problem since the recent upgrade to 2.4.1. My organisation and awarding body insist that we give out letter grades and not percentage grades. I thought this was a bug too as it used to work fine in 2.1 and it seems inconsistent. My students really use the assignment submission feedback page to view their grades. Despite setting it at site level, since the upgrade I cannot get a letter grade to show in the submission feedback page only the actual percentage mark. I am aware that you can tweak the gradebook quite a bit to show either. For me, this is serious set back and although I read all the release notes on 2.4, I did not realise that something that worked before would be lost in an upgrade. It's inconsistent and a core necessity of moodle that online assignments work.
          Hide
          carol howells added a comment -

          Please help fix this issue. Why is is unassigned and why is it considered minor? It's essential for my organisation. I have spent days looking for an answer. Also I can't get this to work-
          removing the grade from the assignment feedback display and will let the students get their grades from the gradebook

          If I don’t put in a grade when marking the submission it does not appear on the feedback submission page but it also does not show in the grade book.

          Any help appreciated, I’ve posted to Moodle org and the JISC mailing lists but I can’t sort this and come the new academic year in September, this will be a major fault with Moodle.

          Show
          carol howells added a comment - Please help fix this issue. Why is is unassigned and why is it considered minor? It's essential for my organisation. I have spent days looking for an answer. Also I can't get this to work- removing the grade from the assignment feedback display and will let the students get their grades from the gradebook If I don’t put in a grade when marking the submission it does not appear on the feedback submission page but it also does not show in the grade book. Any help appreciated, I’ve posted to Moodle org and the JISC mailing lists but I can’t sort this and come the new academic year in September, this will be a major fault with Moodle.
          Hide
          carol howells added a comment -

          With the help of Tim Hunt from the OU I have managed to get this changed from minor bug to major. Please can someone be assigned to it now?

          Show
          carol howells added a comment - With the help of Tim Hunt from the OU I have managed to get this changed from minor bug to major. Please can someone be assigned to it now?
          Hide
          Sean Keogh added a comment -

          Hi,

          We have a number of academic customers that we are going to be upgrading over the summer, and many of them use grade letters so this will be a major blocker for them taking up moodle 2, from 1.9, or moving from earlier M2 to 2.4 or later.

          Please get someone to look at this as a matter of urgency, otherwise a lot of our partner customers will be very unhappy.

          Show
          Sean Keogh added a comment - Hi, We have a number of academic customers that we are going to be upgrading over the summer, and many of them use grade letters so this will be a major blocker for them taking up moodle 2, from 1.9, or moving from earlier M2 to 2.4 or later. Please get someone to look at this as a matter of urgency, otherwise a lot of our partner customers will be very unhappy.
          Hide
          Paul Nicholls added a comment -

          We've just encountered this - since nobody else seems to have picked it up, I've assigned it to myself. I'll tidy up Greg's patch and try it out; if all goes well, I'll submit it for peer review.

          Show
          Paul Nicholls added a comment - We've just encountered this - since nobody else seems to have picked it up, I've assigned it to myself. I'll tidy up Greg's patch and try it out; if all goes well, I'll submit it for peer review.
          Hide
          Paul Nicholls added a comment -

          Requesting peer review - patch is based on Greg's patch, with some tidying up and tweaks. Thanks, Greg!

          Show
          Paul Nicholls added a comment - Requesting peer review - patch is based on Greg's patch, with some tidying up and tweaks. Thanks, Greg!
          Hide
          Rajesh Taneja added a comment -

          Thanks for working on this Paul and Greg,

          Patch looks spot-on.
          Pushing for integration review.

          Show
          Rajesh Taneja added a comment - Thanks for working on this Paul and Greg, Patch looks spot-on. Pushing for integration review.
          Hide
          Marina Glancy added a comment -

          Thanks for working on it guys, this seems like a really needed change.
          Unfortunately I have to reopen it now because it does not fix all the screens when grade is displayed to student (example: attempts summary as in attached screenshot).

          At the same time this solution looks to me a little hacky. Function display_grades() is called from many places.
          Maybe it should be corrected everywhere, not just in one place, and use the logic: if user can grade, display the decimal (as it is now), otherwise - display string from grade.
          Please don't forget about contributed plugins and they can use this function too and they should not break after upgrade.

          Show
          Marina Glancy added a comment - Thanks for working on it guys, this seems like a really needed change. Unfortunately I have to reopen it now because it does not fix all the screens when grade is displayed to student (example: attempts summary as in attached screenshot). At the same time this solution looks to me a little hacky. Function display_grades() is called from many places. Maybe it should be corrected everywhere, not just in one place, and use the logic: if user can grade, display the decimal (as it is now), otherwise - display string from grade. Please don't forget about contributed plugins and they can use this function too and they should not break after upgrade.
          Hide
          Marina Glancy added a comment -

          Also there are plenty of unittests for mod_assign, it will be easy to add tests for this gradebook setting too.

          Show
          Marina Glancy added a comment - Also there are plenty of unittests for mod_assign, it will be easy to add tests for this gradebook setting too.
          Hide
          CiBoT added a comment -

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

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Paul Nicholls added a comment -

          I've redone this by making assign::display_grade() use grade_format_gradevalue() instead of relying on the calling function to pass an extra parameter. All branches in the pull details have been rebased and contain a single commit with this updated patch.

          This covers previous attempts - and anything else that uses assign::display_grade().

          Show
          Paul Nicholls added a comment - I've redone this by making assign::display_grade() use grade_format_gradevalue() instead of relying on the calling function to pass an extra parameter. All branches in the pull details have been rebased and contain a single commit with this updated patch. This covers previous attempts - and anything else that uses assign::display_grade().
          Hide
          carol howells added a comment - - edited

          Hi fab Moodlers thank you for all your work on this but I cannot express enough how URGENT this is to our organisation. My Management are considering disabling the use of the assignment tool in Moodle all together until this works as it did before we upgraded to 2.4. It is very difficult at the moment to sell Moodle (to already quite reluctant staff) when things like this break.

          Show
          carol howells added a comment - - edited Hi fab Moodlers thank you for all your work on this but I cannot express enough how URGENT this is to our organisation. My Management are considering disabling the use of the assignment tool in Moodle all together until this works as it did before we upgraded to 2.4. It is very difficult at the moment to sell Moodle (to already quite reluctant staff) when things like this break.
          Hide
          Rajesh Taneja added a comment -

          Sorry for the delay on this Paul and Carol,

          I am reviewing this now.

          Show
          Rajesh Taneja added a comment - Sorry for the delay on this Paul and Carol, I am reviewing this now.
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Paul,

          Patch looks good, few minor things to consider before pushing it for integration.

          1. There should be space before and after => in array. https://github.com/pauln/moodle/compare/master...MDL-37153_master#diff-c78f4d657da22cf5ddfb4d13335bb97eR1205
          2. You are just comparing grade display type for GRADE_DISPLAY_TYPE_REAL, should it be considered it for GRADE_DISPLAY_TYPE_REAL_PERCENTAGE, GRADE_DISPLAY_TYPE_REAL_LETTER, GRADE_DISPLAY_TYPE_PERCENTAGE_REAL and GRADE_DISPLAY_TYPE_LETTER_REAL ? Not sure about this, so added Andrew Davis for his thoughts.
          3. assign ($this) has all the required information like instance and course for retrieving grade_item. Is it necessary to get cm ?
          4. Also, is itemnumber important ?

          Rest all looks good.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Paul, Patch looks good, few minor things to consider before pushing it for integration. There should be space before and after => in array. https://github.com/pauln/moodle/compare/master...MDL-37153_master#diff-c78f4d657da22cf5ddfb4d13335bb97eR1205 You are just comparing grade display type for GRADE_DISPLAY_TYPE_REAL, should it be considered it for GRADE_DISPLAY_TYPE_REAL_PERCENTAGE, GRADE_DISPLAY_TYPE_REAL_LETTER, GRADE_DISPLAY_TYPE_PERCENTAGE_REAL and GRADE_DISPLAY_TYPE_LETTER_REAL ? Not sure about this, so added Andrew Davis for his thoughts. assign ($this) has all the required information like instance and course for retrieving grade_item. Is it necessary to get cm ? Also, is itemnumber important ? Rest all looks good.
          Hide
          Paul Nicholls added a comment -

          Hi Raj,
          1. Easily fixed - I'll take care of that.
          2. Without the patch, it's always just displayed as though it were GRADE_DISPLAY_TYPE_REAL - but with the addition of the total marks, presumably to give the student some idea how well they've done (otherwise, it's just a number - with no indication what it's out of). The combined options give either a percentage or a letter grade, which serves that purpose - and inserting the total marks would either require various string manipulations depending on display type (which could get messy) or just a generic "(out of 100)" at the end (which would need to be translated).
          3. I'll take a closer look and see if I can get all the required info out of $this rather than getting the cm. I had a quick look, but didn't see everything - so I figured it was simplest to just get the cm as it contains everything needed.
          4. As far as I can tell, itemnumber 0 denotes the primary grade item for the activity - I'm not even sure how you would end up with other grade items associated with the activity.

          I'll tweak again and request another review when I've pushed updated branches.

          Show
          Paul Nicholls added a comment - Hi Raj, 1. Easily fixed - I'll take care of that. 2. Without the patch, it's always just displayed as though it were GRADE_DISPLAY_TYPE_REAL - but with the addition of the total marks, presumably to give the student some idea how well they've done (otherwise, it's just a number - with no indication what it's out of). The combined options give either a percentage or a letter grade, which serves that purpose - and inserting the total marks would either require various string manipulations depending on display type (which could get messy) or just a generic "(out of 100)" at the end (which would need to be translated). 3. I'll take a closer look and see if I can get all the required info out of $this rather than getting the cm. I had a quick look, but didn't see everything - so I figured it was simplest to just get the cm as it contains everything needed. 4. As far as I can tell, itemnumber 0 denotes the primary grade item for the activity - I'm not even sure how you would end up with other grade items associated with the activity. I'll tweak again and request another review when I've pushed updated branches.
          Hide
          Paul Nicholls added a comment -

          Hi Rajesh Taneja,
          I've just pushed updated branches which address points 1 and 3. Please see my previous comment for feedback on points 2 and 4.

          Show
          Paul Nicholls added a comment - Hi Rajesh Taneja , I've just pushed updated branches which address points 1 and 3. Please see my previous comment for feedback on points 2 and 4.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the quick patch Paul and clarifying point 2.

          Looks good to me.
          Pushing for integration.

          Show
          Rajesh Taneja added a comment - Thanks for the quick patch Paul and clarifying point 2. Looks good to me. Pushing for integration.
          Hide
          Damyon Wiese added a comment -

          Point 4. itemnumber > 0 is used by outcomes which uses scales - so should not be affected by this.

          +1 from me.

          Show
          Damyon Wiese added a comment - Point 4. itemnumber > 0 is used by outcomes which uses scales - so should not be affected by this. +1 from me.
          Hide
          Paul Nicholls added a comment -

          Thanks for the quick re-review, Raj - and thanks for clarifying the itemnumber situation, Damyon.

          Show
          Paul Nicholls added a comment - Thanks for the quick re-review, Raj - and thanks for clarifying the itemnumber situation, Damyon.
          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
          Tim Hunt added a comment -

          The main use for itemnumber is where a single activity sends more than one grade to the gradebook. The only core module I know that does this is the Workshop (grade for submission and grade for assessment). This may not be relevant to the Assignment (at least no to date), but I just wanted to explain.

          Show
          Tim Hunt added a comment - The main use for itemnumber is where a single activity sends more than one grade to the gradebook. The only core module I know that does this is the Workshop (grade for submission and grade for assessment). This may not be relevant to the Assignment (at least no to date), but I just wanted to explain.
          Hide
          Marina Glancy added a comment -

          Thanks for the change.
          Just want to double check with Damyon Wiese that it is ok since this patch adds 2 DB queries for each graded assignment (regardless whether we display grades as letters or not), which is especially noticeable on assignment grading page.
          I was expecting the patch to check if user does not have mod:assign/grade capability, something like this:

          diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php
          index 3ae3cb9..33498b1 100644
          --- a/mod/assign/locallib.php
          +++ b/mod/assign/locallib.php
          @@ -1201,11 +1201,16 @@ class assign {
                           if ($grade == -1 || $grade === null) {
                               $o .= '-';
                               return $o;
          -                } else {
          +                } else if (has_capability('mod/assign:grade', $this->context)) {
                               $o .= format_float($grade, 2) .
                                     ' / ' .
                                     format_float($this->get_instance()->grade, 2);
                               return $o;
          +                } else {
          +                    $instance = $this->get_instance();
          +                    $item = grade_item::fetch(array('itemtype' => 'mod', 'itemmodule' => 'assign', 'iteminstance' => $instance->id, 'courseid' => $instance->cour
          +                    $o .= grade_format_gradevalue($grade, $item);
          +                    return $o;
                           }
                       }
           
          
          Show
          Marina Glancy added a comment - Thanks for the change. Just want to double check with Damyon Wiese that it is ok since this patch adds 2 DB queries for each graded assignment (regardless whether we display grades as letters or not), which is especially noticeable on assignment grading page. I was expecting the patch to check if user does not have mod:assign/grade capability, something like this: diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index 3ae3cb9..33498b1 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -1201,11 +1201,16 @@ class assign { if ($grade == -1 || $grade === null ) { $o .= '-'; return $o; - } else { + } else if (has_capability('mod/assign:grade', $ this ->context)) { $o .= format_float($grade, 2) . ' / ' . format_float($ this ->get_instance()->grade, 2); return $o; + } else { + $instance = $ this ->get_instance(); + $item = grade_item::fetch(array('itemtype' => 'mod', 'itemmodule' => 'assign', 'iteminstance' => $instance->id, 'courseid' => $instance->cour + $o .= grade_format_gradevalue($grade, $item); + return $o; } }
          Hide
          Damyon Wiese added a comment -

          Thanks Marina, I like your version better (same result, less DB queries, in either case we are adding special logic to the return from grade_format_gradevalue so same/same).

          Ideally I wouldn't need any of this code and the grade_format_gradevalue would do something sensible in all cases (and use it's own static cache). Never mind.

          Did you want me to post an updated branch for this?

          Show
          Damyon Wiese added a comment - Thanks Marina, I like your version better (same result, less DB queries, in either case we are adding special logic to the return from grade_format_gradevalue so same/same). Ideally I wouldn't need any of this code and the grade_format_gradevalue would do something sensible in all cases (and use it's own static cache). Never mind. Did you want me to post an updated branch for this?
          Hide
          Marina Glancy added a comment -

          Thanks Damyon, branch would be nice. Note also that in my diff I removed part of Paul's code:
          https://github.com/pauln/moodle/compare/master...MDL-37153_master#diff-c78f4d657da22cf5ddfb4d13335bb97eR1207
          (lines 1207-1210)
          So it's up to you how to display grades. Thanks again

          PS From code review it looks like if Advanced grading methods (for example Rubrics) are used, this code is unreachable anyway and student will see his grade in grading method's format. Hope it is an expected behaviour

          Show
          Marina Glancy added a comment - Thanks Damyon, branch would be nice. Note also that in my diff I removed part of Paul's code: https://github.com/pauln/moodle/compare/master...MDL-37153_master#diff-c78f4d657da22cf5ddfb4d13335bb97eR1207 (lines 1207-1210) So it's up to you how to display grades. Thanks again PS From code review it looks like if Advanced grading methods (for example Rubrics) are used, this code is unreachable anyway and student will see his grade in grading method's format. Hope it is an expected behaviour
          Hide
          Damyon Wiese added a comment -

          That is intended with advanced grading methods - it's up to them how they display the grade.

          Show
          Damyon Wiese added a comment - That is intended with advanced grading methods - it's up to them how they display the grade.
          Hide
          Damyon Wiese added a comment -

          Just running unit tests - will post a new branch when they finish

          Show
          Damyon Wiese added a comment - Just running unit tests - will post a new branch when they finish
          Hide
          Paul Nicholls added a comment - - edited

          The bit on lines 1207-1210 is quite important - without it, students won't have any idea what the grade is out of if the grade item is set to GRADE_DISPLAY_TYPE_REAL (when looking at the assignment, that is - which may be the only way for students to see their grades, depending on course setup). A number with no context is entirely meaningless - "45" could be a lousy score (out of 100), or a perfect score (out of 45). GRADE_DISPLAY_TYPE_REAL is the only display type which has this problem, and is addressed by the existing/old code in the assignment module by tacking on what it's out of - which is exactly what 1207-1210 does.

          I hadn't realised that the list of all submissions was calling this repeatedly; some kind of caching of the grade item is certainly a good idea, but I don't think it's a good idea to just default to GRADE_DISPLAY_TYPE_REAL if you're grading - it introduces inconsistencies which could lead to confusion. For instance, if an assignment's grade item is set to GRADE_DISPLAY_TYPE_PERCENTAGE but the assignment is graded out of 80, the teacher and student will see different numbers (yes, one will have a "%" after it, but that's not sufficient to prevent confusion - especially if the student omits the "percent" when talking about it - "I got 80").

          Show
          Paul Nicholls added a comment - - edited The bit on lines 1207-1210 is quite important - without it, students won't have any idea what the grade is out of if the grade item is set to GRADE_DISPLAY_TYPE_REAL (when looking at the assignment, that is - which may be the only way for students to see their grades, depending on course setup). A number with no context is entirely meaningless - "45" could be a lousy score (out of 100), or a perfect score (out of 45). GRADE_DISPLAY_TYPE_REAL is the only display type which has this problem, and is addressed by the existing/old code in the assignment module by tacking on what it's out of - which is exactly what 1207-1210 does. I hadn't realised that the list of all submissions was calling this repeatedly; some kind of caching of the grade item is certainly a good idea, but I don't think it's a good idea to just default to GRADE_DISPLAY_TYPE_REAL if you're grading - it introduces inconsistencies which could lead to confusion. For instance, if an assignment's grade item is set to GRADE_DISPLAY_TYPE_PERCENTAGE but the assignment is graded out of 80, the teacher and student will see different numbers (yes, one will have a "%" after it, but that's not sufficient to prevent confusion - especially if the student omits the "percent" when talking about it - "I got 80").
          Hide
          Paul Nicholls added a comment -

          Perhaps the best way to deal with the extra queries is to add another function, get_gradeitem(), which fetches the grade item and caches it so that it can be reused, as get_instance() currently does. I'm in the middle of something else right now, so I don't have time to do this today - but I can do it tomorrow morning if nobody else can take care of it before then (assuming that Marina and Damyon agree with this approach).

          Show
          Paul Nicholls added a comment - Perhaps the best way to deal with the extra queries is to add another function, get_gradeitem(), which fetches the grade item and caches it so that it can be reused, as get_instance() currently does. I'm in the middle of something else right now, so I don't have time to do this today - but I can do it tomorrow morning if nobody else can take care of it before then (assuming that Marina and Damyon agree with this approach).
          Hide
          Damyon Wiese added a comment -

          No Paul - sorry - I'm not in favour of adding extra complexity to a backported bug fix - and I don't really want this extra complexity in mod_assign (it is the gradebooks responsibility really).

          I'm testing branches now and will post them in a minute.

          There will be no visual difference for students or teachers in the new patch (from your version).

          Show
          Damyon Wiese added a comment - No Paul - sorry - I'm not in favour of adding extra complexity to a backported bug fix - and I don't really want this extra complexity in mod_assign (it is the gradebooks responsibility really). I'm testing branches now and will post them in a minute. There will be no visual difference for students or teachers in the new patch (from your version).
          Hide
          Damyon Wiese added a comment -

          Branches updated.

          Show
          Damyon Wiese added a comment - Branches updated.
          Hide
          Paul Nicholls added a comment -

          Hi Damyon,
          There is a visual difference between yours and mine for teachers - in the list of submissions, the "final grade" column obeys the grade item's display setting in mine, but does not in yours (it shows the original "12.00 / 34.00" style).

          I understand where you're coming from re: complexity in a backported bugfix, but in this case it's not very complex - I'm not suggesting adding MUC into the mix (or anything else like that), just adding another private variable to the "assign" class, and a getter for it (which hits the DB if the variable isn't already set). It would actually simplify the changes in display_grade(), as it would avoid adding the "else if" branch and replace the grade_item::fetch() call with "$this->get_gradeitem()".

          Would you be willing to take a look at it and reconsider if I supply branches (just links in the comments here, only replacing the pull details if you agree? I can do it first thing tomorrow morning.

          Show
          Paul Nicholls added a comment - Hi Damyon, There is a visual difference between yours and mine for teachers - in the list of submissions, the "final grade" column obeys the grade item's display setting in mine, but does not in yours (it shows the original "12.00 / 34.00" style). I understand where you're coming from re: complexity in a backported bugfix, but in this case it's not very complex - I'm not suggesting adding MUC into the mix (or anything else like that), just adding another private variable to the "assign" class, and a getter for it (which hits the DB if the variable isn't already set). It would actually simplify the changes in display_grade(), as it would avoid adding the "else if" branch and replace the grade_item::fetch() call with "$this->get_gradeitem()". Would you be willing to take a look at it and reconsider if I supply branches (just links in the comments here, only replacing the pull details if you agree? I can do it first thing tomorrow morning.
          Hide
          Marina Glancy added a comment -

          I'm holding integration then, waiting for you guys to agree.

          IMHO it is logical that teachers see grade in points

          Show
          Marina Glancy added a comment - I'm holding integration then, waiting for you guys to agree. IMHO it is logical that teachers see grade in points
          Hide
          Paul Nicholls added a comment -

          The grade is displayed in points (including the maximum grade) in the "Grade" column in the table of submissions (which becomes editable if quick grading is enabled), so displaying it again in the "Final grade" column instead of displaying it according to the grade item's display mode doesn't make sense to me - it's just pointless duplication. Making the "Final grade" column obey the grade item's display mode actually aligns it better with the individual assignment grading screen, which displays the "Current grade in gradebook" according to the display mode.

          Show
          Paul Nicholls added a comment - The grade is displayed in points (including the maximum grade) in the "Grade" column in the table of submissions (which becomes editable if quick grading is enabled), so displaying it again in the "Final grade" column instead of displaying it according to the grade item's display mode doesn't make sense to me - it's just pointless duplication. Making the "Final grade" column obey the grade item's display mode actually aligns it better with the individual assignment grading screen, which displays the "Current grade in gradebook" according to the display mode.
          Hide
          Damyon Wiese added a comment -

          Yes - I'll take another look if you update the patch (feel free to overwrite the pull details on the issue). If we are going to need to load the grade_item for every row in the table - I would rather we do a single query to fetch all the grade_items for the table and populate some assign::grade_item cache instead.

          Show
          Damyon Wiese added a comment - Yes - I'll take another look if you update the patch (feel free to overwrite the pull details on the issue). If we are going to need to load the grade_item for every row in the table - I would rather we do a single query to fetch all the grade_items for the table and populate some assign::grade_item cache instead.
          Hide
          Paul Nicholls added a comment -

          I've just finished rebasing and updating; the pull details are back to my repo, and I've updated the existing branches with the latest patch.

          It doesn't behave quite how I'd intended, but having looked at the Grader Report and spoken with my colleagues (who work closely with teaching staff) about it, it seems to be a sensible approach. Just like the Grader Report, it now shows the grade according to the grade item's display setting until you turn quick grading on (equivalent to turning editing on in the Grader Report), at which stage it switches to displaying the raw grade in the "Grade" column; the "Final grade" column always displays according to the display setting. This additional consistency between the Grader Report and Assignment grading screen should help reduce confusion.

          Show
          Paul Nicholls added a comment - I've just finished rebasing and updating; the pull details are back to my repo, and I've updated the existing branches with the latest patch. It doesn't behave quite how I'd intended, but having looked at the Grader Report and spoken with my colleagues (who work closely with teaching staff) about it, it seems to be a sensible approach. Just like the Grader Report, it now shows the grade according to the grade item's display setting until you turn quick grading on (equivalent to turning editing on in the Grader Report), at which stage it switches to displaying the raw grade in the "Grade" column; the "Final grade" column always displays according to the display setting. This additional consistency between the Grader Report and Assignment grading screen should help reduce confusion.
          Hide
          Damyon Wiese added a comment -

          Thanks Paul,

          This new patch seems almost right - but $instance is not defined:

          mod_assign_locallib_testcase::test_submission_graded_event
          Undefined variable: instance

          /home/damyonw/Documents/Moodle/integration/master/moodle/mod/assign/locallib.php:1234

          Thanks for asking some teachers - that helps alot!

          Show
          Damyon Wiese added a comment - Thanks Paul, This new patch seems almost right - but $instance is not defined: mod_assign_locallib_testcase::test_submission_graded_event Undefined variable: instance /home/damyonw/Documents/Moodle/integration/master/moodle/mod/assign/locallib.php:1234 Thanks for asking some teachers - that helps alot!
          Hide
          Paul Nicholls added a comment -

          Whoops - I got lazy and didn't test enough cases to pick that one up. I've just pushed a fix - I should've switched it back from $instance->grade to $this->get_instance()->grade when I switched to using the new get_grade_item() function.

          Show
          Paul Nicholls added a comment - Whoops - I got lazy and didn't test enough cases to pick that one up. I've just pushed a fix - I should've switched it back from $instance->grade to $this->get_instance()->grade when I switched to using the new get_grade_item() function.
          Hide
          Damyon Wiese added a comment -

          OK - thanks Paul, looks good to me now.

          Show
          Damyon Wiese added a comment - OK - thanks Paul, looks good to me now.
          Hide
          Marina Glancy added a comment -

          sorry I don't understand how this solves the performance loss. Will talk to Damyon when he comes online

          Show
          Marina Glancy added a comment - sorry I don't understand how this solves the performance loss. Will talk to Damyon when he comes online
          Hide
          Marina Glancy added a comment -

          Thanks guys, this has been integrated.

          PS talked to Damyon and understood that I was not right - for some reason I was thinking that grade_item is different for each student.

          Show
          Marina Glancy added a comment - Thanks guys, this has been integrated. PS talked to Damyon and understood that I was not right - for some reason I was thinking that grade_item is different for each student.
          Hide
          Paul Nicholls added a comment -

          Marina, the new get_grade_item() function caches the grade_item record the same way that get_instance does - so the assignment grading page only fetches it from the database once. The list of assignments in the course (/mod/assign/index.php?id=xx) seems to just use the str_grade from the gradebook rather than using display_grade(), so shouldn't be affected by this change at all. Is there any other performance issue that you've noticed from this change?

          Show
          Paul Nicholls added a comment - Marina, the new get_grade_item() function caches the grade_item record the same way that get_instance does - so the assignment grading page only fetches it from the database once. The list of assignments in the course (/mod/assign/index.php?id=xx) seems to just use the str_grade from the gradebook rather than using display_grade(), so shouldn't be affected by this change at all. Is there any other performance issue that you've noticed from this change?
          Hide
          Ankit Agarwal added a comment -

          I noticed following error while trying to submit an assignment:-

          Warning: logged very long URL
          line 1687 of /lib/datalib.php: call to debugging()
          line 5809 of /lib/moodlelib.php: call to add_to_log()
          line 91 of /message/output/email/message_output_email.php: call to email_to_user()
          line 225 of /lib/messagelib.php: call to message_output_email->send_message()
          line 4386 of /mod/assign/locallib.php: call to message_send()
          line 4415 of /mod/assign/locallib.php: call to assign::send_assignment_notification()
          line 4467 of /mod/assign/locallib.php: call to assign->send_notification()
          line 5320 of /mod/assign/locallib.php: call to assign->notify_student_submission_receipt()
          line 5364 of /mod/assign/locallib.php: call to assign->save_submission()
          line 384 of /mod/assign/locallib.php: call to assign->process_save_submission()
          line 53 of /mod/assign/view.php: call to assign->view()
          

          Doesn't look related to this issue. I will debug it and see where it is coming from and report back.

          Show
          Ankit Agarwal added a comment - I noticed following error while trying to submit an assignment:- Warning: logged very long URL line 1687 of /lib/datalib.php: call to debugging() line 5809 of /lib/moodlelib.php: call to add_to_log() line 91 of /message/output/email/message_output_email.php: call to email_to_user() line 225 of /lib/messagelib.php: call to message_output_email->send_message() line 4386 of /mod/assign/locallib.php: call to message_send() line 4415 of /mod/assign/locallib.php: call to assign::send_assignment_notification() line 4467 of /mod/assign/locallib.php: call to assign->send_notification() line 5320 of /mod/assign/locallib.php: call to assign->notify_student_submission_receipt() line 5364 of /mod/assign/locallib.php: call to assign->save_submission() line 384 of /mod/assign/locallib.php: call to assign->process_save_submission() line 53 of /mod/assign/view.php: call to assign->view() Doesn't look related to this issue. I will debug it and see where it is coming from and report back.
          Hide
          Ankit Agarwal added a comment -

          The error was not related, reported it in MDL-42275

          Show
          Ankit Agarwal added a comment - The error was not related, reported it in MDL-42275
          Hide
          Ankit Agarwal added a comment -

          Works as described on 25 and master.

          However this step
          "Repeat test by changing grade in "simple view' to different options and make sure expected grade is visible"

          is failing for me on 24. Once I revert the grade to show %, it still keeps on showing grade in letters as student.

          Failing test, sorry.

          Show
          Ankit Agarwal added a comment - Works as described on 25 and master. However this step "Repeat test by changing grade in "simple view' to different options and make sure expected grade is visible" is failing for me on 24. Once I revert the grade to show %, it still keeps on showing grade in letters as student. Failing test, sorry.
          Hide
          Damyon Wiese added a comment -

          Sending back to testing

          Show
          Damyon Wiese added a comment - Sending back to testing
          Hide
          Ankit Agarwal added a comment -

          Works as expected, was looking at the wrong activity.

          Passing.

          Show
          Ankit Agarwal added a comment - Works as expected, was looking at the wrong activity. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels.

          Or, if you prefer, yes, you fixed that boring issue.

          Thanks anyway! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels. Or, if you prefer, yes, you fixed that boring issue. Thanks anyway! Ciao
          Hide
          carol howells added a comment - - edited

          The student view

          Show
          carol howells added a comment - - edited The student view
          Hide
          carol howells added a comment -

          teacher view

          Show
          carol howells added a comment - teacher view
          Hide
          carol howells added a comment -

          Thanks for all your efforts but I can't see this is fixed- it seems the same. I just set up a new assignment to test it. I logged in with my test student account and submitted a file. I then logged in and graded it. The numerical grade is still showing in the submission page and not the letter grade from a student view

          Show
          carol howells added a comment - Thanks for all your efforts but I can't see this is fixed- it seems the same. I just set up a new assignment to test it. I logged in with my test student account and submitted a file. I then logged in and graded it. The numerical grade is still showing in the submission page and not the letter grade from a student view
          Hide
          Paul Nicholls added a comment -

          Carol, have you upgraded to the latest 2.4.6+ weekly release (after it was released on Friday)? If you're not able to upgrade at this stage, it's possible to apply the patch to your site separately - but you'll need your developer (or whoever looks after the technical side of your Moodle installation) to do it, as there's a chance that it won't apply cleanly and will need some manual intervention.

          Show
          Paul Nicholls added a comment - Carol, have you upgraded to the latest 2.4.6+ weekly release (after it was released on Friday)? If you're not able to upgrade at this stage, it's possible to apply the patch to your site separately - but you'll need your developer (or whoever looks after the technical side of your Moodle installation) to do it, as there's a chance that it won't apply cleanly and will need some manual intervention.
          Hide
          carol howells added a comment -

          Thanks. I will try and get the patch installed - I am assuming it is this one?

          http://git.moodle.org/gw?p=moodle.git;a=tree;hb=refs/heads/MOODLE_24_STABLE

          Show
          carol howells added a comment - Thanks. I will try and get the patch installed - I am assuming it is this one? http://git.moodle.org/gw?p=moodle.git;a=tree;hb=refs/heads/MOODLE_24_STABLE
          Hide
          Paul Nicholls added a comment -

          Carol, the patch for this issue can be found at http://git.moodle.org/gw?p=moodle.git;a=patch;h=3e2821e6f5f021782f8d2dd0854cf029de2f58b3 - the link you posted just shows the latest commits to the MOODLE_24_STABLE branch. Hope that helps.

          Show
          Paul Nicholls added a comment - Carol, the patch for this issue can be found at http://git.moodle.org/gw?p=moodle.git;a=patch;h=3e2821e6f5f021782f8d2dd0854cf029de2f58b3 - the link you posted just shows the latest commits to the MOODLE_24_STABLE branch. Hope that helps.
          Hide
          carol howells added a comment -

          Hi fab moodlers, the patch has been installed and the assignment issue has now been fixed. Thanks to you all.

          Can I ask a quick question- the quiz module now seems to be doing the same thing- that is, it shows a number mark out of 100 from the student attempt page but in the grade book it displays correctly and shows a letter grade. Is this a similar issue that may require a patch? I'm using 2.4

          Show
          carol howells added a comment - Hi fab moodlers, the patch has been installed and the assignment issue has now been fixed. Thanks to you all. Can I ask a quick question- the quiz module now seems to be doing the same thing- that is, it shows a number mark out of 100 from the student attempt page but in the grade book it displays correctly and shows a letter grade. Is this a similar issue that may require a patch? I'm using 2.4
          Hide
          Marina Glancy added a comment -

          not afaik. You need to create a new issue with Quiz component

          Show
          Marina Glancy added a comment - not afaik. You need to create a new issue with Quiz component

            People

            • Votes:
              9 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: