Add global and local/instance settings for role to be shown

[Kemmotar83]

Hi Alexander,

this is the 3.5 version of my patch. Check if this is ok.

Thanks,
Giorgio

[abias]

Hi Giorgio,

thank you very much for your code.

I had a look at it and here is my feedback:

• If you don't mind, please keep lines like https://github.com/moodleuulm/moodle-block_people/pull/5/files#diff-3a8270dae2b17999b935edc868c22cebR87 or https://github.com/moodleuulm/moodle-block_people/pull/5/files#diff-3a8270dae2b17999b935edc868c22cebR30 unchanged. I am aware that the plugin contains code which could be prettier, but to keep the patch for this issue short, I would like to avoid to combine adding a feature with code beautification.
• If you don't mind, please remove the @throws PHPDoc lines as these are not used in any of our plugins and if I am not completely mistaken are not used by Moodle core. Perhaps they have been added by your IDE?
• Please keep the release in CHANGES.md and version.php unchanged, just update the version in version.php. I will increase the release later when the plugin is published.
• As the last version of this block has just decoupled the roles to be shown in the block from $CFG->coursecontact, I would prefer not to do a rollback with the block_people/overridecoursecontact setting. Could we just ignore that $CFG->coursecontact exists and only use the block_people/roles setting?
• Do we really need the block_people/allowinstanceoverride checkbox setting? As an admin, I would simply not select any roles in the block_people/overridableroles setting to prevent role overriding. Or am I missing something?
• On the other hand, I like the config_overridedefaultroles instance setting as teachers can clearly decide if the roles should be overridden or not. Otherwise, it would be difficult to proceed with overridden roles as soon as the admin changes the global block_people/roles setting. So, thanks for that :)

I am looking forward for your refinement and I will then do my last review and merge the code.

Thanks,
Alex

[Kemmotar83]

Sorry Alexander for this late reply, I've been taken from other activities. I've seen you have just released an update, so I'll check what you have included.

1. Sorry. ;)
2. Yes. I use PHPStorm and automatically it includes those comments. I should configure it in order to behave differently.
3. Sorry again. I'm not really used to collaborate on GitHub.
4. It seemed useful to leave a compatibility with the global setting, but it's your plugin, so no problem.
5. So, if no role is selected, automatically the override is not available. Could be a valid solution.
6. Ok

Now I have to check your last release.

Thanks,
Giorgio

[Kemmotar83]

Hi Alexander,

is there anything else that I need to do? I've committed the updated code.

Thanks,
Giorgio