search

ndunand / moodle-enrol_attributes

This plugin allows users to be enrolled according to any value stored in their user profile.
https://moodle.org/plugins/enrol_attributes
18 stars 22 forks source link

Allow enrolment even if there is already another enrolment in a course #21

Closed mastnym closed 6 years ago

mastnym commented 6 years ago

Hello Nicolas, thank you for this wonderful plugin first. I just want to elaborate about one feature of your plugin. When enroling users, for yeach user you're checking wheather this user is already enrolled in this course and if he/she is, you skip the enrolment: if (is_enrolled(context_course::instance($enrol_attributes_record->courseid), $user)) { continue; }

Is this the right behaviour? In my case, we have a student, who's enrolled in a course thru external database plugin. After finishing th e course, he is suspended (not unenrolled, because we want to have his/her progress in course in case we want him back). Later on, when he/she becomes a PhD. student, he/she gets an attribute of his/her department and I want to enrol him/her into the same course with a special role (PhD. student, so he/she sees the contents of a course). But I can't do that, becouse he/she gets skipped, because of the prevoiuos role. Are there any reasons (which I obviously don't see:) ), that prevent you from doing this? I think, it should be admns responsibility to set up your plugin right, so that users don't get multiple roles etc. What do you think? I would probbably start with skipping inactive course roles, eg: checking only active: if (is_enrolled(context_course::instance($enrol_attributes_record->courseid), $user, '', true)) { continue; } Where the last true argument filters only active enrolments. Thanks for your answer, Martin

ndunand commented 6 years ago

Hello Martin,

Thanks for your feedback and for pointing this out. This might indeed not be the best way to handle things, I think it just made sense here in our local use case.

There are no other reasons for the plugin excluding people who are already enrolled.

I agree with what you suggest, but maybe the plugin could even disregard completely other enrolments. What do you think ? I'd have to check and test.

mastnym commented 6 years ago

Thanks for your feedback Nicolas. I completely understand the local use case :) Anyway, since Moodle permissions are aggregated and there is an ALLOW and PROHIBIT choice (which overrides every other role setting), there should not be a theoretical problem to disregard all other enrolments and just enrol the user. The only thing I'm now afraid of is how other users use your plugin and if you make a change in your code how it affects them.

We are using your plugin to assign roles with less priviledges than the user might already have. So that is fine and will be fine(permission aggregation) on all other instances that use your plugin. I'm 99% sure, that in our enviroment this change will not make any mess. On the other hand, if you use your plugin to give user a role with more priviledges some users might get access to areas,which they don't have before. But...this is right, everything is up to admin and his setting. This may only lead to some confusion for the user. For example, if there is a non editing teacher, which has an external attribute edit-questionbank, then IMHO he/she should get additional right/role(elevated privilidge) to be able to edit question bank no matter what. If you don't want this to happen, then you should delete the attribute from this user.

If I were you, I would disregard all other enrolments. I can't think of any bad circumstances right now. Does it make sense?

ndunand commented 6 years ago

Thanks Martin for sharing your thoughts about this. I agree with you, the plugin should simply disregard any other erolments. I'll release an update fixing this soon.