From d59d49030def2a946410beb1a50f0c41a899113a Mon Sep 17 00:00:00 2001 From: Renaat Debleu Date: Wed, 22 Nov 2023 19:53:28 +0100 Subject: [PATCH 1/2] code review --- classes/reportbuilder/local/entities/payment.php | 12 ++++++++++-- tests/event/event_test.php | 6 +++--- tests/privacy/privacy_test.php | 2 +- tests/reportbuilder/reports_test.php | 8 ++++---- 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/classes/reportbuilder/local/entities/payment.php b/classes/reportbuilder/local/entities/payment.php index e6cd552..7493ca3 100755 --- a/classes/reportbuilder/local/entities/payment.php +++ b/classes/reportbuilder/local/entities/payment.php @@ -114,10 +114,18 @@ protected function get_all_columns(): array { // Amount column. $columns[] = (new column('amount', new lang_string('cost'), $name)) ->add_joins($this->get_joins()) - ->set_type(column::TYPE_TEXT) + ->set_type(column::TYPE_FLOAT) ->add_field("{$tablealias}.amount") ->set_is_sortable(true) - ->add_attributes(['class' => 'text-right']); + ->add_callback(function(?string $value): string { + if ($value === '') { + return '0'; + } + $floa = floatval($value); + $floa = $formatfloat($floa, 2); + return number_format($floa, 2); + + }); // Currency column. $columns[] = (new column('currency', new lang_string('currency'), $name)) diff --git a/tests/event/event_test.php b/tests/event/event_test.php index aaaf5e4..f147a15 100755 --- a/tests/event/event_test.php +++ b/tests/event/event_test.php @@ -50,7 +50,7 @@ public function setUp(): void { * simply create the event and trigger it. * @covers \report_payments\event\report_viewed */ - public function test_report_viewed() { + public function test_report_viewed(): void { $course = $this->getDataGenerator()->create_course(); $user = $this->getDataGenerator()->create_user(); @@ -111,7 +111,7 @@ public function test_report_viewed() { * Tests the report navigation as an admin. * @coversNothing */ - public function test_report_payments_navigation() { + public function test_report_payments_navigation(): void { global $CFG, $PAGE, $USER; require_once($CFG->dirroot . '/report/payments/lib.php'); $course = $this->getDataGenerator()->create_course(); @@ -138,7 +138,7 @@ public function test_report_payments_navigation() { * Tests the report page type list. * @coversNothing */ - public function test_report_payments_page_type() { + public function test_report_payments_page_type(): void { global $CFG, $PAGE; $course = $this->getDataGenerator()->create_course(); $PAGE->set_url('/course/view.php', ['id' => $course->id]); diff --git a/tests/privacy/privacy_test.php b/tests/privacy/privacy_test.php index f833a61..de08fad 100755 --- a/tests/privacy/privacy_test.php +++ b/tests/privacy/privacy_test.php @@ -39,7 +39,7 @@ class privacy_test extends provider_testcase { * Test returning metadata. * @covers \report_payments\privacy\provider */ - public function test_get_metadata() { + public function test_get_metadata(): void { $str = 'report_payments'; $this->resetAfterTest(true); $collection = new \core_privacy\local\metadata\collection($str); diff --git a/tests/reportbuilder/reports_test.php b/tests/reportbuilder/reports_test.php index 978276b..f7f2574 100755 --- a/tests/reportbuilder/reports_test.php +++ b/tests/reportbuilder/reports_test.php @@ -85,7 +85,7 @@ public function setUp(): void { * @covers \report_payments\reportbuilder\local\systemreports\payments_global * @covers \report_payments\reportbuilder\local\entities\payment */ - public function test_global() { + public function test_global(): void { $context = \context_system::instance(); $report = system_report_factory::create(payments_global::class, $context); $this->assertEquals($report->get_name(), 'Payments'); @@ -100,7 +100,7 @@ public function test_global() { * @covers \report_payments\reportbuilder\local\systemreports\payments_course * @covers \report_payments\reportbuilder\local\entities\payment */ - public function test_course() { + public function test_course(): void { $report = system_report_factory::create(payments_course::class, \context_course::instance($this->course->id)); $this->assertEquals($report->get_name(), 'Payments'); } @@ -111,7 +111,7 @@ public function test_course() { * @covers \report_payments\reportbuilder\local\systemreports\payments_user * @covers \report_payments\reportbuilder\local\entities\payment */ - public function test_user() { + public function test_user(): void { $report = system_report_factory::create(payments_user::class, \context_user::instance($this->userid)); $this->assertEquals($report->get_name(), 'Payments'); } @@ -122,7 +122,7 @@ public function test_user() { * @covers \report_payments\reportbuilder\datasource\payments * @covers \report_payments\reportbuilder\local\entities\payment */ - public function test_datasource() { + public function test_datasource(): void { $gen = self::getDataGenerator()->get_plugin_generator('core_reportbuilder'); $report = $gen->create_report(['name' => 'Pay', 'source' => payments::class, 'default' => true]); $this->assertEquals($report->get_formatted_name(), 'Pay'); From 99284a0e1f64a738c8ce87b36dab00745fac07d8 Mon Sep 17 00:00:00 2001 From: Renaat Debleu Date: Wed, 22 Nov 2023 21:50:27 +0100 Subject: [PATCH 2/2] code review --- .github/workflows/main.yml | 9 +++------ classes/reportbuilder/local/entities/payment.php | 4 +--- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index d71f0d7..775ea58 100755 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -30,15 +30,12 @@ jobs: strategy: fail-fast: false matrix: - php: ['8.0', '8.1'] - moodle-branch: ['MOODLE_401_STABLE', 'MOODLE_402_STABLE', 'MOODLE_403_STABLE', 'master'] + php: ['8.1'] + moodle-branch: ['MOODLE_401_STABLE', 'MOODLE_402_STABLE', 'MOODLE_403_STABLE'] database: ['mariadb', 'pgsql'] include: - - php: '7.4' - moodle-branch: 'MOODLE_401_STABLE' - database: 'pgsql' - php: '8.2' - moodle-branch: 'MOODLE_403_STABLE' + moodle-branch: 'main' database: 'pgsql' steps: diff --git a/classes/reportbuilder/local/entities/payment.php b/classes/reportbuilder/local/entities/payment.php index 7493ca3..30883f3 100755 --- a/classes/reportbuilder/local/entities/payment.php +++ b/classes/reportbuilder/local/entities/payment.php @@ -114,7 +114,7 @@ protected function get_all_columns(): array { // Amount column. $columns[] = (new column('amount', new lang_string('cost'), $name)) ->add_joins($this->get_joins()) - ->set_type(column::TYPE_FLOAT) + ->set_type(column::TYPE_TEXT) ->add_field("{$tablealias}.amount") ->set_is_sortable(true) ->add_callback(function(?string $value): string { @@ -122,9 +122,7 @@ protected function get_all_columns(): array { return '0'; } $floa = floatval($value); - $floa = $formatfloat($floa, 2); return number_format($floa, 2); - }); // Currency column.