Moodle
  1. Moodle
  2. MDL-19147

When moving discussions Single Simple forums should not be available to move to - discussion is orphaned if you do

    Details

    • Testing Instructions:
      Hide

      Log in as an Administrator or teacher.
      Create two forums. Made sure that one forum is normal and the second is a simple discussion.
      Add a post to the normal forum.
      TEST : view your post and look at the drop down box for "Move this discussion to ...". Make sure that your simple discussion is not included in the drop down menu.

      Show
      Log in as an Administrator or teacher. Create two forums. Made sure that one forum is normal and the second is a simple discussion. Add a post to the normal forum. TEST : view your post and look at the drop down box for "Move this discussion to ...". Make sure that your simple discussion is not included in the drop down menu.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-19147-master
    • Rank:
      2386

      Description

      To reproduce...

      • Create a forum - single simple discussion.
      • Create another, standard, forum..
      • Create a thread in the standard forum
      • Move the thread to the single simple discussion.
      • Go back to the course page and display the single simple discussion
      • The moved discussion is lost!

      Seems to me that the single simple discussion should not appear in the 'move' list.

        Issue Links

          Activity

          Hide
          Howard Miller added a comment -

          What about this, just checks that the forums in the list are not the 'single' type.

          Index: discuss.php
          ===================================================================
          RCS file: /cvsroot/moodle/moodle/mod/forum/discuss.php,v
          retrieving revision 1.112.2.13
          diff -u -r1.112.2.13 discuss.php
          — discuss.php 28 Jan 2009 13:11:22 -0000 1.112.2.13
          +++ discuss.php 11 May 2009 13:01:17 -0000
          @@ -188,6 +188,10 @@
          $section = -1;
          $forummenu = array();
          foreach ($modinfo->instances['forum'] as $forumcm) {
          + $foruminstance = get_record('forum','id',$forumcm->instance );
          + if ($foruminstance->type == 'single')

          { + continue; + }

          if (!$forumcm->uservisible || !has_capability('mod/forum:startdiscussion',
          get_context_instance(CONTEXT_MODULE,$forumcm->id))) {
          continue;

          Show
          Howard Miller added a comment - What about this, just checks that the forums in the list are not the 'single' type. Index: discuss.php =================================================================== RCS file: /cvsroot/moodle/moodle/mod/forum/discuss.php,v retrieving revision 1.112.2.13 diff -u -r1.112.2.13 discuss.php — discuss.php 28 Jan 2009 13:11:22 -0000 1.112.2.13 +++ discuss.php 11 May 2009 13:01:17 -0000 @@ -188,6 +188,10 @@ $section = -1; $forummenu = array(); foreach ($modinfo->instances ['forum'] as $forumcm) { + $foruminstance = get_record('forum','id',$forumcm->instance ); + if ($foruminstance->type == 'single') { + continue; + } if (!$forumcm->uservisible || !has_capability('mod/forum:startdiscussion', get_context_instance(CONTEXT_MODULE,$forumcm->id))) { continue;
          Hide
          Howard Miller added a comment -

          Didn't work too well, attaching diff as file. (for mod/forum/discuss.php of course)

          Show
          Howard Miller added a comment - Didn't work too well, attaching diff as file. (for mod/forum/discuss.php of course)
          Hide
          Charles Fulton added a comment -

          This issue is still present in 2.2 and was discovered in MDLQA-1191. Here's a fix for 2.2: https://github.com/mackensen/moodle/compare/master...MDL-19147.

          Show
          Charles Fulton added a comment - This issue is still present in 2.2 and was discovered in MDLQA-1191 . Here's a fix for 2.2: https://github.com/mackensen/moodle/compare/master...MDL-19147 .
          Hide
          Michael de Raadt added a comment -

          Thanks for linking this to the QA test and providing a fix. Top work!

          Show
          Michael de Raadt added a comment - Thanks for linking this to the QA test and providing a fix. Top work!
          Hide
          moodle.com added a comment -

          Special commendation to Charles.

          Thanks for your help in getting these problems fixed. You are getting to these issues quicker than we are. Kudos.

          Show
          moodle.com added a comment - Special commendation to Charles. Thanks for your help in getting these problems fixed. You are getting to these issues quicker than we are. Kudos.
          Hide
          Adrian Greeve added a comment -

          A slight alteration made to fix 1.9

          Show
          Adrian Greeve added a comment - A slight alteration made to fix 1.9
          Hide
          Ankit Agarwal added a comment -

          Hi Guys,
          The patch is looking good and works perfectly.
          But in addition to this patch, since we don't allow discussion to be moved to a single forum, there is no point displaying single forums as options in the drop down.(something that Howard was suggesting).
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Guys, The patch is looking good and works perfectly. But in addition to this patch, since we don't allow discussion to be moved to a single forum, there is no point displaying single forums as options in the drop down.(something that Howard was suggesting). Thanks
          Hide
          Adrian Greeve added a comment -

          I have now changed the patch so that it removes simple discussions from the pull down menu for moving posts to different forums. The original code is still included in the patch, but I think that it is now redundant and could be removed (mod/forum/discuss.php lines 88 - 91) and (all of mod/forum/lang/en/forum.php).

          Show
          Adrian Greeve added a comment - I have now changed the patch so that it removes simple discussions from the pull down menu for moving posts to different forums. The original code is still included in the patch, but I think that it is now redundant and could be removed (mod/forum/discuss.php lines 88 - 91) and (all of mod/forum/lang/en/forum.php).
          Hide
          Charles Fulton added a comment -

          It's not necessarily redundant--there could be some strange edge case where someone did something manually or forum ids got mixed up. I think it makes sense to prevent it both via the interface and in the actual code.

          Show
          Charles Fulton added a comment - It's not necessarily redundant--there could be some strange edge case where someone did something manually or forum ids got mixed up. I think it makes sense to prevent it both via the interface and in the actual code.
          Hide
          Ankit Agarwal added a comment -

          Hi,
          Adrian looking good now.
          re:redundant : I would agree with Fulton on that. Its not enough to disable something just from the UI.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi, Adrian looking good now. re:redundant : I would agree with Fulton on that. Its not enough to disable something just from the UI. Thanks
          Hide
          Adrian Greeve added a comment - - edited

          I've changed one of my comments to be a bit more concise and I've left the original patch intact. If everything is okay I can cherry pick the other branches and submit for integration.

          Show
          Adrian Greeve added a comment - - edited I've changed one of my comments to be a bit more concise and I've left the original patch intact. If everything is okay I can cherry pick the other branches and submit for integration.
          Hide
          Adrian Greeve added a comment -

          I've added the other branches, submitting for integration.

          Show
          Adrian Greeve added a comment - I've added the other branches, submitting for integration.
          Hide
          Sam Hemelryk added a comment -

          Thanks guys - this has been integrated now

          Was cherry-picked to 22 stable as well.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks guys - this has been integrated now Was cherry-picked to 22 stable as well. Cheers Sam
          Hide
          Ankit Agarwal added a comment -

          Everything running smoothly on all 5 branches
          Thanks

          Show
          Ankit Agarwal added a comment - Everything running smoothly on all 5 branches Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, you did it!

          Now your code is part of the best weeklies released ever, many thanks!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, you did it! Now your code is part of the best weeklies released ever, many thanks! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: