From ae500b57286736dc6ef4125e59bc4bfb0c4c1767 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Mon, 14 Oct 2024 10:08:35 +0100 Subject: [PATCH 1/2] MDL-83430 customfield_number: fix storing activities number when 0 --- .../local/numberproviders/nofactivities.php | 7 +- .../nofactivities/nofactivities_test.php | 79 +++++++++++++++++++ 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/customfield/field/number/classes/local/numberproviders/nofactivities.php b/customfield/field/number/classes/local/numberproviders/nofactivities.php index 89f2f451e89..b3b8771a514 100644 --- a/customfield/field/number/classes/local/numberproviders/nofactivities.php +++ b/customfield/field/number/classes/local/numberproviders/nofactivities.php @@ -67,8 +67,7 @@ class nofactivities extends provider_base { // Define the label for the autocomplete element. $valuelabel = get_string('activitytypes', 'customfield_number'); // Add autocomplete element. - $mform->addElement('autocomplete', 'configdata[activitytypes]', $valuelabel, $options, ['multiple' => true]) - ->setHiddenLabel(true); + $mform->addElement('autocomplete', 'configdata[activitytypes]', $valuelabel, $options, ['multiple' => true]); $mform->hideIf('configdata[activitytypes]', 'configdata[fieldtype]', 'ne', get_class($this)); $mform->hideIf('configdata[decimalplaces]', 'configdata[fieldtype]', 'eq', get_class($this)); $mform->hideIf('configdata[display]', 'configdata[fieldtype]', 'eq', get_class($this)); @@ -124,10 +123,10 @@ class nofactivities extends provider_base { $records = $DB->get_records_sql($sql, $params + ['siteid' => SITEID]); foreach ($records as $record) { $value = (int)$record->cnt; - if (!isset($displaywhenzero) && !$value) { + if ((string)$displaywhenzero === '' && !$value) { // Do not display the field when the number of activities is zero. if ($record->dataid) { - (new data_controller((int)$record->dataid, (object)['id' => $record->dataid]))->delete(); + (new data_controller(0, (object)['id' => $record->dataid]))->delete(); } } else if (empty($record->dataid) || (int)$record->decvalue != $value) { // Stored value is out of date. diff --git a/customfield/field/number/tests/local/numberproviders/nofactivities/nofactivities_test.php b/customfield/field/number/tests/local/numberproviders/nofactivities/nofactivities_test.php index 59500c531f9..32b1880273c 100644 --- a/customfield/field/number/tests/local/numberproviders/nofactivities/nofactivities_test.php +++ b/customfield/field/number/tests/local/numberproviders/nofactivities/nofactivities_test.php @@ -113,4 +113,83 @@ final class nofactivities_test extends advanced_testcase { $course1customfield = $DB->get_field('customfield_data', 'decvalue', ['instanceid' => $course1->id]); $this->assertEquals(3.0000, $course1customfield); } + + /** + * Test that the data record is updated/deleted when the value is recalculated + * + * Also test that export_value() is correct + * + * @return void + */ + public function test_recalculate_change_value(): void { + global $DB; + + $this->resetAfterTest(); + $this->setAdminUser(); + + // Create a course with one activity. + $course1 = $this->getDataGenerator()->create_course(); + $assigngenerator = $this->getDataGenerator()->get_plugin_generator('mod_assign'); + $assign1 = $assigngenerator->create_instance(['course' => $course1->id, 'visible' => 1]); + + /** @var \core_customfield_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_customfield'); + + // Create a category and two fields, one with displaywhenzero, another one without. + $category = $generator->create_category(); + $field1 = $generator->create_field([ + 'categoryid' => $category->get('id'), + 'type' => 'number', + 'configdata' => [ + 'fieldtype' => nofactivities::class, + 'activitytypes' => ['assign'], + 'displaywhenzero' => '0', + ], + ]); + $field2 = $generator->create_field([ + 'categoryid' => $category->get('id'), + 'type' => 'number', + 'configdata' => [ + 'fieldtype' => nofactivities::class, + 'activitytypes' => ['assign'], + 'displaywhenzero' => '', + ], + ]); + $getdata = fn(\customfield_number\field_controller $field): \customfield_number\data_controller => + \core_customfield\api::get_instance_fields_data([$field->get('id') => $field], (int)$course1->id)[$field->get('id')]; + + // Recalculate the value of the field and assert it is set to 1 (one activity in the course). + (new \customfield_number\task\cron())->execute(); + $data = $getdata($field1); + $this->assertEquals(1, $data->get('decvalue')); + $this->assertSame('1', $data->export_value()); + $data = $getdata($field2); + $this->assertEquals(1, $data->get('decvalue')); + $this->assertSame('1', $data->export_value()); + + // Add another module, recalculate and assert the value of the field is set to 2 (two activities in the course). + $assign2 = $assigngenerator->create_instance(['course' => $course1->id, 'visible' => 1]); + (new \customfield_number\task\cron())->execute(); + $data = $getdata($field1); + $this->assertEquals(2, $data->get('decvalue')); + $this->assertSame('2', $data->export_value()); + $data = $getdata($field2); + $this->assertEquals(2, $data->get('decvalue')); + $this->assertSame('2', $data->export_value()); + + // Delete both modules, recalculate. + course_delete_module($assign1->cmid); + course_delete_module($assign2->cmid); + (new \customfield_number\task\cron())->execute(); + // Field1 (displaywhenzero='0') has the value zero. + $data = $getdata($field1); + $this->assertNotEmpty($data->get('id')); + $this->assertEquals(0, $data->get('decvalue')); + $this->assertSame('0', $data->export_value()); + // Field2 (displaywhenzero='') no longer has a data record and it is not displayed. + $data = $getdata($field2); + $this->assertEmpty($data->get('id')); + $this->assertEquals(null, $data->get('decvalue')); + $this->assertSame(null, $data->export_value()); + } } From c0b234324531beba4b575071b9ea25b8a55be20a Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Thu, 10 Oct 2024 13:22:15 +0100 Subject: [PATCH 2/2] MDL-83430 customfield_number: call display method from provider earlier --- .../field/number/classes/field_controller.php | 17 ++++++------ .../local/numberproviders/nofactivities.php | 11 ++++++-- .../field/number/classes/provider_base.php | 26 ++++++++++++++++--- 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/customfield/field/number/classes/field_controller.php b/customfield/field/number/classes/field_controller.php index d1e6a5bab78..523449664d3 100644 --- a/customfield/field/number/classes/field_controller.php +++ b/customfield/field/number/classes/field_controller.php @@ -173,6 +173,10 @@ class field_controller extends \core_customfield\field_controller { * @return string|float|null */ public function prepare_field_for_display(mixed $value, ?context $context = null): string|null|float { + if ($provider = provider_base::instance($this)) { + return $provider->prepare_export_value($value, $context); + } + if ($value === null) { return null; } @@ -185,16 +189,11 @@ class field_controller extends \core_customfield\field_controller { } } else { // Let's format the value. - $provider = provider_base::instance($this); - if ($provider) { - $value = $provider->prepare_export_value($value, $context); - } else { - $value = format_float((float)$value, $decimalplaces); + $value = format_float((float)$value, $decimalplaces); - // Apply the display format. - $format = $this->get_configdata_property('display') ?? '{value}'; - $value = str_replace('{value}', $value, $format); - } + // Apply the display format. + $format = $this->get_configdata_property('display') ?? '{value}'; + $value = str_replace('{value}', $value, $format); } return format_string($value, true, ['context' => $context ?? system::instance()]); diff --git a/customfield/field/number/classes/local/numberproviders/nofactivities.php b/customfield/field/number/classes/local/numberproviders/nofactivities.php index b3b8771a514..da64f83c074 100644 --- a/customfield/field/number/classes/local/numberproviders/nofactivities.php +++ b/customfield/field/number/classes/local/numberproviders/nofactivities.php @@ -157,13 +157,20 @@ class nofactivities extends provider_base { /** * Preparation for export for number of activities provider. * - * @param mixed $value String or float + * @param mixed $value String or float or null if the value is not present in the database for this instance * @param \context|null $context Context * @return ?string */ public function prepare_export_value(mixed $value, ?\context $context = null): ?string { - if (trim((string)$value) === '') { + if ($value === null) { return null; + } else if (round((float)$value) == 0) { + $whenzero = $this->field->get_configdata_property('displaywhenzero'); + if ((string) $whenzero === '') { + return null; + } else { + return format_string($whenzero, true, ['context' => $context ?? \core\context\system::instance()]); + } } else { return format_float((float)$value, 0); } diff --git a/customfield/field/number/classes/provider_base.php b/customfield/field/number/classes/provider_base.php index 59660b61987..66957744de3 100644 --- a/customfield/field/number/classes/provider_base.php +++ b/customfield/field/number/classes/provider_base.php @@ -80,14 +80,34 @@ abstract class provider_base { } /** - * Provider specific value preparation for export. + * How the field should be displayed + * + * Called from {@see field_controller::prepare_field_for_display()} + * The return value may contain safe HTML but all user input must be passed through + * format_string/format_text functions * * @param mixed $value String or float * @param context|null $context Context - * @return ?string + * @return ?string null if the field should not be displayed or string representation of the field */ public function prepare_export_value(mixed $value, ?\context $context = null): ?string { - return $value; + if ($value === null) { + return null; + } + + // By default assumes that configuration 'decimalplaces' and 'displaywhenzero' are + // present. If they are not used in this provider, override the method. + $decimalplaces = (int) $this->field->get_configdata_property('decimalplaces'); + if (round((float) $value, $decimalplaces) == 0) { + $result = $this->field->get_configdata_property('displaywhenzero'); + if ((string) $result === '') { + return null; + } else { + return format_string($result, true, ['context' => $context ?? \core\context\system::instance()]); + } + } else { + return format_float((float)$value, $decimalplaces); + } } /**