Moodle
  1. Moodle
  2. MDL-34696 enrol improvements 2.4 META
  3. MDL-35064

when deleting enrol plugin allow migration to enrol_manual

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Enrolments
    • Labels:
    • Testing Instructions:
      Hide

      1/ Run unit tests for all DB engines.
      2/ Set up some complex enrolments in several courses using different enrol plugins.
      3/ Try uninstalling enrol plugins one by one and verify the user enrolments are migrated or completely deleted depending on action selected.

      Show
      1/ Run unit tests for all DB engines. 2/ Set up some complex enrolments in several courses using different enrol plugins. 3/ Try uninstalling enrol plugins one by one and verify the user enrolments are migrated or completely deleted depending on action selected.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w35_MDL-35064_m24_delenrol
    • Rank:
      43682

      Description

      At present when you delete enrol plugin all user enrolments are deleted together with grades, etc.

      There should be an option to migrate deleted plugin to enrol_manual, it should be probably selected by default too.

        Issue Links

          Activity

          Hide
          Aparup Banerjee added a comment - - edited

          Hi Petr,
          just some things i noted:

          ps:units tests are fine here in pg.

          Show
          Aparup Banerjee added a comment - - edited Hi Petr, just some things i noted: https://github.com/skodak/moodle/compare/master...w35_MDL-35064_m24_delenrol#L2R201 : the 'manual' and 'yyyy' test seem incomplete there. can add some param description to enrol_manual_migrate_plugin_enrolments()'s phpdoc, perhaps: "@param string $enrol The enrolment method. 'manual' will just return." https://github.com/skodak/moodle/compare/master...w35_MDL-35064_m24_delenrol#L3R101 : the 'uninstallconfirm string i would swap 'removes also' --> 'also removes' , also not sure about upper case in 'are you SURE ' hehe ps:units tests are fine here in pg.
          Hide
          Petr Škoda added a comment -

          1/ The 'manual' and 'yyyy' is here to make sure it does not throw any errors.
          2/ I do not think converting manual to manual needs extra explanation, the quick return is a valid result.
          3/ The 'SURE' was there already, I am not sure about the 'also'. I will ask Helen to review the lang strings.

          Thanks.

          Show
          Petr Škoda added a comment - 1/ The 'manual' and 'yyyy' is here to make sure it does not throw any errors. 2/ I do not think converting manual to manual needs extra explanation, the quick return is a valid result. 3/ The 'SURE' was there already, I am not sure about the 'also'. I will ask Helen to review the lang strings. Thanks.
          Hide
          Petr Škoda added a comment -

          Helen is looking at it, I will add more incline docs to address your issues + improve strings in extra commit.

          Show
          Petr Škoda added a comment - Helen is looking at it, I will add more incline docs to address your issues + improve strings in extra commit.
          Hide
          Helen Foster added a comment -

          Apu and Petr, thanks for asking my opinion on the lang string changes.

          My suggested improvement for uninstallconfirm is as follows:

          'You are about to uninstall the enrolment plugin \'{$a}\'. This will result in the deletion of all data associated with this enrolment type, including users\' grades, group membership, forum subscriptions and any other course-related data.

          Are you SURE you want to continue?'

          I think the upper-case SURE is OK, to emphasise the word. You'll notice I've shortened the lang string, trying to keep only essential words, as people can be lazy about reading lots of text on the screen! If I've missed anything important out or changed the meaning of it, please let me know!

          All other lang strings look great to me - succinct and perfect English

          Show
          Helen Foster added a comment - Apu and Petr, thanks for asking my opinion on the lang string changes. My suggested improvement for uninstallconfirm is as follows: 'You are about to uninstall the enrolment plugin \'{$a}\'. This will result in the deletion of all data associated with this enrolment type, including users\' grades, group membership, forum subscriptions and any other course-related data. Are you SURE you want to continue?' I think the upper-case SURE is OK, to emphasise the word. You'll notice I've shortened the lang string, trying to keep only essential words, as people can be lazy about reading lots of text on the screen! If I've missed anything important out or changed the meaning of it, please let me know! All other lang strings look great to me - succinct and perfect English
          Hide
          Petr Škoda added a comment -

          Perfect, commits added that fix the string and improve test comment, thanks!

          Show
          Petr Škoda added a comment - Perfect, commits added that fix the string and improve test comment, thanks!
          Hide
          Aparup Banerjee added a comment -

          ok, thanks all, i've itnegrated that into master now.

          (added a slight clarity during conflict resolution to enrol_manual_migrate_plugin_enrolments() phpdoc since param was missing description anyway)

          Show
          Aparup Banerjee added a comment - ok, thanks all, i've itnegrated that into master now. (added a slight clarity during conflict resolution to enrol_manual_migrate_plugin_enrolments() phpdoc since param was missing description anyway)
          Hide
          Frédéric Massart added a comment -

          Passing. Thanks!

          Show
          Frédéric Massart added a comment - Passing. Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm so proud...of you, many thanks!

          http://youtu.be/n64CdfDRnZY

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao
          Hide
          Justin Litalien added a comment -

          We use the SHEBANG enrollment plugin at our university so I thought maybe this fix applies to our situation. However, after deleting the SHEBANG enrollments from a sample course, the users did not get moved over to manual enrollments.

          Am I misinterpreting this ticket or perhaps SHEBANG is not part of this discussion?

          Thanks!

          Show
          Justin Litalien added a comment - We use the SHEBANG enrollment plugin at our university so I thought maybe this fix applies to our situation. However, after deleting the SHEBANG enrollments from a sample course, the users did not get moved over to manual enrollments. Am I misinterpreting this ticket or perhaps SHEBANG is not part of this discussion? Thanks!
          Hide
          Petr Škoda added a comment -

          Hello, you need to ask the author of SHEBANG plugin, sorry.

          Show
          Petr Škoda added a comment - Hello, you need to ask the author of SHEBANG plugin, sorry.
          Hide
          Justin Litalien added a comment -

          Thanks Petr, wishful thinking I suppose. Cheers...

          Show
          Justin Litalien added a comment - Thanks Petr, wishful thinking I suppose. Cheers...

            People

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

              Dates

              • Created:
                Updated:
                Resolved: