Moodle
  1. Moodle
  2. MDL-39397

Need to handle course badges when course is deleted

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Badges
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a course badge and issue it to a user.
      2. Delete a course.
      3. Check that there is no fatal error on 'My badges' page (under User profile settings), and badge image is still properly displayed.

      Also, fixed some deprecated methods warnings.

      Show
      1. Create a course badge and issue it to a user. 2. Delete a course. 3. Check that there is no fatal error on 'My badges' page (under User profile settings), and badge image is still properly displayed. Also, fixed some deprecated methods warnings.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-39397_master
    • Rank:
      50036

      Description

      I think the best solution here is to make sure that badge images get transferred to system context and badges are archived.

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Hi Yuliya,

          The changes for the main part of this change look good, but the award_criteria_course things caught my eye:

          1/ In award_criteria_course::get_options(), I don't really undetand the logic around $param, $this->param and the courseid. I mean it looks like there sometimes could be a courseid coming from the $PAGE->course->id and sometimes from the $this->params. The array_shift vs explicit array setting looks strange. Could you explain a bit whats going on there because i've looked through it a few times and its still confusing me. Ideally, I think the course id should always passed from parameters and not taken form the $PAGE global as that might have side effects.

          2/ Again in award_criteria_course::get_options() it seems like its missing an $param = array() definition too.

          thanks,
          Dan

          Show
          Dan Poltawski added a comment - Hi Yuliya, The changes for the main part of this change look good, but the award_criteria_course things caught my eye: 1/ In award_criteria_course::get_options(), I don't really undetand the logic around $param, $this->param and the courseid. I mean it looks like there sometimes could be a courseid coming from the $PAGE->course->id and sometimes from the $this->params. The array_shift vs explicit array setting looks strange. Could you explain a bit whats going on there because i've looked through it a few times and its still confusing me. Ideally, I think the course id should always passed from parameters and not taken form the $PAGE global as that might have side effects. 2/ Again in award_criteria_course::get_options() it seems like its missing an $param = array() definition too. thanks, Dan
          Hide
          Yuliya Bozhko added a comment -

          Hi Dan,

          Course criteria can be added only to course badges, so there should be no problem with taking $PAGE->course->id. I changed it so that id is passed with the form if you think that it would be better. The problem was that when I was creating an element I needed to know what course it was. I also fixed some errors that would happen if a course gets deleted.

          Criteria are saved in form:
          badge -> criteria -> array(crit1, crit2, crit3). Each criteria except for a single course completion can have multiple parameters (like courses, roles, activities), so they are stored in form crit1 = array(param1, param2, param3, ...). Course criteria can have only one parameter - this course, so through array_shift() I was getting this first parameter. I changed that to reset() now.

          Show
          Yuliya Bozhko added a comment - Hi Dan, Course criteria can be added only to course badges, so there should be no problem with taking $PAGE->course->id. I changed it so that id is passed with the form if you think that it would be better. The problem was that when I was creating an element I needed to know what course it was. I also fixed some errors that would happen if a course gets deleted. Criteria are saved in form: badge -> criteria -> array(crit1, crit2, crit3). Each criteria except for a single course completion can have multiple parameters (like courses, roles, activities), so they are stored in form crit1 = array(param1, param2, param3, ...). Course criteria can have only one parameter - this course, so through array_shift() I was getting this first parameter. I changed that to reset() now.
          Hide
          Dan Poltawski added a comment -

          Thanks, sending to integration.

          Show
          Dan Poltawski added a comment - Thanks, sending to integration.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Some comments:

          1) you can move all the files in the area together. Not sure if that's interesting in this case or the number of badges per course is always small. For your consideration.

          2) I've seen that, after moving the 'badgeimage' files... you proceed to update the 'badge' record with the new information (type, status...) but it has surprised me that you are also storing the file->id into badge->image. Is there any use of that information? I hope it's only being used as sort of bool flag (although it's mandatory so shouldn't be necessary), because files should not be accessed by id ever. More if the id is stored "externally" (in the badges table).

          3) course->fullname needs to processed by format_string() always (to support multilang contents mainly). Note that exactly the same applies to activity->name (although there aren't occurrences in this patch).

          4) finally, although apart from this issue, I'm not really sure about the make_categories_list() call and the HUGE list being the best candidate for picking a list of course. I can imagine sites with thousands of courses running under problems to get that form (courseset) defined. Some search alternative or filtering option may be necessary (as said, unrelated with this issue).

          999) Although I've not idea about badges at all... I've failed to find any backup/restore support about them. While it may be not critical right now... I think it's a MUST once people start using / sharing / moving 2.5 courses with badges information. Please consider adding it ASAP. Structures don't seem to be very complex. Again, in another issue. I just discovered it because I use backup code to learn about the DB schema, hehe. And I did not find anything.

          So, I'm reopening this because of 1-2-3 (mainly because of 3, with 1-2 for your consideration). Otherwise, it looks perfect, I think. Thanks!

          Edited: typos.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Some comments: 1) you can move all the files in the area together. Not sure if that's interesting in this case or the number of badges per course is always small. For your consideration. 2) I've seen that, after moving the 'badgeimage' files... you proceed to update the 'badge' record with the new information (type, status...) but it has surprised me that you are also storing the file->id into badge->image. Is there any use of that information? I hope it's only being used as sort of bool flag (although it's mandatory so shouldn't be necessary), because files should not be accessed by id ever. More if the id is stored "externally" (in the badges table). 3) course->fullname needs to processed by format_string() always (to support multilang contents mainly). Note that exactly the same applies to activity->name (although there aren't occurrences in this patch). 4) finally, although apart from this issue, I'm not really sure about the make_categories_list() call and the HUGE list being the best candidate for picking a list of course. I can imagine sites with thousands of courses running under problems to get that form (courseset) defined. Some search alternative or filtering option may be necessary (as said, unrelated with this issue). 999) Although I've not idea about badges at all... I've failed to find any backup/restore support about them. While it may be not critical right now... I think it's a MUST once people start using / sharing / moving 2.5 courses with badges information. Please consider adding it ASAP. Structures don't seem to be very complex. Again, in another issue. I just discovered it because I use backup code to learn about the DB schema, hehe. And I did not find anything. So, I'm reopening this because of 1-2-3 (mainly because of 3, with 1-2 for your consideration). Otherwise, it looks perfect, I think. Thanks! Edited: typos.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note that MDL-39538 has been integrated, so the ::make_category_list() call is not needed here anymore.

          Show
          Eloy Lafuente (stronk7) added a comment - Note that MDL-39538 has been integrated, so the ::make_category_list() call is not needed here anymore.
          Hide
          Yuliya Bozhko added a comment -

          1) Will have a look into that. Implemented the first solution that I found, really.

          2) File id is never used to access a badge image file. Now that I think about it, it is not really needed. I guess it was in initial design when I was not too familiar with Moodle File API. I can file a bug to remove it later, if you would prefer that.

          3) Will do.

          4) Need to think about that.

          999) Yeah, it's on my todo list (I noticed that recently as well). Haven't got around adding that.

          Thanks for your comments!

          Show
          Yuliya Bozhko added a comment - 1) Will have a look into that. Implemented the first solution that I found, really. 2) File id is never used to access a badge image file. Now that I think about it, it is not really needed. I guess it was in initial design when I was not too familiar with Moodle File API. I can file a bug to remove it later, if you would prefer that. 3) Will do. 4) Need to think about that. 999) Yeah, it's on my todo list (I noticed that recently as well). Haven't got around adding that. Thanks for your comments!
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi, the new patch looks better, really. Just 1 tiny detail for confirmation. Is this line/change correct (cancel name, continue string):

          https://github.com/totara/openbadges/commit/7deff81f80834afc1b84a8d2b65da9d0f6aa7f7f#L1R114

          Of course, it would be great to have separate issues for:

          A) Take rid of badge->image
          B) Review all uses of both course and activity names and ensure they are processed by format_string()
          C) Backup / restore support

          Awaiting for confirmation about that line change before proceeding with integration.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi, the new patch looks better, really. Just 1 tiny detail for confirmation. Is this line/change correct (cancel name, continue string): https://github.com/totara/openbadges/commit/7deff81f80834afc1b84a8d2b65da9d0f6aa7f7f#L1R114 Of course, it would be great to have separate issues for: A) Take rid of badge->image B) Review all uses of both course and activity names and ensure they are processed by format_string() C) Backup / restore support Awaiting for confirmation about that line change before proceeding with integration.
          Hide
          Yuliya Bozhko added a comment - - edited

          Yep, line change is correct. It was supposed to be cancel in first place.

          I already created issue MDL-39558 for backup and restore. Will do the same for the rest.

          Show
          Yuliya Bozhko added a comment - - edited Yep, line change is correct. It was supposed to be cancel in first place. I already created issue MDL-39558 for backup and restore. Will do the same for the rest.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Super, thanks... proceeding with this.

          Show
          Eloy Lafuente (stronk7) added a comment - Super, thanks... proceeding with this.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Jason Fowler added a comment -

          Works perfectly

          Show
          Jason Fowler added a comment - Works perfectly
          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:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: