Moodle
  1. Moodle
  2. MDL-33853

Locking a submission creates a grade entry for a submission

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Assignment
    • Labels:
    • Testing Instructions:
      Hide
      1. Create a new assignment in a course with some students
      2. Go to the assignment grading page (View/grade all submissions)
      3. Choose "Prevent submission changes"
      4. Final grade column for that student should remain as "-"
      5. Last modified (grade) column for that student should remain as "-"
      Show
      Create a new assignment in a course with some students Go to the assignment grading page (View/grade all submissions) Choose "Prevent submission changes" Final grade column for that student should remain as "-" Last modified (grade) column for that student should remain as "-"
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      41953

      Description

      When a student submits, a teacher can "Lock" their submission. When they do this, the student gets a grade of zero.

      Replication steps:

      1. Create a new assignment
      2. Log in as a student and submit
      3. Log in as admin and lock the student's submission

      This zero grade is in the gradebook - you can see it in the right hand columnn of the grading table and the student sees it on their submission summary.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          To be honest, I'm not sure I fully understand what is intended by locking.

          Show
          Michael de Raadt added a comment - To be honest, I'm not sure I fully understand what is intended by locking.
          Hide
          Damyon Wiese added a comment -

          As it was explained to me, it will be used when someone wants to grade an assignment and they want to be sure the student cannot make any more changes to their submission. (E.g. if prevent late submissions is off)

          Show
          Damyon Wiese added a comment - As it was explained to me, it will be used when someone wants to grade an assignment and they want to be sure the student cannot make any more changes to their submission. (E.g. if prevent late submissions is off)
          Hide
          Damyon Wiese added a comment -

          The cause was that the assignment was pushing a rawgrade of -1 to the gradebook and it was being saved as 0. The fix is to only push rawgrades to the gradebook if they are >= 0. This will still allow feedback into the gradebook even if there is no grade.

          Show
          Damyon Wiese added a comment - The cause was that the assignment was pushing a rawgrade of -1 to the gradebook and it was being saved as 0. The fix is to only push rawgrades to the gradebook if they are >= 0. This will still allow feedback into the gradebook even if there is no grade.
          Hide
          Michael de Raadt added a comment - - edited

          Hi, Damyon.

          The change does prevent a grade value from being set, which is good. But it still looks like grading has been done. The dategraded value is still set and that date appears in the assignment grading table, so it looks like the submission has been graded. I'm wondering if that should also be avoided here. Alternately, the display in the table might need to be checked. We've had a similar issue with the old assignment in the past as teachers find this sort of thing confusing (and rightly so).

          The consequential affect on the gradebook looked correct to me.

          Show
          Michael de Raadt added a comment - - edited Hi, Damyon. The change does prevent a grade value from being set, which is good. But it still looks like grading has been done. The dategraded value is still set and that date appears in the assignment grading table, so it looks like the submission has been graded. I'm wondering if that should also be avoided here. Alternately, the display in the table might need to be checked. We've had a similar issue with the old assignment in the past as teachers find this sort of thing confusing (and rightly so). The consequential affect on the gradebook looked correct to me.
          Hide
          Damyon Wiese added a comment -

          I guess we need to be clear on what exactly we mean by the column "modified (grade)" and exactly when we should update the lastmodified date on the assign_grades table.

          Possible "grading" actions:

          • Give feedback
          • Revert to draft
          • Lock submission
          • Unlock submission
          • Modify the value of the grade

          The definition in my head (not well documented anywhere but this ticket) is "grade record modified by a grader".

          Probably the clearest definition for teachers would be: "value of the grade column modified". This would mean the last modified date is only changed for the last of these actions. This gives one issue which is that the lastmodified column is NOT NULL - so we have to set it when the grade record is created (which could be from any of these actions).

          My suggestion for 2.3 is that we leave this as the current behaviour (lastmodified column is modified when any grading action occurs), but hide it from the display until there is a value in the grade column.

          For 2.4 we could look at changing the column definition and only updating it for actions that modify the grade column.

          Does this sound OK?

          Show
          Damyon Wiese added a comment - I guess we need to be clear on what exactly we mean by the column "modified (grade)" and exactly when we should update the lastmodified date on the assign_grades table. Possible "grading" actions: Give feedback Revert to draft Lock submission Unlock submission Modify the value of the grade The definition in my head (not well documented anywhere but this ticket) is "grade record modified by a grader". Probably the clearest definition for teachers would be: "value of the grade column modified". This would mean the last modified date is only changed for the last of these actions. This gives one issue which is that the lastmodified column is NOT NULL - so we have to set it when the grade record is created (which could be from any of these actions). My suggestion for 2.3 is that we leave this as the current behaviour (lastmodified column is modified when any grading action occurs), but hide it from the display until there is a value in the grade column. For 2.4 we could look at changing the column definition and only updating it for actions that modify the grade column. Does this sound OK?
          Hide
          Damyon Wiese added a comment -

          Branch updated to hide grade modified date in grading table if there is no grade.

          Show
          Damyon Wiese added a comment - Branch updated to hide grade modified date in grading table if there is no grade.
          Hide
          Michael de Raadt added a comment - - edited

          I think teachers will consider something graded if (and only if) they have set a grade or given feedback.

          I wasn't sure how values were being set in the grades table, so I tested this out. Unfortunately, with this current solution, if a teacher leaves feedback without setting a grade value then no graded time is shown. Perhaps that's acceptable: there's no grade value, it's not fully graded, no time is shown. I guess I could go either way there. If that's how you are thinking, this is OK.

          I noted that the feedback information is not available to the col_timemarked() function, at least not at this stage, so extending your conjunctive test to check for the presence of feedback would require additional information to be added to the $row object beforehand.

          Why is the locked value with a grade anyway? If the lock applies to a submission, why is it not in the submission table with the submission information. Then all this compensating for creating a false grade record would not be a problem. Also, in my mind, there could possibly be a 1:many relationship between a submission its grades, but adding the lock to the grade entry assumes a 1:1 relationship. Is that what you mean by your point about 2.4? I think it would be easier to make that change now rather than later.

          Show
          Michael de Raadt added a comment - - edited I think teachers will consider something graded if (and only if) they have set a grade or given feedback. I wasn't sure how values were being set in the grades table, so I tested this out. Unfortunately, with this current solution, if a teacher leaves feedback without setting a grade value then no graded time is shown. Perhaps that's acceptable: there's no grade value, it's not fully graded, no time is shown. I guess I could go either way there. If that's how you are thinking, this is OK. I noted that the feedback information is not available to the col_timemarked() function, at least not at this stage, so extending your conjunctive test to check for the presence of feedback would require additional information to be added to the $row object beforehand. Why is the locked value with a grade anyway? If the lock applies to a submission, why is it not in the submission table with the submission information. Then all this compensating for creating a false grade record would not be a problem. Also, in my mind, there could possibly be a 1:many relationship between a submission its grades, but adding the lock to the grade entry assumes a 1:1 relationship. Is that what you mean by your point about 2.4? I think it would be easier to make that change now rather than later.
          Hide
          Michael de Raadt added a comment -

          In your testing instructions you have "Prevent submission changes". Should that be "Select some users, then from the With selected... menu choose Lock submissions and click Go."?

          Show
          Michael de Raadt added a comment - In your testing instructions you have "Prevent submission changes". Should that be "Select some users, then from the With selected... menu choose Lock submissions and click Go."?
          Hide
          Damyon Wiese added a comment -

          Hi Michael - "Prevent submission changes" and "Lock submissions" are the same function (not clear I know).

          The suggestion for 2.4 was to remove the NOT NULL restriction from the database column so it only gets updated when the grade column is modified.

          The reason the "locked" column is on the grade table and not the submission table is that (in my mind) the submission table contains only the information that has been modified by a student and the grade table contains on the information that has been modified by a grader. ("Revert to draft" is the only function that breaks this distinction already). One problem with moving the locked column to the submission is that a submission could be locked before a submission has been made by a student. This would create a submission record even though no submission has been made by a student. We would then need to modify the code everywhere that current checks to see if there is a submission to also check that one of the columns is not null e.g. datecreated or similar.

          My vote is to go with the current patch / functionality ("Last modified(grade) is modified whenever the grade record is updated. Only shown once there is a value in the grade column). This also avoids the fact that we won't know what the plugins are doing (e.g. we could check for is_empty() on the plugin - but it is up to the plugin to decide what constitutes empty and the logic may become inconsistent when different plugins are enabled).

          Show
          Damyon Wiese added a comment - Hi Michael - "Prevent submission changes" and "Lock submissions" are the same function (not clear I know). The suggestion for 2.4 was to remove the NOT NULL restriction from the database column so it only gets updated when the grade column is modified. The reason the "locked" column is on the grade table and not the submission table is that (in my mind) the submission table contains only the information that has been modified by a student and the grade table contains on the information that has been modified by a grader. ("Revert to draft" is the only function that breaks this distinction already). One problem with moving the locked column to the submission is that a submission could be locked before a submission has been made by a student. This would create a submission record even though no submission has been made by a student. We would then need to modify the code everywhere that current checks to see if there is a submission to also check that one of the columns is not null e.g. datecreated or similar. My vote is to go with the current patch / functionality ("Last modified(grade) is modified whenever the grade record is updated. Only shown once there is a value in the grade column). This also avoids the fact that we won't know what the plugins are doing (e.g. we could check for is_empty() on the plugin - but it is up to the plugin to decide what constitutes empty and the logic may become inconsistent when different plugins are enabled).
          Hide
          Michael de Raadt added a comment -

          Hi, Damyon.

          I'm not sure I would have normalised the tables that way, but it's your code, so I suppose it has to make sense in your head.

          Show
          Michael de Raadt added a comment - Hi, Damyon. I'm not sure I would have normalised the tables that way, but it's your code, so I suppose it has to make sense in your head.
          Hide
          Aparup Banerjee added a comment -

          Thanks, this seems to make sense and calm some nerves too
          integrated into master.

          teser: Please reset the mdlqa after testing or re-run tests there too

          Show
          Aparup Banerjee added a comment - Thanks, this seems to make sense and calm some nerves too integrated into master. teser: Please reset the mdlqa after testing or re-run tests there too
          Hide
          Andrew Davis added a comment -

          Works as described. Passing this and resetting the QA test

          Show
          Andrew Davis added a comment - Works as described. Passing this and resetting the QA test
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay!

          Many, many thanks for your hard work!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay! Many, many thanks for your hard work! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: