Skip to content

Instantly share code, notes, and snippets.

@kiklop74

kiklop74/recommendations.md Secret

Last active February 5, 2019 18:26
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save kiklop74/feb19ce6e5e66ca278e77945ec785f9f to your computer and use it in GitHub Desktop.
Save kiklop74/feb19ce6e5e66ca278e77945ec785f9f to your computer and use it in GitHub Desktop.
Download ZIP
Recommendations for bbbb
Raw
recommendations.md

Modifications mod_bigbluebutton plugin

Suggestions for code modifications in mod_bigbluebutton plugin

All suggestions are based on the master branch version of the code.

  • All includes of config.php should use require instead of require_once. This incorrect practice persisted for years. (importance low)

      require_once('config.php');
  // Should be refactored to:
  require('config.php');

  • All places that have dirname(dirname(dirname(__FILE__))) should be transformed to __DIR__.'/../../../' (importance low)

      require_once(dirname(dirname(dirname(__FILE__))).'/config.php');
  // Should be transformed into:
  require(__DIR__.'/../../../config.php');

  • The page that presents content to the user should not have any functions or classes implemented inside it. All such could should be moved at least to separate lib files or refactored completely where needed. (importance high) The files in question are:

    • mod/bigbluebuttonbn/view.php
    • mod/bigbluebuttonbn/bbb_view.php
    • mod/bigbluebuttonbn/bbb_broker.php
    • mod/bigbluebuttonbn/import_view.php
    • mod/bigbluebuttonbn/index.php

  • Given that current master branch of the plugin lists Moodle 3.2 as minimal supported version we should try to move more of the user presented content to use Mustache templates. Benefints are obvious and numerous.

  • It would be also benefitial to refactor all YUI plugins to AMD format. In cases where YUI functionality is required it can still be used within AMD plugin. Here is a small example of YUI Datatable inside Moodle AMD plugin:

      // local_foo/external plugin.
  define(['jquery', 'core/yui', 'core/log'],
      function ($, Yi, log) {
  
          function init_datatable() {
              Yi.use('datasource-io', 'datasource-jsonschema', 'datatable',
                  function (Y) {
                      var dataSource = new Y.DataSource.IO({
                          source: 'https://jsonplaceholder.typicode.com/posts',
                          ioConfig: {
                              method: 'GET',
                              headers: {
                                  'Accept': 'application/json'
                              },
                              timeout: 1000
                          }
                      });
  
                      dataSource.plug(
                          Y.Plugin.DataSourceJSONSchema,
                          {
                              schema: {
                                  resultFields: [{key:'id'}, {key:'title'}, {key:'body'}]
                              }
                          }
                      );
  
                      var table = new Y.DataTable({
                          columns: [
                              {key: 'id'   , label: 'Id'     },
                              {key: 'title', label: 'Country'},
                              {key: 'body' , label: 'Name'   }
                          ],
                          caption: 'Data'
                      });
  
                      table.plug(Y.Plugin.DataTableDataSource, {datasource: dataSource});
  
                      table.datasource.load();
  
                      dataSource.after('response', function() {
                          table.render('.example');
                      });
                  }
              ); // End YUI.
          }
  
          return {
              table: null,
              init: function () {
                  init_datatable();
              }
          };
  
      } // End Main function.
  );

  // This would be later invoked in the page like this:
  $PAGE->requires->js_call_amd('local_foo/external', 'init');

Walkthrough for refactoring of mod/bigbluebuttonbn/view.php

Step 1 - cleaning includes

require_once(dirname(dirname(dirname(__FILE__))).'/config.php');
require_once(dirname(__FILE__).'/locallib.php');

Becomes:

require(__DIR__.'/../../../config.php');
require_once(__DIR__.'/locallib.php');

Step 2 - properly using print_error

$viewinstance = bigbluebuttonbn_view_validator($id, $bn);
if (!$viewinstance) {
    print_error(get_string('view_error_url_missing_parameters', 'bigbluebuttonbn'));
}

Becomes:

