Moodle
  1. Moodle
  2. MDL-18128

Posts moved by Teachers are not assigned to any group (neither the mover or the original poster's) when moved to another forum and so are visible to all

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.4, 2.0.6, 2.1.3, 2.2
    • Fix Version/s: 2.1.4, 2.2.1
    • Component/s: Forum
    • Labels:
    • Testing Instructions:
      Hide

      0. Assign students to separate groups
      1. Create two forums, one an open forum and the other a separate group forum.
      2. Create a post in the open forum as a student
      3. Log in as teacher/admin and move post to group forum
      4. Post is visible to all students, both the original student's group members and students from other groups

      testing instructions for the fix:
      1) Follow the above instructions.
      2) Log in as the teacher/admin and edit the discussion (First post). Test: If you are a teacher/admin you should see a drop down menu for group.
      3) Change the group and click save changes.
      4) Log in as a student of one group and check if you can see the discussion. Test: if you are of the wrong group and the group setting is 'separate group' then you shouldn't see the discussion.
      5) Log in as a student of the opposite group and check if you can see the discussion. Test: if you are of the same group you should be able to see the discussion.
      6) log in and create a discussion as a student. Test: make sure that the group section is only text and not a drop down box.

      Show
      0. Assign students to separate groups 1. Create two forums, one an open forum and the other a separate group forum. 2. Create a post in the open forum as a student 3. Log in as teacher/admin and move post to group forum 4. Post is visible to all students, both the original student's group members and students from other groups testing instructions for the fix: 1) Follow the above instructions. 2) Log in as the teacher/admin and edit the discussion (First post). Test: If you are a teacher/admin you should see a drop down menu for group. 3) Change the group and click save changes. 4) Log in as a student of one group and check if you can see the discussion. Test: if you are of the wrong group and the group setting is 'separate group' then you shouldn't see the discussion. 5) Log in as a student of the opposite group and check if you can see the discussion. Test: if you are of the same group you should be able to see the discussion. 6) log in and create a discussion as a student. Test: make sure that the group section is only text and not a drop down box.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-18128-master-version3
    • Rank:
      2300

      Description

      If you set up a forum with invisible groups you expect posts to be hidden from those in other groups. If you move posts between forums then this fails and posts become fully public to members of any group or those not in any group.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          I can confirm this is still a problem in later versions of 1.9.X. It could also be worth testing in 2.X.

          Show
          Michael de Raadt added a comment - I can confirm this is still a problem in later versions of 1.9.X. It could also be worth testing in 2.X.
          Hide
          Adrian Greeve added a comment - - edited

          Moving a post from one forum to another carries the group status with it. If the post was created in a forum that has no group control then it doesn't have a group id associated with the post. There was no way to edit the group that the post belonged to. This patch introduces a drop box with the current groups available to change into.

          The same issue exists for version 2.X
          I have included a fix for this as well.

          Show
          Adrian Greeve added a comment - - edited Moving a post from one forum to another carries the group status with it. If the post was created in a forum that has no group control then it doesn't have a group id associated with the post. There was no way to edit the group that the post belonged to. This patch introduces a drop box with the current groups available to change into. The same issue exists for version 2.X I have included a fix for this as well.
          Hide
          Jason Fowler added a comment -

          All good

          Show
          Jason Fowler added a comment - All good
          Hide
          Adrian Greeve added a comment -

          I had another look at this and changed it to meet moodle coding guidelines. I'm still not happy with the way that the pull down menu works for changing groups. If someone could have a look and give me any suggestions that would be most appreciated.

          Show
          Adrian Greeve added a comment - I had another look at this and changed it to meet moodle coding guidelines. I'm still not happy with the way that the pull down menu works for changing groups. If someone could have a look and give me any suggestions that would be most appreciated.
          Hide
          Rajesh Taneja added a comment -

          Hello Adrian,

          There are couple of things you might want to consider:

          1. groups_print_activity_menu actually get groups which are accessible by user, and if user has enough permission to move discussion, then I don't feel we need another level of permission.
          2. It might me nice to move this groups menu under edit discussion (Under discussion there is a hyperlink to edit discussion)
          3. add_to_log has a very big message, not sure if that is right.
          Show
          Rajesh Taneja added a comment - Hello Adrian, There are couple of things you might want to consider: groups_print_activity_menu actually get groups which are accessible by user, and if user has enough permission to move discussion, then I don't feel we need another level of permission. It might me nice to move this groups menu under edit discussion (Under discussion there is a hyperlink to edit discussion) add_to_log has a very big message, not sure if that is right.
          Hide
          Adrian Greeve added a comment -

          Thanks Rajesh for the great comments. I can see now that putting the ability into the edit area is a great idea. I've re-written the code to implement that solution.

          Show
          Adrian Greeve added a comment - Thanks Rajesh for the great comments. I can see now that putting the ability into the edit area is a great idea. I've re-written the code to implement that solution.
          Hide
          Adrian Greeve added a comment -

          More alterations have been made. Now the group change drop down box is only accessible from the first post in a discussion.

          Show
          Adrian Greeve added a comment - More alterations have been made. Now the group change drop down box is only accessible from the first post in a discussion.
          Hide
          Rajesh Taneja added a comment -

          Hello Adrian,

          Sorry for the late review on this. Found couple of things you might want to consider:

          1. Discussion post and reply is checked on the basis of parent post, i.e. If a post doesn't have parent, it is considered to be a discussion topic.
            mod/forum/post.php - line 529
            if (!empty($parent)) {
               $heading = get_string("yourreply", "forum");
            } else {
            ...
               $heading = get_string('yournewtopic', 'forum');
            }
            
          2. Some of the comments are going in two lines, which can fit in one line. Probably not an issue, but might want to consider reducing them.
          3. You might want to consider using groups_print_activity_menu() for creating group menu.
          Show
          Rajesh Taneja added a comment - Hello Adrian, Sorry for the late review on this. Found couple of things you might want to consider: Discussion post and reply is checked on the basis of parent post, i.e. If a post doesn't have parent, it is considered to be a discussion topic. mod/forum/post.php - line 529 if (!empty($parent)) { $heading = get_string( "yourreply" , "forum" ); } else { ... $heading = get_string('yournewtopic', 'forum'); } Some of the comments are going in two lines, which can fit in one line. Probably not an issue, but might want to consider reducing them. You might want to consider using groups_print_activity_menu() for creating group menu.
          Hide
          Adrian Greeve added a comment - - edited

          Hello Rajesh,

          1) I took a look at the parent variable and that led me to $post->parent on the post_form.php page, so that has definitely tidied up my code there.
          2) I've put my comments onto one line now.
          3) I did consider the function groups_print_activity, but if you look at that function you will see that it produces a drop down menu which posts the form when selected, and also has some text associated with it (Visible groups), which really isn't appropriate in this instance. Is there some way that I can manipulate the string to get it to behave more like I want it?

          Thanks

          Show
          Adrian Greeve added a comment - - edited Hello Rajesh, 1) I took a look at the parent variable and that led me to $post->parent on the post_form.php page, so that has definitely tidied up my code there. 2) I've put my comments onto one line now. 3) I did consider the function groups_print_activity, but if you look at that function you will see that it produces a drop down menu which posts the form when selected, and also has some text associated with it (Visible groups), which really isn't appropriate in this instance. Is there some way that I can manipulate the string to get it to behave more like I want it? Thanks
          Hide
          Rajesh Taneja added a comment -

          Thanks Adrian,

          I think you might want to consider following:

          1. updating discussion group in forum_update_post, where it actually update discussion and check for post to be parent post. This way you can avoid having an extra sql query
          2. I am sure there is a better way to use groups_print_activity_menu, as it's used at multiple places in moodle. I will personally try to use existing functions rather then replication the functionality.
          3. If you are keen on label of combo box, you can create a separate commit, adding another (optional) param in groups_print_activity_menu
          Show
          Rajesh Taneja added a comment - Thanks Adrian, I think you might want to consider following: updating discussion group in forum_update_post, where it actually update discussion and check for post to be parent post. This way you can avoid having an extra sql query I am sure there is a better way to use groups_print_activity_menu, as it's used at multiple places in moodle. I will personally try to use existing functions rather then replication the functionality. If you are keen on label of combo box, you can create a separate commit, adding another (optional) param in groups_print_activity_menu
          Hide
          Adrian Greeve added a comment -

          Created a new function to get an array of groups for a course. Made an addition to another function to allow group information to be updated.

          Show
          Adrian Greeve added a comment - Created a new function to get an array of groups for a course. Made an addition to another function to allow group information to be updated.
          Hide
          Rajesh Taneja added a comment -

          Hello Adrian,

          Patch looks Good to me .
          You probably want to consider clearing white spaces above

          $groupsmenu = groups_get_groups_for_course($cm, $hideallparticipants);
          
          Show
          Rajesh Taneja added a comment - Hello Adrian, Patch looks Good to me . You probably want to consider clearing white spaces above $groupsmenu = groups_get_groups_for_course($cm, $hideallparticipants);
          Hide
          Adrian Greeve added a comment -

          White space removed. Thanks for the thorough peer review Raj. Submitting for integration with additional testing instructions.

          Show
          Adrian Greeve added a comment - White space removed. Thanks for the thorough peer review Raj. Submitting for integration with additional testing instructions.
          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
          Sam Hemelryk added a comment -

          Hi guys,

          I've just been looking at this now.
          I think we need to have a good think about this new function, and depending on how thing play out there are a couple of changes that I think should be made before this gets integrated:

          First up the function, I'm sorry but I'm really not sure what we need this function. It took me a while to put my finger on it but this is what I've got. Within the groups API everything is split into either for course, or for activity. This new method is called groups_get_course_groups but really it is returning the groups for an activity.
          Next is what the function is doing, no doubt Adrian you are intending for it to reduce code duplication, however looking over what it's doing I think presently is it introducing more duplication. It is within itself making many calls that calling code would still need to make, things like fetching module context, getting the group mode, and getting the active group. I don't think this code can really be clearly separated out like this and having a look groups API it looks like this is visible from the number of functions that have already been created to work out areas as mentioned earlier. Have you seen groups_get_activity_allowed_groups.
          Personally I think we should avoid adding more functions to the groups API unless absolutely required. Really the best solution for it is to revise the groups API as a whole and see about reducing that redundancy. For the time being my vote goes to leaving that functionality directly in groups_print_activity_menu and removing the new function.

          Assuming we keep the new function (which I vote against):

          1. It needs to be renamed, Either something like groups_get_course_groups if it's for course groups, or groups_get_activity_groups if its for activities. This is more in line with existing functions e.g. groups_get_activity_group to get the active group.
          2. New function should take $groupmode as an optional argument, There is a chance that groups_get_activity_groupmode will use a DB query. As the [presently] one use of groups_get_groups_for_course already has already fetched lets make it possible to pass it through and avoid a second unnecessary DB call.

          As for the rest of the code:

          1. post_form.php: If the user has the cap to move a discussion but can only access one group then we should not show the groupinfo selector like we do for regular users. You can test this by removing the teachers capability to access all groups.
          2. lib.php: forum_update_post didn't originally require $post->grouped to be set. We need to support it not being set still for backwards compatibility.
          3. Could you please backport the changes to MOODLE_21_STABLE

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've just been looking at this now. I think we need to have a good think about this new function, and depending on how thing play out there are a couple of changes that I think should be made before this gets integrated: First up the function, I'm sorry but I'm really not sure what we need this function. It took me a while to put my finger on it but this is what I've got. Within the groups API everything is split into either for course, or for activity. This new method is called groups_get_course_groups but really it is returning the groups for an activity. Next is what the function is doing, no doubt Adrian you are intending for it to reduce code duplication, however looking over what it's doing I think presently is it introducing more duplication. It is within itself making many calls that calling code would still need to make, things like fetching module context, getting the group mode, and getting the active group. I don't think this code can really be clearly separated out like this and having a look groups API it looks like this is visible from the number of functions that have already been created to work out areas as mentioned earlier. Have you seen groups_get_activity_allowed_groups. Personally I think we should avoid adding more functions to the groups API unless absolutely required. Really the best solution for it is to revise the groups API as a whole and see about reducing that redundancy. For the time being my vote goes to leaving that functionality directly in groups_print_activity_menu and removing the new function. Assuming we keep the new function (which I vote against): It needs to be renamed, Either something like groups_get_course_groups if it's for course groups, or groups_get_activity_groups if its for activities. This is more in line with existing functions e.g. groups_get_activity_group to get the active group. New function should take $groupmode as an optional argument, There is a chance that groups_get_activity_groupmode will use a DB query. As the [presently] one use of groups_get_groups_for_course already has already fetched lets make it possible to pass it through and avoid a second unnecessary DB call. As for the rest of the code: post_form.php: If the user has the cap to move a discussion but can only access one group then we should not show the groupinfo selector like we do for regular users. You can test this by removing the teachers capability to access all groups. lib.php: forum_update_post didn't originally require $post->grouped to be set. We need to support it not being set still for backwards compatibility. Could you please backport the changes to MOODLE_21_STABLE Cheers Sam
          Hide
          Adrian Greeve added a comment - - edited

          I've provided a second patch. It doesn't touch the functions in grouplib and it also doesn't touch the forum_update_post function either.

          Show
          Adrian Greeve added a comment - - edited I've provided a second patch. It doesn't touch the functions in grouplib and it also doesn't touch the forum_update_post function either.
          Hide
          Rajesh Taneja added a comment -

          Hello Adrian,

          I think Sam meant, you should check for accessallgroups in addition to movediscussions capability.
          If user can access all groups but does not have permission to move discussion then also, it should not show select box. In addition to this, if user has access to only one group then also, the select box should not be visible (Similar to groups_print_activity_menu).

          @Sam:
          Shouldn't group checking code, be a part of grouplib and post_form.php should call this function ?

          Show
          Rajesh Taneja added a comment - Hello Adrian, I think Sam meant, you should check for accessallgroups in addition to movediscussions capability. If user can access all groups but does not have permission to move discussion then also, it should not show select box. In addition to this, if user has access to only one group then also, the select box should not be visible (Similar to groups_print_activity_menu). @Sam: Shouldn't group checking code, be a part of grouplib and post_form.php should call this function ?
          Hide
          Rajesh Taneja added a comment -

          Adrian,
          As this is a security issue, you might want to remove public branches and submit this as patch.

          Show
          Rajesh Taneja added a comment - Adrian, As this is a security issue, you might want to remove public branches and submit this as patch.
          Hide
          Adrian Greeve added a comment -

          I added the additional check for the movediscussions context.

          Show
          Adrian Greeve added a comment - I added the additional check for the movediscussions context.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Sorry looks like I wasn't clear in what I was stating.
          The accessallgroups check doesn't need to be made, the groups API calls ensure that only groups the user can access are returned.
          What I was getting at is that if there is only one group that the user can access/move to we shouldn't show the select box. We should leave the form as it is.
          Imagine, you had a course with 2 groups and 2 teachers, each teacher was assigned to one group, and they don't have the accessallgroups capability.
          In this case with the original patch the user could only access one group and we would show them a select box with one item in it.
          I see that the current patch now includes an All Participants option so that is no longer really a problem, although such an option would need to be thoroughly tested in regards to grading etc.

          As for the group checks that are been made in the post_form.php; I think they are fine there, there arn't very many and they're all very simple.
          The groups API has never been that evolved, although surely it could do with work and a good clean + refactor.
          For this issue I wouldn't worry about it.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Sorry looks like I wasn't clear in what I was stating. The accessallgroups check doesn't need to be made, the groups API calls ensure that only groups the user can access are returned. What I was getting at is that if there is only one group that the user can access/move to we shouldn't show the select box. We should leave the form as it is. Imagine, you had a course with 2 groups and 2 teachers, each teacher was assigned to one group, and they don't have the accessallgroups capability. In this case with the original patch the user could only access one group and we would show them a select box with one item in it. I see that the current patch now includes an All Participants option so that is no longer really a problem, although such an option would need to be thoroughly tested in regards to grading etc. As for the group checks that are been made in the post_form.php; I think they are fine there, there arn't very many and they're all very simple. The groups API has never been that evolved, although surely it could do with work and a good clean + refactor. For this issue I wouldn't worry about it. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks Adrian,

          Changes look Good to me.

          Show
          Rajesh Taneja added a comment - Thanks Adrian, Changes look Good to me.
          Hide
          Adrian Greeve added a comment -

          Thanks Rajesh for your help

          Show
          Adrian Greeve added a comment - Thanks Rajesh for your help
          Hide
          Sam Hemelryk added a comment -

          Sending this back again sorry guys.
          The following changes should be made before it gets integrated:

          1. Remove the accessallgroups checks, they're not needed. Currently the group API function ensure only groups the user can access are there to be moved to. All users should need is the move discussion cap.
          2. post.php line 629: There is no need to check the result of $DB->set_field. It throws an exception if it fails.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Sending this back again sorry guys. The following changes should be made before it gets integrated: Remove the accessallgroups checks, they're not needed. Currently the group API function ensure only groups the user can access are there to be moved to. All users should need is the move discussion cap. post.php line 629: There is no need to check the result of $DB->set_field. It throws an exception if it fails. Cheers Sam
          Hide
          Adrian Greeve added a comment -

          Hi Sam,

          I did have a look at leaving the check for accessallgroups off as I thought that the group API would check for which group I'm allowed access to, but I wasn't happy with its behaviour. If you try out the new patch and remove access all groups in the permissions for the non-editing teacher, make sure that the group visibility is set to visible groups, and then edit a discussion as a non-editing teacher, you will see that you have a drop down box with all the groups listed. Checks in post.php mean that the group won't get updated, but I think that a drop down box should not be visible in this situation.

          Show
          Adrian Greeve added a comment - Hi Sam, I did have a look at leaving the check for accessallgroups off as I thought that the group API would check for which group I'm allowed access to, but I wasn't happy with its behaviour. If you try out the new patch and remove access all groups in the permissions for the non-editing teacher, make sure that the group visibility is set to visible groups, and then edit a discussion as a non-editing teacher, you will see that you have a drop down box with all the groups listed. Checks in post.php mean that the group won't get updated, but I think that a drop down box should not be visible in this situation.
          Hide
          Adrian Greeve added a comment -

          Sorry, I had a talk with Raj and he explained that it should work that way. I've made the changes and I'm resubmitting for integration.

          Show
          Adrian Greeve added a comment - Sorry, I had a talk with Raj and he explained that it should work that way. I've made the changes and I'm resubmitting for integration.
          Hide
          Sam Hemelryk added a comment -

          Thanks Adrian for fixing that up. This has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Adrian for fixing that up. This has been integrated now.
          Hide
          Jason Fowler added a comment -

          Test passed successfully, will be raising opening an issue to address some other concerns about the broader system - mostly the fact that something needs to be done at the time of the move.

          Show
          Jason Fowler added a comment - Test passed successfully, will be raising opening an issue to address some other concerns about the broader system - mostly the fact that something needs to be done at the time of the move.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

          Now... disconnect, relax and enjoy the next days, yay!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao
          Hide
          Michael de Raadt added a comment -

          After consulting a number of people, I'm removing the security level on this issue.

          Show
          Michael de Raadt added a comment - After consulting a number of people, I'm removing the security level on this issue.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: