More accurate LTI module cleanup process

[ferishili]

This PR fixes #346

How it works

• Basically it checks if the related setting for specific ocinstance is enabled and proceeds, otherwise, removes the opencast lti module record from the table to avoid unwanted behavior.
  • ltimodulemanager::update_existing_lti_modules -> checks and decides whether to remove or proceed with its job.
  • ltimodulemanager::record_manually_added_lti_modules -> checks and decides whether to remove or proceed with its job
  • ltimodulemanager::cleanup_lti_module_entries -> refuses to delete the records if the setting is disabled.

NOTE: Please try to review the code and conduct an in-depth test, before merge!

[NinaHerrmann]

Could you take a look at the failing test and write an additional test regarding the feature before requesting the review? :)

[ferishili]

Could you take a look at the failing test and write an additional test regarding the feature before requesting the review? :)

The failing test is only a timing mismatch and will succeed with a re-run! Additional test step is not needed!

[NinaHerrmann]

Why is no test needed? As it caused a bug previously I would expect a test checking if after deleting an opencast instance the number of extern tools is equivalent.

[ferishili]

The checks are all green! Let me see what I can do about additional feature steps, however, it would take some time! Thanks!

[berthob98]

Thank you for the fix @ferishili I experienced one error when testing

Problem

With this patch applied running the task block_opencast\task\cleanup_lti_module_cron produces an error for me. Log:

Step 2: Looking for manually added LTI modules...
... cleanup process failed:
Fehler in der Kodierung gefunden, den nur Programmierer/innen korrigieren können: moodle_database::get_in_or_equal() does not accept empty arrays

Analysis

I think this line list($insqltoolids, $inparamstoolids) = $DB->get_in_or_equal(array_column($toolids, 'id')); in classes/local/ltimodulemanager.php doesn't like the empty toolids[] array, from skipping every entrie in the lines before.

Cheap Fix

My first Idea was to skip the total function body of these 3 functions

• record_manually_added_lti_modules()
• update_existing_lti_modules()
• cleanup_lti_module_entries()

if both addltiepisodeenabled_ and addltienabled_ are disabled. Not having to deal with problems of that sort. If one of these two is enabled the error shouldn't occure, because at least one preconfigured_tool will exists. But maybe there are better ways to handle this.

[ferishili]

if both addltiepisodeenabled_ and addltienabled_ are disabled. Not having to deal with problems of that sort. If one of these two is enabled the error shouldn't occure, because at least one preconfigured_tool will exists. But maybe there are better ways to handle this.

Thanks for your testing and analysis. You are right, escaping the functions when they are not enabled, is one way to do, however, it compromises the whole concept and idea behind this feature and also the multi-instance won't be supported!

[ferishili]

Why is no test needed? As it caused a bug previously I would expect a test checking if after deleting an opencast instance the number of extern tools is equivalent.

To make things clear, it is not about deleting an opencast instance and check if external tool exists, but only to escape the lti cleanup feature when the setting is deactivated.

[ferishili]

@justusdieckmann was there a reason you have disabled the behat feature? I have noticed that, moodle 403 does not like the old way.

[ferishili]

@berthob98 would you please test the latest commit changes.

[berthob98]

Yes

[justusdieckmann]

@justusdieckmann was there a reason you have disabled the behat feature? I have noticed that, moodle 403 does not like the old way.

@ferishili Yes, it seems that manually specifying custom parameters for lti tool activities is not possible anymore in 4.3 because of MDL-78916. That means we can't manually create opencast lti activities, so I had to deactivate the behat feature that tests creating those. I still want to take a closer look, and maybe start a discussion, but since we want to get this release out as soon as possible, I've thought it best to postpone that until after the release :)

[ferishili]

@ferishili Yes, it seems that manually specifying custom parameters for lti tool activities is not possible anymore in 4.3 because of MDL-78916. That means we can't manually create opencast lti activities, so I had to deactivate the behat feature that tests creating those. I still want to take a closer look, and maybe start a discussion, but since we want to get this release out as soon as possible, I've thought it best to postpone that until after the release :)

All right, thanks for the reply. Following the request from @NinaHerrmann to provide behat test features, and according to the test results that were green (except in 4.3), we are now safe to merge this PR. PS: the behat test feature now contains the scenario to cover the problem!

[NinaHerrmann]

As we are planning the 4.3 Release it is not an option to let the 4.3 test fail...

[ferishili]

As we are planning the 4.3 Release it is not an option to let the 4.3 test fail...

It was all green, and I did not change anything regarding the failed tests. This is getting out of hand. And by the way, believe me or not, I am strongly against having to merge a PR with failed tests! And I am insisting on this point (needless to say, it is and always has been my moto! 😄), so, you don't need to worry, I am well aware!

[NinaHerrmann]

I also feel like 70 % of Pull Requests are just replaceable discussions regarding testing etc. :joy: Sorry, I understood you wrong! Looking forward to the ci-results, thank you for all the work - although I write sometimes a little bit unfriendly, I really appreciate it!!!

[ferishili]

@NinaHerrmann All GREEN, you are good to go!

[NinaHerrmann]

Thank you so much! :+1: