Moodle
  1. Moodle
  2. MDL-38311

question engine manual grading methods don't support commentformat

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.4, 2.4.1, 2.5
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Questions
    • Labels:
      None
    • Testing Instructions:
      Hide

      0. Unit tests will have been run as part of integration.

      1. Create a quiz containing at least one Essay question.

      2. Attempt the quiz as a student. You need at least two attempts.

      3. As teacher, review the first attempt, and use the 'Make comment or override grade' link to manually grade it.

      4. Then go to Results -> Manual grading, and use that inteface to grade the other attempt.

      Show
      0. Unit tests will have been run as part of integration. 1. Create a quiz containing at least one Essay question. 2. Attempt the quiz as a student. You need at least two attempts. 3. As teacher, review the first attempt, and use the 'Make comment or override grade' link to manually grade it. 4. Then go to Results -> Manual grading, and use that inteface to grade the other attempt.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      Obviously it should.

        Gliffy Diagrams

          Activity

          Hide
          Tim Hunt added a comment -

          I think this fix is OK.

          Show
          Tim Hunt added a comment - I think this fix is OK.
          Hide
          Andrew Nicols added a comment -

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [N] Testing
          [-] Security
          [N] Documentation
          [N] Git
          [Y] Sanity check

          Code looks good, just a few minor comments:

          Documentation

          phpdoc missing for the commentformat in questionusage.php

          Git

          Few typos in your commit message but nothing serious

          If you're feeling really pro-active, there's a typoe for Actually (Acutally) in the test you've modified in question/engine/tests/unitofwork_test.php

          Otherwise good to go for integration.

          Show
          Andrew Nicols added a comment - [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [N] Testing [-] Security [N] Documentation [N] Git [Y] Sanity check Code looks good, just a few minor comments: Documentation phpdoc missing for the commentformat in questionusage.php Git Few typos in your commit message but nothing serious If you're feeling really pro-active, there's a typoe for Actually (Acutally) in the test you've modified in question/engine/tests/unitofwork_test.php Otherwise good to go for integration.
          Hide
          Tim Hunt added a comment -

          Commit fixed, as per Andrew's comments. (Thanks.) Submitted for integration.

          Show
          Tim Hunt added a comment - Commit fixed, as per Andrew's comments. (Thanks.) Submitted for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          It's curious that there isn't any use of the 4th and 5th params (timestamp, userid) in core.

          But I bet anyone out there using that... will need to change uses or, in other words, this changes the API in a non-bc way. (note I don't know if that's public API or no, but sounds like).

          So... any comment?

          Show
          Eloy Lafuente (stronk7) added a comment - - edited It's curious that there isn't any use of the 4th and 5th params (timestamp, userid) in core. But I bet anyone out there using that... will need to change uses or, in other words, this changes the API in a non-bc way. (note I don't know if that's public API or no, but sounds like). So... any comment?
          Hide
          Tim Hunt added a comment -

          Note I explain the in the commit comment.

          I cannot imagine that anyone is using those parameters. None of the third-party plugins I know of use it. In fact, the only code I know of that uses the method at all is the unit tests.

          So, while it is technically an non-backwards-compatible API change, in practice, no one will care.

          Also, the question_usage API (which is backwards compatible) is the real public API here.

          Show
          Tim Hunt added a comment - Note I explain the in the commit comment. I cannot imagine that anyone is using those parameters. None of the third-party plugins I know of use it. In fact, the only code I know of that uses the method at all is the unit tests. So, while it is technically an non-backwards-compatible API change, in practice, no one will care. Also, the question_usage API (which is backwards compatible) is the real public API here.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks, proceeding...

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks, proceeding...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Baboom:

          master:

          Debugging: You should pass $commentformat to manual_grade.

          • ...
          • ...
          • line 135 of /question/behaviour/manualgraded/tests/walkthrough_test.php: call to qbehaviour_walkthrough_test_base->manual_grade()

          Bet it requires:

          -        $this->manual_grade('Not good enough!', 1);
          +        $this->manual_grade('Not good enough!', 1, FORMAT_HTML);
          

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Baboom: master: Debugging: You should pass $commentformat to manual_grade. ... ... line 135 of /question/behaviour/manualgraded/tests/walkthrough_test.php: call to qbehaviour_walkthrough_test_base->manual_grade() Bet it requires: - $this->manual_grade('Not good enough!', 1); + $this->manual_grade('Not good enough!', 1, FORMAT_HTML); Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Tim, I'm applying it to get all-greens along the testing session, blame & ping me if the fix is wrong, TIA!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Tim, I'm applying it to get all-greens along the testing session, blame & ping me if the fix is wrong, TIA! Ciao
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          Tested in 2.3, 2.4 and master.

          Show
          Michael de Raadt added a comment - Test result: Success! Tested in 2.3, 2.4 and master.
          Hide
          Tim Hunt added a comment -

          Thanks for fixing that Eloy. I can't think how I missed that. I searched the whole code for 'manual_grade'. Sorry. And thanks for fixing my mess (again!).

          Show
          Tim Hunt added a comment - Thanks for fixing that Eloy. I can't think how I missed that. I searched the whole code for 'manual_grade'. Sorry. And thanks for fixing my mess (again!).
          Hide
          Tim Hunt added a comment -

          Sorry, Eloy. I made some changes to fix Andrew's review comments, but forgot to commit them.

          I have just made a new commit: https://github.com/timhunt/moodle/commit/770e4a4 would you like to cherry-pick that to all branches?

          Show
          Tim Hunt added a comment - Sorry, Eloy. I made some changes to fix Andrew's review comments, but forgot to commit them. I have just made a new commit: https://github.com/timhunt/moodle/commit/770e4a4 would you like to cherry-pick that to all branches?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          no worries!

          extra commit applied, hope that changed manual_grade() comment is not tested, lol.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - no worries! extra commit applied, hope that changed manual_grade() comment is not tested, lol. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

          Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

          Thanks, closing as fixed!

          Show
          Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: