Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-33022

editing a forum post set to show to all participants in group mode causes an orphan post

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.4, 2.3.1, 2.4
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Forum
    • Labels:
    • Testing Instructions:
      Hide

      set your course to use seperate groups
      create some groups in your course
      Create a forum set to use seperate groups
      create a new post in the forum set to show to all participants - save.
      login as a user that is in one of the groups created earlier - check to make sure they can see the post
      log back in as teacher/admin and edit the existing post - make sure all participants is still selected
      login as a user that is in one of the groups created earlier - check to make sure they can see the post - they can't because the groupid in the mdl_forum_discussions record has changed to 0 instead of -1

      Show
      set your course to use seperate groups create some groups in your course Create a forum set to use seperate groups create a new post in the forum set to show to all participants - save. login as a user that is in one of the groups created earlier - check to make sure they can see the post log back in as teacher/admin and edit the existing post - make sure all participants is still selected login as a user that is in one of the groups created earlier - check to make sure they can see the post - they can't because the groupid in the mdl_forum_discussions record has changed to 0 instead of -1
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-33022-master

      Description

      If you edit an existing forum post set to display to all participants inside a forum set to seperate groups
      after editing the post it becomes hidden to all participants - it seems on re-editing the post, the "groupid" flag in the mdl_forum_discussions is set to '0' instead of the original '-1' which it should be set to.

        Gliffy Diagrams

          Activity

          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi Dan,

          I've just been looking at this now. Certainly your code works and fixes the issue.
          But what I can't help but think is that it may be a better idea to change the all participants value in the select box from 0 to -1 rather than dealing with the 0 which may lead to regressions in the future?

          Anyway totally up to you, feel free to put it up for integration when you are ready

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi Dan, I've just been looking at this now. Certainly your code works and fixes the issue. But what I can't help but think is that it may be a better idea to change the all participants value in the select box from 0 to -1 rather than dealing with the 0 which may lead to regressions in the future? Anyway totally up to you, feel free to put it up for integration when you are ready Cheers Sam
          Hide
          danmarsden Dan Marsden added a comment -

          yeah - I wasn't sure why the participants value was set to 0 in the drop list either - I didn't look closely enough at the code to see if it would be simple to change it - will push this for integration but I'm happy if the integrator wants to reject and change to using -1 themselves

          Show
          danmarsden Dan Marsden added a comment - yeah - I wasn't sure why the participants value was set to 0 in the drop list either - I didn't look closely enough at the code to see if it would be simple to change it - will push this for integration but I'm happy if the integrator wants to reject and change to using -1 themselves
          Hide
          danmarsden Dan Marsden added a comment -

          rebased against latest code and pushing for integration - thanks for the review.

          Show
          danmarsden Dan Marsden added a comment - rebased against latest code and pushing for integration - thanks for the review.
          Hide
          poltawski Dan Poltawski 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
          poltawski Dan Poltawski 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
          poltawski Dan Poltawski added a comment -

          Sorry Dan, but I think we need to think about the -1 instead, since this is explicitly set to 0 in the form:

          $groupinfo = array('0' => get_string('allparticipants'));

          I haven't look deeply enough to work out where its being set -1 initially, but this does seem a bit confusing and worth looking deeper.

          Show
          poltawski Dan Poltawski added a comment - Sorry Dan, but I think we need to think about the -1 instead, since this is explicitly set to 0 in the form: $groupinfo = array('0' => get_string('allparticipants')); I haven't look deeply enough to work out where its being set -1 initially, but this does seem a bit confusing and worth looking deeper.
          Hide
          danmarsden Dan Marsden added a comment -

          hmmm - Disagree...

          agree that someone should look at the -1/0 issue... but.... That will take some time that I don't have and this bug is pretty critical for forums in groups mode causing orphaned posts and teachers are unaware of this expecting the post is available to their students.

          If you want to fix the -1/0 confusion I'd suggest logging that as a seperate issue for Moodle HQ to look at.

          Show
          danmarsden Dan Marsden added a comment - hmmm - Disagree... agree that someone should look at the -1/0 issue... but.... That will take some time that I don't have and this bug is pretty critical for forums in groups mode causing orphaned posts and teachers are unaware of this expecting the post is available to their students. If you want to fix the -1/0 confusion I'd suggest logging that as a seperate issue for Moodle HQ to look at.
          Hide
          poltawski Dan Poltawski added a comment -

          Well, I appreciate that you are time constrained, the time you have spent on it so far and that sometimes we introduce bureaucracy into the process, which is frustrating.

          But I think from a technical viewpoint it is worth looking deeper to ensure that we are fixing it in the right place. If we always take a 'shallow view' of looking at each bug then we end up causing more regressions than we solve. I'm not saying that is the case here, but you are saying to us 'take this or leave it because I dont have any time to do any more on it'. I'm saying that from a moodle core point of view we should spend a little more time looking at it to make sure its fixed in the right place.

          Show
          poltawski Dan Poltawski added a comment - Well, I appreciate that you are time constrained, the time you have spent on it so far and that sometimes we introduce bureaucracy into the process, which is frustrating. But I think from a technical viewpoint it is worth looking deeper to ensure that we are fixing it in the right place. If we always take a 'shallow view' of looking at each bug then we end up causing more regressions than we solve. I'm not saying that is the case here, but you are saying to us 'take this or leave it because I dont have any time to do any more on it'. I'm saying that from a moodle core point of view we should spend a little more time looking at it to make sure its fixed in the right place.
          Hide
          danmarsden Dan Marsden added a comment -

          I'm happy for someone to reject if they want to go through the effort of improving the patch themselves... but rejecting a valid patch that causes the code to act consistently as designed and suggesting a rewrite of code in a module that is short-lived (possibly replaced in 2.4) seems like taking bureaucracy over the top to me. I don't like my chances of anyone else picking this up up unless you're volunteering to do it?

          de-allocating myself from this one - if someone wants to pick it up and rewrite the way groups are managed in core - feel free.

          Show
          danmarsden Dan Marsden added a comment - I'm happy for someone to reject if they want to go through the effort of improving the patch themselves... but rejecting a valid patch that causes the code to act consistently as designed and suggesting a rewrite of code in a module that is short-lived (possibly replaced in 2.4) seems like taking bureaucracy over the top to me. I don't like my chances of anyone else picking this up up unless you're volunteering to do it? de-allocating myself from this one - if someone wants to pick it up and rewrite the way groups are managed in core - feel free.
          Hide
          poltawski Dan Poltawski added a comment -

          Dan, I wasn't suggesting that you rewrite the way that the groups are implemented, at all.

          Just investigate if we could switch to using -1 in the line in mod forum which is setting to 0.

          But I will pick this up, yes.

          Show
          poltawski Dan Poltawski added a comment - Dan, I wasn't suggesting that you rewrite the way that the groups are implemented, at all. Just investigate if we could switch to using -1 in the line in mod forum which is setting to 0. But I will pick this up, yes.
          Hide
          cibot CiBoT added a comment -

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

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

          So i've succeeded in slowing this process down here.. . and we are doing the same thing here:

           
             // 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;
                  }

          So going rebase dans patch for integration.

          Show
          poltawski Dan Poltawski added a comment - So i've succeeded in slowing this process down here.. . and we are doing the same thing here:   // 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; } So going rebase dans patch for integration.
          Hide
          danmarsden Dan Marsden added a comment -

          hehe - thanks Dan!

          Show
          danmarsden Dan Marsden added a comment - hehe - thanks Dan!
          Hide
          poltawski Dan Poltawski 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
          poltawski Dan Poltawski 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
          nebgor Aparup Banerjee added a comment -

          Thanks for polishing this up carefully . Integrated into 22, 23 and master and up for testing.

          Show
          nebgor Aparup Banerjee added a comment - Thanks for polishing this up carefully . Integrated into 22, 23 and master and up for testing.
          Hide
          andyjdavis Andrew Davis added a comment -

          The testing instructions are reproduction instructions instead of testing instructions. tisk tisk.

          I can guess whats meant to happen. Passing.

          Show
          andyjdavis Andrew Davis added a comment - The testing instructions are reproduction instructions instead of testing instructions. tisk tisk. I can guess whats meant to happen. Passing.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Fixed STOP Closed STOP Thanks STOP

          Yay, imagination! Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Fixed STOP Closed STOP Thanks STOP Yay, imagination! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Sep/12