Moodle
  1. Moodle
  2. MDL-39169

Restoring activity using question bank does not restore question's tags

    Details

    • Testing Instructions:
      Hide
      • Create a test user
      • Create a tag
      • Create a course
      • Create another course (used to restore the quiz activity into)
      • Add a question to the question bank within the course
      • Associated that question with the tag that was created
      • Add a quiz instance to the course
      • Add a question to the quiz
      • Login as a test user and submit an answer to the quiz
      • Backup the quiz activity
      • Restore the quiz activity into the other course
        • Go into that course
        • Go into the question bank and edit the question

      Verify:

      • The restored questions have the same tags as the originals.
      Show
      Create a test user Create a tag Create a course Create another course (used to restore the quiz activity into) Add a question to the question bank within the course Associated that question with the tag that was created Add a quiz instance to the course Add a question to the quiz Login as a test user and submit an answer to the quiz Backup the quiz activity Restore the quiz activity into the other course Go into that course Go into the question bank and edit the question Verify: The restored questions have the same tags as the originals.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Rank:
      49770

      Description

      Questions that have tags assigned to them (both official and user defined) and were created in a course do not have the question-tag association properly restored, performing a backup and restore of an activity (quiz for example) that uses the question bank into a different course.

        Activity

        Hide
        Akin Delamarre added a comment -

        This is not just for the Quiz module buy any activity that utilizes the Question API by creating a question_usage_by_activity record for attempts.

        Show
        Akin Delamarre added a comment - This is not just for the Quiz module buy any activity that utilizes the Question API by creating a question_usage_by_activity record for attempts.
        Hide
        Maria Torres added a comment -

        Hi,

        I have attached a patch that could be a solution for this bug.

        Since I am not familiar with Moodle I would like to receive some feedback from you and see if I am in the right way.

        Thanks,
        Maria

        Show
        Maria Torres added a comment - Hi, I have attached a patch that could be a solution for this bug. Since I am not familiar with Moodle I would like to receive some feedback from you and see if I am in the right way. Thanks, Maria
        Hide
        Tim Hunt added a comment -

        Thank you very much for looking at this.

        I have to say that tags is not an area I know a lot about, so my review has mainly been by comparing your patch against the code in restore_course_structure_step (in backup/moodle2/backup_stepslib.php and backup/moodle2/restore_stepslib.php).

        I think the course code there does two things better than your code:

        1. It uses a flatter XML structure (course/tags/tag not question/question_tags/tags/tag/tag_instance).

        2. It uses the tag API tag_get_tags / tag_set rather than direct DB manipulation.

        In the area of coding style, generally your code is excellent, and follows the moodle style. Just two things:

        3. We do not add comments like "Begin Tracker Number: MDL-39169. Tags in questions" within the code. (Since git tracks all that information and more without cluttering the code.)

        4. There is a standard way of writing commit messages, see http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages

        (I note that you did make the patch using git. A lot of Moodle developers share their patches by pushing them to github and then just linking to them. You are welcome to do that in future.)

        Show
        Tim Hunt added a comment - Thank you very much for looking at this. I have to say that tags is not an area I know a lot about, so my review has mainly been by comparing your patch against the code in restore_course_structure_step (in backup/moodle2/backup_stepslib.php and backup/moodle2/restore_stepslib.php). I think the course code there does two things better than your code: 1. It uses a flatter XML structure (course/tags/tag not question/question_tags/tags/tag/tag_instance). 2. It uses the tag API tag_get_tags / tag_set rather than direct DB manipulation. In the area of coding style, generally your code is excellent, and follows the moodle style. Just two things: 3. We do not add comments like "Begin Tracker Number: MDL-39169 . Tags in questions" within the code. (Since git tracks all that information and more without cluttering the code.) 4. There is a standard way of writing commit messages, see http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages (I note that you did make the patch using git. A lot of Moodle developers share their patches by pushing them to github and then just linking to them. You are welcome to do that in future.)
        Hide
        Maria Torres added a comment -

        Tim, thank you very much for taking time to review my code and giving these tips to me.

        I am going to make these changes you have recommended and submit them again.

        Cheers,
        Maria

        Show
        Maria Torres added a comment - Tim, thank you very much for taking time to review my code and giving these tips to me. I am going to make these changes you have recommended and submit them again. Cheers, Maria
        Hide
        Maria Torres added a comment - - edited

        Hi Tim, here is the patch I've created after following your recommendations
        https://github.com/marial49/moodle/commit/2c8eb20423f168635a5b7c35bfe60cf50b0963a5 (Moodle 2.3)
        https://github.com/marial49/moodle/commit/a8c736a3617bd052894f0e4d1a508bef819f6bbb (Moodle 2.4)

        I hope this time everything is ok, if it isn't please let me know.

        Regards,
        Maria

        Show
        Maria Torres added a comment - - edited Hi Tim, here is the patch I've created after following your recommendations https://github.com/marial49/moodle/commit/2c8eb20423f168635a5b7c35bfe60cf50b0963a5 (Moodle 2.3) https://github.com/marial49/moodle/commit/a8c736a3617bd052894f0e4d1a508bef819f6bbb (Moodle 2.4) I hope this time everything is ok, if it isn't please let me know. Regards, Maria
        Hide
        Tim Hunt added a comment -

        Sorry, I keep not getting around to looking at this. I will try again on Monday.

        Show
        Tim Hunt added a comment - Sorry, I keep not getting around to looking at this. I will try again on Monday.
        Hide
        Tim Hunt added a comment -

        Right, I am sorry it took me so long to look at that.

        The code now looks great. I am submitting it for integration.

        They way you have put the commits onto github is not ideal. Hopefully the integrators can get your changes. For future reference:

        1. Look at some issues that other people have sumbitted for integration (https://tracker.moodle.org/secure/Dashboard.jspa?selectPageId=11350) and see how they did it. Try to make your branches on github like that.
        2. http://docs.moodle.org/dev/Git_for_developers.
        3. http://docs.moodle.org/dev/User:Sam_Hemelryk/My_Moodle_Git_workflow
        4. http://tjhunt.blogspot.co.uk/2012/03/fixing-bug-in-moodle-core-mechanics.html
        5. http://docs.moodle.org/dev/Commit_cheat_sheet
        Show
        Tim Hunt added a comment - Right, I am sorry it took me so long to look at that. The code now looks great. I am submitting it for integration. They way you have put the commits onto github is not ideal. Hopefully the integrators can get your changes. For future reference: Look at some issues that other people have sumbitted for integration ( https://tracker.moodle.org/secure/Dashboard.jspa?selectPageId=11350 ) and see how they did it. Try to make your branches on github like that. http://docs.moodle.org/dev/Git_for_developers . http://docs.moodle.org/dev/User:Sam_Hemelryk/My_Moodle_Git_workflow http://tjhunt.blogspot.co.uk/2012/03/fixing-bug-in-moodle-core-mechanics.html http://docs.moodle.org/dev/Commit_cheat_sheet
        Hide
        Tim Hunt added a comment -

        To INTEGRATORS: Please cherry-pick the 2.4 fix onto master.

        Show
        Tim Hunt added a comment - To INTEGRATORS: Please cherry-pick the 2.4 fix onto master.
        Hide
        Maria Torres added a comment -

        Hi Tim, thank you for checking my code and for all the information you have provided.
        I am sorry for the way I put the commits but I did not know anything about them. I'll check these links, so in the future I won't make the same mistake.
        I really appreciate your help and guidance.

        Best regards,
        Maria

        Show
        Maria Torres added a comment - Hi Tim, thank you for checking my code and for all the information you have provided. I am sorry for the way I put the commits but I did not know anything about them. I'll check these links, so in the future I won't make the same mistake. I really appreciate your help and guidance. Best regards, Maria
        Hide
        Dan Poltawski added a comment -

        Thanks Maria! I've integrated this into master, 24 and 23.

        I noticed that your email address in the git commit had a typo in it, so i fixed that for you (hoping it wasn't an anti-spam technique!) You might want to check your git config.

        Show
        Dan Poltawski added a comment - Thanks Maria! I've integrated this into master, 24 and 23. I noticed that your email address in the git commit had a typo in it, so i fixed that for you (hoping it wasn't an anti-spam technique!) You might want to check your git config.
        Hide
        Maria Torres added a comment -

        Hi Dan, That's great news!

        I see that you figured out my anti-spam technique. No, I did not know about that. Thanks for the tip. I've already fixed it.

        Thank you very much.

        Show
        Maria Torres added a comment - Hi Dan, That's great news! I see that you figured out my anti-spam technique. No, I did not know about that. Thanks for the tip. I've already fixed it. Thank you very much.
        Hide
        Adrian Greeve added a comment -

        Tested on the 2.3 and 2.4 integration branches. The question's tag remained intact when restored.
        Test passed.

        Show
        Adrian Greeve added a comment - Tested on the 2.3 and 2.4 integration branches. The question's tag remained intact when restored. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Did you think this day was not going to arrive ever?

        Your patience has been rewarded, yay, sent upstream, thanks!

        Closing...ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: