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

New Setting: Preserve manual User assignments. #10

Merged

Conversation

gnat
Copy link

@gnat gnat commented Nov 28, 2018

When Users are manually assigned to a group managed by autogroup, preserve these assignments when autogroups are automatically updated (other Users added, removed).

@gnat
Copy link
Author

gnat commented Nov 28, 2018

@emmarichardson If this looks good to you, feel free to merge it in. We release this code as public domain.

Copy link
Collaborator

@ak4t0sh ak4t0sh left a comment

Choose a reason for hiding this comment

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

Thanks for sharing it seems a really good feature !
I'll try to review and test it properly soon but at first sight I think we should not merge in master as it is because :

  • composer has nothing to do with this feature. composer.json should be removed.
  • in version.php "release" should not be changed in this PR. This changes should be done by a maintainer. (As this PR add a new function the release number should be bumped to 2.5)

Note for maintainer : Use "Squash and merge" to merge this PR.

@gnat
Copy link
Author

gnat commented Nov 29, 2018

Thank you for your review @ak4t0sh I've made the changes.
This should now be ready for you. @emmarichardson

@emmarichardson
Copy link
Owner

So I am confused - if you turn off the groups setting, doesn't this accomplish the same thing?

@senaiboy
Copy link

senaiboy commented Nov 30, 2018

So I am confused - if you turn off the groups setting, doesn't this accomplish the same thing?

If I understand it right, this might solve our problem discussed a while ago with users belonging to more than one group. Eg Groups can have 'permanent' members (teachers) and temporary members (students). As students change group, Autogroup plugin will move them into their new group accordingly, while the teachers remain in these groups.

Turning off auto-update setting (presumably this is what you're referring to?) means students will remain in their old group as well as in the new groups. The way to overcome this issue is by deleting all groups every new term/year.

Turning on auto-update (currently) only allow teachers to be in 1 group.

With this new feature, we could have both the convenience of auto-updating members of a group as well as assigning permanent members to it.

@gnat
Copy link
Author

gnat commented Nov 30, 2018

With this new feature, we could have both the convenience of auto-updating members of a group as well as assigning permanent members to it.

@senaiboy yup, you've got it. This use case is the exact reason this was developed.

It's intuitive because Users who have been manually assigned, remain manual. Users automatically assigned, remain automatic.

@emmarichardson
Copy link
Owner

emmarichardson commented Nov 30, 2018 via email

@emmarichardson
Copy link
Owner

@ak4t0sh Could you review the changes and let me know if it looks good? Then I will merge.

@ak4t0sh
Copy link
Collaborator

ak4t0sh commented Dec 5, 2018

(Technically) Review OK 👍 Thanks for the changes @gnat

Test OK it works as expected.

But I think that it can produce inconstencies.
Example :

  1. Create a course
  2. Create an autogroup based on "Preferred Language"
  3. Enrol 2 users with different languages. (English and French)
  4. 2 groups are created : "En" and "Fr" with one user in each group.
  5. Manually add user with "French" language into the "En" group.

Result : The user is in the 2 groups.
This could lead to weird behaviour if the course use group based restrictions.

I think that an user should not be added more than 1 group from the same group set.
Example :

  1. Create a course
  2. Create an autogroup based on "Preferred Language"
  3. Enrol 2 users with different languages. (English and French)
  4. 2 groups are created : "En" and "Fr" with one user in each group.
  5. Manually add user with "French" language into the "En" group.
    Expected : The user is remove from group "Fr" and languages changes in profile are ignored.

@emmarichardson
Copy link
Owner

I think that the way it works is the intended use case. From what I understand, it is so that they can add teachers to multiple groups, for example. Seeing as it is an optional setting, I see no issue with it as long as we have no concerns about database/code confusion. For example, does the setting automatically uncheck the groups setting that looks for changes in the group and updates if need be...I haven't had time to test it fully.

@ak4t0sh
Copy link
Collaborator

ak4t0sh commented Dec 6, 2018

@emmarichardson if you are OK with this behaviour you can merge :)

@emmarichardson emmarichardson merged commit 4993414 into emmarichardson:master Jan 31, 2019
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.

4 participants