Moodle
  1. Moodle
  2. MDL-39558

Add backup and restore for badges

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5.1
    • Component/s: Badges
    • Labels:
    • Testing Instructions:
      Hide
      1. Create a course with badges enabled.
      2. Create different badges using each of the different criteria available.
      3. Complete activities in the course as different students etc in order to earn each of the badges available. (You may need to run cron)
      4. Backup the course with badges enabled.
      5. Restore the course to a new course with badges enabled.
      6. Verify that the badges have been restored and are unavailable.
      7. Activate each of the badges and verify that the badges are awarded to the same students as in the previous course.
      8. Restore the course to an existing course with badges enabled.
      9. Verify that the badges have been restored and are unavailable.
      10. Activate each of the badges and verify that the badges are awarded to the same students as in the previous course (you may need to set the course completion criteria again).
      11. Backup the original course with no activities and verify that you cannot include badges
      12. Backup the original course with no users and verify that you cannot include badges
      13. Disable badges on the server
      14. Verify that you can backup and restore a course and that the new course doesn't have any badges (even if the original did).
      Show
      Create a course with badges enabled. Create different badges using each of the different criteria available. Complete activities in the course as different students etc in order to earn each of the badges available. (You may need to run cron) Backup the course with badges enabled. Restore the course to a new course with badges enabled. Verify that the badges have been restored and are unavailable. Activate each of the badges and verify that the badges are awarded to the same students as in the previous course. Restore the course to an existing course with badges enabled. Verify that the badges have been restored and are unavailable. Activate each of the badges and verify that the badges are awarded to the same students as in the previous course (you may need to set the course completion criteria again). Backup the original course with no activities and verify that you cannot include badges Backup the original course with no users and verify that you cannot include badges Disable badges on the server Verify that you can backup and restore a course and that the new course doesn't have any badges (even if the original did).
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.5 Branch:
      MDL-39558_master
    • Rank:
      50241

      Description

      Need to make sure course badges are getting backed up and restored. Missed this in initial implementation.

        Issue Links

          Activity

          Hide
          Matteo Scaramuccia added a comment -

          Just for the record, the Community is talking about a similar issue here: https://moodle.org/mod/forum/discuss.php?d=228637. There the scope is somewhat different since it seems that the lack of an include, the badges library, blocks restoring courses into a 2.5 instance.

          HTH,
          Matteo

          Show
          Matteo Scaramuccia added a comment - Just for the record, the Community is talking about a similar issue here: https://moodle.org/mod/forum/discuss.php?d=228637 . There the scope is somewhat different since it seems that the lack of an include , the badges library, blocks restoring courses into a 2.5 instance. HTH, Matteo
          Hide
          Yuliya Bozhko added a comment -

          I am working in this issue right now, will try to get it fixed ASAP.

          Show
          Yuliya Bozhko added a comment - I am working in this issue right now, will try to get it fixed ASAP.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Yuliya and Matteo,
          The problem pointed out by Matteo is not really a badges backup/restore problem because we see this problem when restoring a Moodle 2.4 backup in Moodle 2.5 so we can be sure the backup doesn't contain any badge data.
          The problem is in lib/moodlelib.php in the delete_course function
          There is

          // Handle course badges.
              badges_handle_course_deletion($courseid);
          

          but badgeslib is not included when this function is executed.
          I don't know why by deactivating badges at the site level in the site administration seems to suppress the problem and I was able to restore my 2.4 backup on my freshly installed 2.5 Moodle site after doing that.
          I was about to open a new tracker issue for that specific problem but as Matteo posted a link to the forum discussion in that issue better to comment here.
          Tell me if you think a separate issue would help.
          Thanks for working on this.

          Show
          Jean-Michel Vedrine added a comment - Hello Yuliya and Matteo, The problem pointed out by Matteo is not really a badges backup/restore problem because we see this problem when restoring a Moodle 2.4 backup in Moodle 2.5 so we can be sure the backup doesn't contain any badge data. The problem is in lib/moodlelib.php in the delete_course function There is // Handle course badges. badges_handle_course_deletion($courseid); but badgeslib is not included when this function is executed. I don't know why by deactivating badges at the site level in the site administration seems to suppress the problem and I was able to restore my 2.4 backup on my freshly installed 2.5 Moodle site after doing that. I was about to open a new tracker issue for that specific problem but as Matteo posted a link to the forum discussion in that issue better to comment here. Tell me if you think a separate issue would help. Thanks for working on this.
          Hide
          Matteo Scaramuccia added a comment -

          Hi Jean-Michel,
          I agree, a new issue should be IMHO better: I've just commented here since this issue has been posted in that Community thread, to be sure that Yuliya would have the opportunity to be informed about that post.

          Show
          Matteo Scaramuccia added a comment - Hi Jean-Michel, I agree, a new issue should be IMHO better: I've just commented here since this issue has been posted in that Community thread, to be sure that Yuliya would have the opportunity to be informed about that post.
          Hide
          Dan Poltawski added a comment -

          Hi Everyone,

          Thanks for your comments on this issue to find the major issue. I've created MDL-39778 for the specifc error breaking backup/restore.

          Show
          Dan Poltawski added a comment - Hi Everyone, Thanks for your comments on this issue to find the major issue. I've created MDL-39778 for the specifc error breaking backup/restore.
          Hide
          Yuliya Bozhko added a comment -

          Thanks, Jean-Michel! I submitted a patch for MDL-39778. Have no idea why I never noticed missing library... :-/

          Show
          Yuliya Bozhko added a comment - Thanks, Jean-Michel! I submitted a patch for MDL-39778 . Have no idea why I never noticed missing library... :-/
          Hide
          Martin Dougiamas added a comment -

          Yuliya are you working on this?

          Show
          Martin Dougiamas added a comment - Yuliya are you working on this?
          Hide
          Yuliya Bozhko added a comment -

          Yes, I am. Sorry for the delay. A bit overloaded with work due to our project lead being on leave making me responsible for both release and support... I will do my best to get this fixed by the end of the week.

          Still not quite sure what to do with restoring issued badges. I will be like issuing the same badges over again which doesn't quite make sense to me. Or should there be an option of restoring issued badges at all?

          Show
          Yuliya Bozhko added a comment - Yes, I am. Sorry for the delay. A bit overloaded with work due to our project lead being on leave making me responsible for both release and support... I will do my best to get this fixed by the end of the week. Still not quite sure what to do with restoring issued badges. I will be like issuing the same badges over again which doesn't quite make sense to me. Or should there be an option of restoring issued badges at all?
          Hide
          Yuliya Bozhko added a comment -

          Hopefully will submit it today. Just some minor stuff to test

          Show
          Yuliya Bozhko added a comment - Hopefully will submit it today. Just some minor stuff to test
          Hide
          Yuliya Bozhko added a comment -

          Sorry, guys, I am stuck at files restore If anyone can help me out on that, here is current code https://github.com/totara/openbadges/commit/a8ebac2db5d13d202a6fef9311745113c603f025 I put badges backup/restore in the final step, i.e. final_task (I want to make sure that no activities are missing for id mapping). But in this case, image files are not getting restored If I move backup/restore to course_task, I can't get activities IDs, but all images are there...

          Show
          Yuliya Bozhko added a comment - Sorry, guys, I am stuck at files restore If anyone can help me out on that, here is current code https://github.com/totara/openbadges/commit/a8ebac2db5d13d202a6fef9311745113c603f025 I put badges backup/restore in the final step, i.e. final_task (I want to make sure that no activities are missing for id mapping). But in this case, image files are not getting restored If I move backup/restore to course_task , I can't get activities IDs, but all images are there...
          Hide
          Yuliya Bozhko added a comment -

          I found the problem!!! Needed to add badges to process_file() in restore_stepslib.php Will submit a patch first thing tomorrow morning

          Show
          Yuliya Bozhko added a comment - I found the problem!!! Needed to add badges to process_file() in restore_stepslib.php Will submit a patch first thing tomorrow morning
          Hide
          Yuliya Bozhko added a comment - - edited

          Patch submitted. Not the most elegant solution, but it works with current course criteria.

          Badges are restored if course is restored into a new course or instead of existing course. All activities have to be backed up and restored as well.

          I put restore into the final task to make sure that all activities that can potentially be among criteria are restored.

          In MDL-40112, I am getting rid of 'image' column in 'badge' table. It is still present here, though. I will update whichever patch gets in first.

          Please let me know what you think!

          Edit: I can also provide master branch patch. Currently only 2.5 stable submitted.

          Show
          Yuliya Bozhko added a comment - - edited Patch submitted. Not the most elegant solution, but it works with current course criteria. Badges are restored if course is restored into a new course or instead of existing course. All activities have to be backed up and restored as well. I put restore into the final task to make sure that all activities that can potentially be among criteria are restored. In MDL-40112 , I am getting rid of 'image' column in 'badge' table. It is still present here, though. I will update whichever patch gets in first. Please let me know what you think! Edit: I can also provide master branch patch. Currently only 2.5 stable submitted.
          Hide
          Dan Poltawski added a comment -

          I'm pulling this into integration as its been sat waiting for peer review for nearly a month and it would be good to get it into 2.5.1

          Show
          Dan Poltawski added a comment - I'm pulling this into integration as its been sat waiting for peer review for nearly a month and it would be good to get it into 2.5.1
          Hide
          Yuliya Bozhko added a comment -

          Yes please! I should have probably reminded about this. It might be a bit outdated atm, but shouldn't have any problems merging.

          Show
          Yuliya Bozhko added a comment - Yes please! I should have probably reminded about this. It might be a bit outdated atm, but shouldn't have any problems merging.
          Hide
          Damyon Wiese added a comment -

          Hi Yuliya,

          Looking at the patch I have a few comments:

          First - nice patch. The backup/restore is complicated but this patch looks mostly perfect.

          Some minor things to look at:

          1. No testing instructions
          2. require_once $CFG->blah requires brackets in backup/moodle2/restore_stepslib.php

          Some minor questions:

          1. Should we be including issued badges in the backup? It makes sense to me (especially for manually issued badges that won't be automatically awarded again).
          2. Along the same lines - why are restored badges always inactive?
          3. Also - why can't we restore badges into an existing course?

          Slightly bigger questions:

          1. Shouldn't the badges backup settings also depend on users (implies roles)?
          2. The roleids are not mapped on restore. This is a big issue when restoring to a different site - it is unlikely the roleids will be the same.

          I think addressing the first 2 and the last 2 points would be a minimum for integrating this patch and we can add issues to address the others, or give reasons why they shouldn't be done.

          Thanks again!

          Show
          Damyon Wiese added a comment - Hi Yuliya, Looking at the patch I have a few comments: First - nice patch. The backup/restore is complicated but this patch looks mostly perfect. Some minor things to look at: No testing instructions require_once $CFG->blah requires brackets in backup/moodle2/restore_stepslib.php Some minor questions: Should we be including issued badges in the backup? It makes sense to me (especially for manually issued badges that won't be automatically awarded again). Along the same lines - why are restored badges always inactive? Also - why can't we restore badges into an existing course? Slightly bigger questions: Shouldn't the badges backup settings also depend on users (implies roles)? The roleids are not mapped on restore. This is a big issue when restoring to a different site - it is unlikely the roleids will be the same. I think addressing the first 2 and the last 2 points would be a minimum for integrating this patch and we can add issues to address the others, or give reasons why they shouldn't be done. Thanks again!
          Hide
          Yuliya Bozhko added a comment -

          Hi Damyon,

          Thanks for the comments!

          I will add some testing instructions. Sorry that I missed them in first place.

          require_once is a statement, not a function and therefore doesn't require brackets. I will add them just to follow general Moodle coding style

          On the bigger questions, you are right, roles should be mapped and probably that's why it should depend on users too. Will fix that.

          On the minor questions. I was looking at restoring badges as restoring completion (which is being restored when all activities are being restored). This works pretty good with issuing badges: if user completion records the activities are included, then badge will be issued on the next cron run anyway. But you are right here, that in this case I should include information about manually issued badges too... (2) I made all restored badges inactive because in this case a user can go and change any criteria they want and manually control badges availability to student. If I don't do that and user completion records are being restored, then some badges might get issued before teacher even get a chance to change anything. (3) I just never thought about that really Restoring into existing course will require all activities to be restored as well anyway. I might try and see if it works ok.

          I will fix as much as I can (definitely the major issues) by the end of the day and resubmit for review.

          Thanks again!

          Yuliya

          Show
          Yuliya Bozhko added a comment - Hi Damyon, Thanks for the comments! I will add some testing instructions. Sorry that I missed them in first place. require_once is a statement, not a function and therefore doesn't require brackets. I will add them just to follow general Moodle coding style On the bigger questions, you are right, roles should be mapped and probably that's why it should depend on users too. Will fix that. On the minor questions. I was looking at restoring badges as restoring completion (which is being restored when all activities are being restored). This works pretty good with issuing badges: if user completion records the activities are included, then badge will be issued on the next cron run anyway. But you are right here, that in this case I should include information about manually issued badges too... (2) I made all restored badges inactive because in this case a user can go and change any criteria they want and manually control badges availability to student. If I don't do that and user completion records are being restored, then some badges might get issued before teacher even get a chance to change anything. (3) I just never thought about that really Restoring into existing course will require all activities to be restored as well anyway. I might try and see if it works ok. I will fix as much as I can (definitely the major issues) by the end of the day and resubmit for review. Thanks again! Yuliya
          Hide
          Yuliya Bozhko added a comment -

          Hi Damyon,

          Got some stuff done today Please have a look at submitted diff url, top two commits. I rebased it as well just in case. If you need, I will squash my commits in one for integration. Just wanted to leave them as they are for now, so that you could see the changes.

          On the side note, I am still not restoring issued badges, but I am restoring manual award records, which means that badges can be automatically reissued when active.

          Yuliya

          Show
          Yuliya Bozhko added a comment - Hi Damyon, Got some stuff done today Please have a look at submitted diff url, top two commits. I rebased it as well just in case. If you need, I will squash my commits in one for integration. Just wanted to leave them as they are for now, so that you could see the changes. On the side note, I am still not restoring issued badges, but I am restoring manual award records, which means that badges can be automatically reissued when active. Yuliya
          Hide
          Damyon Wiese added a comment -

          Thanks Yuliya,

          The code with the extra patch looks great to me - you have addressed all of the points I raised earlier.

          I have integrated this to 25 and master and will assign myself as tester (as this is just past our testing deadline).

          Show
          Damyon Wiese added a comment - Thanks Yuliya, The code with the extra patch looks great to me - you have addressed all of the points I raised earlier. I have integrated this to 25 and master and will assign myself as tester (as this is just past our testing deadline).
          Hide
          Damyon Wiese added a comment -

          The ui change is the extra checkbox on the backup/restore pages for badges.

          Show
          Damyon Wiese added a comment - The ui change is the extra checkbox on the backup/restore pages for badges.
          Hide
          Damyon Wiese added a comment -

          Step 12 failed - I'll check the code now.

          Show
          Damyon Wiese added a comment - Step 12 failed - I'll check the code now.
          Hide
          Yuliya Bozhko added a comment -

          I think I didn't add dependency somewhere... Looking at it now too

          Show
          Yuliya Bozhko added a comment - I think I didn't add dependency somewhere... Looking at it now too
          Hide
          Damyon Wiese added a comment -

          Pushed a fix for the failure.

          Show
          Damyon Wiese added a comment - Pushed a fix for the failure.
          Hide
          Damyon Wiese added a comment -

          You are very quick Yuliya!

          Show
          Damyon Wiese added a comment - You are very quick Yuliya!
          Hide
          Yuliya Bozhko added a comment -

          Yeah, backup_root_task was missing users dependency. Restore seems to be fine

          Show
          Yuliya Bozhko added a comment - Yeah, backup_root_task was missing users dependency. Restore seems to be fine
          Hide
          Damyon Wiese added a comment -

          Thanks Yuliya,

          I tested this on master and 25 and haven't found anything else. I like how the badges are unavailable on restore so you get a chance to tweak them but then all the issued badges come back once they are enabled.

          Show
          Damyon Wiese added a comment - Thanks Yuliya, I tested this on master and 25 and haven't found anything else. I like how the badges are unavailable on restore so you get a chance to tweak them but then all the issued badges come back once they are enabled.
          Hide
          Damyon Wiese added a comment -

          This issue is fixed! Hurray! Hurray!
          Your issue is fixed, what a wonderful day!

          Cheers, Damyon

          Show
          Damyon Wiese added a comment - This issue is fixed! Hurray! Hurray! Your issue is fixed, what a wonderful day! Cheers, Damyon
          Hide
          Elizabeth Dalton added a comment -

          Can this be used to import a badge from one course to another? I realize that the criteria may not be present in the new course, and may need to be reset, but having the definition, image, message, etc. all be the same would still be helpful.

          Should I open a separate ticket?

          Show
          Elizabeth Dalton added a comment - Can this be used to import a badge from one course to another? I realize that the criteria may not be present in the new course, and may need to be reset, but having the definition, image, message, etc. all be the same would still be helpful. Should I open a separate ticket?
          Hide
          Mary Cooch added a comment -

          Just going through the docs_required labels and spotted this. I am not sure we need docs for this as it is something people would be expecting to happen by default anyway - so I would like to remove the docs_required label, but before I do, I would appreciate it if people let me know whether they agree with me or not (ie does it need extra documentation or not?)

          Show
          Mary Cooch added a comment - Just going through the docs_required labels and spotted this. I am not sure we need docs for this as it is something people would be expecting to happen by default anyway - so I would like to remove the docs_required label, but before I do, I would appreciate it if people let me know whether they agree with me or not (ie does it need extra documentation or not?)
          Hide
          Yuliya Bozhko added a comment -

          @Mary: There is nothing specific about badges restore/backup. It is a similar checkbox as other course backup/restore settings. If there is no documentation for those settings, I see no reason of writing it for badges.

          @Elizabeth: Restoring/backing up badges usually depends on other restore/backup setting of the course. As far as I remember, you cannot backup a badge with activity criteria if you are not backing up activities and users. It would be better if you could try it out on test course before doing anything with existing courses.

          Show
          Yuliya Bozhko added a comment - @Mary: There is nothing specific about badges restore/backup. It is a similar checkbox as other course backup/restore settings. If there is no documentation for those settings, I see no reason of writing it for badges. @Elizabeth: Restoring/backing up badges usually depends on other restore/backup setting of the course. As far as I remember, you cannot backup a badge with activity criteria if you are not backing up activities and users. It would be better if you could try it out on test course before doing anything with existing courses.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: