Opencast Recording Feature, refs #284

[ferishili]

No description provided.

[ferishili]

the # 2 in commit 416e19e is not a reference but simply a second try indicator!

[abias]

Hi @ferishili ,

thank you very much for submitting this pull request!
I tried to give it a peer review from a neutral position.

Unfortunately, I didn't have a BBB and OC setup up and running yet to test the whole process lane. Thus, I will mainly focus on the code changes and additions.

The basic goal of this PR, as far as I understand, is
a) to check if the Opencast plugin set is installed in Moodle
b) to send the Opencast series ID of the Moodle course alongside of the other metadata fields to BigBlueButton when a new BBB meeting is initiated.
c) to show existing meeting recordings in Opencast alongside BBB recordings on view.php.

As a general feedback not related to particular code lines, please allow these comments:

• You are injecting Opencast-specific code into the BBB activity module and you are especially injecting an Opencast-specific setting into the middle of the existing 'Configuration for "Show recordings" feature' admin settings section.
  While a Moodle administrator who installs the Opencast plugins alongside of the BBB plugins might understand the purpose of this additional setting, I think you would make the connection of both plugins more understandable if you would:

1. Add a whole new 'Opencast integration' settings section.
2. Add a short introduction paragraph to this section, something like 'These settings control the integration of BBB meeting recordings and Opencast'.
3. Move the new 'Opencast can be used for Recording' setting which you have created to this section.

• In addition to that, I would propose to restrict the scope of the 'Opencast can be used for Recording' setting to just control if the Opencast series ID of the Moodle course is sent to BBB or not.
  After that, you can add a second new admin setting which controls if the recordings are also shown on the activity overview page or not.
  This way, admins can decide if they just want to use Opencast for processing the recordings or if they want to show the recordings within the BBB activity as well.

• As a small note: Please raise the version in version.php just by one increment. This will help everyone who is testing your patch as you introduce a new setting and new language strings.

Thanks again and keep up the good work!

Cheers,
Alex

[ferishili]

I would like to write a few suggestions to add to the latest commit.

• Change Opencast metadata subjects to source if possible.
• There should be more clear declaration text about filter_opencast, that it is important to display the videos (specially in admin setting)
• amd/src/mod_lti_form_handler.js exists in blocks/opencast/amd/src/block_lti_form_handler.js should we use it instead of creating one!

[abias]

Hii @ferishili ,

thank we for your additional and extensive work for this PR!

I skimmed over your latest changes and comments and would like to send you this feedback:

• I have commented on several place in the code during my last review. Unfortunately, due to the great number of comments, Github did shorten the list and just showed the first and the last comments. There were more comments in the middle of the list and these are not resolved yet:
  
  I do not want to force you to process these comments as well, I just want to be sure that you didn't miss them just because of the way how Github shortened the list.

• I saw that you produced a huge amount of code to implement the Opencast recording playback feature which I proposed. Thank you very much for that!
  I am afraid that I might not have time to fully review this feature now and will have to chat with @moodlebeuth first who asked me to fo this peer review.

• You wrote:

Change Opencast metadata subjects to source if possible.

I am unsure what you mean with this proposal, could you please tell us some more?

• You wrote:

There should be more clear declaration text about filter_opencast, that it is important to display the videos (specially in admin setting)

Well, my gut feeling is that you should handle this case similar to how the bigbluebuttonbn_check_opencast() is implemented. The code should check itself if filter_opencast is installed and correctly configured. If it is not, then the video playback functionality must not be offered to the user in the UI.

• You wrote:

amd/src/mod_lti_form_handler.js exists in blocks/opencast/amd/src/block_lti_form_handler.js should we use it instead of creating one!

If you ensure that the code which is requiring the AMD file is only called after bigbluebuttonbn_check_opencast() has verified that block_opencast is installed, it should be fine to require the AMD file from block_opencast instead of duplicating existing code and making the codebase more cluttered.

Cheers,
Alex

[ferishili]

Hi everyone,
@abias, thanks for the feedback, and sorry I did not see those comments :( my bad!
I will take care of them ASAP and update the PR again! @jfederico sorry for the mess :D

[abias]

Hi @ferishili ,

You wrote:

sorry I did not see those comments :( my bad!
I will take care of them ASAP and update the PR again!

Thank you!

Furthermore, I have taken another look at your last commit 5fb2a53.
I have commented to the most obvious things which I could spot, but I didn't have the time to do a full review again. I hope that my feedback helps you at least a little bit.

In the end, I would like to thank you again for your work!

[ferishili]

Hi @abias, it's been a while and this PR is not yet reviewed. Would you mind taking a look?

[abias]

Hi @ferishili ,

I am really sorry for my lack of reply until now.

I have skimmed over your latest changes and can say that you have handled my initial feedback properly and you have adopted the spirit of my review in your additional work. I am happy to see that you have implemented the recording playback functionality (which was a quite big piece of work) and have still strived to implement the Opencast-specific code in separate functions instead of interweaving it into the existing plugin.

Having said that, I am afraid that you have produced so much new / changed code that I do not have enough time to do a full review again at the moment. Additionally, I see that there is so much change in the master branch currently with the goal to integrate this plugin into Moodle core that more changes might be necessary before this gets considered for integration by the plugin maintainers.

That's why I would like to ping @jfederico who has given an early feedback in #284 to ask for some feedback if this contribution is on a good way or not and when this could be considered for an integration review.

Cheers,
Alex

[jojoob]

Since the v3.11-r3 release of tool_opencast and block_opencast the plugins now support multiple Opencast instances.
block_opencast\local\apibridge::get_instance() now requires the ocinstanceid parameter. I did a dirty workaround by just passing 1 to select the default instance. But a setting for selecting the Opencast Instance would be necessary.

Sadly I could not do any further testing because I've some problem with my BBB to Opencast integration not related to this PR.

[jojoob]

I could now test a little bit more and found a bug: Listing of Recordings does not work when the Moodle course uses groups.

But meanwhile there were a lot of changes force pushed to the master branch... :/