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

request: process comma delimited text profile field #42

Closed MURBASLMS closed 1 year ago

MURBASLMS commented 1 year ago

Hi, i'm not sure if we need to try to populate a different or custom profile field but if using defaults, we'd aim to populate multiple values to a text field. Use case is course and major codes which could often be multiple values eg "B1330,B1335".

i'd really like to be able to use auto group to group based on these values, so i'd end up with B1330 group and B1335 group.

I'm not sure if this would require an enhancement to auto group or if we need to get something custom built.

Thanks for any help you can offer - cheers

emmarichardson commented 1 year ago

Yes, actually I love the idea. If you have someone to code that and want to put in a pull request that would be awesome.

MURBASLMS commented 1 year ago

nice one, i'll see what we can cook up...

dmitriim commented 1 year ago

Hi @emmarichardson

How do you see this feature implemented from the design point of view?

I'm thinking about a new sort module something like user_info_field_multivalue or something. So in eligible_groups_for_user method we can parse a field value string using a delimiter and return a result as a list.

However, I don't like an idea hardcoding a delimiter to a comma. We can add a new setting that could be hidden until required sort module is selected. So the setting can be updated. We often use that way in our plugins.

We can pre define a list of delimiters to , and | or simply leave it open so an admin can set one (will be a comma by default)

Please let me know if you are ok with that approach.

MURBASLMS commented 1 year ago

a sensible delimiter choice is a good idea.

example of a student with two real course codes:

"B1368 BACHELOR OF EDUCATION (SECONDARY TEACHING),N1096 NGOOLARK: BA, BED, BBUS AND BNURS EXTENSION PROGRAM"

so we'd have to use a "|" and would expect in this scenario for auto group to create two groups:

"B1368 BACHELOR OF EDUCATION (SECONDARY TEACHING)" "N1096 NGOOLARK: BA, BED, BBUS AND BNURS EXTENSION PROGRAM"

if they didn't exist, and assign that student to both.

However, for our purposes we are leaning towards just using the course code for simplicity's sake. Generally this will be good enough, but this could change - i can see having the full title would be a good option.

emmarichardson commented 1 year ago

Yes, I like the idea of , and | - I can’t see where one of those would not cover the vast majority of cases.

Emma Richardson

On Mar 30, 2023 at 6:52 PM -0600, MURBASLMS @.***>, wrote:

a sensible delimiter choice is a good idea. example of a student with two real course codes: "B1368 BACHELOR OF EDUCATION (SECONDARY TEACHING),N1096 NGOOLARK: BA, BED, BBUS AND BNURS EXTENSION PROGRAM" so we'd have to use a "|" and would expect in this scenario for auto group to create two groups: "B1368 BACHELOR OF EDUCATION (SECONDARY TEACHING)" "N1096 NGOOLARK: BA, BED, BBUS AND BNURS EXTENSION PROGRAM" if they didn't exist, and assign that student to both. However, for our purposes we are leaning towards just using the course code for simplicity's sake. Generally this will be good enough, but this could change - i can see having the full title would be a good option. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

TomoTsuyuki commented 1 year ago

Hi,

I made PR for this enhancement.

Please review and test, and let me know if you have any comments.

Tomo

dmitriim commented 1 year ago

hi @emmarichardson! Any chance you can look at proposed PR? Please let me know if you need any help with reviewing. Thanks!

emmarichardson commented 1 year ago

Sorry for the delay in this - I am currently trying to find time to deal with the 4.2 required sql and php upgrades...I want to get that done before merging this and others so that we know it works with the current release...

emmarichardson commented 1 year ago

Ok, got further - testing now - so far so good...