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 Bug
    • Status: Closed
    • Priority: Major 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
    • Rank:
      40198

      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.

        Activity

        Hide
        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
        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
        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
        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
        Dan Marsden added a comment -

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

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

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

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        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
        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
        Dan Marsden added a comment -

        hehe - thanks Dan!

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

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

        Show
        Aparup Banerjee added a comment - Thanks for polishing this up carefully . Integrated into 22, 23 and master and up for testing.
        Hide
        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
        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
        Eloy Lafuente (stronk7) added a comment -

        Fixed STOP Closed STOP Thanks STOP

        Yay, imagination! Ciao

        Show
        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: