Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add moodle filter adaption to the meeting topic and description #615

Merged
merged 8 commits into from
Sep 27, 2024

Conversation

karenliulll
Copy link
Contributor

@karenliulll karenliulll commented Aug 27, 2024

Process Zoom meeting topic and description with proper Moodle filters such as filterCodes and Generico when being viewed outside of Moodle (tested on our NC State Zoom site).

Fixes #610
Fixes #616

@karenliulll karenliulll self-assigned this Aug 27, 2024
@karenliulll karenliulll marked this pull request as ready for review August 27, 2024 15:43
Copy link
Collaborator

@jrchamp jrchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @karenliulll ! There were a couple bumps during our testing, but it's definitely doing some nice things for the Zoom description/agenda. Hopefully we can find a more-reliable way to handle the context piece and determine if Moodle activity names are supported by filters.

lib.php Show resolved Hide resolved
classes/webservice.php Outdated Show resolved Hide resolved
classes/webservice.php Outdated Show resolved Hide resolved
classes/webservice.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@jrchamp jrchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks great overall and fixes two existing edge cases. The only concern I have now is getting the module context in a reliable way so that the filters can produce the expected output.

lib.php Outdated Show resolved Hide resolved
lib.php Outdated Show resolved Hide resolved
backup/moodle2/restore_zoom_stepslib.php Show resolved Hide resolved
@jrchamp
Copy link
Collaborator

jrchamp commented Sep 12, 2024

This should be tested to see if/how well the code paths are working:

  • Backup/restore activity
  • Duplicate activity
  • Create new activity
  • Edit existing activity
  • Quick-edit (for example, the quick-rename functionality on the main course page)
  • Recreate expired meeting

@jrchamp jrchamp added the bug Fixes problems or reduces technical debt label Sep 12, 2024
@karenliulll
Copy link
Contributor Author

karenliulll commented Sep 18, 2024

I did some quick testing and came across the following issues -

  1. Error occurred for both creating and updating meetings when testing the code - "Exception - Cannot access protected property mod_zoom_mod_form::$_cm". Should be related to the value $mform->_cm->id being passed to the zoom_webservice()->create_meeting() and update_meeting() functions in lib.php.
  2. Quick edit of Zoom meeting title on the main course page will not auto-update the title on the Zoom site outside of Moodle (does quick edit also make the API call like in full edit?)
  3. when performing course backup and restore, the meetings in the new course will update the title and description accordingly in Moodle, but not on the Zoom site - for example, if the {coursename} filter is used in the meeting description, it will display the new course name in Zoom but still show the old one on Zoom site. (Looks like the restore function should make the API call via the zoom_webservice()->create_meeting() function, but somehow it does not update the content outside of Moodle.)

@jrchamp
Copy link
Collaborator

jrchamp commented Sep 19, 2024

I did some quick testing and came across the following issues
Thank you! 🥂

  1. Error occurred for both creating and updating meetings when testing the code - "Exception - Cannot access protected property mod_zoom_mod_form::$_cm". Should be related to the value $mform->_cm->id being passed to the zoom_webservice()->create_meeting() and update_meeting() functions in lib.php.
  • $mform->get_context() will provide course context on add and module context on update
  • $mform->get_coursemodule() will provide the $cm on update, but is expected to be null on add. 😞
  1. Quick edit of Zoom meeting title on the main course page will not auto-update the title on the Zoom site outside of Moodle (does quick edit also make the API call like in full edit?)

It should, but the $zoom->name = $zoom->topic; line was removed (which is still correct). The following code block needs to be modified:

moodle-mod_zoom/lib.php

Lines 426 to 429 in 8dc452f

if ($zoom->name !== $fullzoom->name) {
$fullzoom->name = $zoom->name;
zoom_webservice()->update_meeting($zoom);
}

My idea is: "Move the substr(format_string($zoom->name, true, $options + ['escape' => false]), 0, 200), code to a helper function, such as a lib.php function." That will allow us to perform the same filtering/modifications on the activity name that we do within the webservice so that the values will be comparable. Please check if the $fullzoom->name = $zoom->name; line should be removed (it only affects the calendar and grade item names, so if filters are not being applied to those strings then we should keep the line).

  1. when performing course backup and restore, the meetings in the new course will update the title and description accordingly in Moodle, but not on the Zoom site - for example, if the {coursename} filter is used in the meeting description, it will display the new course name in Zoom but still show the old one on Zoom site. (Looks like the restore function should make the API call via the zoom_webservice()->create_meeting() function, but somehow it does not update the content outside of Moodle.)

I don't know what the sequence is or what the context is as the point when the restore steps run. And it's possible that the full course backup and restore would behave differently than an individual activity backup/restore and even a activity duplicate action. However, I do think that the fixes for question "2" might significantly help fix some of these issues because the "calculated" name will be updated semi-regularly by Moodle and send the updates into Zoom.

@karenliulll
Copy link
Contributor Author

This should be tested to see if/how well the code paths are working:

  • Backup/restore activity
  • Duplicate activity
  • Create new activity
  • Edit existing activity
  • Quick-edit (for example, the quick-rename functionality on the main course page)
  • Recreate expired meeting

I tested all the above after the latest code change, it works fine for me.

Copy link
Collaborator

@jrchamp jrchamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much @karenliulll!

This journey was much more complicated than expected, but the resulting code is definitely an improvement. When we know the context, we are now correctly filtering it without any significant overhead.

@jrchamp jrchamp merged commit 8136026 into main Sep 27, 2024
14 checks passed
@jrchamp jrchamp deleted the enhancement/moodle-filters-adaption branch September 27, 2024 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes problems or reduces technical debt
Projects
Status: No status
Status: Done
Development

Successfully merging this pull request may close these issues.

Respect Zoom's API limits on topic and agenda Moodle display filters for zoom meeting topics and description
3 participants