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

Adjust class admin_setting_configmultiselect_modules to include a default

          Details

          • Type: Improvement
          • Status: Closed
          • Priority: 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

                Attachments

                  Activity

                  Ascending order - Click to sort in descending order
                  Hide
                  Permalink
                  jb23347 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
                  jb23347 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
                  samhemelryk 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
                  samhemelryk 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
                  jb23347 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
                  jb23347 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
                  skodak Petr Skoda added a comment -

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

                  Integrated, thanks.

                  Show
                  skodak 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
                  rajeshtaneja Rajesh Taneja added a comment -

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

                  Thanks for providing the patch John

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

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

                  Show
                  skodak 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:
                        Fix Release Date:
                        1/Aug/11