Moodle
  1. Moodle
  2. MDL-19670

Allow "Create new discussion for each group" to enable teachers to post the same question for each group with only one posting

    Details

    • Type: Improvement Improvement
    • Status: Development in progress
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.9.5, 2.6, 2.8
    • Fix Version/s: None
    • Component/s: Forum, Groups
    • Testing Instructions:
      Hide

      Setup
      1. Create a new course.
      2. Create a teacher and multiple students (5-6 will do).
      3. Enrol the teacher and students into the course.
      4. Auto create 2 groups
      5. Pick one of the students and enrol that student in both groups.

      Separate groups
      1. In the course you just created, create a forum that has separate groups set as the Group Mode
      2. Go to the "add a new discussion" form for the forum you just created.
      3. Ensure that students are unable to see the "Post a copy to all groups" checkbox, and that teachers/admin can see the checkbox.

      Visible groups
      1. Repeat the tests from the above section.

      No groups
      1. Repeat the tests from the Separate Groups section. Ensure that in all cases, that the "Post a copy to all groups" checkbox does not display for any user.

      Behat
      1. Run all behat tests for the @mod_forum tag (mdk behat --tags @mod_forum)

      Show
      Setup 1. Create a new course. 2. Create a teacher and multiple students (5-6 will do). 3. Enrol the teacher and students into the course. 4. Auto create 2 groups 5. Pick one of the students and enrol that student in both groups. Separate groups 1. In the course you just created, create a forum that has separate groups set as the Group Mode 2. Go to the "add a new discussion" form for the forum you just created. 3. Ensure that students are unable to see the "Post a copy to all groups" checkbox, and that teachers/admin can see the checkbox. Visible groups 1. Repeat the tests from the above section. No groups 1. Repeat the tests from the Separate Groups section. Ensure that in all cases, that the "Post a copy to all groups" checkbox does not display for any user. Behat 1. Run all behat tests for the @mod_forum tag (mdk behat --tags @mod_forum)
    • Difficulty:
      Difficult
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_26_STABLE, MOODLE_28_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-19670-master
    • Sprint:
      Team A Sprint 1, Team '; drop tables Sprint 2, Team ';drop tables Sprint 3, Team ';drop tables Sprint 4
    • Issue size:
      Small

      Description

      When using the forums in group mode new posts currently have to be started individually for each group in order to maintain group participation rather than the whole-class involvement. The option for new discussion threads to be started that will automatically post that new discussion for each group would decrease some confusion. (The post/discussion would multiply itself so it is created for each group)

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Matthew Davidson added a comment -

            There needs to be a mechanism to duplicate a topic for each group when separate groups mode is on and the topic is created for "All participants". Currently it just says "You do not have permission to add a new discussion topic for all participants". This is not correct. Teachers DO have permission to create a topic for all participants....Moodle just doesn't want to do it for them. This should be fixed.

            Show
            Matthew Davidson added a comment - There needs to be a mechanism to duplicate a topic for each group when separate groups mode is on and the topic is created for "All participants". Currently it just says "You do not have permission to add a new discussion topic for all participants". This is not correct. Teachers DO have permission to create a topic for all participants....Moodle just doesn't want to do it for them. This should be fixed.
            Hide
            Brandy Weaver added a comment -

            This would be very helpful, and the proposed solution seems simplest. However, I'd suggest for consideration an alternate implementation:

            As proposed, the fix would provide the option to automate the duplication of a post for multiple groups. However, in this implementation, the teacher would then show multiple copies of each discussion on his/her screen. In the case of only a few groups and only a few discussions within a forum (in this example, Q&A), this wouldn't be unmanageable. But in a case such as discussed here (https://moodle.org/mod/forum/discuss.php?d=172853#p850072) (30 separate groups!) and/or with multiple discussions within a forum, the screen for the teacher could quickly become very unwieldy.

            I'd love to see a sort of "all groups" option that created a single discussion, but tagged each contribution (after the instructor's open) by group. A student then viewing that single discussion would see only discussions tagged with his/her own group tags. Ideally the instructor would have a single discussion link with a drop-down box (or radio controls, or list with checkboxes?) to "filter" the content of the discussion by group, essentially simulating the view of a student within that group).

            Just a thought. Our staff is definitely looking forward to any implementation of this feature.

            Show
            Brandy Weaver added a comment - This would be very helpful, and the proposed solution seems simplest. However, I'd suggest for consideration an alternate implementation: As proposed, the fix would provide the option to automate the duplication of a post for multiple groups. However, in this implementation, the teacher would then show multiple copies of each discussion on his/her screen. In the case of only a few groups and only a few discussions within a forum (in this example, Q&A), this wouldn't be unmanageable. But in a case such as discussed here ( https://moodle.org/mod/forum/discuss.php?d=172853#p850072 ) (30 separate groups!) and/or with multiple discussions within a forum, the screen for the teacher could quickly become very unwieldy. I'd love to see a sort of "all groups" option that created a single discussion, but tagged each contribution (after the instructor's open) by group. A student then viewing that single discussion would see only discussions tagged with his/her own group tags. Ideally the instructor would have a single discussion link with a drop-down box (or radio controls, or list with checkboxes?) to "filter" the content of the discussion by group, essentially simulating the view of a student within that group). Just a thought. Our staff is definitely looking forward to any implementation of this feature.
            Hide
            Matthew Davidson added a comment -

            I actually implemented this concept a long time ago, but it wasn't used. https://tracker.moodle.org/browse/MDL-14270

            They didn't seem very interested in it.

            Show
            Matthew Davidson added a comment - I actually implemented this concept a long time ago, but it wasn't used. https://tracker.moodle.org/browse/MDL-14270 They didn't seem very interested in it.
            Hide
            Derek Chirnside added a comment -

            I'm wondering if we can get some traction on this as an issue. If to be able to use this for MOOC's alone it is worthwhile.

            Set up random groups of five, then set a question (the same question) to kick off a discussion . . . you need ot be able to have a post in each group. Not practical if there are a lot of groups. And as matthew says, there is code here: MDL-14270

            -Derek

            Show
            Derek Chirnside added a comment - I'm wondering if we can get some traction on this as an issue. If to be able to use this for MOOC's alone it is worthwhile. Set up random groups of five, then set a question (the same question) to kick off a discussion . . . you need ot be able to have a post in each group. Not practical if there are a lot of groups. And as matthew says, there is code here: MDL-14270 -Derek
            Hide
            Timothy Allen added a comment - - edited

            I'd just like to add my voice to this issue. While the current behaviour may be logical, it is not intuitive at all. Our instructors don't understand why they can't just post once and have it work for each group. It seems an odd quirk, and an annoying one especially for some courses may have many groups, as many as 20 or more, and it is very tedious for an instructor to post the same instructions so many times in a busy day.

            This is particularly a problem with the Q&A Forum type, which forces an instructor to post a new thread (if one posts it in the description field students can not contribute because they are unable by default to post new threads, they can only reply to a question). Even if a solution for the Q&A Forum type could be found, this would be great. The latter forum type is very popular because of the hiding feature it has, but it is currently not feasible for courses with many groups.

            Show
            Timothy Allen added a comment - - edited I'd just like to add my voice to this issue. While the current behaviour may be logical, it is not intuitive at all. Our instructors don't understand why they can't just post once and have it work for each group. It seems an odd quirk, and an annoying one especially for some courses may have many groups, as many as 20 or more, and it is very tedious for an instructor to post the same instructions so many times in a busy day. This is particularly a problem with the Q&A Forum type, which forces an instructor to post a new thread (if one posts it in the description field students can not contribute because they are unable by default to post new threads, they can only reply to a question). Even if a solution for the Q&A Forum type could be found, this would be great. The latter forum type is very popular because of the hiding feature it has, but it is currently not feasible for courses with many groups.
            Hide
            moodle.com added a comment - - edited

            To fix the main problem here - we should modify the form to have a "Post a copy to every group" (explain it well in the help - ask Helen) option when creating a new discussion. It still means if you want to edit the post, you will need to edit each post separately. This should send the post to all groups the user has access to (might not be all the groups).

            Show
            moodle.com added a comment - - edited To fix the main problem here - we should modify the form to have a "Post a copy to every group" (explain it well in the help - ask Helen) option when creating a new discussion. It still means if you want to edit the post, you will need to edit each post separately. This should send the post to all groups the user has access to (might not be all the groups).
            Hide
            Andrew Nicols added a comment -

            Adding a commit from when I did some exploratory work on this recently.
            Rather than the approach suggested, I looked at posting a single post to all groups. Users can reply in various groups. The view is the bit haven't yet solved.

            Putting it here in case others can work improve upon it before I get a chance to look at it again,

            Andrew

            Show
            Andrew Nicols added a comment - Adding a commit from when I did some exploratory work on this recently. Rather than the approach suggested, I looked at posting a single post to all groups. Users can reply in various groups. The view is the bit haven't yet solved. Putting it here in case others can work improve upon it before I get a chance to look at it again, Andrew
            Hide
            Dave Cooper added a comment -

            Have updated the commit branch.

            I'm going to put this up for peer review but I also have a magical crystal ball (also known as Andrew Nicols) who has pointed out a few issues with the approach I've taken. These definitely need to be discussed.

            1. The UI may be confusing. I've just taken a crack at the wording of the form, but I imagine it needs to be worded better.
            2. Once someone has posted a copy of a message to all groups, if they want to edit the message, they have to edit it in X places (X being the number of posts that were created)
            3. If someone is a member of multiple groups and a copy of a post gets sent, they will get a ton of emails.

            One other thing I'd like to know is how far we need to go with the Behat tests for this - they're basic at the moment, but I can add more if needed.

            -Dave

            Show
            Dave Cooper added a comment - Have updated the commit branch. I'm going to put this up for peer review but I also have a magical crystal ball (also known as Andrew Nicols ) who has pointed out a few issues with the approach I've taken. These definitely need to be discussed. 1. The UI may be confusing. I've just taken a crack at the wording of the form, but I imagine it needs to be worded better. 2. Once someone has posted a copy of a message to all groups, if they want to edit the message, they have to edit it in X places (X being the number of posts that were created) 3. If someone is a member of multiple groups and a copy of a post gets sent, they will get a ton of emails. One other thing I'd like to know is how far we need to go with the Behat tests for this - they're basic at the moment, but I can add more if needed. -Dave
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-19670 using repository: git://github.com/gurgus/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-19670 using repository: git://github.com/gurgus/moodle.git Testing instructions are missing. master (12 errors / 3 warnings) [branch: MDL-19670-master | CI Job ] phplint (0/0) , php (12/3) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Andrew Nicols added a comment -

            As we discussed, your points 1, 2, and 3 are pretty big ones.

            2) Since we have no way in which to preview posts before posting, people often use the save button and then preview things before correcting things which haven't quite worked. This will totally suck;
            3) Yup - that poor departmental administrator is going to get 32 copies of the same message because they are enrolled in the course and can see all of the groups. Lucky them!; and
            4) Oops - I forgot to create a group, I'll do so now. Oh... they don't have access to all of those forum posts I made yesterday explaining X, Y, Z. This one would hit us even more painfully with timed release of posts.

            This approach as a whole gets a big thumbs-down from me. As discussed, we have the beginnings of a better (IMO) approach where we do not duplicate data. The difficulty is still in the UI, but all of these other issues are solved.

            Andrew

            Show
            Andrew Nicols added a comment - As we discussed, your points 1, 2, and 3 are pretty big ones. 2) Since we have no way in which to preview posts before posting, people often use the save button and then preview things before correcting things which haven't quite worked. This will totally suck; 3) Yup - that poor departmental administrator is going to get 32 copies of the same message because they are enrolled in the course and can see all of the groups. Lucky them!; and 4) Oops - I forgot to create a group, I'll do so now. Oh... they don't have access to all of those forum posts I made yesterday explaining X, Y, Z. This one would hit us even more painfully with timed release of posts. This approach as a whole gets a big thumbs-down from me. As discussed, we have the beginnings of a better (IMO) approach where we do not duplicate data. The difficulty is still in the UI, but all of these other issues are solved. Andrew
            Hide
            Dave Cooper added a comment -

            Just fixing up the cibot errors now - was in a rush getting out yesterday.

            Going to summon Frédéric Massart and Damyon Wiese to this conversation.

            Show
            Dave Cooper added a comment - Just fixing up the cibot errors now - was in a rush getting out yesterday. Going to summon Frédéric Massart and Damyon Wiese to this conversation.
            Hide
            Frédéric Massart added a comment -

            A wild Fred appears

            Show
            Frédéric Massart added a comment - A wild Fred appears
            Hide
            Damyon Wiese added a comment -

            2) So we should add a preview before posting thing as a separate issue - this would be VERY useful - is there an issue for this already?
            3) No the departmental adminstrator should not get an email just because they can see the group, only if they are in it.
            4) This is the same situation as we have currently.

            Overall this approach is an improvement on the current behaviour IMO.

            Show
            Damyon Wiese added a comment - 2) So we should add a preview before posting thing as a separate issue - this would be VERY useful - is there an issue for this already? 3) No the departmental adminstrator should not get an email just because they can see the group, only if they are in it. 4) This is the same situation as we have currently. Overall this approach is an improvement on the current behaviour IMO.
            Hide
            Damyon Wiese added a comment -

            MDL-11788 - preview forum posts.

            Show
            Damyon Wiese added a comment - MDL-11788 - preview forum posts.
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-19670 using repository: git://github.com/gurgus/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-19670 using repository: git://github.com/gurgus/moodle.git Testing instructions are missing. master (1 errors / 0 warnings) [branch: MDL-19670-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Andrew Nicols added a comment -

            2) Yes, but we don't have that yet, and people will still continue to edit posts after posting;
            3) That's not how it works (see below);
            4) True, but we'd be making it worse in many respects.

            I may be missing something, but why aren't we looking at the alternative approach I've suggested. The patch I've submitted goes 90% of the way there? We have to solve similar UI issues but we have a sensible solution instead of workarounds.

            Regarding 3
            If a forum is in force or automatically subscribed mode, then notifications are sent to all users who are subscribed to the forum and can view that post, not just the ones in the roup. That includes all users who are enrolled in the course.
            So if you have an administrator enrolled in the course, and the forum is force/auto subscribed, even if they are not in any of the 32 groups, they will receive 32 notifications with this patch in place.

            Show
            Andrew Nicols added a comment - 2) Yes, but we don't have that yet, and people will still continue to edit posts after posting; 3) That's not how it works (see below); 4) True, but we'd be making it worse in many respects. I may be missing something, but why aren't we looking at the alternative approach I've suggested. The patch I've submitted goes 90% of the way there? We have to solve similar UI issues but we have a sensible solution instead of workarounds. Regarding 3 If a forum is in force or automatically subscribed mode, then notifications are sent to all users who are subscribed to the forum and can view that post, not just the ones in the roup. That includes all users who are enrolled in the course. So if you have an administrator enrolled in the course, and the forum is force/auto subscribed, even if they are not in any of the 32 groups, they will receive 32 notifications with this patch in place.
            Hide
            Damyon Wiese added a comment -

            Re: 3 - but the department admin should not be enrolled in the course. And this is the same result as if they manually posted to 32 groups.

            Show
            Damyon Wiese added a comment - Re: 3 - but the department admin should not be enrolled in the course. And this is the same result as if they manually posted to 32 groups.
            Hide
            Damyon Wiese added a comment -

            Re: the alternative solution - you did not solve the UI part of it which IMO will be the hardest part. There will be people who can see the post, but not all the replies, and then who can see the replies to the replies etc.

            Martin agreed to the approach in this patch in the sprint planning meeting and it addresses the most annoying part of the existing behaviour without requiring weeks of dev work.

            Show
            Damyon Wiese added a comment - Re: the alternative solution - you did not solve the UI part of it which IMO will be the hardest part. There will be people who can see the post, but not all the replies, and then who can see the replies to the replies etc. Martin agreed to the approach in this patch in the sprint planning meeting and it addresses the most annoying part of the existing behaviour without requiring weeks of dev work.
            Hide
            Andrew Nicols added a comment -

            Re 3: Real life doesn't always work like this though. Not everyone makes good uses of course categories and category enrolments.
            Yes, it is the same as if they posted manually to 32 groups, but we can do it better and we should.

            Re the UI, the part which hasn't been solved is viewing the threaded replies with groups in mind. There are ways in which we can achieve this in the UI, and it will not require weeks of time. For example:

            1. show all posts and clearly identify which group the post was too;
            2. show all posts to all participants, and the first reply to a group with a link to just view that group; or
            3. have a dropdown select at the top (like we do in the discussion view) to select which group you are viewing the discussion as (possibly combined with option 2).

            I did start working on prototyping several of these before it was assigned in sprint, but I hadn't got anything sorted codewise yet.

            Andrew

            Show
            Andrew Nicols added a comment - Re 3: Real life doesn't always work like this though. Not everyone makes good uses of course categories and category enrolments. Yes, it is the same as if they posted manually to 32 groups, but we can do it better and we should. Re the UI, the part which hasn't been solved is viewing the threaded replies with groups in mind. There are ways in which we can achieve this in the UI, and it will not require weeks of time. For example: show all posts and clearly identify which group the post was too; show all posts to all participants, and the first reply to a group with a link to just view that group; or have a dropdown select at the top (like we do in the discussion view) to select which group you are viewing the discussion as (possibly combined with option 2). I did start working on prototyping several of these before it was assigned in sprint, but I hadn't got anything sorted codewise yet. Andrew
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-19670 using repository: git://github.com/gurgus/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-19670 using repository: git://github.com/gurgus/moodle.git master (1 errors / 0 warnings) [branch: MDL-19670-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Frédéric Massart added a comment -

            Thanks Dave,

            With regards to Andrew's comments, Martin sort of gave us direction to post a copy to all groups that a user has access to. It was understood during the planning that a user that wanted to edit a post would have to go and edit each of them manually.

            From my point of view, I think it also easier to solve going with the approach of the copy rather than changing the way we deal with "All participants". Especially that "All participants" is available to everyone where as "Copy to all groups" would only copy to the groups that you are in (or to all if you have the capability to see them all).

            Here are some comments about the patch:

            1. I don't think this feature should be accessible to students, we might need a new capability 'canposttoallgroups' or something which would be given by defaults to all the archetypes above student.
            2. As we are constructing the group dropdown manually, I would use it rather than a new checkbox. So you could add the list of all the groups, and an option "All of the above". We might want to use the form element selectgroups to avoid confusion.
            3. We might need to check the post threshold before posting in each group. It's very unlikely to happen as you wouldn't give a permission to all groups but not the permission postwithoutthrottling, but there is no reason why we should ignore that. Or we have to document/justify it such as "Posting to multiple groups might allow a user to post more than expected, and then be blocked".
            4. You could simplify how we get all the groups when $fromform->posttoallgroups is set by first checking if they have access to all, else getting their groups.
            5. Rather than using isset() we generally use empty(), also note that the chain of if/elseif will not account for empty values.
            6. I wonder if we should handle the errors differently, we would exist the loop here and print an error, not really knowing how many posts have been made. Probably not worth it as it should work really...

            Cheers Dave, kudos for the Behat test. If your behat test is doing exactly the same thing than your testing instructions you can ask the tester to just run your test.

            Thanks!
            Fred

            Show
            Frédéric Massart added a comment - Thanks Dave, With regards to Andrew's comments, Martin sort of gave us direction to post a copy to all groups that a user has access to. It was understood during the planning that a user that wanted to edit a post would have to go and edit each of them manually. From my point of view, I think it also easier to solve going with the approach of the copy rather than changing the way we deal with "All participants". Especially that "All participants" is available to everyone where as "Copy to all groups" would only copy to the groups that you are in (or to all if you have the capability to see them all). Here are some comments about the patch: I don't think this feature should be accessible to students, we might need a new capability 'canposttoallgroups' or something which would be given by defaults to all the archetypes above student . As we are constructing the group dropdown manually, I would use it rather than a new checkbox. So you could add the list of all the groups, and an option "All of the above". We might want to use the form element selectgroups to avoid confusion. We might need to check the post threshold before posting in each group. It's very unlikely to happen as you wouldn't give a permission to all groups but not the permission postwithoutthrottling , but there is no reason why we should ignore that. Or we have to document/justify it such as "Posting to multiple groups might allow a user to post more than expected, and then be blocked". You could simplify how we get all the groups when $fromform->posttoallgroups is set by first checking if they have access to all, else getting their groups. Rather than using isset() we generally use empty(), also note that the chain of if/elseif will not account for empty values. I wonder if we should handle the errors differently, we would exist the loop here and print an error, not really knowing how many posts have been made. Probably not worth it as it should work really... Cheers Dave, kudos for the Behat test. If your behat test is doing exactly the same thing than your testing instructions you can ask the tester to just run your test. Thanks! Fred
            Hide
            Dave Cooper added a comment -

            Hey Frédéric Massart - thanks for the review. Just a couple of questions.

            If I add "All of the above" to that dropdown we get the following situation where we have a dropdown that can contain the following:
            All participants
            Group A
            Group B
            Group C
            All of the above

            It is kind of confusing (to me, at least) to have "All participants" and "All of the above". Should we just get rid of "All participants"?

            Show
            Dave Cooper added a comment - Hey Frédéric Massart - thanks for the review. Just a couple of questions. If I add "All of the above" to that dropdown we get the following situation where we have a dropdown that can contain the following: All participants Group A Group B Group C All of the above It is kind of confusing (to me, at least) to have "All participants" and "All of the above". Should we just get rid of "All participants"?
            Hide
            Frédéric Massart added a comment - - edited

            Maybe then we could have:

            • Special
              • Group 'All participants'
              • All the groups listed below
            • Groups
              • Group A
              • Group B
              • Group C
              • Group D

            Or something like that.

            Show
            Frédéric Massart added a comment - - edited Maybe then we could have: Special Group 'All participants' All the groups listed below Groups Group A Group B Group C Group D Or something like that.
            Hide
            Dave Cooper added a comment -

            Hmm I'm unsure how this looks. I'm about to upload a screenshot of the dropdown. With updated documentation this would make sense, but I think to someone untrained it's still a bit confusing.

            Show
            Dave Cooper added a comment - Hmm I'm unsure how this looks. I'm about to upload a screenshot of the dropdown. With updated documentation this would make sense, but I think to someone untrained it's still a bit confusing.
            Hide
            Andrew Nicols added a comment -

            How about "Copy to all of my groups"?

            Show
            Andrew Nicols added a comment - How about "Copy to all of my groups"?
            Hide
            Dave Cooper added a comment -

            I think the problem here is being able to clearly resolve the ambiguity between what "all participants" means and what "all of the groups listed below" means. I've made some changes but am waiting for behat to finish before I push the patch (since modifying anything in this dropdown will immediately break a bunch of behat tests).

            Show
            Dave Cooper added a comment - I think the problem here is being able to clearly resolve the ambiguity between what "all participants" means and what "all of the groups listed below" means. I've made some changes but am waiting for behat to finish before I push the patch (since modifying anything in this dropdown will immediately break a bunch of behat tests).
            Hide
            Dave Cooper added a comment -

            Hey Fred,

            I've addressed some of the points you mentioned, and probably require clarification on a couple of them:

            1. Well spotted - I've added the capability and the required checks.
            2. I've added the choice to the dropdown but was unsure what you meant when you were referring to the selectgroups element.
            3. I'm not sure how I managed to remove this check when I was writing the patch, but I've added it back in
            4. I'm not quite sure how to tweak my logic to do this more efficiently - to me it looks like I'm doing the correct check here.
            5. Along the same lines as the previous point - we are checking for the specific cases in the if-else branches you are referring to. If I switch this to use empty() instead of isset() I'm going to have to rework all the logic but the outcome will be the same (and equally as efficient as my current implementation). However, I might be missing something here.
            6. I agree, probably not worth it, but if you think I should do it then I can

            Thanks for taking the time for that peer review

            -Dave

            Show
            Dave Cooper added a comment - Hey Fred, I've addressed some of the points you mentioned, and probably require clarification on a couple of them: 1. Well spotted - I've added the capability and the required checks. 2. I've added the choice to the dropdown but was unsure what you meant when you were referring to the selectgroups element. 3. I'm not sure how I managed to remove this check when I was writing the patch, but I've added it back in 4. I'm not quite sure how to tweak my logic to do this more efficiently - to me it looks like I'm doing the correct check here. 5. Along the same lines as the previous point - we are checking for the specific cases in the if-else branches you are referring to. If I switch this to use empty() instead of isset() I'm going to have to rework all the logic but the outcome will be the same (and equally as efficient as my current implementation). However, I might be missing something here. 6. I agree, probably not worth it, but if you think I should do it then I can Thanks for taking the time for that peer review -Dave
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-19670 using repository: git://github.com/gurgus/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-19670 using repository: git://github.com/gurgus/moodle.git master (1 errors / 0 warnings) [branch: MDL-19670-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Frédéric Massart added a comment -

            I meant to ping Helen Foster to get her input about the dropdown (or other form option), but I forgot. Would you mind have a look Helen?

            Show
            Frédéric Massart added a comment - I meant to ping Helen Foster to get her input about the dropdown (or other form option), but I forgot. Would you mind have a look Helen?
            Hide
            Helen Foster added a comment -

            Thanks Fred for asking for my input. I've read through all the comments, however Dave's comment "to someone untrained it's still a bit confusing" definitely applies to me!

            If the 'All participants' option is selected, does that mean everyone can see the post but nobody can reply (as currently)? If so, should this option be removed?

            Otherwise, how about having:

            All participants
            Group A
            Group B
            Group C
            ...
            All groups

            Fred, you mentioned 'other form option', however if we're allowing for the possibility of lots of groups, I don't think we want to allow multiple selections do we?

            Show
            Helen Foster added a comment - Thanks Fred for asking for my input. I've read through all the comments, however Dave's comment "to someone untrained it's still a bit confusing" definitely applies to me! If the 'All participants' option is selected, does that mean everyone can see the post but nobody can reply (as currently)? If so, should this option be removed? Otherwise, how about having: All participants Group A Group B Group C ... All groups Fred, you mentioned 'other form option', however if we're allowing for the possibility of lots of groups, I don't think we want to allow multiple selections do we?
            Hide
            Andrew Nicols added a comment -

            Hi Helen,

            I don't think that it should be removed. There's a big difference between the two functionalities here:

            • The "All participants" option in the dropdown creates a post for all participants, both those in groups, and those not in groups. It is only accessible to users with the seeallgroups cap.
            • The new functionality copies posts to each group. It does not make that post availability to people in all groups, or those not in a group at all.

            Andrew

            Show
            Andrew Nicols added a comment - Hi Helen, I don't think that it should be removed. There's a big difference between the two functionalities here: The "All participants" option in the dropdown creates a post for all participants, both those in groups, and those not in groups. It is only accessible to users with the seeallgroups cap. The new functionality copies posts to each group. It does not make that post availability to people in all groups, or those not in a group at all. Andrew
            Hide
            Helen Foster added a comment -

            Thanks Andrew for your comment. When you say that the All participants option is 'only accessible to users with the seeallgroups cap' do you mean only these users can reply to it?

            Also, I'm confused by what you mean by 'The new functionality copies posts to each group. It does not make that post availability to people in all groups...' as the two sentences seem to be contradictory? If there's a post in each group then surely people in all groups can see it and reply to it?

            Show
            Helen Foster added a comment - Thanks Andrew for your comment. When you say that the All participants option is 'only accessible to users with the seeallgroups cap' do you mean only these users can reply to it? Also, I'm confused by what you mean by 'The new functionality copies posts to each group. It does not make that post availability to people in all groups...' as the two sentences seem to be contradictory? If there's a post in each group then surely people in all groups can see it and reply to it?
            Hide
            Helen Foster added a comment -

            Linking to another current issue relating to the groups dropdown menu - MDL-48437. It's concerned with visible groups only, whereas I think that this issue concerns forums set to visible or separate groups - am I correct?

            Thanks to Andrew for chatting with me about this issue. To answer my own questions:

            The 'All participants' option is only available to users with the viewallgroups capability and only users with this capability can reply to such posts. Everyone, including users not in any group, can view such posts. Thus, it is a way for teachers to make announcements in the forum.

            The proposed new functionality enables you to copy a forum post to each of the groups you have access to - not necessarily all groups in the course.

            Show
            Helen Foster added a comment - Linking to another current issue relating to the groups dropdown menu - MDL-48437 . It's concerned with visible groups only, whereas I think that this issue concerns forums set to visible or separate groups - am I correct? Thanks to Andrew for chatting with me about this issue. To answer my own questions: The 'All participants' option is only available to users with the viewallgroups capability and only users with this capability can reply to such posts. Everyone, including users not in any group, can view such posts. Thus, it is a way for teachers to make announcements in the forum. The proposed new functionality enables you to copy a forum post to each of the groups you have access to - not necessarily all groups in the course.
            Hide
            Dave Cooper added a comment - - edited

            Ok so as per Damyon Wiese's advice, I've made a checkbox that disables the group selection if chosen. This seems reasonable and way less ambiguous than the other solutions I've seen so far. What are everyone's thoughts on this? I've attached a screenshot.

            Show
            Dave Cooper added a comment - - edited Ok so as per Damyon Wiese 's advice, I've made a checkbox that disables the group selection if chosen. This seems reasonable and way less ambiguous than the other solutions I've seen so far. What are everyone's thoughts on this? I've attached a screenshot.
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-19670 using repository: git://github.com/gurgus/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-19670 using repository: git://github.com/gurgus/moodle.git master (1 errors / 0 warnings) [branch: MDL-19670-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Frédéric Massart added a comment - - edited

            Hi Dave,

            thanks for working on this issue, I still have a few comments about the patch, here they are:

            1. You left behind an new unused lang string
            2. I find it odd that you need the permission to move discussions in order to post to all groups. Please discuss this with others. If we decide that it is not a requirement, the logic has to be changed in several places. If it is a requirement, please make sure it is documented.
            3. I would place the checkbox above the group selector because it disables it.
            4. Can you confirm that the checkbox is not displayed when you edit a discussion?
            5. My previous point 4 was about this, which can be written differently:

                      if (isset($fromform->posttoallgroups)) {
                          $groups = groups_get_all_groups($fromform->course, $fromform->userid);
                          foreach ($groups as $group) {
                              $groupstopostto[] = $group->id;
                          }
               
                          // If user has capability to access all groups but isn't enrolled in this course.
                          if (has_capability('moodle/site:accessallgroups', $modcontext)) {
                              $allgroups = groups_get_all_groups($fromform->course);
                              foreach ($allgroups as $group) {
                                  $groupstopostto[] = $group->id;
                              }
                          }
                      }
               
                      // I find the following easier to read because you don't override the array if
                      // they have more permissions
                      if (isset($fromform->posttoallgroups)) {
               
                          // If user has capability to access all groups.
                          if (has_capability('moodle/site:accessallgroups', $modcontext)) {
                              $allgroups = groups_get_all_groups($fromform->course);
                          } else {
                              $groups = groups_get_all_groups($fromform->course, $fromform->userid);
                          }
               
                          $groupstopostto = array_keys($allgroups);
                      }
              
              

            6. About my previous point 5, what I mean is that the suite of if/elseif does not check if the value is valid afterwards. The else statement will not be triggered even if something is set but not valid. Also I don't understand why there is a logic change as compared to before.
              • Previously forum_user_can_post_discussion received 0 for all participants, now it will receive 1. It was called before $fromform>groupid from 0 to -1.
              • $contextcheck is not used any more, meaning that you might find a way to post to more groups than you're allowed to.

                            if (empty($fromform->groupid) && empty($fromform->groupinfo)) {
                                // If we are posting to all groups.
                                $groupstopostto[] = -1;
                            } else if (isset($fromform->groupinfo) && empty($fromform->groupid)) {
                                // If we have selected a group from the dropdown.
                                $groupstopostto[] = $fromform->groupinfo;
                            } else if (empty($fromform->groupinfo) && isset($fromform->groupid)) {
                                // If we only have access to one group.
                                $groupstopostto[] = $fromform->groupid;
                            } else {
                                print_error('cannotcreatediscussion', 'forum');
                            }
                 
                            // Should be re-using the previous code (or a variation of it):
                 
                            // If the user has access all groups capability let them choose the group.
                            if ($contextcheck) {
                                $fromform->groupid = $fromform->groupinfo;
                            }
                            if (empty($fromform->groupid)) {
                                $fromform->groupid = -1;
                            }
                            $groupstopostto = array($fromform->groupid);
                
                

            7. Your behat steps are not enough, they should confirm that ticking the checkbox will post to all groups, not just that it is displayed or not.
            8. I had a chat with Helen about the checkbox, we both are not sure if it is the best approach but it solves the issue and so it seems good enough.
            9. I'm still on the fence about checking the blocking threshold between each write operation. A bad error would be displayed and only half the posts would be sent. Please seek others opinion.

            Thanks Dave. Please go through those comments, I might be wrong as I did not try the code myself, I just made those assumptions based on the diff and the existing code.

            Cheers!
            Fred

            Show
            Frédéric Massart added a comment - - edited Hi Dave, thanks for working on this issue, I still have a few comments about the patch, here they are: You left behind an new unused lang string I find it odd that you need the permission to move discussions in order to post to all groups. Please discuss this with others. If we decide that it is not a requirement, the logic has to be changed in several places. If it is a requirement, please make sure it is documented. I would place the checkbox above the group selector because it disables it. Can you confirm that the checkbox is not displayed when you edit a discussion? My previous point 4 was about this, which can be written differently: if (isset($fromform->posttoallgroups)) { $groups = groups_get_all_groups($fromform->course, $fromform->userid); foreach ($groups as $group) { $groupstopostto[] = $group->id; }   // If user has capability to access all groups but isn't enrolled in this course. if (has_capability('moodle/site:accessallgroups', $modcontext)) { $allgroups = groups_get_all_groups($fromform->course); foreach ($allgroups as $group) { $groupstopostto[] = $group->id; } } }   // I find the following easier to read because you don't override the array if // they have more permissions if (isset($fromform->posttoallgroups)) {   // If user has capability to access all groups. if (has_capability('moodle/site:accessallgroups', $modcontext)) { $allgroups = groups_get_all_groups($fromform->course); } else { $groups = groups_get_all_groups($fromform->course, $fromform->userid); }   $groupstopostto = array_keys($allgroups); } About my previous point 5, what I mean is that the suite of if/elseif does not check if the value is valid afterwards. The else statement will not be triggered even if something is set but not valid. Also I don't understand why there is a logic change as compared to before. Previously forum_user_can_post_discussion received 0 for all participants, now it will receive 1. It was called before $fromform >groupid from 0 to -1. $contextcheck is not used any more, meaning that you might find a way to post to more groups than you're allowed to. if (empty($fromform->groupid) && empty($fromform->groupinfo)) { // If we are posting to all groups. $groupstopostto[] = -1; } else if (isset($fromform->groupinfo) && empty($fromform->groupid)) { // If we have selected a group from the dropdown. $groupstopostto[] = $fromform->groupinfo; } else if (empty($fromform->groupinfo) && isset($fromform->groupid)) { // If we only have access to one group. $groupstopostto[] = $fromform->groupid; } else { print_error('cannotcreatediscussion', 'forum'); }   // Should be re-using the previous code (or a variation of it):   // If the user has access all groups capability let them choose the group. if ($contextcheck) { $fromform->groupid = $fromform->groupinfo; } if (empty($fromform->groupid)) { $fromform->groupid = -1; } $groupstopostto = array($fromform->groupid); Your behat steps are not enough, they should confirm that ticking the checkbox will post to all groups, not just that it is displayed or not. I had a chat with Helen about the checkbox, we both are not sure if it is the best approach but it solves the issue and so it seems good enough. I'm still on the fence about checking the blocking threshold between each write operation. A bad error would be displayed and only half the posts would be sent. Please seek others opinion. Thanks Dave. Please go through those comments, I might be wrong as I did not try the code myself, I just made those assumptions based on the diff and the existing code. Cheers! Fred

              People

              • Votes:
                29 Vote for this issue
                Watchers:
                26 Start watching this issue

                Dates

                • Created:
                  Updated:

                  Agile