Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-43071

Cannot import grades when using non-numeric scale (in certain cases)

    Details

    • Testing Instructions:
      Hide
      1. Create an assignment with non-numerical grading scale, and using the Offline feedback type
      2. Make students submit an assignment.
      3. Open grading page and choose grading action "Download grading worksheet".
      4. Open downloaded fine and add grades, make sure grades you use are defined in chosen scale.
      5. On the grading page choose "Upload grading worksheet" and upload the file with changes.
      6. Make sure that all new grades are reflected on inspection page and recorded after confirmation.
      Show
      Create an assignment with non-numerical grading scale, and using the Offline feedback type Make students submit an assignment. Open grading page and choose grading action "Download grading worksheet". Open downloaded fine and add grades, make sure grades you use are defined in chosen scale. On the grading page choose "Upload grading worksheet" and upload the file with changes. Make sure that all new grades are reflected on inspection page and recorded after confirmation.
    • Workaround:
      Hide

      In the scale definition, scale could be defined with no spaces after comma, e.g. "A,B,C,D".

      Show
      In the scale definition, scale could be defined with no spaces after comma, e.g. "A,B,C,D".
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull Master Branch:
      MDL-43071-master

      Description

      Grades importing when non-numeric scale in use does not work. The problem is in scale options conversion used in offline grades upload form and its processing. More precisely, values are not trimmed, so "A, B, C, D" scale is converted into array of:

      1 => A,
      2 => <space>B,
      3 => <space>C,
      4 => <space>D
      

      Which in turn misleads the importing check done by array_search function that finds if csv file grades matches the scale.

      Notice that using space after comma is suggested in the scale field help on scale editing page. Defining scale with no spaces is workaround, but ideally trimming needs to be implemented rather help file changes and existing scale definition fields upgrade.

        Gliffy Diagrams

          Activity

          Hide
          kabalin Ruslan Kabalin added a comment -

          The patch will follow in a moment.

          Show
          kabalin Ruslan Kabalin added a comment - The patch will follow in a moment.
          Hide
          salvetore Michael de Raadt added a comment -

          Thanks for reporiting that and providing a fix. Hopefully it will be reviewed soon.

          Show
          salvetore Michael de Raadt added a comment - Thanks for reporiting that and providing a fix. Hopefully it will be reviewed soon.
          Hide
          poltawski Dan Poltawski added a comment -

          Hi Ruslan,

          It looks ok to me - could you explain about the +1 being removed from the grade?

          Show
          poltawski Dan Poltawski added a comment - Hi Ruslan, It looks ok to me - could you explain about the +1 being removed from the grade?
          Hide
          kabalin Ruslan Kabalin added a comment -

          Hi Dan, that index shifting is done already in make_menu_from_list, so +1 removed.

          Show
          kabalin Ruslan Kabalin added a comment - Hi Dan, that index shifting is done already in make_menu_from_list, so +1 removed.
          Hide
          poltawski Dan Poltawski added a comment -

          Sending for integration (and hoping Damyon can look it)

          Show
          poltawski Dan Poltawski added a comment - Sending for integration (and hoping Damyon can look it)
          Hide
          damyon Damyon Wiese added a comment -

          +1 from me. The explanation and the patch both make sense (but haven't tested it)

          Show
          damyon Damyon Wiese added a comment - +1 from me. The explanation and the patch both make sense (but haven't tested it)
          Hide
          marina Marina Glancy added a comment -

          Hi Ruslan, Damyon.

          Changing the explode() into make_menu_from_list() is a great move.

          Lines 135-140 in mod/assign/feedback/offline/importgradesform.php look like an improvement:
          https://github.com/lucisgit/moodle/commit/3bc1bd6ee8fd5d86e3217b9bcadd74b9a0814af6#diff-4db3a9a7d695ca681152db229b85d030R133
          Are you sure it's ok to go in stable versions?
          In any case you have to make sure that grade was found in scaleoptions. As I see from the code, it can be ==='' and in this case the "undefined index" warning will be shown.

          Show
          marina Marina Glancy added a comment - Hi Ruslan, Damyon. Changing the explode() into make_menu_from_list() is a great move. Lines 135-140 in mod/assign/feedback/offline/importgradesform.php look like an improvement: https://github.com/lucisgit/moodle/commit/3bc1bd6ee8fd5d86e3217b9bcadd74b9a0814af6#diff-4db3a9a7d695ca681152db229b85d030R133 Are you sure it's ok to go in stable versions? In any case you have to make sure that grade was found in scaleoptions. As I see from the code, it can be ==='' and in this case the "undefined index" warning will be shown.
          Hide
          kabalin Ruslan Kabalin added a comment -

          Hi Marina, thanks for review.

          Without lines 135-140 the fix does not work I an afraid. Did not quite get what you mean by using ===? It is already compared using !==.

          Show
          kabalin Ruslan Kabalin added a comment - Hi Marina, thanks for review. Without lines 135-140 the fix does not work I an afraid. Did not quite get what you mean by using ===? It is already compared using !==.
          Hide
          marina Marina Glancy added a comment -

          Thanks Ruslan, integrated in 2.5, 2.6 and master

          my bad, I did not notice that empty grade is being skipped.

          Show
          marina Marina Glancy added a comment - Thanks Ruslan, integrated in 2.5, 2.6 and master my bad, I did not notice that empty grade is being skipped.
          Hide
          dobedobedoh Andrew Nicols added a comment -

          Tested and passed as per testing instructions.
          However, I notice that if I put whitespace around the grade in the uploaded file, it does not manage to match it. Should we trim whitespace or not? I'll leave it up to you to decide whether we should raise a new issue here.

          Cheers,

          Andrew

          Show
          dobedobedoh Andrew Nicols added a comment - Tested and passed as per testing instructions. However, I notice that if I put whitespace around the grade in the uploaded file, it does not manage to match it. Should we trim whitespace or not? I'll leave it up to you to decide whether we should raise a new issue here. Cheers, Andrew
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks for the code, its now upstream!

          Heres a fun trick to try in the spirit of Friday the 13th.
          I hear if you stand in front a mirror, alone, in the dark, and say "Oracle" three times Petr Skoka will appear in the mirror and you'll see him deleting the Oracle driver from Moodle.

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks for the code, its now upstream! Heres a fun trick to try in the spirit of Friday the 13th. I hear if you stand in front a mirror, alone, in the dark, and say "Oracle" three times Petr Skoka will appear in the mirror and you'll see him deleting the Oracle driver from Moodle.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                13/Jan/14