Moodle
  1. Moodle
  2. MDL-39000

Import can include section conditions which still point to old course

    Details

    • Testing Instructions:
      Hide

      1. Create course with shortname/fullname xA and all default settings.
      2. In section 1 add a new Lesson called 'Lesson A' with all default settings.
      3. Edit settings for section 2. Set grade condition to 'Lesson A' and at least 50%. Set to show restriction information. Save changes.
      4. In section 2 add a new Book called 'Book A' with all default settings.
      5. Create course with shortname/fullname xB and all default settings.
      6. Click 'Import' under course administration. Select course xA and continue.
      7. Leave the three tickboxes ticked (default), and continue.
      8. Select None, then tick both Book A and the section that contains Book A, and continue.
      9. Perform import.

      EXPECTED: Book A should appear in section 2, but there should be no conditions on that section.
      BEFORE FIX: Section 2 displays the following: 'Restricted (completely hidden, no message): Not available until you achieve a required score in !missing.'

      11. Delete Book A from section 2.
      12. In section 1 add a new Lesson called 'Lesson B' with all default settings.
      13. Edit settings for section 2. Set the grade condition to 'Lesson B' and at least 50%. Set to show restriction information. Save changes. (The message should now show it's not available until you get a score in Lesson B.)
      14. Repeat the import process in steps 6-9.

      EXPECTED: Book A should appear in section 2. The existing condition (on Lesson B) should remain on section 2.
      BEFORE FIX: Section 2 displays conditions on both 'Lesson B' AND 'Lesson A'.

      15. Delete Book A from section 2.
      16. Repeat the import process, but this time select Lesson A (and its section) in addition to Book A and its section.

      EXPECTED: Book A should be imported again, and so should Lesson A; section 2 should now have conditions on both 'Lesson B' and 'Lesson A'. (The 'Lesson A' refers to the new copy on this course; I checked in database.)

      Note: You can repeat the test from step 13 if you delete the book again, or from step 6 if you delete the book and edit the section settings to reset them.

      Show
      1. Create course with shortname/fullname xA and all default settings. 2. In section 1 add a new Lesson called 'Lesson A' with all default settings. 3. Edit settings for section 2. Set grade condition to 'Lesson A' and at least 50%. Set to show restriction information. Save changes. 4. In section 2 add a new Book called 'Book A' with all default settings. 5. Create course with shortname/fullname xB and all default settings. 6. Click 'Import' under course administration. Select course xA and continue. 7. Leave the three tickboxes ticked (default), and continue. 8. Select None, then tick both Book A and the section that contains Book A, and continue. 9. Perform import. EXPECTED: Book A should appear in section 2, but there should be no conditions on that section. BEFORE FIX: Section 2 displays the following: 'Restricted (completely hidden, no message): Not available until you achieve a required score in !missing.' 11. Delete Book A from section 2. 12. In section 1 add a new Lesson called 'Lesson B' with all default settings. 13. Edit settings for section 2. Set the grade condition to 'Lesson B' and at least 50%. Set to show restriction information. Save changes. (The message should now show it's not available until you get a score in Lesson B.) 14. Repeat the import process in steps 6-9. EXPECTED: Book A should appear in section 2. The existing condition (on Lesson B) should remain on section 2. BEFORE FIX: Section 2 displays conditions on both 'Lesson B' AND 'Lesson A'. 15. Delete Book A from section 2. 16. Repeat the import process, but this time select Lesson A (and its section) in addition to Book A and its section. EXPECTED: Book A should be imported again, and so should Lesson A; section 2 should now have conditions on both 'Lesson B' and 'Lesson A'. (The 'Lesson A' refers to the new copy on this course; I checked in database.) Note: You can repeat the test from step 13 if you delete the book again, or from step 6 if you delete the book and edit the section settings to reset them.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-39000-m24
    • Pull Master Branch:
      MDL-39000-master
    • Rank:
      49108

      Description

      This is not trivial bug to reproduce, but it actually happened and caused a lot of unrest between students complaining about blocked access to course sections that they should have available.

      ----------------
      How to reproduce:
      1. Enable conditional access in Advanced functions.
      2. Create 2 courses A and B, inside each course section 1 create Lesson A and B respectively.
      3. In Course A section 2 create Book A.
      4. Set Course A section 2 available when grade from Lesson A is above 50% and set to display availablity conditions.
      5. In Course B set section 2 available when grade from Lesson B is above 50%and set to display availablity conditions.
      6. Import Book A from Course A to Course B. It gets imported with section 2 condition reference to Lesson A from Course A!
      7. Sign test Student into both Course A and Course B as student

      8. Log out or in another browser log in as Student and go to Course A. Complete Lesson A and you should have grade for Lesson A.
      9. Check that you have avalable section 2 with Book A.
      10. Log out to clear your session.
      11. Log in as Student and go directly to Course B. The conditional logic will try to evaluate your access to Course B section 2 and will set both your Lesson B and Lesson A to "false" in $SESSION->gradescorecache.
      11. Go to Course A and check that you do not have secton 2 with Book A available despite that you should have (Student still have proper grade in database, but it is ignored while it is set in session cache).
      ---------------

      Location of the bug:
      lib/conditionlib.php condition_info_base->get_cached_grade_score()
      lines 1144-1150:
      // And if it's still not set, well it doesn't exist (eg
      // maybe the user set it as a condition, then deleted the
      // grade item) so we call it false
      if (!array_key_exists($gradeitemid, $SESSION->gradescorecache))

      { $SESSION->gradescorecache[$gradeitemid] = false; }

      This doesn't account for grade items existing in other courses with conditional references to them imported to the current course. Or maybe the problem is to allow conditional references to non-existent modules with selective import.

        Activity

        Hide
        Sam Marshall added a comment -

        Thanks for reporting this and listing detailed steps to reproduce.

        Please could you clarify one thing: when you say you 'import' the book (step 6), how are you doing that? Do you mean you are choosing the 'Import' option under the 'Course administration' menu, then continuing to the next page, then doing 'select none' to get rid of any other activities, then selecting just the book?

        I actually tried some variants of this and didn't reproduce the problem so I want to make sure I know exactly what you mean by import (it's not a feature I've ever used). Maybe there is another way to import that I haven't noticed.

        By the way, the correct behaviour here is that the section 2 condition reference should not be imported, i.e. the section should not have a condition. The system only supports conditions on activities within the same course. When you restore a backup of something (call it X) that has a condition on another thing (Y), the expected behaviour is:

        • if your restore includes both X and Y, then the new version of X should have a condition on the new version of Y.
        • if your restore only includes X, then the condition will not be included in the new version of X.

        So if everything is working correctly, it should not be possible for the database to contain any condition references to activities that are on a different course.

        Show
        Sam Marshall added a comment - Thanks for reporting this and listing detailed steps to reproduce. Please could you clarify one thing: when you say you 'import' the book (step 6), how are you doing that? Do you mean you are choosing the 'Import' option under the 'Course administration' menu, then continuing to the next page, then doing 'select none' to get rid of any other activities, then selecting just the book? I actually tried some variants of this and didn't reproduce the problem so I want to make sure I know exactly what you mean by import (it's not a feature I've ever used). Maybe there is another way to import that I haven't noticed. By the way, the correct behaviour here is that the section 2 condition reference should not be imported, i.e. the section should not have a condition. The system only supports conditions on activities within the same course. When you restore a backup of something (call it X) that has a condition on another thing (Y), the expected behaviour is: if your restore includes both X and Y, then the new version of X should have a condition on the new version of Y. if your restore only includes X, then the condition will not be included in the new version of X. So if everything is working correctly, it should not be possible for the database to contain any condition references to activities that are on a different course.
        Hide
        Pavel Krejci added a comment -

        Thanks for the explanation.

        I made video with the import to be as clear as possible:
        http://www.youtube.com/watch?v=mShZyqf0N7I

        Show
        Pavel Krejci added a comment - Thanks for the explanation. I made video with the import to be as clear as possible: http://www.youtube.com/watch?v=mShZyqf0N7I
        Hide
        Sam Marshall added a comment -

        Thanks for the video, very clear. I've managed to reproduce it now. Strange! I'm going to see if I can reduce the test script a bit, then I'll look at fixing the problem.

        Show
        Sam Marshall added a comment - Thanks for the video, very clear. I've managed to reproduce it now. Strange! I'm going to see if I can reduce the test script a bit, then I'll look at fixing the problem.
        Hide
        Sam Marshall added a comment -

        OK, I've written a new test script. This test script is not really any shorter but it indicates two distinct problems which (I think) are:

        1. When you restore something with a condition on an activity which doesn't exist on the target course, the system correctly removes the required item (gradeitemid field) but it does not clear the grademin or grademax fields. This causes the !missing behaviour.

        2. When you restore something with a condition on an activity which doesn't exist on the target course, BUT the target course already has a condition for that activity (obviously on something which does exist), then the system fails to remove the gradeitemid for the activity that doesn't exist.

        Show
        Sam Marshall added a comment - OK, I've written a new test script. This test script is not really any shorter but it indicates two distinct problems which (I think) are: 1. When you restore something with a condition on an activity which doesn't exist on the target course, the system correctly removes the required item (gradeitemid field) but it does not clear the grademin or grademax fields. This causes the !missing behaviour. 2. When you restore something with a condition on an activity which doesn't exist on the target course, BUT the target course already has a condition for that activity (obviously on something which does exist), then the system fails to remove the gradeitemid for the activity that doesn't exist.
        Hide
        Sam Marshall added a comment -

        Note: The first problem could apply to conditions on activities as well as sections, but I have just tested: it does not. Both problems are specific to section conditions.

        Show
        Sam Marshall added a comment - Note: The first problem could apply to conditions on activities as well as sections, but I have just tested: it does not. Both problems are specific to section conditions.
        Hide
        Sam Marshall added a comment -

        Updated test script again to include a check that I didn't break the behaviour for when the item that the condition depends on is included in the import.

        Show
        Sam Marshall added a comment - Updated test script again to include a check that I didn't break the behaviour for when the item that the condition depends on is included in the import.
        Hide
        Sam Marshall added a comment -

        Fixed in the patches provided. There were two problems:

        1. When the source (grade item or cmid) is not present because it wasn't included in the import, the system needs to actually delete that row from course_modules_availability. Instead it was keeping the row while setting both fields null, which is unexpected data.

        2. Code in a loop was supposed to check whether a condition was an existing one, or a new one created by the import (in which case it should be modified). For existing ones, instead of just skipping it and going onto the next one, it accidentally exited the loop.

        Confirmed same bug exists on Moodle 2.3. (NOTE: I did all the testing on 2.4, but the change cherry-picks cleanly both ways.)

        Show
        Sam Marshall added a comment - Fixed in the patches provided. There were two problems: 1. When the source (grade item or cmid) is not present because it wasn't included in the import, the system needs to actually delete that row from course_modules_availability. Instead it was keeping the row while setting both fields null, which is unexpected data. 2. Code in a loop was supposed to check whether a condition was an existing one, or a new one created by the import (in which case it should be modified). For existing ones, instead of just skipping it and going onto the next one, it accidentally exited the loop. Confirmed same bug exists on Moodle 2.3. (NOTE: I did all the testing on 2.4, but the change cherry-picks cleanly both ways.)
        Hide
        Jason Fowler added a comment -

        [Y] Syntax
        [-] Output
        [Y] Whitespace
        [-] Language
        [Y] Databases
        [Y] Testing
        [-] Security
        [-] Documentation
        [Y] Git
        [Y] Sanity check

        All good Sam, pushing for integration for you.

        Show
        Jason Fowler added a comment - [Y] Syntax [-] Output [Y] Whitespace [-] Language [Y] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check All good Sam, pushing for integration for you.
        Hide
        Sam Marshall added a comment -

        Thanks for the review Jason.

        Show
        Sam Marshall added a comment - Thanks for the review Jason.
        Hide
        Dan Poltawski added a comment -

        Integrated to master, 24 and 23 - thanks Sam

        Show
        Dan Poltawski added a comment - Integrated to master, 24 and 23 - thanks Sam
        Hide
        Ankit Agarwal added a comment - - edited

        Hi Sam,
        Am I correct in assuming section 11-14 needs to be carried out in xB?
        Edit:-
        Couldn't be xA since subsequent steps wont make sense than.
        continued with testing. It works as described on 23/24/master.
        Passing.
        Thanks

        Show
        Ankit Agarwal added a comment - - edited Hi Sam, Am I correct in assuming section 11-14 needs to be carried out in xB? Edit:- Couldn't be xA since subsequent steps wont make sense than. continued with testing. It works as described on 23/24/master. Passing. Thanks
        Hide
        Sam Marshall added a comment -

        Thanks Ankit - yes your assumption was correct, sorry the test case was not explicit about that!

        Show
        Sam Marshall added a comment - Thanks Ankit - yes your assumption was correct, sorry the test case was not explicit about that!
        Hide
        Dan Poltawski added a comment -

        Blooming Marvelous! It's time for a knees up - your changes are upstream!

        Thanks for making Moodle better!

        Toodle pip

        Show
        Dan Poltawski added a comment - Blooming Marvelous! It's time for a knees up - your changes are upstream! Thanks for making Moodle better! Toodle pip

          People

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

            Dates

            • Created:
              Updated:
              Resolved: