Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            nebgor Aparup Banerjee added a comment - - edited

            Hi Petr,
            just some things i noted:

            ps:units tests are fine here in pg.

            Show
            nebgor 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda added a comment - Helen is looking at it, I will add more incline docs to address your issues + improve strings in extra commit.
            Hide
            tsala 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
            tsala 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
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda added a comment - Perfect, commits added that fix the string and improve test comment, thanks!
            Hide
            nebgor 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
            nebgor 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
            fred Frédéric Massart added a comment -

            Passing. Thanks!

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

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

            http://youtu.be/n64CdfDRnZY

            Closing as fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao
            Hide
            justinlitalien 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
            justinlitalien 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
            skodak Petr Skoda added a comment -

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

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

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

            Show
            justinlitalien 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:
                  Fix Release Date:
                  3/Dec/12