moodle-an-hochschulen / moodle-theme_boost_campus

Moodle 3.x Boost child theme which is intended to meet the needs of university campuses and adds several features and improvements ––– for Moodle 4.x please use our Theme Boost Union
GNU General Public License v3.0
38 stars 25 forks source link

Adding 'both' option for 'Switch to role…' menu fixes #38. #39

Closed lucaboesch closed 5 years ago

lucaboesch commented 6 years ago

I'd say that's a neat small change. Wonder whether – since it's forward and backwards compatible – there is the need for an upgrade step. I think it could be omitted. Best, Luca

Kathrin84 commented 6 years ago

Hi @lucaboesch , thank you very much for your proposal and the submitted patch - we really appreciate that. Sorry, that I come back to you so late but I was ill and so my work is delayed a little bit.

I've got the following feedback for you:

Furthermore, please check the following: Does Moodle present the altered setting to the admin after the database update?

I hope this helps and if you have some questions, please come back to me.

Best, Kathrin

lucaboesch commented 5 years ago

I've updated the README.md and CHANGES.md file to feature the changed option presented to the Admin. Did not bother about upgrade steps and renaming everything unnecessarily. Instead of values 'yes' and 'no' now there are values 'yes', 'no' and 'both' possible for the incoursesettingsswitchtorolesetting. Nothing breaks if you insert 'both' in your db and switch to older code of theme_boost_campus.

Yes, no DB upgrade script is running upon update (since it is not necessary) and hence no notification of a new setting is shown (it is merely a new possible value, not a new setting) and that IMHO can be neglected.

Best, Luca

Kathrin84 commented 5 years ago

Hi Luca,

I've took your code basis and added the changes I proposed to you regarding a new setting and the upgrade step. Because of this, I could not apply your patch directly but I gave you credits in the CHANGES.md.

The change is now integrated into the master branch: a7024d6

So, I'll close this pull request and thank you very much for your work on this!

Best, Kathrin