Moodle

Adjust class admin_setting_configmultiselect_modules to include a default

Details

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

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

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: