Details

    • Type: Bug
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.9, 2.3, 2.7.3, 2.8, 2.9
    • Fix Version/s: DEV backlog
    • Component/s: Forum
    • Labels:
    • Testing Instructions:
      Hide

      Note that to test this properly you need a database with some orphaned posts. Follow these steps with a 2.3 instance without this fix applied.

      1. Revert MDL-31664.
      2. Create a forum.
      3. Create a discussion (post A) as a user.
      4. Post a reply (post B) as another user.
      5. Post another reply (post C) to Post B.
      6. Post a separate reply (post D) to Post A.
      7. Delete Post B.
      8. Verify that Post C and Post D are still in the database.

      Now apply this fix to upgrade mod/forum. Run the upgrade. Post C should be deleted from the database. Posts A and D should still be there.

      Show
      Note that to test this properly you need a database with some orphaned posts. Follow these steps with a 2.3 instance without this fix applied. 1. Revert MDL-31664 . 2. Create a forum. 3. Create a discussion (post A) as a user. 4. Post a reply (post B) as another user. 5. Post another reply (post C) to Post B. 6. Post a separate reply (post D) to Post A. 7. Delete Post B. 8. Verify that Post C and Post D are still in the database. Now apply this fix to upgrade mod/forum. Run the upgrade. Post C should be deleted from the database. Posts A and D should still be there.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_23_STABLE, MOODLE_27_STABLE, MOODLE_28_STABLE, MOODLE_29_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-13516-master

      Description

      posts without parents

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              skodak Petr Skoda created issue -
              skodak Petr Skoda made changes -
              Field Original Value New Value
              Fix Version/s 2.0 [ 10122 ]
              Hide
              skodak Petr Skoda added a comment -

              reassigning to HQ

              Show
              skodak Petr Skoda added a comment - reassigning to HQ
              skodak Petr Skoda made changes -
              Assignee Petr ?koda (skodak) [ skodak ] moodle.com [ moodle.com ]
              dougiamas Martin Dougiamas made changes -
              Fix Version/s 2.0.1 [ 10420 ]
              Fix Version/s 2.0 [ 10122 ]
              dougiamas Martin Dougiamas made changes -
              Workflow jira [ 24953 ] MDL Workflow [ 43054 ]
              dougiamas Martin Dougiamas made changes -
              Fix Version/s 2.0.2 [ 10421 ]
              Fix Version/s 2.0.1 [ 10420 ]
              dougiamas Martin Dougiamas made changes -
              Fix Version/s 2.0.3 [ 10537 ]
              Fix Version/s 2.0.2 [ 10421 ]
              dougiamas Martin Dougiamas made changes -
              Workflow MDL Workflow [ 43054 ] MDL Full Workflow [ 71456 ]
              stronk7 Eloy Lafuente (stronk7) made changes -
              Fix Version/s 2.0.4 [ 10652 ]
              Fix Version/s 2.0.3 [ 10537 ]
              dougiamas Martin Dougiamas made changes -
              Fix Version/s 2.0.5 [ 10950 ]
              Fix Version/s 2.0.4 [ 10652 ]
              stronk7 Eloy Lafuente (stronk7) made changes -
              Fix Version/s 2.0.6 [ 11250 ]
              Fix Version/s 2.0.5 [ 10950 ]
              stronk7 Eloy Lafuente (stronk7) made changes -
              Fix Version/s 2.0.7 [ 11451 ]
              Fix Version/s 2.0.6 [ 11250 ]
              stronk7 Eloy Lafuente (stronk7) made changes -
              Fix Version/s 2.0.8 [ 11554 ]
              Fix Version/s 2.0.7 [ 11451 ]
              stronk7 Eloy Lafuente (stronk7) made changes -
              Fix Version/s 2.0.9 [ 12051 ]
              Fix Version/s 2.0.8 [ 11554 ]
              cfulton Charles Fulton made changes -
              Link This issue has been marked as being related by MDL-31664 [ MDL-31664 ]
              Hide
              skodak Petr Skoda added a comment -

              This should be done in forum/db/upgrade.php, it might be enough to do it in master only because sooner or later everybody will upgrade...

              Show
              skodak Petr Skoda added a comment - This should be done in forum/db/upgrade.php, it might be enough to do it in master only because sooner or later everybody will upgrade...
              skodak Petr Skoda made changes -
              Fix Version/s DEV backlog [ 10464 ]
              Fix Version/s 2.0.9 [ 12051 ]
              Labels triaged
              cfulton Charles Fulton made changes -
              Pull Master Diff URL https://github.com/mackensen/moodle/compare/master...MDL-13516-master
              Pull Master Branch MDL-13516-master
              Testing Instructions Note that to test this properly you need a database with some orphaned posts. Follow these steps with 2.3 instance without this fix applied.

              1. Revert MDL-13516.
              2. Create a forum.
              3. Create a discussion (post A) as a user.
              4. Post a reply (post B) as another user.
              5. Post another reply (post C) to Post B.
              6. Post a separate reply (post D) to Post A.
              7. Delete Post B.
              8. Verify that Post C and Post D are still in the database.

              Now apply this fix to upgrade mod/forum. Run the upgrade. Post C should be deleted from the database. Posts A and D should still be there.
              Pull from Repository https://github.com/mackensen/moodle
              Labels triaged patch triaged
              Assignee moodle.com [ moodle.com ] Charles Fulton [ cfulton ]
              Affects Version/s 2.3 [ 10657 ]
              cfulton Charles Fulton made changes -
              Status Open [ 1 ] Waiting for peer review [ 10012 ]
              ankit_frenz Ankit Agarwal made changes -
              Original Estimate 0 minutes [ 0 ]
              Remaining Estimate 0 minutes [ 0 ]
              Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
              Peer reviewer ankit_frenz
              Hide
              skodak Petr Skoda added a comment -

              hi, I think the patch is not going to perform well:

              • NOT IN should be probably replaced by LEFT JOIN with NULL condition
              • it might hit memory problems or query limits (oracle), I would recommend using recordsets
              • who is going to delete attachments and embedded images?
              Show
              skodak Petr Skoda added a comment - hi, I think the patch is not going to perform well: NOT IN should be probably replaced by LEFT JOIN with NULL condition it might hit memory problems or query limits (oracle), I would recommend using recordsets who is going to delete attachments and embedded images?
              ankit_frenz Ankit Agarwal made changes -
              Comment [ Hi Charles,
              Code looks good to me.
              delete_record_list takes care of the case when $ids is empty. still it might be better to do :-
              {code}
              if(!empty($ids)) {
              $DB->delete_records_list('forum_posts', 'id', array_keys($ids));
              }
              {code}
              But feel free to ignore that and submit this for integration.(let me know if you want me to submit it for integration)
              Thanks ]
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              assigning to petr as the reviewer
              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - assigning to petr as the reviewer Thanks
              ankit_frenz Ankit Agarwal made changes -
              Peer reviewer ankit_frenz skodak
              ankit_frenz Ankit Agarwal made changes -
              Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
              cfulton Charles Fulton made changes -
              Testing Instructions Note that to test this properly you need a database with some orphaned posts. Follow these steps with 2.3 instance without this fix applied.

              1. Revert MDL-13516.
              2. Create a forum.
              3. Create a discussion (post A) as a user.
              4. Post a reply (post B) as another user.
              5. Post another reply (post C) to Post B.
              6. Post a separate reply (post D) to Post A.
              7. Delete Post B.
              8. Verify that Post C and Post D are still in the database.

              Now apply this fix to upgrade mod/forum. Run the upgrade. Post C should be deleted from the database. Posts A and D should still be there.
              Note that to test this properly you need a database with some orphaned posts. Follow these steps with a 2.3 instance without this fix applied.

              1. Revert MDL-31664.
              2. Create a forum.
              3. Create a discussion (post A) as a user.
              4. Post a reply (post B) as another user.
              5. Post another reply (post C) to Post B.
              6. Post a separate reply (post D) to Post A.
              7. Delete Post B.
              8. Verify that Post C and Post D are still in the database.

              Now apply this fix to upgrade mod/forum. Run the upgrade. Post C should be deleted from the database. Posts A and D should still be there.
              cfulton Charles Fulton made changes -
              Status Development in progress [ 3 ] Open [ 1 ]
              Hide
              skodak Petr Skoda added a comment -

              oh, I missed this one, +1

              Show
              skodak Petr Skoda added a comment - oh, I missed this one, +1
              poltawski Dan Poltawski made changes -
              Peer reviewer Petr Skoda [ skodak ]
              Hide
              poltawski Dan Poltawski added a comment -

              Charles - is this still relevant? If so, are you able to rebase it and send it for peer review?

              Show
              poltawski Dan Poltawski added a comment - Charles - is this still relevant? If so, are you able to rebase it and send it for peer review?
              Hide
              cfulton Charles Fulton added a comment -

              Hi Dan, any database which started life before MDL-31664 was integrated potentially has orphaned posts. That's presupposing there's no new source of difficulties. My patch doesn't address Petr's concerns about Oracle performance and embedded images. I'd say it's still relevant but it needs more work than a simple rebase.

              Show
              cfulton Charles Fulton added a comment - Hi Dan, any database which started life before MDL-31664 was integrated potentially has orphaned posts. That's presupposing there's no new source of difficulties. My patch doesn't address Petr's concerns about Oracle performance and embedded images. I'd say it's still relevant but it needs more work than a simple rebase.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks Charles,

              I'm going to remove you as the assignee as your last comment suggests that you don't intend to address the issues Petr raised.

              Cheers,

              Andrew

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks Charles, I'm going to remove you as the assignee as your last comment suggests that you don't intend to address the issues Petr raised. Cheers, Andrew
              dobedobedoh Andrew Nicols made changes -
              Status Open [ 1 ] Open [ 1 ]
              Affects Version/s 2.8 [ 13150 ]
              Affects Version/s 2.7.3 [ 14152 ]
              Affects Version/s 2.9 [ 14251 ]
              Assignee Charles Fulton [ cfulton ]
              marina Marina Glancy made changes -
              Parent MDL-13513 [ 24950 ]
              Issue Type Sub-task [ 5 ] Bug [ 1 ]
              Rank (Obsolete) 1020000000
              Hide
              marina Marina Glancy added a comment -

              This issue is converted from sub-task into a separate issue because parent is no longer active

              Show
              marina Marina Glancy added a comment - This issue is converted from sub-task into a separate issue because parent is no longer active

                People

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

                  Dates

                  • Created:
                    Updated: