Batch/Bulk Upload

[ferishili]

This issue fixes #350,

Description

please refer to the issue,

How it works

• A new setting is provide to activate/deactivate the feature "Enable batch video upload".
• A new button called "Add videos (batch)" is provided in overview page as well as block sidebar component, which redirects to the batch upload page.
• In order to provide any event metadata, you should check the Batchable option of the desired metadata in the Event metadata settings.
• Only Presenter videos can be uploaded into the file manage component in the batch upload page.
• The title of the file will be selected as for the title of the video.
• One could drag and drop files in the filemanager component or select multiple files one at a time.
• The uploaded files will get a upload job record (sit in the upload queue) to be picked up and transferred to the Opencast.

Important

• You need consider increasing the upload job limit as well as timeouts for upload and the Opencast API instance setting.

[taraghi]

Hi @ferishili

We had the honor of testing this patch at Graz university of technology, running on Moodle 4.1.9 with PHP 7.4

I, as manager, switched my role to another user role in a course with block opencast added containing a series.
Then I switched back my role to the manager, and got the following error message

The php function str_contains() exist since PHP 8. As we have PHP 7.4 running, this leads to a PHP error. There are alternative php functions such as strpos() that can be used instead, in order to support lower PHP versions.

Thank you!

[ferishili]

Hi @taraghi,
thanks for your feedback, it is indeed a backward incompatibility of PHP versions and has to be fixed. However, it requires a new (separate) issue, because it is not related to this PR and its content!
Would you mind creating that issue, I will then proceed with the fix ASAP.
Thanks in advance.

[llttugraz]

Hello @ferishili,
when testing this the following behaviour has been observed:
When using the bulk upload, you have to load all the videos into the one upload window. You can drag and drop several videos and it works. However, if you do not use drag and drop but the file browser, you can only select one file at a time and have to choose all of them individually, which is confusing (expected behaviour would be that you can choose several files inthe file browser with shift or command and upload them in bulk as well).

Should we open a new issue for this?

Thanks,
y

[ferishili]

Hi @llttugraz,
I have explained this behavior in the last showcase meeting, and it is unfortunately the way that is provided by Moodle itself, therefore, we have no control over this.
Please refer to filemanager form element of the Moodle for more info
PS: every feedback related to batch/bulk upload (this PR) does not need a separate issue, we could discuss it here, and if any change is required, I will commit to this PR.
BR

[taraghi]

Hi @taraghi, thanks for your feedback, it is indeed a backward incompatibility of PHP versions and has to be fixed. However, it requires a new (separate) issue, because it is not related to this PR and its content! Would you mind creating that issue, I will then proceed with the fix ASAP. Thanks in advance.

Hi @ferishili

As far as I see the specified php function is used in this patch. It is not used in the last stable version. But I can for sure write a separate issue for that. No problem.

[ferishili]

Hi @taraghi, thanks for your feedback, it is indeed a backward incompatibility of PHP versions and has to be fixed. However, it requires a new (separate) issue, because it is not related to this PR and its content! Would you mind creating that issue, I will then proceed with the fix ASAP. Thanks in advance.

Hi @ferishili

As far as I see the specified php function is used in this patch. It is not used in the last stable version. But I can for sure write a separate issue for that. No problem.

Hi,
the php 8 function was introduced here:

moodle-block_opencast/block_opencast.php

Line 217 in 8c00eec

return str_contains($actionlink->attributes['class'], 'editing_delete');

via this commit: 8c00eec

And of course, it is in master not in the latest stable version!

Since we are at it, the following php 8 function will also make problem:
str_starts_with:

moodle-block_opencast/block_opencast.php

Line 228 in 8c00eec

if (str_starts_with($k, 'data-modal')) {

With that said, the changes in this PR and its context has nothing to do with above-mentioned places

[ferishili]

And the reason, I insist on having a separate issue, is just to have a clear issue to PR relationship and not fix/add something else other than the context described in the issue #358. In this way, future developers do not have a hard time debugging or looking for something when referring to the git history.

[taraghi]

@ferishili I see. Thank you very much for clarification.

I just wrote a separate issue including also your comments: #359

[ferishili]

@ferishili I see. Thank you very much for clarification.

I just wrote a separate issue including also your comments: #359

Thank you so much, I will fix that ASAP.

[ferishili]

Hi @taraghi,
the incompatibility problem is now fixed and is also merged from master to this PR. Please give it another try after the CI checks are all green.
Thanks & BR

[llttugraz]

@taraghi, it all looks green :)

[ferishili]

Hi @taraghi, @llttugraz,
This PR is still waiting for your review, please make sure that it does wait much longer.
Thanks in advance,
BR

[justusdieckmann]

Hey @ferishili,

Thank you very much for your work! The pull request looks good to me in general. I have added review comments for a few bugs and typos I found and some nit-picking. Please see what you think of them :)

Cheers
Justus

[llttugraz]

@ferishili, thanks very much :)
It all looks good on our end. We can provide the German translation strings if you could include them — unless @taraghi we can add them directly in the code(?)

[ferishili]

@ferishili, thanks very much :) It all looks good on our end. We can provide the German translation strings if you could include them — unless @taraghi we can add them directly in the code(?)

Happy to hear that, thanks for the feedback. I will proceed with change requests and merging it.
PS: The plugin has to have only English translation (default) in the master, other translations need to be added via external locale tools that Moodle offers, which I don't know much about (You need to ask the community, or event better @mwuttke who has the most experience 🥇 )

[NinaHerrmann]

Translations can be inserted on the Moodle Page - when we release a new Version. Sorry, but it is the "standard" Moodle way 🙈
If you are interested you can check:
https://docs.moodle.org/403/en/Translation

[ferishili]

Hi @justusdieckmann,
thanks for the review, the change requests have been resolved, please have a look at them, I answered some of them directly if not changed!
BR

[ferishili]

Hi @justusdieckmann,
I would be very happy to proceed with this PR, therefore, if there is nothing left, I would like to merge this PR.
Thank you for your review.

[NinaHerrmann]

@ferishili Please take in mind that @justusdieckmann is not working full-time for us. We are working on Pull Request as fast as we can but we still have an ongoing discussion regarding the batchupload.php file.

[ferishili]

@ferishili Please take in mind that @justusdieckmann is not working full-time for us. We are working on Pull Request as fast as we can but we still have an ongoing discussion regarding the batchupload.php file.

All right, thanks for the info.
I resolved all the requests and answered all questions, please let me know if you have more question.
PS: I admire the strictness on the review, however, I have a feel that is getting a bit more than expected and I don't see any valid objection against this PR!

[justusdieckmann]

I'm sorry you feel that way. I can see that the two unused imports and unused global $variables might not be necessary, but apart from that and the $totalfiles comment, I think that every other reviewer should have mentioned those points I raised. What do you think is more than expected?

[ferishili]

I'm sorry you feel that way. I can see that the two unused imports and unused global $variables might not be necessary, but apart from that and the $totalfiles comment, I think that every other reviewer should have mentioned those points I raised. What do you think is more than expected?

Hi @justusdieckmann, no need to be sorry at all.
I think exactly otherwise, I appreciate the review and your time for those unused import and variables as well as string adjustments, but I don't see any useful point or improvement in adding that series checkers which is your only objection against this PR.
Good that you added the PR for that, so we can continue there!

All in all, thank you both for your efforts and time, just keep in mind that the main goal is to work together to improve the plugins, not to prove that the other ones work is incorrect!

I would suggest that we schedule a meeting to discuss the point because it appears that our local dev systems are not working the same!

Thanks and best regards

[justusdieckmann]

Hey @ferishili,
my only objection against this PR is the link in the settings. As I said, the $series comment in batchupload.php was just a completely optional alternative, because I felt that you didn't seem to like the current option. So I'm completely fine with it staying as it is. The pull request I created actually fixes the same thing in addvideo.php, since it is currently still a open security vulnerability.

My goal definitely isn't to prove you wrong, but to ensure that we both think that the final result is error-free and well working code. If we don't do that, a code review doesn't really have a point in my opinion.

On both my machines the link in the settings doesn't work, and I can see that behavior reflected in the code here. Please have a look at that, and see if you can tell why it doesn't happen for you.

Thank you very much :)

[ferishili]

Hi @justusdieckmann
The tool instance url is covered.
Please check it out.

Thanks and BR

[justusdieckmann]

Thank you very much for your work! :)

[ferishili]

Thank you very much as well, I am happy that it is done :)

[mwuttke]

Hello @taraghi, Hello @llttugraz, if you have some translations for this batch/bulk upload feature you can use AMOS for the translations into german or you can send me your snippets and I will do that.

And BTW: Many thanks to you for the feature idea and many thanks for the developers and thanks to the developers who realised this! ;-)

[mwuttke]

@ferishili, thanks for the exelent work. But I think this feature should be optional. At the moment the batch upload is by default an. Please could you change the admin settings accordingly?