Moodle
  1. Moodle
  2. MDL-30421

Editing a normal forum to become a simple discussion creates errors

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.0.7, 2.1.4
    • Component/s: Forum
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide
      1. create a general discussion forum (Forum 1)
      2. post atleast 3 discussion in the forum
      3. Create another general discussion forum (Forum 2)
      4. post only 1 thread in Forum 2
      5. Create another general discussion forum (Forum 3)
      6. Donot post any thread in this forum
      7. Now goto edit settings of each forum and convert them to "simple single discussion forum".
      8. Again goto edit settings of each forum and convert them to "standard forum for general use".
      9. Goto Forum 1 and change setting back to "simple single discussion forum".
      10. Try creating a new "Simple single discussion forum"
      11. No error should be generated anywhere during the whole process.
      Show
      create a general discussion forum (Forum 1) post atleast 3 discussion in the forum Create another general discussion forum (Forum 2) post only 1 thread in Forum 2 Create another general discussion forum (Forum 3) Donot post any thread in this forum Now goto edit settings of each forum and convert them to "simple single discussion forum". Again goto edit settings of each forum and convert them to "standard forum for general use". Goto Forum 1 and change setting back to "simple single discussion forum". Try creating a new "Simple single discussion forum" No error should be generated anywhere during the whole process.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      wip-mdl-30421
    • Rank:
      33057

      Description

      If you take a normal forum that has more than one post in it and then edit it to become a simple discussion, it will generate the following error:

      Error: mdb->get_record() found more than one record!
      
          line 1309 of /lib/dml/moodle_database.php: call to debugging()
          line 1269 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
          line 1249 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
          line 156 of /mod/forum/lib.php: call to moodle_database->get_record()
          line 363 of /course/modedit.php: call to forum_update_instance()
      

      From that point on access to the forum will display similar messages.

        Issue Links

          Activity

          Hide
          Ankit Agarwal added a comment -

          This issue can be resolved by removing the option "single discussion forum" from drop down when the current forum has more than 1 discussion.
          That should be enough to resolve this issue as already we have a check in place during the code flow for this in forum_update_instance().
          I have left the logic in forum_update_instance() unaltered atm.

          Show
          Ankit Agarwal added a comment - This issue can be resolved by removing the option "single discussion forum" from drop down when the current forum has more than 1 discussion. That should be enough to resolve this issue as already we have a check in place during the code flow for this in forum_update_instance(). I have left the logic in forum_update_instance() unaltered atm.
          Hide
          Jason Fowler added a comment -

          Code looks good to me, does this need to be back-ported too
          ?

          Show
          Jason Fowler added a comment - Code looks good to me, does this need to be back-ported too ?
          Hide
          Ankit Agarwal added a comment -

          Hi Jason,
          Thanks for the review.
          I have back ported all changes to STABLE now.
          submitting for integration
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Jason, Thanks for the review. I have back ported all changes to STABLE now. submitting for integration Thanks
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I had a quick look at the changes you've made and certainly they work 100% however I think there are changes required and you need to double check your approach.

          What caught my eye at first was the changes made to forum_get_forum_types. I understand why it was done however I think it is subverting the purpose of that method to remove that option and then add it on the fly when appropriate.
          Instead I think that option should be left as part of the return from that method and instead removed from the result if appropriate (this is also good for backwards compatibility in case someone else out there is using that function for something).
          The next thing that got me was that you were using using definition_after_data so that you could collect value of the instance field to use in database queries.
          Did you know that moodleform_mod has an _instance property that you can access? This is likely a much safer option than using the value within the form's element and certainly more direct.
          The final thing that caught my eye is that now in forum_update_instance there is a call to count records, then if there is more or less than 1 discussion a second call to get the discussions sorted my time-modified. Certainly the count could be avoided and the results of the get_records limited to 2 or something from which if there is two we use to first. Anyway avoidable query here.

          So what I think should change:

          1. Let forum_get_forum_types return single still
          2. In definition check if instance is set and if it set count the discussions, if more than one unset('single') from the result of forum_get_forum_types.
          3. Remove the code from definition_after_date (its now in the definition which is a better place)
          4. Tidy up the queries in forum_update_instance.

          Now there is one final thing to note here, technically the code deals with converting from a normal forum with several posts to a single forum. The problem was just that get_record was used where potentially there were more than one record.
          So this is in some ways a regression as much as it is a bug fix.
          Personally I don't like the idea of losing information so I'd rather the approach you've taken (with the above fixes) but still I think you should check with Martin who is our forum expert and get his opinion.
          Should you be able to convert from a normal forum to a single forum losing all but the most recently updated discussion?
          Perhaps for stable branches you just fix the use of the query, and for master you refactor it with the changes you have presently.
          Anyway seek Martin's opinion.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I had a quick look at the changes you've made and certainly they work 100% however I think there are changes required and you need to double check your approach. What caught my eye at first was the changes made to forum_get_forum_types. I understand why it was done however I think it is subverting the purpose of that method to remove that option and then add it on the fly when appropriate. Instead I think that option should be left as part of the return from that method and instead removed from the result if appropriate (this is also good for backwards compatibility in case someone else out there is using that function for something). The next thing that got me was that you were using using definition_after_data so that you could collect value of the instance field to use in database queries. Did you know that moodleform_mod has an _instance property that you can access? This is likely a much safer option than using the value within the form's element and certainly more direct. The final thing that caught my eye is that now in forum_update_instance there is a call to count records, then if there is more or less than 1 discussion a second call to get the discussions sorted my time-modified. Certainly the count could be avoided and the results of the get_records limited to 2 or something from which if there is two we use to first. Anyway avoidable query here. So what I think should change: Let forum_get_forum_types return single still In definition check if instance is set and if it set count the discussions, if more than one unset('single') from the result of forum_get_forum_types. Remove the code from definition_after_date (its now in the definition which is a better place) Tidy up the queries in forum_update_instance. Now there is one final thing to note here, technically the code deals with converting from a normal forum with several posts to a single forum. The problem was just that get_record was used where potentially there were more than one record. So this is in some ways a regression as much as it is a bug fix. Personally I don't like the idea of losing information so I'd rather the approach you've taken (with the above fixes) but still I think you should check with Martin who is our forum expert and get his opinion. Should you be able to convert from a normal forum to a single forum losing all but the most recently updated discussion? Perhaps for stable branches you just fix the use of the query, and for master you refactor it with the changes you have presently. Anyway seek Martin's opinion. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Hello Martin and Helen,

          Can you please provide some inputs on Sam's concern:

          Should you be able to convert from a normal forum to a single forum losing all but the most recently updated discussion?
          
          Show
          Rajesh Taneja added a comment - Hello Martin and Helen, Can you please provide some inputs on Sam's concern: Should you be able to convert from a normal forum to a single forum losing all but the most recently updated discussion?
          Hide
          Rajesh Taneja added a comment -

          There are 3 commits:

          1. Actually solves the issue.
          2. Post and discussions should not get updated with forum title and description. As per my understanding that should not happen, hence this commit solves that issue.
          3. Redundant db queries removed. Not sure, please check if I missed something while removing this query.

          As per my understanding of this issue, user should be able to change forum type, from General to single simple discussion. As, we show warning message to notify user about this, it might not be right to remove single discussion option for a standard forum type.

          Warning! There is more than one discussion in this forum - using the most recent
          

          FYI:
          will update testing instructions after it passes through integration review.

          Show
          Rajesh Taneja added a comment - There are 3 commits: Actually solves the issue. Post and discussions should not get updated with forum title and description. As per my understanding that should not happen, hence this commit solves that issue. Redundant db queries removed. Not sure, please check if I missed something while removing this query. As per my understanding of this issue, user should be able to change forum type, from General to single simple discussion. As, we show warning message to notify user about this, it might not be right to remove single discussion option for a standard forum type. Warning! There is more than one discussion in this forum - using the most recent FYI: will update testing instructions after it passes through integration review.
          Hide
          Jason Fowler added a comment -

          Code looks great, and works well.

          I suggest raising another issue to handle the display of the warning, so only teachers+ get to see it, so it isn't shown to users who have no control over the situation

          Show
          Jason Fowler added a comment - Code looks great, and works well. I suggest raising another issue to handle the display of the warning, so only teachers+ get to see it, so it isn't shown to users who have no control over the situation
          Hide
          Helen Foster added a comment -

          I can't ever imagine a teacher deliberately wanting to change a standard forum containing several discussions to a single simple discussion forum. Surely they would just create a new single simple discussion forum?

          Is it possible to just prevent it happening i.e. prevent the edit forum settings form from being saved and instead display a message (like when trying to save a form when not all the required fields are filled in)?

          Show
          Helen Foster added a comment - I can't ever imagine a teacher deliberately wanting to change a standard forum containing several discussions to a single simple discussion forum. Surely they would just create a new single simple discussion forum? Is it possible to just prevent it happening i.e. prevent the edit forum settings form from being saved and instead display a message (like when trying to save a form when not all the required fields are filled in)?
          Hide
          Rajesh Taneja added a comment -

          Yes Helen, it is possible to hide the setting and restrict user to change discussion type.
          But saying so, we restrict user to change the forum view type. Existing forum implementation allow user to change forum type and show warning message:

          Warning! There is more than one discussion in this forum - using the most recent

          if there is more then one discussions in forum.

          FYI:

          1. Nothing gets deleted in forum when forum type is changed.
          2. Existing implementation works fine if there is one discussion and user change forum type from standard to single.
          3. Bug appears (because of coding error), only if forum has multiple discussions.
          Show
          Rajesh Taneja added a comment - Yes Helen, it is possible to hide the setting and restrict user to change discussion type. But saying so, we restrict user to change the forum view type. Existing forum implementation allow user to change forum type and show warning message: Warning! There is more than one discussion in this forum - using the most recent if there is more then one discussions in forum. FYI: Nothing gets deleted in forum when forum type is changed. Existing implementation works fine if there is one discussion and user change forum type from standard to single. Bug appears (because of coding error), only if forum has multiple discussions.
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          Sending this back again sorry.
          Because of the additional cleanup there is essentially a change in functionality, one which I see as a regression.

          You can test it out in the following way...

          1/ Without the patch:

          • Log in as an admin
          • Enter a course and create a new single forum with the name and description `Test 1`.
          • Save and display. Note the title and description of the discussion.
          • Edit the forum and change the name and description to `Test 2`.
          • Save and display. Note the title and description of the discussion has been changed as well.

          2/ With the patch applied

          • Repeat the above steps.
          • Notice when you change the forum name and description it no longer changes the discussion.

          What is happening, or meant to happen (as there is a bug I will describe later on) is that the Forum name and description should be the first discussion. In reality it is not we still require a discussion and post which is why we copy the information about.
          I think maintaining that functionality is important.

          The bug I mentioned above is that the editing should work in the reverse direction as well.
          Try this after the first set of instructions (without the patch).

          • Edit the discussion and set the title and description to `Test 3`
          • Save it and then browse to edit the forum
          • Notice that the forums description has been changed but the title has not.

          Pretty sure that is a bug, however don't solve it now as it may be intentional.

          Either way I think this needs more work, you need to be careful with the optimisations you are making after your changes.
          Looking at your three commits the first commit 35b81f27ca62ebc12ab5b5f5324da86f657428d6 is pretty spot on by the looks.

          I think 4 things should happen:

          1. Verify the first commit does fix the issue, strip the branch back to that, and put it up for integration. (Or let me know you're happy with it and I'll review just that and cherry-pick it in if I'm happy with it).
          2. Find out if this needs to be back ported (I'm betting yes but needs to be verified)
          3. Create a new issue, an improvement to stop people from being able to convert from a forum with multiple discussions to a single forum.
          4. Create a new issue, a bug, to research whether when you edit the discussion in a single forum the forum title should be updated as well. I bet yes, but needs proper investigation.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, Sending this back again sorry. Because of the additional cleanup there is essentially a change in functionality, one which I see as a regression. You can test it out in the following way... 1/ Without the patch: Log in as an admin Enter a course and create a new single forum with the name and description `Test 1`. Save and display. Note the title and description of the discussion. Edit the forum and change the name and description to `Test 2`. Save and display. Note the title and description of the discussion has been changed as well. 2/ With the patch applied Repeat the above steps. Notice when you change the forum name and description it no longer changes the discussion. What is happening, or meant to happen (as there is a bug I will describe later on) is that the Forum name and description should be the first discussion. In reality it is not we still require a discussion and post which is why we copy the information about. I think maintaining that functionality is important. The bug I mentioned above is that the editing should work in the reverse direction as well. Try this after the first set of instructions (without the patch). Edit the discussion and set the title and description to `Test 3` Save it and then browse to edit the forum Notice that the forums description has been changed but the title has not. Pretty sure that is a bug, however don't solve it now as it may be intentional. Either way I think this needs more work, you need to be careful with the optimisations you are making after your changes. Looking at your three commits the first commit 35b81f27ca62ebc12ab5b5f5324da86f657428d6 is pretty spot on by the looks. I think 4 things should happen: Verify the first commit does fix the issue, strip the branch back to that, and put it up for integration. (Or let me know you're happy with it and I'll review just that and cherry-pick it in if I'm happy with it). Find out if this needs to be back ported (I'm betting yes but needs to be verified) Create a new issue, an improvement to stop people from being able to convert from a forum with multiple discussions to a single forum. Create a new issue, a bug, to research whether when you edit the discussion in a single forum the forum title should be updated as well. I bet yes, but needs proper investigation. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Sam
          I am aware of the bug (you mentioned). I introduced it intentionally. It was not making sense to me, that if you change forum title or description then discussion was getting changed. As, we have separate settings for discussion and forum, we should not change it by default.
          Anyways, you are right this should have gone as separate issue and should not be solved as part of this.

          As per my understanding, first commit is spot-on. It will be very nice, if you can please review it (Stripped last two commits.)
          Yes, this needs to be backported to previous branches.

          Will create two separate bugs to handle title and conversion

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Sam I am aware of the bug (you mentioned). I introduced it intentionally. It was not making sense to me, that if you change forum title or description then discussion was getting changed. As, we have separate settings for discussion and forum, we should not change it by default. Anyways, you are right this should have gone as separate issue and should not be solved as part of this. As per my understanding, first commit is spot-on. It will be very nice, if you can please review it (Stripped last two commits.) Yes, this needs to be backported to previous branches. Will create two separate bugs to handle title and conversion
          Hide
          Sam Hemelryk added a comment -

          Thanks Raj - this has been integrated now

          Please create those new issues when you get a chance.

          Show
          Sam Hemelryk added a comment - Thanks Raj - this has been integrated now Please create those new issues when you get a chance.
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam
          Issues created.

          Show
          Rajesh Taneja added a comment - Thanks Sam Issues created.
          Hide
          Ankit Agarwal added a comment -

          all good in here
          passing
          Thanks

          Show
          Ankit Agarwal added a comment - all good in here passing Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sent upstream! Just in time for Moodle 2.2rc1 (if related), yay!

          Closing and big thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Sent upstream! Just in time for Moodle 2.2rc1 (if related), yay! Closing and big thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: