Skip to content

Commit

Permalink
CONTRIB-8518: Fix most issues raised in moodle-plugin-ci (#374)
Browse files Browse the repository at this point in the history
* Moodle plugin ci / codechecker was updated, leading to new detections
of issues in codechecker and phpunit
* Fix a PHP unit test regarding completion
* Replace print_error by moodle_exception
* Fix issue with new completion API
  • Loading branch information
laurentdavid authored Apr 29, 2021
1 parent 618f5cb commit 01d5241
Show file tree
Hide file tree
Showing 23 changed files with 259 additions and 711 deletions.
24 changes: 12 additions & 12 deletions bbb_view.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@

$bbbviewinstance = view::bigbluebuttonbn_view_validator($id, $bn);
if (!$bbbviewinstance) {
print_error(get_string('view_error_url_missing_parameters', 'bigbluebuttonbn'));
throw new moodle_exception('view_error_url_missing_parameters', 'bigbluebuttonbn');
}

$cm = $bbbviewinstance['cm'];
Expand All @@ -71,16 +71,16 @@
$serverversion = bigbluebutton::bigbluebuttonbn_get_server_version();
if (is_null($serverversion)) {
if ($bbbsession['administrator']) {
print_error('view_error_unable_join', 'bigbluebuttonbn',
throw new moodle_exception('view_error_unable_join', 'bigbluebuttonbn',
$CFG->wwwroot.'/admin/settings.php?section=modsettingbigbluebuttonbn');
exit;
}
if ($bbbsession['moderator']) {
print_error('view_error_unable_join_teacher', 'bigbluebuttonbn',
throw new moodle_exception('view_error_unable_join_teacher', 'bigbluebuttonbn',
$CFG->wwwroot.'/course/view.php?id='.$bigbluebuttonbn->course);
exit;
}
print_error('view_error_unable_join_student', 'bigbluebuttonbn',
throw new moodle_exception('view_error_unable_join_student', 'bigbluebuttonbn',
$CFG->wwwroot.'/course/view.php?id='.$bigbluebuttonbn->course);
exit;
}
Expand Down Expand Up @@ -155,7 +155,7 @@
break;
case 'join':
if (is_null($bbbsession)) {
print_error('view_error_unable_join', 'bigbluebuttonbn');
throw new moodle_exception('view_error_unable_join', 'bigbluebuttonbn');
break;
}
// Check the origin page.
Expand Down Expand Up @@ -186,31 +186,31 @@
if (empty($response)) {
// The server is unreachable.
if ($bbbsession['administrator']) {
print_error('view_error_unable_join', 'bigbluebuttonbn',
throw new moodle_exception('view_error_unable_join', 'bigbluebuttonbn',
$CFG->wwwroot.'/admin/settings.php?section=modsettingbigbluebuttonbn');
break;
}
if ($bbbsession['moderator']) {
print_error('view_error_unable_join_teacher', 'bigbluebuttonbn',
throw new moodle_exception('view_error_unable_join_teacher', 'bigbluebuttonbn',
$CFG->wwwroot.'/admin/settings.php?section=modsettingbigbluebuttonbn');
break;
}
print_error('view_error_unable_join_student', 'bigbluebuttonbn',
throw new moodle_exception('view_error_unable_join_student', 'bigbluebuttonbn',
$CFG->wwwroot.'/admin/settings.php?section=modsettingbigbluebuttonbn');
break;
}
if ($response['returncode'] == 'FAILED') {
// The meeting was not created.
if (!$printerrorkey) {
print_error($response['message'], 'bigbluebuttonbn');
throw new moodle_exception($response['message'], 'bigbluebuttonbn');
break;
}
$printerrorkey = plugin::bigbluebuttonbn_get_error_key($response['messageKey'], 'view_error_create');
print_error($printerrorkey, 'bigbluebuttonbn');
throw new moodle_exception($printerrorkey, 'bigbluebuttonbn');
break;
}
if ($response['hasBeenForciblyEnded'] == 'true') {
print_error(get_string('index_error_forciblyended', 'bigbluebuttonbn'));
throw new moodle_exception(get_string('index_error_forciblyended', 'bigbluebuttonbn'));
break;
}
// Moodle event logger: Create an event for meeting created.
Expand Down Expand Up @@ -435,7 +435,7 @@ function bigbluebuttonbn_bbb_view_errors($serrors, $id) {
$msgerrors .= html_writer::tag('p', $error->{'message'}, array('class' => 'alert alert-danger'))."\n";
}
echo $OUTPUT->header();
print_error('view_error_bigbluebutton', 'bigbluebuttonbn',
throw new moodle_exception('view_error_bigbluebutton', 'bigbluebuttonbn',
$CFG->wwwroot.'/mod/bigbluebuttonbn/view.php?id='.$id, $msgerrors, $serrors);
echo $OUTPUT->footer();
}
141 changes: 141 additions & 0 deletions classes/completion/custom_completion.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.

/**
* The mod_bigbluebuttonbn custom_completion event.
*
* @package mod_bigbluebuttonbn
* @copyright 2010 onwards, Blindside Networks Inc
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
* @author Laurent David ([email protected])
*/

namespace mod_bigbluebuttonbn\completion;

use core_completion\activity_custom_completion;
use Exception;
use mod_bigbluebuttonbn\local\bbb_constants;

defined('MOODLE_INTERNAL') || die();

/**
* Class custom_completion
*
* @package mod_bigbluebuttonbn
* @copyright 2010 onwards, Blindside Networks Inc
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
* @author Laurent David ([email protected])
*/
class custom_completion extends activity_custom_completion {

/**
* @param string $rule
* @return int
* @throws \dml_exception
*/
public function get_state(string $rule): int {
global $DB;

// Get bigbluebuttonbn details.
$bigbluebuttonbn = $DB->get_record('bigbluebuttonbn', array('id' => $this->cm->instance), '*',
MUST_EXIST);
if (!$bigbluebuttonbn) {
throw new Exception("Can't find bigbluebuttonbn {$this->cm->instance}");
}

// Default return value.
$value = false;

$sql = "SELECT * FROM {bigbluebuttonbn_logs} ";
$sql .= "WHERE bigbluebuttonbnid = ? AND userid = ? AND log = ?";
$logs =
$DB->get_records_sql($sql, array($bigbluebuttonbn->id, $this->userid, bbb_constants::BIGBLUEBUTTON_LOG_EVENT_SUMMARY));

switch ($rule) {
case 'completionattendance':
if (!$logs) {
// As completion by attendance was required, the activity hasn't been completed.
return false;
}
$attendancecount = 0;
foreach ($logs as $log) {
$summary = json_decode($log->meta);
$attendancecount += $summary->data->duration;
}
$attendancecount /= 60;
$value =
isset($bigbluebuttonbn->completionattendance) && $bigbluebuttonbn->completionattendance <= $attendancecount;
break;
case 'completionengagementchats':
if (!$logs) {
// As completion by engagement with chat was required, the activity hasn't been completed.
return false;
}
$engagementchatscount = 0;
foreach ($logs as $log) {
$summary = json_decode($log->meta);
$engagementchatscount += $summary->data->engagement->chats;
}
$value = isset($bigbluebuttonbn->completionengagementchats) &&
$bigbluebuttonbn->completionengagementchats <= $engagementchatscount;
break;
case 'completionengagementtalks':
if (!$logs) {
// As completion by engagement with talk was required, the activity hasn't been completed.
return false;
}
$engagementtalkscount = 0;
foreach ($logs as $log) {
$summary = json_decode($log->meta);
$engagementtalkscount += $summary->data->engagement->talks;
}
$value = isset($bigbluebuttonbn->completionengagementtalks) &&
$bigbluebuttonbn->completionengagementtalks <= $engagementtalkscount;
break;
}
return $value ? COMPLETION_COMPLETE : COMPLETION_INCOMPLETE;

}

/**
* Fetch the list of custom completion rules that this module defines.
*
* @return array
*/
public static function get_defined_custom_rules(): array {
return ['completionattendance', 'completionengagementchats', 'completionengagementtalks'];
}

/**
* Returns an associative array of the descriptions of custom completion rules.
*
* @return array
* @throws \coding_exception
*/
public function get_custom_rule_descriptions(): array {
$completionengagementchats = $this->cm->customdata['customcompletionrules']['completionengagementchats'] ?? 0;
$completionengagementtalks = $this->cm->customdata['customcompletionrules']['completionengagementtalks'] ?? 0;
$completionattendance = $this->cm->customdata['customcompletionrules']['completionattendance'] ?? 0;
return [
'completionengagementchats' => get_string('completionengagementchatsdesc', 'mod_bigbluebuttonbn',
$completionengagementchats),
'completionengagementtalks' => get_string('completionengagementtalksdesc', 'mod_bigbluebuttonbn',
$completionengagementtalks),
'completionattendance' => get_string('completionattendancedesc', 'mod_bigbluebuttonbn',
$completionattendance),
];
}
}
1 change: 0 additions & 1 deletion classes/external/completion_validate.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ public static function execute_parameters(): external_function_parameters {
* Mark activity as complete
*
* @param int $bigbluebuttonbnid the bigbluebuttonbn instance id
* @param string $meetingid
* @return array (empty array for now)
* @throws \coding_exception
* @throws \dml_exception
Expand Down
2 changes: 1 addition & 1 deletion classes/external/get_recordings.php
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ public static function execute_returns(): external_single_structure {
'key' => new external_value(PARAM_ALPHA),
'label' => new external_value(PARAM_TEXT),
'width' => new external_value(PARAM_ALPHANUMEXT),
// See https://datatables.net/reference/option/columns.type
// See https://datatables.net/reference/option/columns.type .
'type' => new external_value(PARAM_ALPHANUMEXT, 'Column type', VALUE_OPTIONAL),
'sortable' => new external_value(PARAM_BOOL, 'Whether this column is sortable', VALUE_OPTIONAL, false),
'allowHTML' => new external_value(PARAM_BOOL, 'Whether this column contains HTML', VALUE_OPTIONAL, false),
Expand Down
11 changes: 10 additions & 1 deletion classes/external/meeting_info.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,14 @@ public static function execute_parameters(): external_function_parameters {
*
* @param int $bigbluebuttonbnid the bigbluebuttonbn instance id
* @param string $meetingid
* @param bool $updatecache
* @return array (empty array for now)
* @throws \coding_exception
* @throws \dml_exception
* @throws \invalid_parameter_exception
* @throws \required_capability_exception
* @throws \restricted_context_exception
* @throws moodle_exception
* @throws restricted_context_exception
*/
public static function execute(
int $bigbluebuttonbnid,
Expand Down Expand Up @@ -110,6 +111,14 @@ public static function execute(
return static::get_meeting_info($bbbsession, $updatecache);
}

/**
* Get meeting information
*
* @param array $bbbsession
* @param bool $updatecache
* @return array
* @throws \coding_exception
*/
public static function get_meeting_info($bbbsession,
bool $updatecache = false) {
global $USER;
Expand Down
5 changes: 4 additions & 1 deletion classes/local/bigbluebutton.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
use context_module;
use curl;
use Exception;
use mod_bigbluebuttonbn\completion\custom_completion;
use mod_bigbluebuttonbn\local\helpers\files;
use mod_bigbluebuttonbn\local\helpers\logs;
use mod_bigbluebuttonbn\local\helpers\meeting;
Expand Down Expand Up @@ -559,7 +560,9 @@ public static function bigbluebuttonbn_completion_update_state($bigbluebuttonbn,
mtrace("Completion not enabled");
return;
}
if (bigbluebuttonbn_get_completion_state($course, $cm, $userid, COMPLETION_AND)) {

$bbbcompletion = new custom_completion($cm, $userid);
if ($bbbcompletion->get_overall_completion_state()) {
mtrace("Completion succeeded for user $userid");
$completion->update_state($cm, COMPLETION_COMPLETE, $userid, true);
} else {
Expand Down
5 changes: 4 additions & 1 deletion classes/local/broker.php
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,7 @@ public static function recording_action_unpublish($params, $recordings) {
* @param array $recordings
*
* @return array
* @throws \dml_exception
*/
public static function recording_action_protect($params, $recordings) {
global $DB;
Expand Down Expand Up @@ -603,7 +604,9 @@ public static function recording_ready($params, $bigbluebuttonbn) {
return;
}
// We make sure messages are sent only once.
if (\mod_bigbluebuttonbn\local\helpers\logs::bigbluebuttonbn_get_count_callback_event_log($decodedparameters->record_id) == 0) {
if (
\mod_bigbluebuttonbn\local\helpers\logs::bigbluebuttonbn_get_count_callback_event_log(
$decodedparameters->record_id) == 0) {
recording::bigbluebuttonbn_send_notification_recording_ready($bigbluebuttonbn);
}
$overrides = array('meetingid' => $decodedparameters->meeting_id);
Expand Down
2 changes: 1 addition & 1 deletion classes/local/helpers/recording.php
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ public static function bigbluebuttonbn_get_recordings_array($meetingids, $record
public static function bigbluebuttonbn_get_recordings_array_fetch($meetingidsarray) {
// TODO: We will need to completely rewrite this by
// Having a recording singleton which instantiation would depend on the condition here or
// overriding the higher level function (bigbluebuttonbn_get_recordings_array)
// overriding the higher level function (bigbluebuttonbn_get_recordings_array).

if ((defined('PHPUNIT_TEST') && PHPUNIT_TEST)
|| defined('BEHAT_SITE_RUNNING')
Expand Down
2 changes: 1 addition & 1 deletion classes/local/helpers/reset.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public static function bigbluebuttonbn_reset_logs($courseid) {
* @return array status array
*/
public static function bigbluebuttonbn_reset_recordings($courseid) {
// Criteria for search [courseid | bigbluebuttonbn=null | subset=false | includedeleted=true].
// Criteria for search : courseid or bigbluebuttonbn=null or subset=false or includedeleted=true.
$recordings = recording::bigbluebuttonbn_get_recordings($courseid, null, false, true);
// Remove all the recordings.
recording::bigbluebuttonbn_delete_recordings(implode(",", array_keys($recordings)));
Expand Down
3 changes: 3 additions & 0 deletions classes/local/settings/settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ protected function add_conditional_element($name, $item, &$settings) {
/**
* Helper function renders general settings if the feature is enabled.
*
* @param string $sectioname
*
* @return admin_settingpage
* @throws \coding_exception
*/
public function bigbluebuttonbn_settings_general($sectioname) {
Expand Down
7 changes: 3 additions & 4 deletions classes/local/view.php
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,9 @@ public static function view_message_box(&$bbbsession, $message, $type = 'warning
* Displays the general view.
*
* @param array $bbbsession
* @param string $activity
* @return void
* @throws \coding_exception
* @throws \moodle_exception
*/
public static function view_render(&$bbbsession) {
global $OUTPUT, $PAGE;
Expand Down Expand Up @@ -193,16 +194,13 @@ public static function view_warning_shown($bbbsession) {
* Renders the view for room.
*
* @param array $bbbsession
* @param string $activity
* @param array $jsvars
*
* @return string
* @throws \moodle_exception
*/
public static function view_render_room(&$bbbsession) {
global $OUTPUT;
$context = meeting_info::get_meeting_info($bbbsession, false);
/* @var core_renderer $OUTPUT */
return $OUTPUT->render_from_template('mod_bigbluebuttonbn/room_view', $context);
}

Expand All @@ -213,6 +211,7 @@ public static function view_render_room(&$bbbsession) {
* @param array $enabledfeatures
*
* @return string
* @throws \coding_exception
*/
public static function view_render_imported($bbbsession, $enabledfeatures) {
global $CFG;
Expand Down
2 changes: 1 addition & 1 deletion classes/output/import_view.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public function export_for_template(renderer_base $output) {
}
$context['has_recordings'] = $hasrecordings;

// Now the selects
// Now the selects.
if ($this->courseidscope) {
$bbbrecords = $DB->get_records('bigbluebuttonbn', array('course' => $this->courseidscope));
$selectrecords = [];
Expand Down
Loading

0 comments on commit 01d5241

Please sign in to comment.