$viewinstance = bigbluebuttonbn_view_validator($id, $bn);
if (!$viewinstance) {
    print_error('view_error_url_missing_parameters', 'mod_bigbluebuttonbn');
}

Step 2a - cleaning bigbluebuttonbn_view_validator function

function bigbluebuttonbn_view_validator($id, $bigbluebuttonbnid) {
    if ($id) {
        return bigbluebuttonbn_view_instance_id($id);
    }
    if ($bigbluebuttonbnid) {
        return bigbluebuttonbn_view_instance_bigbluebuttonbn($bigbluebuttonbnid);
    }
    return;
}

Should become: (it is essentially the same but there is no ambiguity (implicit null on empty return)

function bigbluebuttonbn_view_validator($id, $bigbluebuttonbnid) {
    $result = null;
    if ($id) {
        $result =  bigbluebuttonbn_view_instance_id($id);
    } else if ($bigbluebuttonbnid) {
        $result = bigbluebuttonbn_view_instance_bigbluebuttonbn($bigbluebuttonbnid);
    }
    return $result;
}

Step 3 - removing unneeded code

if (is_null($serverversion)) {
    if ($bbbsession['administrator']) {
        print_error('view_error_unable_join', 'bigbluebuttonbn',
            $CFG->wwwroot.'/admin/settings.php?section=modsettingbigbluebuttonbn');
        exit;
    }
    if ($bbbsession['moderator']) {
        print_error('view_error_unable_join_teacher', 'bigbluebuttonbn',
            $CFG->wwwroot.'/course/view.php?id='.$bigbluebuttonbn->course);
        exit;
    }
    print_error('view_error_unable_join_student', 'bigbluebuttonbn',
        $CFG->wwwroot.'/course/view.php?id='.$bigbluebuttonbn->course);
    exit;
}

Should become: (no need for exit given that print_error throws exception)

if ($serverversion === null) {
    $errmsg = 'view_error_unable_join_student';
    $errurl = '/course/view.php';
    $errurlparams = ['id' => $bigbluebuttonbn->course];
    if ($bbbsession['administrator']) {
        $errmsg = 'view_error_unable_join';
        $errurl = '/admin/settings.php';
        $errurlparams = ['section' => 'modsettingbigbluebuttonbn'];
    } else if ($bbbsession['moderator']) {
        $errmsg = 'view_error_unable_join_teacher';
    }
    print_error($errmsg, 'mod_bigbluebuttonbn', new moodle_url($errurl, $errurlparams));
}

Step 4 - standard page setting

// Print the page header.
$PAGE->set_context($context);
$PAGE->set_url($CFG->wwwroot.'/mod/bigbluebuttonbn/view.php', array('id' => $cm->id));
$PAGE->set_title(format_string($bigbluebuttonbn->name));
$PAGE->set_cacheable(false);
$PAGE->set_heading($course->fullname);
$PAGE->set_pagelayout('incourse');

Becomes:

// Print the page header.
$PAGE->set_context($context);
$PAGE->set_url('/mod/bigbluebuttonbn/view.php', array('id' => $cm->id)); // Relative url is accepted here.
$PAGE->set_title($bigbluebuttonbn->name); // No need for format_string, given that it is already executed in set_title.
$PAGE->set_cacheable(false);
$PAGE->set_heading($course->fullname);
$PAGE->set_pagelayout('incourse');

Step 5 - optimizing checks

// Validate if the user is in a role allowed to join.
if (!has_capability('moodle/category:manage', $context) && !has_capability('mod/bigbluebuttonbn:join', $context)) {
    echo $OUTPUT->header();
    if (isguestuser()) {
        echo $OUTPUT->confirm('<p>'.get_string('view_noguests', 'bigbluebuttonbn').'</p>'.get_string('liketologin'),
            get_login_url(), $CFG->wwwroot.'/course/view.php?id='.$course->id);
    } else {
        echo $OUTPUT->confirm('<p>'.get_string('view_nojoin', 'bigbluebuttonbn').'</p>'.get_string('liketologin'),
            get_login_url(), $CFG->wwwroot.'/course/view.php?id='.$course->id);
    }
    echo $OUTPUT->footer();
    exit;
}

Becomes:

// Validate if the user is in a role allowed to join.
if (!has_all_capabilities(['moodle/category:manage', 'mod/bigbluebuttonbn:join'], $context)) {
    echo $OUTPUT->header();
    $msgtext = isguestuser() ? 'view_noguests' : 'view_nojoin';
    echo $OUTPUT->confirm(
        sprintf('<p>%s</p>%s'), get_string($msgtext, 'mod_bigbluebuttonbn', get_string('liketologin')),
        get_login_url(),
        new moodle_url('/course/view.php', ['id' => $course->id])
    );
    echo $OUTPUT->footer();
    exit;
}

Step 6 - URL generation

// Operation URLs.
$bbbsession['bigbluebuttonbnURL'] = $CFG->wwwroot . '/mod/bigbluebuttonbn/view.php?id=' . $bbbsession['cm']->id;
$bbbsession['logoutURL'] = $CFG->wwwroot . '/mod/bigbluebuttonbn/bbb_view.php?action=logout&id='.$id .
    '&bn=' . $bbbsession['bigbluebuttonbn']->id;
$bbbsession['recordingReadyURL'] = $CFG->wwwroot . '/mod/bigbluebuttonbn/bbb_broker.php?action=recording_' .
    'ready&bigbluebuttonbn=' . $bbbsession['bigbluebuttonbn']->id;
$bbbsession['meetingEventsURL'] = $CFG->wwwroot . '/mod/bigbluebuttonbn/bbb_broker.php?action=meeting' .
    '_events&bigbluebuttonbn=' . $bbbsession['bigbluebuttonbn']->id;
$bbbsession['joinURL'] = $CFG->wwwroot . '/mod/bigbluebuttonbn/bbb_view.php?action=join&id=' . $id .
    '&bn=' . $bbbsession['bigbluebuttonbn']->id;

Becomes:

$bbbsession['bigbluebuttonbnURL'] = new moodle_url('/mod/bigbluebuttonbn/view.php', ['id' => $bbbsession['cm']->id]);
$bbbsession['logoutURL'] = new moodle_url(
    '/mod/bigbluebuttonbn/bbb_view.php',
    ['action' => 'logout', 'id' => $id, 'bn' => $bbbsession['bigbluebuttonbn']->id]
);
$bbbsession['recordingReadyURL'] = new moodle_url(
    '/mod/bigbluebuttonbn/bbb_broker.php',
    ['action' => 'recording_ready', 'bigbluebuttonbn' => $bbbsession['bigbluebuttonbn']->id]
);
$bbbsession['meetingEventsURL'] = new moodle_url(
    '/mod/bigbluebuttonbn/bbb_broker.php',
    ['action' => 'meeting_events', 'bigbluebuttonbn' => $bbbsession['bigbluebuttonbn']->id]
);
$bbbsession['joinURL'] = new moodle_url(
    '/mod/bigbluebuttonbn/bbb_view.php',
    ['action' => 'join', 'id' => $id, 'bn' => $bbbsession['bigbluebuttonbn']->id]
);

This is safer approach as all variables are properly encoded and checked and it is cleaner for dev maintennace.

Step 7 - Move all functions from view.php to new library file

This can be performed in several ways.

New include file

Move all functions as they are into new file viewlib.php. Add appropriate include to the view.php

Autoloaded class

Move all functions as they are into new abstract class located in classes/locallib/view.php. Add appropriate use directive to the view.php

Example:

// Top of the view.php.
use mod_bigbluebuttonbn\locallib\view;

// Somewhere in the code.
view::bigbluebuttonbn_view_message_box(... parameters);

Step 8 - Refactor completely view.php to use Mustache template

This would be the ideal case that would require most of the work.

Walkthrough refactoring of mod/bigbluebuttonbn/index.php

Step 1 - cleaning includes

require_once(dirname(dirname(dirname(__FILE__))).'/config.php');
require_once(dirname(__FILE__).'/locallib.php');

Becomes:

require(__DIR__.'/../../../config.php');
require_once(__DIR__.'/locallib.php');

Step 2 - passing output to renderer

In this case given that html_table is being used it is structurally better to transform this into renderable.

  • Create class mod_bigbluebuttonbn\output\index that will implement renderable

      namespace mod_bigbluebuttonbn\output;
  
  use renderable;
  use html_table;
  use html_writer;
  use stdClass;
  use coding_exception;
  
  defined('MOODLE_INTERNAL') || die();
  
  require_once($CFG->dirroot.'/mod/bigbluebuttonbn/locallib.php');
  
  class index implements renderable {
  
      /** @var html_table */
      public $table = null;
  
      /**
       * index constructor.
       * @param  stdClass $course
       * @throws coding_exception
       */
      public function __construct($course) {
          global $DB;
  
          // Get all the appropriate data.
          if (!$bigbluebuttonbns = get_all_instances_in_course('bigbluebuttonbn', $course)) {
              notice('There are no instances of bigbluebuttonbn', "../../course/view.php?id=$course->id");
          }
  
          // Print the list of instances.
          $timenow = time();
          $strweek = get_string('week');
          $strtopic = get_string('topic');
          $headingname = get_string('index_heading_name', 'bigbluebuttonbn');
          $headinggroup = get_string('index_heading_group', 'bigbluebuttonbn');
          $headingusers = get_string('index_heading_users', 'bigbluebuttonbn');
          $headingviewer = get_string('index_heading_viewer', 'bigbluebuttonbn');
          $headingmoderator = get_string('index_heading_moderator', 'bigbluebuttonbn');
          $headingactions = get_string('index_heading_actions', 'bigbluebuttonbn');
          $headingrecording = get_string('index_heading_recording', 'bigbluebuttonbn');
  
          $table = new html_table();
          $table->head = array($strweek, $headingname, $headinggroup, $headingusers, $headingviewer, $headingmoderator,
              $headingrecording, $headingactions);
          $table->align = array('center', 'left', 'center', 'center', 'center', 'center', 'center');
  
          foreach ($bigbluebuttonbns as $bigbluebuttonbn) {
              if ($bigbluebuttonbn->visible) {
                  $cm = get_coursemodule_from_id('bigbluebuttonbn', $bigbluebuttonbn->coursemodule, 0, false, MUST_EXIST);
                  // User roles.
                  $participantlist = bigbluebuttonbn_get_participant_list($bigbluebuttonbn, $context);
                  $moderator = bigbluebuttonbn_is_moderator($context, $participantlist);
                  $administrator = is_siteadmin();
                  $canmoderate = ($administrator || $moderator);
                  // Add a the data for the bigbluebuttonbn instance.
                  $groupobj = null;
                  if (groups_get_activity_groupmode($cm) > 0) {
                      $groupobj = (object) array('id' => 0, 'name' => get_string('allparticipants'));
                  }
                  $table->data[] = self::bigbluebuttonbn_index_display_room($canmoderate, $course, $bigbluebuttonbn, $groupobj);
                  // Add a the data for the groups belonging to the bigbluebuttonbn instance, if any.
                  $groups = groups_get_activity_allowed_groups($cm);
                  foreach ($groups as $group) {
                      $table->data[] = self::bigbluebuttonbn_index_display_room($canmoderate, $course, $bigbluebuttonbn, $group);
                  }
              }
          }
  
          $this->table = $table;
      }
  
      /**
       * Displays the general view.
       *
       * @param boolean $moderator
       * @param object $course
       * @param object $bigbluebuttonbn
       * @param object $groupobj
       * @return array
       */
      public static function bigbluebuttonbn_index_display_room($moderator, $course, $bigbluebuttonbn, $groupobj = null) {
          $meetingid = $bigbluebuttonbn->meetingid.'-'.$course->id.'-'.$bigbluebuttonbn->id;
          $paramgroup = '';
          $groupname = '';
          if ($groupobj) {
              $meetingid .= '['.$groupobj->id.']';
              $paramgroup = '&group='.$groupobj->id;
              $groupname = $groupobj->name;
          }
          $meetinginfo = bigbluebuttonbn_get_meeting_info_array($meetingid);
          if (empty($meetinginfo)) {
              // The server was unreachable.
              print_error(get_string('index_error_unable_display', 'bigbluebuttonbn'));
              return;
          }
          if (isset($meetinginfo['messageKey']) && $meetinginfo['messageKey'] == 'checksumError') {
              // There was an error returned.
              print_error(get_string('index_error_checksum', 'bigbluebuttonbn'));
              return;
          }
          // Output Users in the meeting.
          $joinurl = '<a href="view.php?id='.$bigbluebuttonbn->coursemodule.$paramgroup.'">'.format_string($bigbluebuttonbn->name).'</a>';
          $group = $groupname;
          $users = '';
          $viewerlist = '';
          $moderatorlist = '';
          $recording = '';
          $actions = '';
          // The meeting info was returned.
          if (array_key_exists('running', $meetinginfo) && $meetinginfo['running'] == 'true') {
              $users = self::bigbluebuttonbn_index_display_room_users($meetinginfo);
              $viewerlist = self::bigbluebuttonbn_index_display_room_users_attendee_list($meetinginfo, 'VIEWER');
              $moderatorlist = self::bigbluebuttonbn_index_display_room_users_attendee_list($meetinginfo, 'MODERATOR');
              $recording = self::bigbluebuttonbn_index_display_room_recordings($meetinginfo);
              $actions = self::bigbluebuttonbn_index_display_room_actions($moderator, $course, $bigbluebuttonbn, $groupobj);
          }
          return array($bigbluebuttonbn->section, $joinurl, $group, $users, $viewerlist, $moderatorlist, $recording, $actions);
      }
  
      /**
       * Count the number of users in the meeting.
       *
       * @param array $meetinginfo
       * @return integer
       */
      public static function bigbluebuttonbn_index_display_room_users($meetinginfo) {
          $users = '';
          if (count($meetinginfo['attendees']) && count($meetinginfo['attendees']->attendee)) {
              $users = count($meetinginfo['attendees']->attendee);
          }
          return $users;
      }
  
      /**
       * Returns attendee list.
       *
       * @param array $meetinginfo
       * @param string $role
       * @return string
       */
      public static function bigbluebuttonbn_index_display_room_users_attendee_list($meetinginfo, $role) {
          $attendeelist = '';
          if (count($meetinginfo['attendees']) && count($meetinginfo['attendees']->attendee)) {
              $attendeecount = 0;
              foreach ($meetinginfo['attendees']->attendee as $attendee) {
                  if ($attendee->role == $role) {
                      $attendeelist .= ($attendeecount++ > 0 ? ', ' : '').$attendee->fullName;
                  }
              }
          }
          return $attendeelist;
      }
  
      /**
       * Returns indication of recording enabled.
       *
       * @param array $meetinginfo
       * @return string
       */
      public static function bigbluebuttonbn_index_display_room_recordings($meetinginfo) {
          $recording = '';
          if (isset($meetinginfo['recording']) && $meetinginfo['recording'] === 'true') {
              // If it has been set when meeting created, set the variable on/off.
              $recording = get_string('index_enabled', 'bigbluebuttonbn');
          }
          return $recording;
      }
  
      /**
       * Returns room actions.
       *
       * @param boolean $moderator
       * @param object $course
       * @param object $bigbluebuttonbn
       * @param object $groupobj
       * @return string
       */
      public static function bigbluebuttonbn_index_display_room_actions($moderator, $course, $bigbluebuttonbn, $groupobj = null) {
          $actions = '';
          if ($moderator) {
              $actions .= '<form name="form1" method="post" action="">'."\n";
              $actions .= '  <INPUT type="hidden" name="id" value="'.$course->id.'">'."\n";
              $actions .= '  <INPUT type="hidden" name="a" value="'.$bigbluebuttonbn->id.'">'."\n";
              if ($groupobj != null) {
                  $actions .= '  <INPUT type="hidden" name="g" value="'.$groupobj->id.'">'."\n";
              }
              $actions .= '  <INPUT type="submit" name="submit" value="' .
                  get_string('view_conference_action_end', 'bigbluebuttonbn') .
                  '" class="btn btn-primary btn-sm" onclick="return confirm(\'' .
                  get_string('index_confirm_end', 'bigbluebuttonbn') . '\')">' . "\n";
              $actions .= '</form>'."\n";
          }
          return $actions;
      }
  }

  • Create class mod_bigbluebuttonbn\output\renderer that will implement rendering

      namespace mod_bigbluebuttonbn\output;
  
  use html_writer;
  use html_table;
  use plugin_renderer_base;
  
  defined('MOODLE_INTERNAL') || die();
  
  class renderer extends plugin_renderer_base {
  
      /**
       * @param  index $indexobj
       * @return string
       */
      protected function render_index(index $indexobj) {
          return html_writer::table($indexobj->table);
      }
  
  }

  • Alter the index.php to look more or less like this:

      require(__DIR__.'/../../../config.php');
  require_once(__DIR__.'/locallib.php');
  
  $id = required_param('id', PARAM_INT);
  $a = optional_param('a', 0, PARAM_INT);
  $g = optional_param('g', 0, PARAM_INT);
  
  if (!$id) {
      print_error('You must specify a course_module ID or an instance ID');
      return;
  }
  
  $course = $DB->get_record('course', array('id' => $id), '*', MUST_EXIST);
  
  require_login($course, true);
  
  $context = context_course::instance($course->id);
  
  // Print the header.
  $PAGE->set_url('/mod/bigbluebuttonbn/index.php', array('id' => $id));
  $PAGE->set_title(get_string('modulename', 'bigbluebuttonbn'));
  $PAGE->set_heading($course->fullname);
  $PAGE->set_cacheable(false);
  $PAGE->set_pagelayout('incourse');
  
  $PAGE->navbar->add(get_string('modulename', 'bigbluebuttonbn'), "index.php?id=$course->id");
  
  $submit = optional_param('submit', '', PARAM_TEXT);
  if ($submit === 'end') {
      // A request to end the meeting.
      $bigbluebuttonbn = $DB->get_record('bigbluebuttonbn', array('id' => $a), '*', MUST_EXIST);
      if (!$bigbluebuttonbn) {
          print_error("BigBlueButton ID $a is incorrect");
      }
      $course = $DB->get_record('course', array('id' => $bigbluebuttonbn->course), '*', MUST_EXIST);
      $cm = get_coursemodule_from_instance('bigbluebuttonbn', $bigbluebuttonbn->id, $course->id, false, MUST_EXIST);
      // User roles.
      $participantlist = bigbluebuttonbn_get_participant_list($bigbluebuttonbn, $context);
      $moderator = bigbluebuttonbn_is_moderator($context, $participantlist);
      $administrator = is_siteadmin();
      if ($moderator || $administrator) {
          bigbluebuttonbn_event_log(\mod_bigbluebuttonbn\event\events::$events['meeting_end'], $bigbluebuttonbn);
          echo get_string('index_ending', 'bigbluebuttonbn');
          $meetingid = $bigbluebuttonbn->meetingid.'-'.$course->id.'-'.$bigbluebuttonbn->id;
          if ($g != '0') {
              $meetingid .= '['.$g.']';
          }
  
          bigbluebuttonbn_end_meeting($meetingid, $bigbluebuttonbn->moderatorpass);
          redirect('index.php?id='.$id);
      }
  }
  
  /** @var mod_bigbluebuttonbn\output\renderer $renderer */
  $renderer = $PAGE->get_renderer('mod_bigbluebuttonbn');
  
  echo $OUTPUT->header();
  echo $OUTPUT->heading(get_string('index_heading', 'bigbluebuttonbn'));
  echo $renderer->render(new \mod_bigbluebuttonbn\output\index($course));
  echo $OUTPUT->footer();
@kiklop74
Copy link
Author

kiklop74 commented Feb 5, 2019

This is still work in progress, it shall be updated or new gists created as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment