Moodle
  1. Moodle
  2. MDL-40013

Make question_type::save_hints more suitable for child classes

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5.1
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a question without parts (shortanswer for example), enter several hints and save it.
      2. Re-open created question for editing and make sure all hints was correctly saved.
      3. Create a question with parts (match for example), enter several hints and check several hint options. In last hint don't write text, just check hint options.
      4. Re-open created question for editing and make sure all hints was correctly saved.

      Show
      1. Create a question without parts (shortanswer for example), enter several hints and save it. 2. Re-open created question for editing and make sure all hints was correctly saved. 3. Create a question with parts (match for example), enter several hints and check several hint options. In last hint don't write text, just check hint options. 4. Re-open created question for editing and make sure all hints was correctly saved.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.5 Branch:
      MDL-40013_25stable
    • Pull Master Branch:
      MDL-40013_master
    • Rank:
      50764

      Description

      Overloading of save_hints() method involves duplicating many strings of code.

      This could be avoided by adding 3 methods, called from it, designed for overloading.

        Issue Links

          Activity

          Hide
          Oleg Sychev added a comment -

          While still fighting with git, who right now hangs trying switching to master branch, I attach solution here, so you may review it if you want.

          It will be enought to help both questions you mentioned and my too. If you don't like some function name - just tell me how they could be better, I tried my best.

          Show
          Oleg Sychev added a comment - While still fighting with git, who right now hangs trying switching to master branch, I attach solution here, so you may review it if you want. It will be enought to help both questions you mentioned and my too. If you don't like some function name - just tell me how they could be better, I tried my best.
          Hide
          Tim Hunt added a comment -

          That looks good. Thanks for working on this.

          Some comments/thoughts:

          1. count_hints_on_form is a very clear method name. save_hint_options is not perfect, since it just prepares the data, it does not acutally save it, but it is probably the best name to use here. is_hint_empty worries me. I am wondering if is_hint_empty_in_form_data is better? It is long and a bit ugly, but it is clearer.

          2. In the PHPdoc comemnts, you need to add @param lines for all the arguments and @return where applicable.

          3. Coding style is almost all perfect. Just the line

          return null;//By default, options field is unused.
          

          is wrong (spacing in the comment). Did you run that past codechecker?

          4. Can you write some testing instructions for this?

          5. Good luck getting your patch into git.

          Show
          Tim Hunt added a comment - That looks good. Thanks for working on this. Some comments/thoughts: 1. count_hints_on_form is a very clear method name. save_hint_options is not perfect, since it just prepares the data, it does not acutally save it, but it is probably the best name to use here. is_hint_empty worries me. I am wondering if is_hint_empty_in_form_data is better? It is long and a bit ugly, but it is clearer. 2. In the PHPdoc comemnts, you need to add @param lines for all the arguments and @return where applicable. 3. Coding style is almost all perfect. Just the line return null ; //By default , options field is unused. is wrong (spacing in the comment). Did you run that past codechecker? 4. Can you write some testing instructions for this? 5. Good luck getting your patch into git.
          Hide
          Oleg Sychev added a comment -

          1 OK, I'll change it.
          2. PHPDoc comments is a good thing, but original save_hints lack them too. Would you suggest what to write about $withparts? I didn't think I coould formulate it clearly...
          3. OK.
          4. Done

          Show
          Oleg Sychev added a comment - 1 OK, I'll change it. 2. PHPDoc comments is a good thing, but original save_hints lack them too. Would you suggest what to write about $withparts? I didn't think I coould formulate it clearly... 3. OK. 4. Done
          Hide
          Oleg Sychev added a comment -

          I tried to write PHPDoc comments as I could. I don't really know what to write about $withparts in save_hint_options, it was used by ddmarker only.

          Show
          Oleg Sychev added a comment - I tried to write PHPDoc comments as I could. I don't really know what to write about $withparts in save_hint_options, it was used by ddmarker only.
          Hide
          Tim Hunt added a comment -

          That looks very good. There is just one thing that is not quite right, the first line of the commit comment is too long. Can you do a git commit --amend to fix it? My suggestion would be:

          MDL-40013 Easier to override saving question hints
          
          This adds 3 new methods, called from question_type::save_hints,
          to make it easier for different question types to overload this
          functionality without duplicating code.
          
          Question types that will benefit from this change include
          oumultiresponse, ddmarker, preg and correctwriting.
          

          Sorry I did not spot that before. Testing instructions, and PHPdoc comments look good. I hope we can submit this for integration tomorrow. Then I am on holiday for a week.

          Show
          Tim Hunt added a comment - That looks very good. There is just one thing that is not quite right, the first line of the commit comment is too long. Can you do a git commit --amend to fix it? My suggestion would be: MDL-40013 Easier to override saving question hints This adds 3 new methods, called from question_type::save_hints, to make it easier for different question types to overload this functionality without duplicating code. Question types that will benefit from this change include oumultiresponse, ddmarker, preg and correctwriting. Sorry I did not spot that before. Testing instructions, and PHPdoc comments look good. I hope we can submit this for integration tomorrow. Then I am on holiday for a week.
          Hide
          Oleg Sychev added a comment -

          I amended commits, changing messages, thought I have to use --force to push changes.
          " I hope we can submit this for integration tomorrow" - I hope too...

          Show
          Oleg Sychev added a comment - I amended commits, changing messages, thought I have to use --force to push changes. " I hope we can submit this for integration tomorrow" - I hope too...
          Hide
          Tim Hunt added a comment -

          Thanks Oleg. Submitting for integration now.

          To INTEGRATORS. This API improvement is completely backwards-compatible, so I agree that this should go into 2.5 as well as master.

          Show
          Tim Hunt added a comment - Thanks Oleg. Submitting for integration now. To INTEGRATORS. This API improvement is completely backwards-compatible, so I agree that this should go into 2.5 as well as master.
          Hide
          Dan Poltawski added a comment -

          Integrated to master and 25. Thanks a lot for your patch Oleg!

          Show
          Dan Poltawski added a comment - Integrated to master and 25. Thanks a lot for your patch Oleg!
          Hide
          Adrian Greeve added a comment -

          Failing this test at the moment as I am having problems with the matching question type.
          When I try to add blanks for 3 more questions I find that the form submits and redirects back to the question bank screen.
          I don't personally think that this issue is the cause of the problem, but after talking with Dan I am putting these comments here so that we can find the problem and fix it.

          Show
          Adrian Greeve added a comment - Failing this test at the moment as I am having problems with the matching question type. When I try to add blanks for 3 more questions I find that the form submits and redirects back to the question bank screen. I don't personally think that this issue is the cause of the problem, but after talking with Dan I am putting these comments here so that we can find the problem and fix it.
          Hide
          Sam Hemelryk added a comment -

          Sending back to testing as the offending issue has now been reverted.

          Show
          Sam Hemelryk added a comment - Sending back to testing as the offending issue has now been reverted.
          Hide
          Adrian Greeve added a comment -

          Tested on the 2.5 and master integration branches.
          No problems found.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on the 2.5 and master integration branches. No problems found. Test passed.
          Hide
          Dan Poltawski added a comment -

          (For the record the regression was from MDL-38555)

          Show
          Dan Poltawski added a comment - (For the record the regression was from MDL-38555 )
          Hide
          Marina Glancy added a comment -

          Thanks for your awesome work! This has now become a part of Moodle.

          Closing as fixed!

          Show
          Marina Glancy added a comment - Thanks for your awesome work! This has now become a part of Moodle. Closing as fixed!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: