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 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      48176

      Description

      Obviously it should.

        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: