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

observe event course_category_deleted #1

Closed

Conversation

mserve
Copy link

@mserve mserve commented Apr 2, 2021

Monitor the course_category_deleted event and delete the instances, update required Moodle version

@mserve
Copy link
Author

mserve commented Apr 2, 2021

Remark: there should also be a test on deleting the category, I only tested manually yet

@mserve
Copy link
Author

mserve commented Apr 4, 2021

Tests added 👍🏻

@mserve mserve force-pushed the feature/sync-on-category branch from bcedf1b to 8235b7a Compare April 4, 2021 09:41
@mserve
Copy link
Author

mserve commented Apr 4, 2021

Found an issue within local_cohortrole_render_add_buttons() when adding the plugin to our local testing installation, will check on it later:

Debug-Info:

SELECT id,parent FROM {course_categories} WHERE id = ?
[array (
0 => 1,
)]
Error code: invalidrecord 

Stack trace:

line 1599 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown
line 1575 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
line 6732 of /lib/accesslib.php: call to moodle_database->get_record()
line 126 of /local/cohortrole/locallib.php: call to context_coursecat::instance()
line 141 of /local/cohortrole/locallib.php: call to local_cohortrole_get_context()
line 162 of /local/cohortrole/locallib.php: call to local_cohortrole_get_context_name()
line 37 of /local/cohortrole/index.php: call to local_cohortrole_render_add_buttons()

@mserve
Copy link
Author

mserve commented Apr 5, 2021

So, I found a way around using the issue depending on a category with id "1" by using the default category's context to obtain the roles.

I noted a glitch on the form as it lost the "$mode" after validation, this is also fixed and found a bug in the sort order of the table. Additionally, one was able to assign course-category level only roles to the system context by not choosing any role

One might discuss if the UI changes I introduced are useful. In our case, we have some categories having the same name in different top-level categories and it would be quite useful to see the full path. Also, the direct links to the roles and categories are more or less useful from a (i.e. my ....) admin perspective handling many roles and categories.

I did not squash the commits yet but could do so to keep the history a bit cleaner :)

@adpe
Copy link

adpe commented Apr 13, 2021

Monitor the course_category_deleted event and delete the instances, update required Moodle version

Thanks for implementing this missing part. For me we should not bump the version here yet. When the feature branch is done completely the version.php will be bumped at the end from the plugin maintainer.

Will review your stuff and push it to this merge request. Thanks for your hard work! So I'll clean up the history and remain you as the author.

Greets
Adrian

@adpe
Copy link

adpe commented Apr 13, 2021

Found an issue within local_cohortrole_render_add_buttons() when adding the plugin to our local testing installation, will check on it later:

Debug-Info:

SELECT id,parent FROM {course_categories} WHERE id = ?
[array (
0 => 1,
)]
Error code: invalidrecord 

Stack trace:

line 1599 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown
line 1575 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
line 6732 of /lib/accesslib.php: call to moodle_database->get_record()
line 126 of /local/cohortrole/locallib.php: call to context_coursecat::instance()
line 141 of /local/cohortrole/locallib.php: call to local_cohortrole_get_context()
line 162 of /local/cohortrole/locallib.php: call to local_cohortrole_get_context_name()
line 37 of /local/cohortrole/index.php: call to local_cohortrole_render_add_buttons()

Thanks for raising this new introduced issue. Can you please raise an own issue for that part? We'll only handle/merge the course category deletion handling in this PR.

Greets
Adrian

@adpe adpe closed this Jun 16, 2021
@uchida-nunet
Copy link

uchida-nunet commented Dec 20, 2022

Very helpful. Thanks to both of you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants