Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-67548

Allow plugins to hook into course category deletion form

    XMLWordPrintable

    Details

    • Testing Instructions:
      Hide

      The actual hooks functionality is covered in the unittest. The manual test is only verifying there is no regression.

      Pre-requirements

      1. Create category CAT1 at top level
      2. Within CAT1 create categories CAT11 and CAT12
      3. Within CAT11 create course CR1

      Delete with move

      1. Login as admin
      2. Navigate to "Manage courses and categories"
      3. Open CAT11 category action menu, click on "Delete"
      4. Category deletion interface is presented.
      5. This category contais lists "Courses"
      6. Observe "What to do" has 2 options: "Move contents to another category" as well as "Delete all - cannot be undone"
      7. Observe "Move into" (only visible if "Move contents to another category" seleced) has a selection of all categories that exist in the system.
      8. Select "CAT1 / CAT12" in the "Move into" select element, click on "Delete"
      9. Observe that CAT11 has been deleted and CR1 course was moved into CAT12.

      Full delete

      1. Login as admin
      2. Navigate to "Manage courses and categories"
      3. Open CAT12 category action menu, click on "Delete"
      4. Category deletion interface is presented.
      5. This category contais lists "Courses"
      6. Observe "What to do" has 2 options: "Move contents to another category" as well as "Delete all - cannot be undone"
      7. Observe "Move into" (only visible if "Move contents to another category" seleced) has a selection of all categories that exist in the system.
      8. Select "Delete all - cannot be undone" in the "What to do" select element, click on "Delete"
      9. Observe that CAT12 has been deleted and CR1 course has been deleted.
      Show
      The actual hooks functionality is covered in the unittest. The manual test is only verifying there is no regression. Pre-requirements Create category CAT1 at top level Within CAT1 create categories CAT11 and CAT12 Within CAT11 create course CR1 Delete with move Login as admin Navigate to "Manage courses and categories" Open CAT11 category action menu, click on "Delete" Category deletion interface is presented. This category contais lists "Courses" Observe "What to do" has 2 options: "Move contents to another category" as well as "Delete all - cannot be undone" Observe "Move into" (only visible if "Move contents to another category" seleced) has a selection of all categories that exist in the system. Select "CAT1 / CAT12" in the "Move into" select element, click on "Delete" Observe that CAT11 has been deleted and CR1 course was moved into CAT12. Full delete Login as admin Navigate to "Manage courses and categories" Open CAT12 category action menu, click on "Delete" Category deletion interface is presented. This category contais lists "Courses" Observe "What to do" has 2 options: "Move contents to another category" as well as "Delete all - cannot be undone" Observe "Move into" (only visible if "Move contents to another category" seleced) has a selection of all categories that exist in the system. Select "Delete all - cannot be undone" in the "What to do" select element, click on "Delete" Observe that CAT12 has been deleted and CR1 course has been deleted.
    • Affected Branches:
      MOODLE_38_STABLE
    • Fixed Branches:
      MOODLE_39_STABLE
    • Pull Master Branch:
      MDL-67548-master

      Description

      When manager deletes course category, there is a confirmation dialogue that says something like this: "This category contains: ...", "What to do"

      Plugins also may have contents associated with the category. Would be good if they can hook into this form and tell the manager about it. Also plugins need to know what is the selected action and perform their checks/execute code in:

      core_course_category::can_move_content_to()
      core_course_category::delete_move()
      etc.

      Currently if "Move content to" was selected, there is no information about the new category in the "category_deleted" event and plugins can not distinguish it from the full deletion event.

      ====

      A viable plan to better allow plugins to intercept coursecat actions (delete & move right now) can be something like:

      A) hooks will be used to both modify the UI/form. It's not clear if we just need to "display" information or, also, be able to get new data on form submission. That will define how many hooks the UI/form need.

      B) hook will be also used to check permissions. The idea is to have one unique hook called something like (disclaimer, name invented, just showing what it will do): xxx_delete_category_can_action($category, $action) that will enable plugins to decide if a given action can happen.

      C1) For the execution itself... there was a little bit of debate. Finally, based on how course delete/reset is doing currently, it was agreed that we would be using the existing cat_delete and cat_update events to implement the action execution in plugins. That's the primary idea.

      C2) If, for any reason, the event is not enough (because it's created AFTER core has performed the execution... it's possible to use the already existing xxxx__pre_course_category_delete() hook, that happens BEFORE core execution.

      D) It's acknowledged by all parties that event observers are forbidden solution for core (and that hook callbacks, when possible, should be also avoided in core, though they are not forbidden). So, in case any core plugin wants some day to take part on the "delete cat" & "move cat" actions... we should create a new xxxx__pre_course_category_move() to avoid any event observer in core.

      Associated aspects to decide:

      • TODO1: Decide who wins when checking permissions. Surely the easier (and also logic) is to require all the hook permissions to return "true" (allowed). If there is just one plugin not allowing the action to happen (false) then the action should be prevented. Note that's a simplification (true/false), surely the check can come with a textual reason too.
      • TODO2: Some small refactoring, keeping BC will be needed, for example the update category event needs to be enriched to easily know which action has happened, and associated information (like the new parent category in a move and other details). That way, observers can fine-decide when they have something to do or no.
      • TODO3: Point D... if that new callback for move (or update in general, although maybe that's too much) action was created now... then we wouldn't need to perform any refactor like the TODO2 above. and the solution here would be 100% hook-based (no need for events at all). This should be considered, IMO.
      • TODO4: Consider how recursion affects to all this, related to the "delete" action. Both if the existing hook and/or the events are used.

      That's it, so far. Please, feel free to amend it any time, discuss, whatever. Ciao

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              kabalin Ruslan Kabalin
              Reporter:
              marina Marina Glancy
              Peer reviewer:
              Marina Glancy
              Integrator:
              Adrian Greeve
              Tester:
              Janelle Barcega
              Participants:
              Component watchers:
              Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Sara Arjona (@sarjona)
              Votes:
              1 Vote for this issue
              Watchers:
              10 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Fix Release Date:
                15/Jun/20

                  Time Tracking

                  Estimated:
                  Original Estimate - 0 minutes
                  0m
                  Remaining:
                  Remaining Estimate - 0 minutes
                  0m
                  Logged:
                  Time Spent - 2 days
                  2d