Numerous issues and code re-factoring. #39
Conversation
Patch contains indents and control stuctures style changes only to match https://docs.moodle.org/dev/Coding_style. Some white lines and closing php tags were removed as well.
Ideally it should be Moodle with appropriate copyright of actual developer/contributor in the @copyright tag, see https://docs.moodle.org/dev/Coding_style#Files
Any idea when they could be useful (apart of cases if you need to cat ./* > /dev/lp)? ;-)
Make sure there is no empty elements in roles array if roles mapping has not been defined.
If some mapping has been set, it should be possible to unset it. Also patch changes the use of course object and removes unnecessary 'else' structure.
…ers. When you see block_panopto_server_number = 1 one would need to keep in mind that 1 actually means 2. As the variable name implies the number, lets make it using correct value.
Changes made: 1. Removed use of $_SESSION. Using sessions is not requred. $_SESSION is used here to pass over "numservers" variable to course provisioning scripts. When block configuration has not been saved yet, it is useless to do because other values such as host name and key are not saved and provisioning is not possible. When configuration has been saved, one would not need to pass "numservers" via session as it can be retrieved from $CFG->block_panopto_server_number. As a side note for future, $_SESSION should never be used directly, there is session API in Moodle for that purpose. 2. Given there is no point to let user provisioning the course before block configuration has been saved, provisioning link will be shown on block settings page only if at least one Panopto server has been added to configuration. 3. Refactored the code to remove duplicates, unused variables and excessive structures.
As per moodle plugin structure convension, all plugin functions should live in lib.php. If this is not a library that implements something sophisitcated and requires to be logically separated, there is no need to keep it outside standard block functions file.
This will help avoiding panopto_data.php file loading before actual block class instance creation. Also accesslib require is not needed, it is avialable by now.
This makes files loading more efficient as "require_once" is called only when class instance has been requested.
It is a good practive to store plugin configuration in config_plugins table to avoid name collision and ordering. This patch updates plugin to move all existing configuration from global level to config_plugins and changes the way of accessing configuration values accordingly.
Since the block relies heavily on enrolments and course data, it can be used only in courses.
This is not supported yet (or any more), adding block to my page does not work die to the lack of course context and users to enrol. Also, it has been disabled in applicable formats for a while.
Since events tracking has been added (aka rollingsync), there is no need to run cron to sync enrolments. Besides, cron function is broken anyway for a while (after introduction of multiple panopto servers $panopto_data->servername is undefined unless courseid has been specified).
When block is deleted, we need to remove "viewer" users from Panopto folder access and clean-up instance configuration table. We keep Publishers and Creators in folder access list, so that they may continue managing folder via Panopto interface. This includes some refactoring of classes/task/update_user.php as function to determine role is now needed elsewhere and moved to panopto_data class.
Add phpdoc statements and require library only when task is executed.
Since all Panopto communication is performed via panopto_data instance, it would be handy to have is_provisioned function that indicates course provisioning status.
We need to make sure that course has been properly provisioned before attempting to do any remote enrolment or role change.
For course context, provision_course_internal.php is used when course has not been provisioned yet, so there is no way to retrieve server/key from foldermap record as they simply not there yet. In fact those removed statements are never reached in reality. For system context, provision_course.php is used irrespective whether course has been provisioned, again all requred server/key data is in the config, no need to check if it has been stored in foldermap. And again, in reality those statements are never reached because although provision_course.php has some coverage for the case when being called from the course level (in which case $selectedserver is not defined and supposed to be retrieved from the foldermap table), this is never being used as from the course context provision_course_internal.php is being used instead.
As they are no longer used anywhere in the code.
|
It took nearly 6 hours to rebase all my commits on top of changes you merged last week, had to send pull request earlier ;) I tired to describe each change in details in commit message. Just to keep revision process clear enough, if you have any questions or comments related to particular commit I suggest to discuss it within that particular commit (if you click on any commits listed above it will open the diff with comments box at the bottom). More general questions can be discussed here in the pull request itself. |
|
Thanks for all this hard work! We did a round of clean up to try to port your changes manually and that lead to the other change that you referenced. I am tracking an internal work item to consider and test this pull request next week. |
To be consistent with new block provisioning, the link to re-provision the block should use provision_course_internal.php. Also refactored the code to use html_writer and moodle_url methods.
|
One more commit is added that is fixing the provisioning link within the block editing screen of the block that has already been provisioned (see above). |
|
Hello Kabalin, We attempted to review this pull request for inclusion in the main branch, but it has too many changes for us to properly manage. Would you be willing to submit the changes one at a time so we can consider each one's merits individually? We would like to take your changes, but we cannot accept them all at once. Jenn |
|
Jenn, thanks for reviewing the pull request. You could just manage them on one-by-one basis (review by applying and testing individually) and share your concerns once you spotted something that require clarification or changes. Commits are pretty granular and contain only what is stated in the commit message. The main problem is that many of my changes depend on each other, so submitting them as individual patches means a lot of work to rebase each on latest master, and does not make sense in some cases (e.g. if one of them changes something that affects other patches). What if you tell me up to which commit you can merge, then I will shorten the pull request down to that patch, and create another pull request with the rest of patches for further discussion? |
|
Hmm, just noticed you merged ActiveDev that introduced massive formatting change prior to any of my patches. OK, more work then to rebase my changes on top of master again... |
Since we can't guarantee that all required parameters exist at the point of block_panopto class instance creation, we can't define soap_client variable within class constructor. But it would be far safer to access it via getter method that will create a new PanoptoSoapClient if required, than using direct validation and instance creation in any method where this variable is needed (prone to errors e.g. instantiate_soap_client has already being called as static in some places and directly in others). Getter method has been added and variable is defined as private to avoid its direct use from block_panopto class instances in the future.
This ideally should be a part of 7741c05 that introduce multiple servers. The server host/key remain undefined after upgrade since new variable names are used. The patch will only be useful for those upgrading from earlier version than 2014121702, it is generally a bad practice to modify upgrade.php retrospectively, but important in this case to fix panopto connection for the small proportion of users who has not upgraded yet.
So that the class instance can be used for other requests as if it has been created for provisioned course.
|
@kabalin |
|
@Hiroshi-p no problem. The code diverted since March 2015, so most of the patches are either irrelevant or require significant refactoring I suppose. At the point of fulfilling pull request it could have been applied cleanly though, shame you did not allocate anyone to carry out revision and integration - this was a large chunk of work to bring Panopto to Moodle core standards, optimising performance and clean-up. Anyway, feel free to use any commits if you find anything is still relevant. |
|
@kabalin |
This pull request contains block_panopto code re-factoring to match Moodle coding style and fixes issues:
#26 - fixed in 5d837fc
#34 - fixed in cfe7292
#35 - fixed in 0106332
#36 - fixed in e24e7f0
#37 - fixed in 0758965
The pull request includes some significant changes that removes obsolete code instances and optimises existing to make future development work on plugin simplier and functionality more clear.