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:

          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!

            Gliffy Diagrams

              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 Skoda added a comment -

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

              Integrated, thanks.

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

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

              Show
              Petr Skoda 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: