danmarsden / moodle-local_recompletion

Allows course recompletion
29 stars 45 forks source link

Feature: Setting to move course completion of unenrolled users to recompletion table #153

Open davidpesce opened 8 months ago

davidpesce commented 8 months ago

The way the current recompletion task works is to look at the course_completions table and (based on the recompletion interval) send recompletion notifications to users. It doesn't take into account users who have been unenrolled from the course.

The specific use case for our client is compliance training. Each role has a specific list of courses and recompletion duration. If they change roles, it's possible that a course is no longer required. With the plugin as it is, these users are notified that they need to re-complete the course, but are not enrolled (and can't self-enroll).

Our thought is to create a new admin setting that enables a new task to move course completions of unenrolled users to the recompletion_cc table.

The other option is to have the admin setting filter the existing recompletion task to ignore users who are unenrolled. That seemed inefficient because those user completions would always remain in the course_completions table.

We can complete the development but we want to make sure it will be something that can/will be merged into the plugin.

Thoughts?

danmarsden commented 8 months ago

definitely happy for features that control this to land in the plugin - I'm not quite sure that it makes sense to have a new setting to "move" course completion records to the recompletion_cc table though - the plugin already does that when triggering a recompletion process.

What I think you want to occur is allow the normal recompletion process to trigger - clearing their completion record, and archiving it if set, but if the user is not still enrolled - don't send a notification asking them to recomplete.

To do this - I'd a new setting that allows the teacher to decide if the notification should be sent when the user is no longer enrolled in the course - but I'd be just as happy to accept a patch that set it at the ite-level setting if you didn't want to give teachers the ability to control it.

doest that make sense? - would that suit your needs?

davidpesce commented 8 months ago

Yeah, that's even better! We'll get to work on that.

davidpesce commented 8 months ago

Where would you like the function that evaluates whether they are actively enrolled? I think the old way was to include it in locallib.php, but wondered if you wanted a helper.php/utilities.php in classes dir.

danmarsden commented 8 months ago

I would probably just make the recompletion email checkbox into a drop list: https://github.com/danmarsden/moodle-local_recompletion/blob/74c8cc7fabc1c0881e9711d5d9b3f865298e3ece/classes/recompletion_form.php#L70C41-L70C64

with the options "disabled", "Send to all users with a completion record", "Send to users with a current enrolment"

of course don't use magic numbers - we'd need to define some new constants for the recompletionemailenable setting - mayve LOCAL_RECOMPLETION_NOTIFICATION_DISABLED, LOCAL_RECOMPLETION_NOTIFICATION_ENABLED, LOCAL_RECOMPLETION_NOTIFICATION_ENROLLED (or something similar.

hopefully that makes it even simpler to implement and meets your needs?

davidpesce commented 8 months ago

That's a similar approach to what I was thinking, but yours simplifies it even further.

There is the possibility of a fourth option. Is it worthwhile to differentiate between the last two? Our client is interested in the last one (only actively enrolled users are notified - ignore suspended enrollments).

Disabled Send to all users (with a completion record) Send to all users (with an enrollment) Send to all users (with an active enrollment)

Thoughts?

danmarsden commented 8 months ago

If you see value there and do the development go for it!

davidpesce commented 8 months ago

Ok, back at it today. The existing admin name is recompletionemailenable. Would you rather it remain the same or be changed to a different name? Either way, I'll need to add an upgrade step. Thoughts?

danmarsden commented 8 months ago

happy for your pull request to use the same name and we can change it later - but also happy for you to change it as part of your patch - up to you. :-)

davidpesce commented 8 months ago

I ended up leaving the enable/disable checkbox and when enabled, it allows various levels. Defaults to previous behavior.