Moodle
  1. Moodle
  2. MDL-31973

groups_members table should have 'component' field

    Details

    • Testing Instructions:
      Hide

      Test level DIFFICULT (requires use of manual database queries and edits to code)

      1. After upgrade to new version, look at the groups_members table using a database viewer.
      + The table should have fields 'component' and 'itemid'.
      + These fields should be, respectively, '' and zero for all entries.

      2. Go to any course that has at least 2 users in (or set up a new one).
      3. Create a new group 'Group A' for testing. Go to 'Add/remove users' for the group.
      4. Select both users and add them to the group.
      + The add should work OK
      + Check in the database - the new groups_members entries should have '' (blank field) and 0.

      5. In the database, manually edit one of the two entries to set the component name to 'mod_page' (the Page module cannot really add group members; this is just an example for testing).

      6. Return to the group page.
      + Under the entry you set up, there should be grey (disabled), indented text 'Added by Page'. (If using Firefox, this text will also be smaller than usual. On other browsers, CSS doesn't work on list items.)

      7. Under the search box, type in the name the user who was not set to mod_page.
      + Verify that only that user displays; the 'Added by Page' text which belongs to the other user has disappared.

      8. Change the search to type in the user who was set to mod_page.
      + Only that user should appear; this should include the 'Added by Page' text below.

      9. Clear the search and select both entries (note that you cannot select the 'Added automatically' lines because these are disabled).
      10. Click 'Remove'.
      + The remove should work OK with both entries removed.

      11. Add both users again and manually edit one of them to have component 'mod_page' again.
      12. Return to the group page and verify that the entry shows up as added by Page again.
      13. Modify the following function in mod/page/lib.php:

      function mod_page_allow_group_member_remove($itemid, $groupid, $userid) {
          return false;  // change this line from true to false.
      }
      

      14. Attempt to remove both users.
      + You should now receive an error 'You do not have permission to remove automatically-added group member Anne Other' (with the name of the group member that was owned by Page).
      + Depending on the order, the other user may or may not have been removed.

      15. Return to the group members page. If the other user got removed, add them back so that you again have two users, one of whom is owned by Page.
      16. Edit the code above to change the 'false' return to true.
      17. Attempt to remove both users.
      + Removing both users should now be OK.

      18. Add both users back and set one in database to have mod_page as component, itemid as 13.
      19. Reload group page and check the 'Added by Page' appears again.
      20. Back up course using default settings.
      21. Restore to new course using default settings.
      22. View groups page and show members for the group.
      + The 'Added by Page' should still be there.
      23. Alter the new course to have a component of 'mod_foo'
      24. Back up the new course using default settings.
      25. Restore to a new course using default settings
      22. View groups page and show members for the group.
      + The 'Added by xxx' should no longer be shown.

      Show
      Test level DIFFICULT (requires use of manual database queries and edits to code) 1. After upgrade to new version, look at the groups_members table using a database viewer. + The table should have fields 'component' and 'itemid'. + These fields should be, respectively, '' and zero for all entries. 2. Go to any course that has at least 2 users in (or set up a new one). 3. Create a new group 'Group A' for testing. Go to 'Add/remove users' for the group. 4. Select both users and add them to the group. + The add should work OK + Check in the database - the new groups_members entries should have '' (blank field) and 0. 5. In the database, manually edit one of the two entries to set the component name to 'mod_page' (the Page module cannot really add group members; this is just an example for testing). 6. Return to the group page. + Under the entry you set up, there should be grey (disabled), indented text 'Added by Page'. (If using Firefox, this text will also be smaller than usual. On other browsers, CSS doesn't work on list items.) 7. Under the search box, type in the name the user who was not set to mod_page. + Verify that only that user displays; the 'Added by Page' text which belongs to the other user has disappared. 8. Change the search to type in the user who was set to mod_page. + Only that user should appear; this should include the 'Added by Page' text below. 9. Clear the search and select both entries (note that you cannot select the 'Added automatically' lines because these are disabled). 10. Click 'Remove'. + The remove should work OK with both entries removed. 11. Add both users again and manually edit one of them to have component 'mod_page' again. 12. Return to the group page and verify that the entry shows up as added by Page again. 13. Modify the following function in mod/page/lib.php: function mod_page_allow_group_member_remove($itemid, $groupid, $userid) { return false ; // change this line from true to false . } 14. Attempt to remove both users. + You should now receive an error 'You do not have permission to remove automatically-added group member Anne Other' (with the name of the group member that was owned by Page). + Depending on the order, the other user may or may not have been removed. 15. Return to the group members page. If the other user got removed, add them back so that you again have two users, one of whom is owned by Page. 16. Edit the code above to change the 'false' return to true. 17. Attempt to remove both users. + Removing both users should now be OK. 18. Add both users back and set one in database to have mod_page as component, itemid as 13. 19. Reload group page and check the 'Added by Page' appears again. 20. Back up course using default settings. 21. Restore to new course using default settings. 22. View groups page and show members for the group. + The 'Added by Page' should still be there. 23. Alter the new course to have a component of 'mod_foo' 24. Back up the new course using default settings. 25. Restore to a new course using default settings 22. View groups page and show members for the group. + The 'Added by xxx' should no longer be shown.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31973-master-6
    • Rank:
      38641

      Description

      The role_assignments table has a 'component' field which is used to record which internal component created the assignment (it is blank if the assignment was added manually using the UI).

      Unfortunately there is no 'component' field in the groups_members table.

      We have a dataload system which automatically sets up tutor groups (and several other types of group) and it would make life significantly easier for this system if we could set a 'component' field in groups_members so that it can tell which rows it created. (I.e. there are cases where it needs to delete entries, but only if the system created them, otherwise it wipes manual changes.)

      My proposal is to add the 'component' field to groups_members (with same semantics as in role_assignments, blank = manually added/normal).

      I am not sure there is anywhere in Moodle that automatically creates group_members entries, but I'll search; if there is, I can make this fill in the component field. Otherwise, this feature would be solely for the use of plugins, so apart from the database change and maybe some changes to one or two groups_* functions (to add the new field, where there are lists of fields) it shouldn't change much.

      Note: I am proposing to implement this myself.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          If it is agreed to add the 'component' that we should also add 'itemid' column - see enrolment plugins where itemid is instance id.

          my 1 for 'component''itemid'

          Show
          Petr Škoda added a comment - If it is agreed to add the 'component' that we should also add 'itemid' column - see enrolment plugins where itemid is instance id. my 1 for 'component' 'itemid'
          Hide
          Sam Marshall added a comment -

          Updating test script to reflect current development; added screenshot; will comment further and provide code soon.

          Show
          Sam Marshall added a comment - Updating test script to reflect current development; added screenshot; will comment further and provide code soon.
          Hide
          Sam Marshall added a comment - - edited

          I've finished this and will submit for review. It was way more work than I thought it would be...

          Here's a complete list of changes and rationale:

          1. Changed database fields as described (including Petr's advice to add itemid). Added parameters to groups_add_member so these can be set. Added fields to backup.

          2. Added logic in new groups API function groups_remove_member_allowed. If the new component field is set, this will call a component callback function _allow_group_member_remove with parameters itemid, groupid, and userid. The function could return true or false e.g. based on a capability check, issiteadmin, or something specific about that group/user. If the function does not exist, it is assumed to always return true (=ok).

          Rationale: It is the component that added the group member item which knows whether users should be permitted to manually remove the entry. Some components might be happy for users to remove the entries, others (e.g. the type which will automatically add it again overnight anyway, which is what our dataload does) might not.

          3. Made it check this function when deleting users manually in the main group editing API and in the equivalent web service.
          Rationale: I assume the webservice is basically equivalent to the normal UI and is not supposed to be a lower-level interface, so it ought to do this check just the same.

          4. It does not call this function e.g. if you delete a whole group, or a course.
          Rationale: Deleting whole groups is probably separate; if there is a need to control this it would be done by setting the group idnumber (see linked issue). If the group or course is deleted there is obviously no point trying to stop people delete a group member.

          5. When users were added by a component, this is indicated in the group members list (on add/remove users page only) by an entry below their name with text like 'Added by ' [pluginname of component that added them].
          Rationale: This is useful information, especially when troubleshooting. And we need to have some way for users to be able to tell why the system won't let them remove someone.

          6. This display was implemented by adding a new 'infobelow' feature to userselector which allows for a line of plain text information to display below the user's name (in small type if using Firefox, otherwise just indented and grey/disabled). I made it work in the JavaScript search mode as well as on initial display. (Changes are not that scary and are tested in IE 9, Firefox latest, Chrome latest.)

          7. After searching the code, the only place I found that looked like it really made sense to use the 'component' value in core code when calling groups_add_member was the IMSenterprise enrol plugin, so I added this. Haven't actually tested this part but I think the code is safe (even if it's wrong, it can't do more than print a php warning, I would think). We could remove it if preferred. I didn't add the function, so it won't cause a behaviour change when deleting these users.

          If this is approved, I propose documenting the new optional lib.php function in here:

          http://docs.moodle.org/dev/lib.php

          You might wish to remind me that I promised that, if I look like I haven't done so. I will note in here if/when I do change the documentation.

          Show
          Sam Marshall added a comment - - edited I've finished this and will submit for review. It was way more work than I thought it would be... Here's a complete list of changes and rationale: 1. Changed database fields as described (including Petr's advice to add itemid). Added parameters to groups_add_member so these can be set. Added fields to backup. 2. Added logic in new groups API function groups_remove_member_allowed. If the new component field is set, this will call a component callback function _allow_group_member_remove with parameters itemid, groupid, and userid. The function could return true or false e.g. based on a capability check, issiteadmin, or something specific about that group/user. If the function does not exist, it is assumed to always return true (=ok). Rationale: It is the component that added the group member item which knows whether users should be permitted to manually remove the entry. Some components might be happy for users to remove the entries, others (e.g. the type which will automatically add it again overnight anyway, which is what our dataload does) might not. 3. Made it check this function when deleting users manually in the main group editing API and in the equivalent web service. Rationale: I assume the webservice is basically equivalent to the normal UI and is not supposed to be a lower-level interface, so it ought to do this check just the same. 4. It does not call this function e.g. if you delete a whole group, or a course. Rationale: Deleting whole groups is probably separate; if there is a need to control this it would be done by setting the group idnumber (see linked issue). If the group or course is deleted there is obviously no point trying to stop people delete a group member. 5. When users were added by a component, this is indicated in the group members list (on add/remove users page only) by an entry below their name with text like 'Added by ' [pluginname of component that added them] . Rationale: This is useful information, especially when troubleshooting. And we need to have some way for users to be able to tell why the system won't let them remove someone. 6. This display was implemented by adding a new 'infobelow' feature to userselector which allows for a line of plain text information to display below the user's name (in small type if using Firefox, otherwise just indented and grey/disabled). I made it work in the JavaScript search mode as well as on initial display. (Changes are not that scary and are tested in IE 9, Firefox latest, Chrome latest.) 7. After searching the code, the only place I found that looked like it really made sense to use the 'component' value in core code when calling groups_add_member was the IMSenterprise enrol plugin, so I added this. Haven't actually tested this part but I think the code is safe (even if it's wrong, it can't do more than print a php warning, I would think). We could remove it if preferred. I didn't add the function, so it won't cause a behaviour change when deleting these users. If this is approved, I propose documenting the new optional lib.php function in here: http://docs.moodle.org/dev/lib.php You might wish to remind me that I promised that, if I look like I haven't done so. I will note in here if/when I do change the documentation.
          Hide
          Sam Marshall added a comment -

          Realised I had forgotten backup/restore... added now.

          Show
          Sam Marshall added a comment - Realised I had forgotten backup/restore... added now.
          Hide
          Dan Poltawski added a comment -

          Hi Sam,

          Thanks a lot for doing this work! Some notes from my review:

          1. Is it right for us to be blindly restoring the group_member component details in restore? What if the component which created the group doesn't exist on the destination site? From the look of things the teacher will at least see missing strings on the group members selector which is less than ideal I think.
          2. On first read of the code I thought that you were adding a component for IMS enrolment and then not allowing it to be removed as the callback for checking removal permission didn't exist. That was until I read your comment here and saw that component_callback had a default true param. I think it'd be better to make that behaviour a little clearer in the comment as I didn't immediately spot it.
          3. Since you touched webservices it made me think of web services.. I can imagine that web services would want to use this component field in order to prevent teachers from manually removing members automatically populated by web-services from external systems (think student management systems). This is something to ponder I think.

          An immediate use case we have for this component field is populating groups from cohorts and keeping them in sync.

          cheers,
          dan

          Show
          Dan Poltawski added a comment - Hi Sam, Thanks a lot for doing this work! Some notes from my review: Is it right for us to be blindly restoring the group_member component details in restore? What if the component which created the group doesn't exist on the destination site? From the look of things the teacher will at least see missing strings on the group members selector which is less than ideal I think. On first read of the code I thought that you were adding a component for IMS enrolment and then not allowing it to be removed as the callback for checking removal permission didn't exist. That was until I read your comment here and saw that component_callback had a default true param. I think it'd be better to make that behaviour a little clearer in the comment as I didn't immediately spot it. Since you touched webservices it made me think of web services.. I can imagine that web services would want to use this component field in order to prevent teachers from manually removing members automatically populated by web-services from external systems (think student management systems). This is something to ponder I think. An immediate use case we have for this component field is populating groups from cohorts and keeping them in sync. cheers, dan
          Hide
          Sam Marshall added a comment -

          Thanks Dan!

          1) Agree, good point. I have changed restore and tested. (backup/moodle2/restore_stepslib.php)

          2) Sure, I added comments (a) in IMS Enterprise, (b) group/lib.php.

          3) I also thought of this and actually at one point I started to change the webservices to allow setting this value - but actually it is a difficult problem because if you do set the value, what do you set it to? (I.e. it has to be a Moodle component that 'owns' the entries. I guess it could be like, core_webservice but not sure how useful that is.) So I decided that, at least in this version, the webservices are treated the same as the public UI and therefore you can't set a component and also it won't delete an entry that has a component (if the component doesn't allow it). Makes sense particularly with the 'phone app' type intended use for webservices where it's just another way to get at stuff you can do in the Moodle UI.

          Have pushed the new version.

          Show
          Sam Marshall added a comment - Thanks Dan! 1) Agree, good point. I have changed restore and tested. (backup/moodle2/restore_stepslib.php) 2) Sure, I added comments (a) in IMS Enterprise, (b) group/lib.php. 3) I also thought of this and actually at one point I started to change the webservices to allow setting this value - but actually it is a difficult problem because if you do set the value, what do you set it to? (I.e. it has to be a Moodle component that 'owns' the entries. I guess it could be like, core_webservice but not sure how useful that is.) So I decided that, at least in this version, the webservices are treated the same as the public UI and therefore you can't set a component and also it won't delete an entry that has a component (if the component doesn't allow it). Makes sense particularly with the 'phone app' type intended use for webservices where it's just another way to get at stuff you can do in the Moodle UI. Have pushed the new version.
          Hide
          Sam Marshall added a comment -

          Requesting review again - just to confirm you're happy with these changes. If so, will submit for integration.

          Show
          Sam Marshall added a comment - Requesting review again - just to confirm you're happy with these changes. If so, will submit for integration.
          Hide
          Dan Poltawski added a comment -

          Thanks Sam,

          All looks good to me - please submit for integration!

          Show
          Dan Poltawski added a comment - Thanks Sam, All looks good to me - please submit for integration!
          Hide
          Sam Marshall added a comment -

          thanks Dan submitting

          Show
          Sam Marshall added a comment - thanks Dan submitting
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm...

          does this cover all the groups_add_member | add_user_to_group uses across codebase?

          What happens with members loaded from uploaduser, or webservice, or enrol/self... In some of those cases the itemid is not necessary (but the component is, I guess). Also, it's the case of autogroups... not sure which component it belongs to... perhaps it should be moved to admin tool or any other valid component....

          Perhaps, it would be also a good idea to make the new parameters mandatory (at least the component), no matter if we end passing nulls/blanks for some cases for now?

          Keeping this open for some hours... but all those ways of adding members should be covered by this IMO.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm... does this cover all the groups_add_member | add_user_to_group uses across codebase? What happens with members loaded from uploaduser, or webservice, or enrol/self... In some of those cases the itemid is not necessary (but the component is, I guess). Also, it's the case of autogroups... not sure which component it belongs to... perhaps it should be moved to admin tool or any other valid component.... Perhaps, it would be also a good idea to make the new parameters mandatory (at least the component), no matter if we end passing nulls/blanks for some cases for now? Keeping this open for some hours... but all those ways of adding members should be covered by this IMO. Ciao
          Hide
          Sam Marshall added a comment -

          I searched all usages of groups_add_member and decided only to add component to the enrol plugin shown (to be honest not sure if I should really have done that).

          My intention was that for all 'normal' methods that are equivalent to doing it in the interface, the componentid should be blank.

          This definitely includes uploaduser (users would not expect anything to be treated differently because they added them in a .csv file instead of in the UI). As discussed in previous comment, it also includes web service because there is no useful way to treat this differently, and initial use of webservices has concentrated on using mobile app to do things that are the equivalent of the normal UI.

          (To put it another way... if normal usage puts things in the component field it will be annoying, because I made it display it on the groups_members screen, and we don't really want it to do that for everything...)

          Regarding autogroups, imo this is the same way (should be equivalent to manually adding) *IF* autogroups is a one-off operation, which I think it is at present. If autogroups needs to 'keep track of' the groups created somehow (e.g. so you can redo autogroups without affecting any manually added users) then it should use this feature in future.

          Basically... I think this is an infrastructure thing which CAN be used if necessary to improve some other core features. It's ok if it is not actually used (by core or other features) initially as that will continue current behaviour. But in cases where different behaviour is required (i.e. it is necessary/useful to be able to tell which entries were created by a specific system component vs. by the user) this could now be added in future, once this feature is available. I would envision this mainly being used by ours (and other people's) systems to load user data from external sources.

          I didn't want to make the new parameters mandatory because then it would break all plugins which call groups_add_member. I think it's fine not to set the parameter if you want to continue with current behaviour...

          Show
          Sam Marshall added a comment - I searched all usages of groups_add_member and decided only to add component to the enrol plugin shown (to be honest not sure if I should really have done that). My intention was that for all 'normal' methods that are equivalent to doing it in the interface, the componentid should be blank. This definitely includes uploaduser (users would not expect anything to be treated differently because they added them in a .csv file instead of in the UI). As discussed in previous comment, it also includes web service because there is no useful way to treat this differently, and initial use of webservices has concentrated on using mobile app to do things that are the equivalent of the normal UI. (To put it another way... if normal usage puts things in the component field it will be annoying, because I made it display it on the groups_members screen, and we don't really want it to do that for everything...) Regarding autogroups, imo this is the same way (should be equivalent to manually adding) * IF * autogroups is a one-off operation, which I think it is at present. If autogroups needs to 'keep track of' the groups created somehow (e.g. so you can redo autogroups without affecting any manually added users) then it should use this feature in future. Basically... I think this is an infrastructure thing which CAN be used if necessary to improve some other core features. It's ok if it is not actually used (by core or other features) initially as that will continue current behaviour. But in cases where different behaviour is required (i.e. it is necessary/useful to be able to tell which entries were created by a specific system component vs. by the user) this could now be added in future, once this feature is available. I would envision this mainly being used by ours (and other people's) systems to load user data from external sources. I didn't want to make the new parameters mandatory because then it would break all plugins which call groups_add_member. I think it's fine not to set the parameter if you want to continue with current behaviour...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Sam,

          I see your logic... but in the other side is 100% nosense to add one component+itemid couple to group members if we are not going to (be allowed to) use it.

          My reasoning is about those terms being "reserved" for something existing into Moodle, and with a way to behave in other places.

          So, if we go ahead with the component+itemid, I think we must go in the correct way, aka, when one component (enrol plugin, auth plugin, tool or whatever) assign members, use them.

          In the other side... perhaps, after all, you don't really need one component+itemid for this at all but just some sort of "externalsysteminfo/extkey/idnumber/whatever" column to be used at your installation.

          I'm going to keep this open to further discuss it @ HQ (plz ignite it if I'm not around), IMO there are 4 options:

          0) Your component + itemid idea is accepted. We assume no use from core.
          1) We go with component + itemid. But using them properly, also for core components handling groups members.
          2) We go with one custom container that won't be used by core, but will be allowed for external systems to introduce any arbitrary information.
          3) We don't introduce anything, and you keep that working exclusively in your site (new column or 1:1 table).

          Obviously my +1 goes for anything but 0) as commented. IMO it's an incorrect implementation of component+itemid.

          So, reopening until discussion/agreement happens...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Sam, I see your logic... but in the other side is 100% nosense to add one component+itemid couple to group members if we are not going to (be allowed to) use it. My reasoning is about those terms being "reserved" for something existing into Moodle, and with a way to behave in other places. So, if we go ahead with the component+itemid, I think we must go in the correct way, aka, when one component (enrol plugin, auth plugin, tool or whatever) assign members, use them. In the other side... perhaps, after all, you don't really need one component+itemid for this at all but just some sort of "externalsysteminfo/extkey/idnumber/whatever" column to be used at your installation. I'm going to keep this open to further discuss it @ HQ (plz ignite it if I'm not around), IMO there are 4 options: 0) Your component + itemid idea is accepted. We assume no use from core. 1) We go with component + itemid. But using them properly, also for core components handling groups members. 2) We go with one custom container that won't be used by core, but will be allowed for external systems to introduce any arbitrary information. 3) We don't introduce anything, and you keep that working exclusively in your site (new column or 1:1 table). Obviously my +1 goes for anything but 0) as commented. IMO it's an incorrect implementation of component+itemid. So, reopening until discussion/agreement happens...ciao
          Hide
          Dan Poltawski added a comment -

          I'm not sure that I quite understand that not always using component + itemid is problematic.

          But just to comment that I don't think that options 2 or 3 are viable, there is a 'missing feature' of cohorts that they can't currently populate groups and keep them in sync (MDL-31437). I don't think we can do that without this sort of field (see Petr's comments on MDL-22927) being introduced.

          Show
          Dan Poltawski added a comment - I'm not sure that I quite understand that not always using component + itemid is problematic. But just to comment that I don't think that options 2 or 3 are viable, there is a 'missing feature' of cohorts that they can't currently populate groups and keep them in sync ( MDL-31437 ). I don't think we can do that without this sort of field (see Petr's comments on MDL-22927 ) being introduced.
          Hide
          Tim Hunt added a comment -

          I think the issue with 1) is that sam implemented a system where it displayed extra information about group memberships where component is not blank, and so if that information was present for all group members, then the UI would get really crowded and be a pain to use.

          A suppose the solution is to use call-backs to get things like the text to display under each member, and whether this group member can be removed through the manual groups UI (and have it understood that it is OK to return no explanatory text if this user can be removed manually).

          Show
          Tim Hunt added a comment - I think the issue with 1) is that sam implemented a system where it displayed extra information about group memberships where component is not blank, and so if that information was present for all group members, then the UI would get really crowded and be a pain to use. A suppose the solution is to use call-backs to get things like the text to display under each member, and whether this group member can be removed through the manual groups UI (and have it understood that it is OK to return no explanatory text if this user can be removed manually).
          Hide
          Sam Marshall added a comment - - edited

          OK, agree with Tim - basically this is Eloy's option 1 but with custom display. So for example:

          • If you add an entry using the normal user interface then it will get component 'core_group' (no itemid)
          • If you add an entry using the web service then it will get component 'core_webservice' (no itemid)

          Similar for csv upload and other areas. Mostly these will not have itemid, it's only enrol plugins and similar that need to track an itemid.

          Existing 'legacy' entries will all continue to have blank component as we don't know how they were populated.

          For groups_add_member function etc I will leave the new parameters as optional - for API backward compatibility - but will make it display a developer debug warning if you omit componentid (obviously itemid will keep being really-optional).

          Then new optional api e.g. mod_page_display_group_member_origin($itemid, $groupid, $userid) which can return blank - or not include the function - if no extra info is required, otherwise return plain text string.

          This function will not be implemented initially for any part of core code so all existing behaviour (no extra display) will be maintained.

          It might be that the IMS enrol plugin would actually benefit from using this feature (i.e. preventing delete and/or displaying information), also other new core code like cohorts might want to use it, but in this case it would be added later by the people implementing those features. In this commit, I would only make sure that all parts of core code do actually set the component value when adding group members.

          How's that?

          Show
          Sam Marshall added a comment - - edited OK, agree with Tim - basically this is Eloy's option 1 but with custom display. So for example: If you add an entry using the normal user interface then it will get component 'core_group' (no itemid) If you add an entry using the web service then it will get component 'core_webservice' (no itemid) Similar for csv upload and other areas. Mostly these will not have itemid, it's only enrol plugins and similar that need to track an itemid. Existing 'legacy' entries will all continue to have blank component as we don't know how they were populated. For groups_add_member function etc I will leave the new parameters as optional - for API backward compatibility - but will make it display a developer debug warning if you omit componentid (obviously itemid will keep being really-optional). Then new optional api e.g. mod_page_display_group_member_origin($itemid, $groupid, $userid) which can return blank - or not include the function - if no extra info is required, otherwise return plain text string. This function will not be implemented initially for any part of core code so all existing behaviour (no extra display) will be maintained. It might be that the IMS enrol plugin would actually benefit from using this feature (i.e. preventing delete and/or displaying information), also other new core code like cohorts might want to use it, but in this case it would be added later by the people implementing those features. In this commit, I would only make sure that all parts of core code do actually set the component value when adding group members. How's that?
          Hide
          Petr Škoda added a comment - - edited

          Hi,

          1/ 'core_webservice' does not make much sense from the original externallib&WS design perspective, the webservice was supposed to be only the top most wrapper around PHP external API - there was not supposed to be a single word 'webservice' outside of the /webservice/ directory or WS admin UI - that means ALWAY use the component of the plugin (or subsystem) manipulating groups (where is the externallib.php defined). Unfortunately these fundamental external API/WS rules are being constantly ignored...

          2/ all current groups belong to 'core', nothing else can claim exclusive ownership of existing groups or members now - I believe the upgrade should unconditionally add default component and later any custom plugin may take over the group in own upgrade.php if really necessary

          3/ hmmm, I am not sure about 'core_group' component because groups actually belonged to teachers that may create/modify/delete/assign them, they are not owned by 'core' - the empty string '' used in other similar areas means "anybody/anything can modify this at any time"

          Show
          Petr Škoda added a comment - - edited Hi, 1/ 'core_webservice' does not make much sense from the original externallib&WS design perspective, the webservice was supposed to be only the top most wrapper around PHP external API - there was not supposed to be a single word 'webservice' outside of the /webservice/ directory or WS admin UI - that means ALWAY use the component of the plugin (or subsystem) manipulating groups (where is the externallib.php defined). Unfortunately these fundamental external API/WS rules are being constantly ignored... 2/ all current groups belong to 'core', nothing else can claim exclusive ownership of existing groups or members now - I believe the upgrade should unconditionally add default component and later any custom plugin may take over the group in own upgrade.php if really necessary 3/ hmmm, I am not sure about 'core_group' component because groups actually belonged to teachers that may create/modify/delete/assign them, they are not owned by 'core' - the empty string '' used in other similar areas means "anybody/anything can modify this at any time"
          Hide
          Petr Škoda added a comment -

          oh, one more thing: what happens when two different plugins want to add user membership - do we allow duplicates in group_members? I guess current code is not compatible with that.

          Show
          Petr Škoda added a comment - oh, one more thing: what happens when two different plugins want to add user membership - do we allow duplicates in group_members? I guess current code is not compatible with that.
          Hide
          Sam Marshall added a comment -

          Petr:

          1/ Sorry this is my mistake, it should be core_group, you are correct it is defined in that externallib. What I intended was to say that whenever there is a call to groups_add_member, I can add in the component of the area of code that is making that call (which is what Eloy requested).

          2/ I don't think 'core' is a valid component, doesn't it need to be listed in get_core_subsystems? So could use core_group for that too if you would like an initial setting other than blank.

          3/ This was my initial reasoning for leaving the component blank when groups are created by the user interface (including the normal 'add members' UI, also the 'upload csv' UI, the 'automatically create N groups' UI, and the web service interface which might be used by a mobile UI). But Eloy didn't like that approach. I'm happy with either way (i.e. Eloy = we set the component to core_group or whatever suitable, but we know that core_group is never going to prevent anyone modifying the group; my original = we leave it blank in those cases), can somebody please decide.

          Regarding duplicates, this is not allowed - there's no check in database but the PHP code stops it (groups_add_member). The code I've done does not change that. Can we leave that for a future issue and have it defined that, at present, a group membership can only be owned by a single component? I think there are lots of places (including third-party) that will join with groups_members expecting it to be unique, so that change would potentially affect other code.

          OR, are you suggesting that in order to create this feature we should instead add a new table about which component owns each group membership (with 1:n possible and would not affect any existing code) rather than adding fields to groups_members...?

          Show
          Sam Marshall added a comment - Petr: 1/ Sorry this is my mistake, it should be core_group, you are correct it is defined in that externallib. What I intended was to say that whenever there is a call to groups_add_member, I can add in the component of the area of code that is making that call (which is what Eloy requested). 2/ I don't think 'core' is a valid component, doesn't it need to be listed in get_core_subsystems? So could use core_group for that too if you would like an initial setting other than blank. 3/ This was my initial reasoning for leaving the component blank when groups are created by the user interface (including the normal 'add members' UI, also the 'upload csv' UI, the 'automatically create N groups' UI, and the web service interface which might be used by a mobile UI). But Eloy didn't like that approach. I'm happy with either way (i.e. Eloy = we set the component to core_group or whatever suitable, but we know that core_group is never going to prevent anyone modifying the group; my original = we leave it blank in those cases), can somebody please decide. Regarding duplicates, this is not allowed - there's no check in database but the PHP code stops it (groups_add_member). The code I've done does not change that. Can we leave that for a future issue and have it defined that, at present, a group membership can only be owned by a single component? I think there are lots of places (including third-party) that will join with groups_members expecting it to be unique, so that change would potentially affect other code. OR, are you suggesting that in order to create this feature we should instead add a new table about which component owns each group membership (with 1:n possible and would not affect any existing code) rather than adding fields to groups_members...?
          Hide
          Andrew Nicols added a comment -

          Petr, Eloy: Have you reached a consensus on the implementation of this?

          We'll be needing exactly this functionality shortly and I was hoping to help Sam finish this off.

          My personal preference is for '' rather than 'core_group' - it's not immediately obvious that core_group has a special meaning, whilst it is much more obvious that an empty string means it has no owner and therefore is special.

          Show
          Andrew Nicols added a comment - Petr, Eloy: Have you reached a consensus on the implementation of this? We'll be needing exactly this functionality shortly and I was hoping to help Sam finish this off. My personal preference is for '' rather than 'core_group' - it's not immediately obvious that core_group has a special meaning, whilst it is much more obvious that an empty string means it has no owner and therefore is special.
          Hide
          Andrew Nicols added a comment -

          I've rebased on this weeks master - in doing so, I hit issues with setting the database fields to be not nullable as existing data contains nulls. As a result, I've switched them to NULLable fields.

          As I mentioned before, I'm in Petr's camp on this one - an empty value screams special far more than a core_groups value, and also suggests that it doesn't truly have an owner.

          Show
          Andrew Nicols added a comment - I've rebased on this weeks master - in doing so, I hit issues with setting the database fields to be not nullable as existing data contains nulls. As a result, I've switched them to NULLable fields. As I mentioned before, I'm in Petr's camp on this one - an empty value screams special far more than a core_groups value, and also suggests that it doesn't truly have an owner.
          Hide
          Andrew Nicols added a comment -

          I think we need to make these not nullable, with a default value of '' - this is currently not achievable with the XMLDB editor, but can be entered manually.

          Show
          Andrew Nicols added a comment - I think we need to make these not nullable, with a default value of '' - this is currently not achievable with the XMLDB editor, but can be entered manually.
          Hide
          Andrew Nicols added a comment -

          Discussed briefly in developer chat - general feeling is that NULL fields are fine.

          Rebased on weekly master.

          Show
          Andrew Nicols added a comment - Discussed briefly in developer chat - general feeling is that NULL fields are fine. Rebased on weekly master.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Andrew Nicols added a comment -

          After discussing with Sam, have taken on this issue to try and get it integrated.
          Rebased on weekly master.

          Show
          Andrew Nicols added a comment - After discussing with Sam, have taken on this issue to try and get it integrated. Rebased on weekly master.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          LOL, was near pushing this... but watching the upgrade in my dev site... I realised that the itemid has been declared nullable and default 0. The question is why 2 different possible values meaning "no itemid" ?

          Note I'm not either 100% sold to make component nullable (default null), mainly because all the rest of "component" columns out there are not null (default ''). It's correct (from a philosophical POV), but it makes it a difference from previous components.

          So finally, I'm going to reopen this, asking for both the component and itemid columns to follow the specs used in other tables. It should not change much in the rest of code, as far as empty() is being used.

          Some day, if we move, we'll move all them together (to nullable, default null), but better to have them consistent for now, IMO.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - LOL, was near pushing this... but watching the upgrade in my dev site... I realised that the itemid has been declared nullable and default 0. The question is why 2 different possible values meaning "no itemid" ? Note I'm not either 100% sold to make component nullable (default null), mainly because all the rest of "component" columns out there are not null (default ''). It's correct (from a philosophical POV), but it makes it a difference from previous components. So finally, I'm going to reopen this, asking for both the component and itemid columns to follow the specs used in other tables. It should not change much in the rest of code, as far as empty() is being used. Some day, if we move, we'll move all them together (to nullable, default null), but better to have them consistent for now, IMO. Ciao
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Andrew Nicols added a comment -

          Your wish is my command.

          I've changed the fields to not-null and with defaults of '' and 0.

          Show
          Andrew Nicols added a comment - Your wish is my command. I've changed the fields to not-null and with defaults of '' and 0.
          Hide
          Andrew Nicols added a comment -

          Oh, also rebased on latest integration/master

          Show
          Andrew Nicols added a comment - Oh, also rebased on latest integration/master
          Hide
          Eloy Lafuente (stronk7) added a comment -

          hoho, reopening!

          1) groups_add_member() with $component=null ==> BOOM, I bet.
          2) same function... I think component and itemid should be validated, right now they accept "anything".
          3) same, function... perhaps also, the "groups_member_added" event should include the new information, for potential consumers.
          4) if we want imsenterprise to be an example of use of those new component and itemid... shouldn't we also implement the "allow_group_member_remove" callback there? Yes, I've seen the "Note", but having the callback doesn't hurt IMO.
          5) This requires dev_docs and, if we change the behavior of imsenterprise enrolment (to avoid manual removing) also user_docs, so I'm adding both labels here.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - hoho, reopening! 1) groups_add_member() with $component=null ==> BOOM, I bet. 2) same function... I think component and itemid should be validated, right now they accept "anything". 3) same, function... perhaps also, the "groups_member_added" event should include the new information, for potential consumers. 4) if we want imsenterprise to be an example of use of those new component and itemid... shouldn't we also implement the "allow_group_member_remove" callback there? Yes, I've seen the "Note", but having the callback doesn't hurt IMO. 5) This requires dev_docs and, if we change the behavior of imsenterprise enrolment (to avoid manual removing) also user_docs, so I'm adding both labels here. Ciao
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Andrew Nicols added a comment -

          I'm adding a check to groups_add_member() which checks:

          • whether the $component has a value;
          • if so, it checks whether the component exists

          If $component has a value; and the component exists, then it is used
          If not, then those fields aren't set and we let the database insert defaults ('', and 0 respectively).
          If the component is invalid, then the itemid is not set either.

          We can't validate the itemid as this is specific to each component.

          If the component does not exist, do you think it should:

          • throw an exception;
          • show a debugging warning but create the group member anyway; or
          • create the group member and ignore the error.

          I'm currently silently ignoring the error.

          This addresses points 1, and 2.

          With regard to point 3, I'm trying to decide between:

          • call the events_trigger with $member rather than creating a new stdClass duplicating the same data (the component and itemid will be included only if they're set and valid);
          • run a get_record against the new member's ID and use the record; or
          • just add the fields to the existing stdClass.

          I don't see any reason not to just call with the $member class, though I do wonder whether it would be better to use the DB record instead – this would however add an additional get_record() for no real benefit.

          Agree on point 4 and have now implemented.

          Show
          Andrew Nicols added a comment - I'm adding a check to groups_add_member() which checks: whether the $component has a value; if so, it checks whether the component exists If $component has a value; and the component exists, then it is used If not, then those fields aren't set and we let the database insert defaults ('', and 0 respectively). If the component is invalid, then the itemid is not set either. We can't validate the itemid as this is specific to each component. If the component does not exist, do you think it should: throw an exception; show a debugging warning but create the group member anyway; or create the group member and ignore the error. I'm currently silently ignoring the error. This addresses points 1, and 2. With regard to point 3, I'm trying to decide between: call the events_trigger with $member rather than creating a new stdClass duplicating the same data (the component and itemid will be included only if they're set and valid); run a get_record against the new member's ID and use the record; or just add the fields to the existing stdClass. I don't see any reason not to just call with the $member class, though I do wonder whether it would be better to use the DB record instead – this would however add an additional get_record() for no real benefit. Agree on point 4 and have now implemented.
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          2. I haven't looked at this in any detail at all, but regarding component I think you should replicate what other areas of Moodle do. This component/itemid 'pattern' is common now, so maybe role assignemnts and enrolments could be looked at (note maybe Eloy wants to improve there, so my advice might be bad).

          3. I think just add the fields to the existing stdClass (its less radical, thus easier for us to accept ). One reason we might want to avoid just adding the $member directly is because one day someone might add a big text field to that structure which is not useful for events, and then we go around sending and storing much more data than is required. By building the stdClass individually we are focusing on sending the 'minimum required information'.

          Show
          Dan Poltawski added a comment - Hi Andrew, 2. I haven't looked at this in any detail at all, but regarding component I think you should replicate what other areas of Moodle do. This component/itemid 'pattern' is common now, so maybe role assignemnts and enrolments could be looked at (note maybe Eloy wants to improve there, so my advice might be bad). 3. I think just add the fields to the existing stdClass (its less radical, thus easier for us to accept ). One reason we might want to avoid just adding the $member directly is because one day someone might add a big text field to that structure which is not useful for events, and then we go around sending and storing much more data than is required. By building the stdClass individually we are focusing on sending the 'minimum required information'.
          Hide
          Andrew Nicols added a comment -

          Rebased on latest (integration/)master

          It looks like it's not particularly clear anywhere this already happens - lib/accesslib.php::role_assign() only comlains if the itemid is specified without a component, or the component is poorly formatted.
          If an invalid component is specified, a coding_exception is thrown.
          If an itemid is specified without a valid component, a new coding_exception is thrown.

          I've done as Dan suggests for the events_trigger

          Show
          Andrew Nicols added a comment - Rebased on latest (integration/)master It looks like it's not particularly clear anywhere this already happens - lib/accesslib.php::role_assign() only comlains if the itemid is specified without a component, or the component is poorly formatted. If an invalid component is specified, a coding_exception is thrown. If an itemid is specified without a valid component, a new coding_exception is thrown. I've done as Dan suggests for the events_trigger
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (master only), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
          Hide
          Rossiani Wijaya added a comment -

          This works great.

          Thank you for fixing this.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This works great. Thank you for fixing this. Test passed.
          Hide
          Andrew Nicols added a comment -

          Thanks to Rossiana's update to the testing instructions, I've just noticed that I inadvertently committed the function introduced by the test instructions - my apologies.

          I've added a commit to the branch removing the function from mod/page/lib.php – https://git.luns.net.uk/moodle.git/commitdiff/b677b83d571c0feb9ab237cc52774f9ebedd16a7 on MDL-31973-master-7

          Apologies once again!

          Show
          Andrew Nicols added a comment - Thanks to Rossiana's update to the testing instructions, I've just noticed that I inadvertently committed the function introduced by the test instructions - my apologies. I've added a commit to the branch removing the function from mod/page/lib.php – https://git.luns.net.uk/moodle.git/commitdiff/b677b83d571c0feb9ab237cc52774f9ebedd16a7 on MDL-31973 -master-7 Apologies once again!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm so proud...of you, many thanks!

          http://youtu.be/n64CdfDRnZY

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao
          Hide
          Dan Poltawski added a comment -

          Andrew, did your latest comment get accommodated? If not, please create a new issue for it.

          In future if there is something like this its worth shouting to get the status set to failed (or do it if you can yourself, not sure what can be done when its in 'test passed' state, that way as integrators we have to take some action on it.)

          Show
          Dan Poltawski added a comment - Andrew, did your latest comment get accommodated? If not, please create a new issue for it. In future if there is something like this its worth shouting to get the status set to failed (or do it if you can yourself, not sure what can be done when its in 'test passed' state, that way as integrators we have to take some action on it.)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Crap, why I did not see that change, apologies!

          Yes, plz, creating a new (regression) issue is the best way, or else there are high chances to get things like this 100% ignored, unseen, forgotten.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Crap, why I did not see that change, apologies! Yes, plz, creating a new (regression) issue is the best way, or else there are high chances to get things like this 100% ignored, unseen, forgotten. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          FYI: MDL-35210 has been created to fix that testing code. Integrating it right now.

          Show
          Eloy Lafuente (stronk7) added a comment - FYI: MDL-35210 has been created to fix that testing code. Integrating it right now.
          Hide
          Petr Škoda added a comment -

          Hi, this is missing one important part in groups restore, the itemid needs to be recalculated, storing the original value is wrong. I am going to adda callback a new plugin callback for enrol plugins as part of MDL-31437. I am not sure what to do with the rest of plugins.

          Show
          Petr Škoda added a comment - Hi, this is missing one important part in groups restore, the itemid needs to be recalculated, storing the original value is wrong. I am going to adda callback a new plugin callback for enrol plugins as part of MDL-31437 . I am not sure what to do with the rest of plugins.
          Hide
          Petr Škoda added a comment -

          Hmmm, I was surprised that the UI allowed me to remove membership created by my enrol plugin, I expected that it would be protected in the UI.

          Show
          Petr Škoda added a comment - Hmmm, I was surprised that the UI allowed me to remove membership created by my enrol plugin, I expected that it would be protected in the UI.
          Hide
          Petr Škoda added a comment -

          Arrrgghh, the restore is a lot worse than I expected, the groups are restored before enrolments which is very wrong, we can restore group membership for enrolled users only, also this prevents calculation of new itemid for the group_members records...

          I suppose this will require major surgery in the restore code and maybe also backups.

          Show
          Petr Škoda added a comment - Arrrgghh, the restore is a lot worse than I expected, the groups are restored before enrolments which is very wrong, we can restore group membership for enrolled users only, also this prevents calculation of new itemid for the group_members records... I suppose this will require major surgery in the restore code and maybe also backups.
          Hide
          Petr Škoda added a comment - - edited

          Hi, I think I have resolved all problems I found in MDL-31437:

          • the group memberships are restored after enrol instances
          • there is a new plugin callback for 'xx_yy_restore_group_member($step, $data)' - please update your existing plugins if you want the restore in your plugin to work properly
          • I found the 'xx_yy_allow_group_member_remove()' callback and fixed the enrol JS UI to respect it
          • unenrolment code removes the linked group memberships automatically

          Thanks a lot for this improvement!

          Show
          Petr Škoda added a comment - - edited Hi, I think I have resolved all problems I found in MDL-31437 : the group memberships are restored after enrol instances there is a new plugin callback for 'xx_yy_restore_group_member($step, $data)' - please update your existing plugins if you want the restore in your plugin to work properly I found the 'xx_yy_allow_group_member_remove()' callback and fixed the enrol JS UI to respect it unenrolment code removes the linked group memberships automatically Thanks a lot for this improvement!
          Hide
          Mary Cooch added a comment -

          I notice this has both dev_docs and docs_required labels. Could someone tell me what is required for user docs as I'm not really sure, or is this just a dev_docs thing? Thanks

          Show
          Mary Cooch added a comment - I notice this has both dev_docs and docs_required labels. Could someone tell me what is required for user docs as I'm not really sure, or is this just a dev_docs thing? Thanks
          Hide
          Sam Marshall added a comment -

          Mary: This is probably borderline but the user documentation relates to the fact that with this feature, if group membership is created by an enrol plugin instead of manually by the user, you may then not be able to remove those group members using the normal interface (because the plugin owns the group membership, so you'd have to get rid of them some other way, like turning off the plugin).

          In core this feature is only turned on for the 'IMS Enterprise' enrol plugin. So group members added by that enrol plugin cannot be deleted. It may be used more widely by other third-party plugins.

          Usually the reason for doing this is that it doesn't make sense to delete people or that it would result in inconsistency. For example, in the OU's case (this is probably a common pattern), supposing staff were to delete a group member that was automatically created by our internal systems. This group member would be added back in the next day by an overnight data update. Good thing pantomime season is here because it's like, you've fixed the problem... OH NO YOU HAVEN'T! Staff shouldn't delete the user in the Moodle interface, but instead need to get them moved properly in the university systems that Moodle is getting its data from. So that's why it's good to prevent people deleting entries in Moodle that were added by this kind of enrolment plugin.

          Also, when group members are owned by a plugin like this, there is information below their name on the group screen. See the screenshot in this issue.

          Show
          Sam Marshall added a comment - Mary: This is probably borderline but the user documentation relates to the fact that with this feature, if group membership is created by an enrol plugin instead of manually by the user, you may then not be able to remove those group members using the normal interface (because the plugin owns the group membership, so you'd have to get rid of them some other way, like turning off the plugin). In core this feature is only turned on for the 'IMS Enterprise' enrol plugin. So group members added by that enrol plugin cannot be deleted. It may be used more widely by other third-party plugins. Usually the reason for doing this is that it doesn't make sense to delete people or that it would result in inconsistency. For example, in the OU's case (this is probably a common pattern), supposing staff were to delete a group member that was automatically created by our internal systems. This group member would be added back in the next day by an overnight data update. Good thing pantomime season is here because it's like, you've fixed the problem... OH NO YOU HAVEN'T! Staff shouldn't delete the user in the Moodle interface, but instead need to get them moved properly in the university systems that Moodle is getting its data from. So that's why it's good to prevent people deleting entries in Moodle that were added by this kind of enrolment plugin. Also, when group members are owned by a plugin like this, there is information below their name on the group screen. See the screenshot in this issue.
          Hide
          Petr Škoda added a comment - - edited

          It is now also used in the cohort sync plugin.

          Show
          Petr Škoda added a comment - - edited It is now also used in the cohort sync plugin.
          Hide
          Mary Cooch added a comment -

          Thanks to Sam and Peter for the explanatory help - removing docs_required as this information has now been added to http://docs.moodle.org/24/en/Groups and http://docs.moodle.org/24/en/Cohort_sync

          Show
          Mary Cooch added a comment - Thanks to Sam and Peter for the explanatory help - removing docs_required as this information has now been added to http://docs.moodle.org/24/en/Groups and http://docs.moodle.org/24/en/Cohort_sync

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: