explicitly call the 'text' key for the ->recompletionemailbody to capture the text

[davidpesce]

This is a fix for issue #142

If I had to guess, it is only because of PHP 8.1. I don't think this should impact earlier versions of PHP, but have not tested it.

[danmarsden]

Hi David! - this doesn't look quite right - the function local_recompletion_get_data should get called somewhere in there which should prevent this (I don't like the way that code works) but there's probably a bug a bit higher there.

[danmarsden]

ah... no that comments not quite right... digging into this a bit more...

[danmarsden]

looks like this was caused by this patch (pinging @keevan fyi)
a4e620b#diff-cb7675fa6b307529fc68ebfb6acf519df41ced364b6b385fbbde374ec84a79deL83-R134

the call to local_recompletion_get_data() shouldn't be there - that function prepares the code for the html editor to use - it's not quite right for the config to pass to the notify_user function.

I don't recall why that was changed from local_recompletion_get_config() in that patch though - @keevan do you remember?

[keevan]

Hi @danmarsden, I believe I removed the whole thing during a refactor, but looking back now, the direct $DB call could potentially be replaced by a local_recompletion_get_config call instead, which does the defaulting of certain fields, one of which is mentioned by this PR.

moodle-local_recompletion/locallib.php

Lines 171 to 177 in 9489d6d

	if (!empty($dbconfig)) {
	foreach ($dbconfig as $key => $value) {
	if (!isset($config[$key]) || $config[$key] !== $value) {
	$config[$key] = $value;
	}
	}
	}

[guillogo]

Hi @danmarsden and @keevan

I was thinking that putting back the call to local_recompletion_get_config in https://github.com/danmarsden/moodle-local_recompletion/blob/MOODLE_401_STABLE/classes/task/check_recompletion.php#L114-L115 fixes the issue reported:

Changing:

$rc = $DB->get_records_list('local_recompletion_config', 'course', [$course->id], '', 'name, id, value');
$configs[$user->course] = (object) local_recompletion_get_data($rc);

With:

$config = local_recompletion_get_config($course);
$configs[$user->course] = $config;

I can see local_recompletion_get_data is used to prepare the form. However, I don't think is required for resetting the user or sending the notification, unless I'm missing something 🤔. I can send the PR if you agree with above.

Best regards,
Guillermo

[danmarsden]

returning to use local_recompletion_get_config looks correct to me for normal usage although we need to check to make sure that function returns "all" config settings as the new period and schedule types might have config settings that don't get pulled through by that call. - eg this list:

moodle-local_recompletion/locallib.php

Lines 156 to 164 in 9489d6d

	'enable' => 0,
	'assignevent' => null,
	'archivecompletiondata' => 0,
	'recompletionemailenable' => 0,
	'recompletionunenrolenable' => 0,
	'recompletionemailbody' => '',
	'recompletionemailsubject' => '',
	'deletegradedata' => 0,
	'course' => null // This isn't in the form.

may not be a complete list of all config vars including the ones used in the period/schedule types that were added since that patch.

[guillogo]

Hi @danmarsden

I have created this PR #144 to address issue #142 putting back local_recompletion_get_config when resetting and sending notification. I also added the default values missing.

Best regards,
Guillermo