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
    • 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

        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
        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
        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
        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
        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
        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: