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

Feature/sync on category #8

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

adpe
Copy link

@adpe adpe commented Feb 18, 2021

Will help to solve #1

Further I've added GitHub Actions as we don't have any free TravisCI credits.

PS: As I'm using a Moodle core function not available in Moodle 3.5. Do you want really keep a simple master branch? I think you should go for different branches. So that this new feature will only be available for newer Moodle releases.

HTH
Adrian

@paulholden
Copy link
Owner

Thanks Adrian, please can you remove the last 2 commits (I already have an MR from someone else I need to merge for moving to GHA, and I don't think we should be removing the Travis file - I still use it for instance)

Also, we are only retrieving roles assignable in the system context (from memory), these aren't necessarily the same roles that we can assign in category context - can you double check that?

Thanks again!

@adpe
Copy link
Author

adpe commented Feb 18, 2021

Hi @paulholden

Oh, you're right I didn't give attention to the roles contexts. Because in our case we only want to add system roles to categories. Maybe we should then first select where we want to make user synchronizations (system/category) and depending on that option load only the given roles, or maybe it's easier to split it up in two different edit forms?

For me this feature can also be integrated on a later point. So in this iteration, the plugin only allows to set system roles to categories. Which is already a gain!

What do you think?

PS: I will remove the last 2 commits, therefore I'll make two branches. One for merging and one to have the test results.

Greets
Adrian

@adpe adpe force-pushed the feature/sync-on-category branch 2 times, most recently from 7affba7 to f0baf3f Compare February 19, 2021 07:46
@adpe
Copy link
Author

adpe commented Feb 19, 2021

Hi @paulholden

I've implemented now your mentioned point, about assign only given roles in a context. Hope you are happy about this implementation. If not, feel free to address some minor issues.

I'll have actually no more resources to help to solve issue #1. Thank you for your hard work to finally integrate it!

Greets
Adrian

@adpe adpe force-pushed the feature/sync-on-category branch from f0baf3f to 071ffdf Compare February 19, 2021 08:10
@adpe
Copy link
Author

adpe commented Mar 1, 2021

Hi @paulholden

Just fixed a bug, which throws this output:

-->local_cohortrole
Default exception handler: Unbekannter DDL Library Fehler Debug: Field local_cohortrole->categoryid cannot be added. Not null fields added to non empty tables require default value. Create skipped
Error code: ddlunknownerror
* line 540 of /lib/ddl/database_manager.php: ddl_exception thrown
* line 129 of /local/cohortrole/db/upgrade.php: call to database_manager->add_field()
* line 692 of /lib/upgradelib.php: call to xmldb_local_cohortrole_upgrade()
* line 1917 of /lib/upgradelib.php: call to upgrade_plugins()
* line 193 of /admin/cli/upgrade.php: call to upgrade_noncore()

!!! Unbekannter DDL Library Fehler !!!
!! Field local_cohortrole->categoryid cannot be added. Not null fields added to non empty tables require default value. Create skipped
Error code: ddlunknownerror !!
!! Stack trace: * line 540 of /lib/ddl/database_manager.php: ddl_exception thrown
* line 129 of /local/cohortrole/db/upgrade.php: call to database_manager->add_field()
* line 692 of /lib/upgradelib.php: call to xmldb_local_cohortrole_upgrade()
* line 1917 of /lib/upgradelib.php: call to upgrade_plugins()
* line 193 of /admin/cli/upgrade.php: call to upgrade_noncore()
 !!

This Wednesday we'll use this branch on our productive system. So I would really appreciate it, if you could review this pull request soon.

Greets
Adrian

@adpe
Copy link
Author

adpe commented Apr 13, 2021

Hi @paulholden

With the help of @mserve I've included some new improvements. Hope this feature will land soon and you can find some time to review it.

Best
Adrian

@mserve
Copy link

mserve commented May 25, 2021

Hi @paulholden , hi @adpe ,

I did not find the time to recheck this PR - it might include an issue with Moodle installations not having a category id=1 anymore, such as our Moodle installation where categories have been coming and going through the last 10 years since being in productive state. Will try to do so by End of June when semester ended!

BR,
Martin

@adpe adpe force-pushed the feature/sync-on-category branch from d9ca459 to 9665e77 Compare February 9, 2022 14:27
@adpe
Copy link
Author

adpe commented Feb 9, 2022

Hi @paulholden

I've just rebased our branch and bumped the versions... Didn't have more time to solve the problem @mserve with the ID's of the category, e.g. 1.

Cheers
Adrian

@uchida-nunet
Copy link

uchida-nunet commented Dec 20, 2022

Hi @paulholden

Thank you for developing the great plugin. We are using it conveniently in our organization.

I too would like to see Adrian's commits merged into this plugin!

Thanks
Uchida

@peta3000
Copy link

Dear @paulholden
I totally second @uchida-nunet and many others - thank you so much for this great plugin and the feature to be able to sync to roles on category level would be really awesome!!

Many thanks and a lovely day!!!

peta

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.

5 participants