Moodle
  1. Moodle
  2. MDL-28421

Adjust class admin_setting_configmultiselect_modules to include a default

        Details

        • Type: Improvement Improvement
        • Status: Closed
        • Priority: Minor Minor
        • Resolution: Fixed
        • Affects Version/s: 2.1
        • Fix Version/s: 2.1.1
        • Component/s: Administration
        • Labels:
          None
        • Testing Instructions:
          Hide

          How to test:

          Just make sure that the Default allowed modules setting on Site administration / 〉 Security / 〉 Module security still works. That is, that there are no regressions.

          Show
          How to test: Just make sure that the Default allowed modules setting on Site administration / 〉 Security / 〉 Module security still works. That is, that there are no regressions.
        • Affected Branches:
          MOODLE_21_STABLE
        • Fixed Branches:
          MOODLE_21_STABLE
        • Pull from Repository:
        • Pull Master Branch:
          wip-MDL-28421-master
        • Pull Master Diff URL:
        • Rank:
          18202

          Description

          In MDL-27123 a new admin class admin_setting_configmultiselect_modules was created that used lazy loading to help improve performance.

          Unfortunately I did not include the use of a default value for this class as it was not needed at the time. Of course I now do have a need to use this class with a default value. Yes it is my own fault I know!

            Activity

            Ascending order - Click to sort in descending order
            Hide
            Permalink
            John Beedell added a comment -

            I have put the improvement in two branches on github and can be viewed with:
            https://github.com/Beedell/moodle/compare/master...wip-MDL-28421-master
            https://github.com/Beedell/moodle/compare/MOODLE_21_STABLE...wip-MDL-28421-MOODLE_21_STABLE

            Show
            John Beedell added a comment - I have put the improvement in two branches on github and can be viewed with: https://github.com/Beedell/moodle/compare/master...wip-MDL-28421-master https://github.com/Beedell/moodle/compare/MOODLE_21_STABLE...wip-MDL-28421-MOODLE_21_STABLE
            Hide
            Permalink
            Sam Hemelryk added a comment -

            Hi guys,

            John - thanks for the work on this.
            There is one thing that I'm not convinced about however. This patch adds the ability to set the choices as part of the constructor which really just makes this new setting a admin_setting_configmultiselect. Personally I would remove the ability to set choices so that this setting type is only usable to modules as suggested by its name, really just reducing the risk of abuse in the future. Other than that the default setting changes look spot on.
            Presently this gets a -1 from me, however I have assigned to Petr as the original integrator to give him a chance to look over it and make a call.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi guys, John - thanks for the work on this. There is one thing that I'm not convinced about however. This patch adds the ability to set the choices as part of the constructor which really just makes this new setting a admin_setting_configmultiselect. Personally I would remove the ability to set choices so that this setting type is only usable to modules as suggested by its name, really just reducing the risk of abuse in the future. Other than that the default setting changes look spot on. Presently this gets a -1 from me, however I have assigned to Petr as the original integrator to give him a chance to look over it and make a call. Cheers Sam
            Hide
            Permalink
            John Beedell added a comment -

            Thank you for double checking this for me, and I have to agree. I have adjusted the commits on github.

            Show
            John Beedell added a comment - Thank you for double checking this for me, and I have to agree. I have adjusted the commits on github.
            Hide
            Permalink
            Petr Škoda added a comment -

            I was going to propose the same thing as Sam, thanks for tweaking the pull request.

            Integrated, thanks.

            Show
            Petr Škoda added a comment - I was going to propose the same thing as Sam, thanks for tweaking the pull request. Integrated, thanks.
            Hide
            Permalink
            Rajesh Taneja added a comment -

            Site administration => Security => Module security, works fine.

            Thanks for providing the patch John

            Show
            Rajesh Taneja added a comment - Site administration => Security => Module security, works fine. Thanks for providing the patch John
            Hide
            Permalink
            Petr Škoda added a comment -

            Thanks everybody, this is now part of the weekly build.

            Show
            Petr Škoda added a comment - Thanks everybody, this is now part of the weekly build.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: