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

          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:
                4 Start watching this issue

                Dates

                • Created:
                  Updated: