search

emmarichardson / local_autogroup

A local plugin for Moodle 2.7 onwards which handles the dynamic creation, population and cleanup of groups on courses.
6 stars 19 forks source link

Implement #42 delimited text profile field #48

Closed TomoTsuyuki closed 1 year ago

TomoTsuyuki commented 1 year ago

Hi,

I just implemented #42 by adding a new sort module.

I found a bug for current sort modules to show values in the course participant Auto Group setting, so I fixed it as well.

emmarichardson commented 1 year ago

Thank you so much for this - I will have a few people review and then get it merged! @ak4t0sh do you have time to look at this - would be a great enhancement.

MURBASLMS commented 1 year ago

Hi, any word on this one please? keen to get it in our set up :)

emmarichardson commented 1 year ago

I had a chance to test this in 4.1 and it worked as expected - my one thought is that would it better to integrate this in the existing Custom Profile Field option instead of having a whole separate option? So, if you selected Custom Profile FIeld, there would be a check box on that setting page for Multi Values? and then the option to select the delimiter. Not a huge deal either way - just a thought. If anyone else would like to test to, that would be appreciated but for now it is looking good.
I am at the point now of having three separate requests that I need to merge and I need to go through and see if any will clash with each other - if anyone is familiar enough with github to know how to handle multiple changes to a single file (I am thinking lang files for sure), I would appreciate knowing how to deal with that or if I need to merge one request and then have the others adjust theirs to match the latest release?

TomoTsuyuki commented 1 year ago

Hi @emmarichardson ,

I checked the other PRs. It looks #34 has conflict renderer.php, and #43 has conflict lang/en/local_autogroup.php with my PR. There are no same files changed between #34 and #43. I think you can merge #34 and #43, then I'll fix conflict in this PR, then merge it.

TomoTsuyuki commented 1 year ago

Hi @emmarichardson

Please let me know what I can do for this PR.

emmarichardson commented 1 year ago

Ok, finally got the other two merged. So sorry for the delay - just have very little time to work on this. If you can fix your pull request and resubmit, I will do a final test and merge and then upload new version to plugin directory with all new functionality.

TomoTsuyuki commented 1 year ago

Hi @emmarichardson

Thanks for updating the repo. I rebased and fixed conflicts.

Please test again and merge if it's ok.

MURBASLMS commented 1 year ago

hi, understand time is limited so not hassling (well, a bit!), just wondering if this can get done fairly soon please?

emmarichardson commented 1 year ago

Sorry, it is summer break but I am trying to get it to pull into my test server to test today or tomorrow...

emmarichardson commented 1 year ago

Sorry, I am still trying to checkout this pr but everytime I do that locally, GH desktop tells me there are no changes and cli says it has switched to this branch but I do not see the changes in my code - not sure what is going on. I am traveling Mon/Tues but will keep trying...

emmarichardson commented 1 year ago

Ok, this appears to be working correctly. I am now seeing a few errors while on full debugging but to be honest, do not think this is due to your code as it does not reference any added lines but was wondering if it was worth trying to fix..not a good enough coder to know how much of an issue this is..looks like it might be a simple fix and didn't know if you would be willing to look at it and resolve..?

Coding problem: $PAGE->context was not set. You may have forgotten to call require_login() or $PAGE->set_context(). The page may not display correctly as a result line 567 of /lib/pagelib.php: call to debugging() line 962 of /lib/pagelib.php: call to moodle_page->magic_get_context() line 1474 of /lib/weblib.php: call to moodle_page->get() line 85 of /local/autogroup/classes/sort_module/user_info_field.php: call to format_string() line 65 of /local/autogroup/classes/sort_module/user_info_field.php: call to local_autogroup\sort_module\user_info_field->get_config_options() line 49 of /local/autogroup/classes/sort_module/user_info_field.php: call to local_autogroup\sort_module\user_info_field->config_is_valid() line 172 of /local/autogroup/classes/domain/autogroup_set.php: call to local_autogroup\sort_module\user_info_field->__construct() line 115 of /local/autogroup/classes/domain/autogroup_set.php: call to local_autogroup\domain\autogroup_set->initialise() line 62 of /local/autogroup/manage.php: call to local_autogroup\domain\autogroup_set->construct()

TomoTsuyuki commented 1 year ago

Hi @emmarichardson ,

I tested with Moodle 3.9, 4,1, and master branch, but I couldn't see the warning in my local with Developer level debugging message. Which version of Moodle are you using? Which page did you see the message? If it's seen from master branch as well, then it's not from this patch.