Moodle
  1. Moodle
  2. MDL-33133

Conditional section availability - incorrect restore handling on partial restores

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Conditional activities
    • Labels:
      None
    • Testing Instructions:
      Hide

      0. Server must have completion and conditions enabled.
      1. Create a new course A set to topics mode with 2 sections and with completion enabled.
      2. In section 0, add a label text 'Course A label', set so students can manually mark complete.
      3. Edit settings for section 1 and set it to only be available if the label is marked complete.

      • Note that the availability restriction is now displayed when editing.

      4. Create a new course B with the same settings.
      5. Set up course B the same, except call the label 'Course B label', and edit settings for section 2 (not 1) to make it only available if the label is marked complete.
      6. Additionally, add a new label 'Section 2 label' into section 2. (Not sure this is actually needed...)
      7. Back up course B using default settings.
      8. Restore the backup. On screen 2, choose 'Restore into an existing course' and select course A from this test. (Other options default.)

      • The restored course now has two sections.
      • The first section should still indicate that it's not available until you complete the 'Course A label'. The added second section should indicate that it's not available until you complete the 'Course B label'.

      9. Turn editing off and turn on/off the two tickboxes.

      • The two tickboxes on this course should make the 'Not available until...' messages on each section appear/disappear, indicating that they are indeed connected to the right sections.

      Before fixing this bug, this test failed at point 8. Section 1 says 'Not available unless you have a grade in !missing'.

      Show
      0. Server must have completion and conditions enabled. 1. Create a new course A set to topics mode with 2 sections and with completion enabled. 2. In section 0, add a label text 'Course A label', set so students can manually mark complete. 3. Edit settings for section 1 and set it to only be available if the label is marked complete. Note that the availability restriction is now displayed when editing. 4. Create a new course B with the same settings. 5. Set up course B the same, except call the label 'Course B label', and edit settings for section 2 (not 1) to make it only available if the label is marked complete. 6. Additionally, add a new label 'Section 2 label' into section 2. (Not sure this is actually needed...) 7. Back up course B using default settings. 8. Restore the backup. On screen 2, choose 'Restore into an existing course' and select course A from this test. (Other options default.) The restored course now has two sections. The first section should still indicate that it's not available until you complete the 'Course A label'. The added second section should indicate that it's not available until you complete the 'Course B label'. 9. Turn editing off and turn on/off the two tickboxes. The two tickboxes on this course should make the 'Not available until...' messages on each section appear/disappear, indicating that they are indeed connected to the right sections. Before fixing this bug, this test failed at point 8. Section 1 says 'Not available unless you have a grade in !missing'.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-33133-master
    • Rank:
      40994

      Description

      Eloy pointed out problems with the restore of this new feature:

      We need to know which records are existing ones and which have been introduced by the restore. And also we need to detect invalid duplicates.

      All that makes me thing if really the introduction of the after_restore() in steps is a good idea. They are only available at task level because only modules not being auto-contained should use them (bad citizens). And also, they are supported by restore_plugins, because some of them require access to the whole thing (plagiarism...). But bringing support to steps... could lead to abuse, so my proposal would be about to:

      1) drop support for restore_steps->after_restore()
      2) implement the section_availability restore as a new step, much like modules availability is handled, in 2 steps:

      a) reading section_availability information simply store it in backupids table
      b) later one new step "restore_process_course_sections_availability" executed by the restore_final_task.

        Activity

        Hide
        Sam Marshall added a comment -

        Fixed test script...

        Show
        Sam Marshall added a comment - Fixed test script...
        Hide
        Sam Marshall added a comment -

        Requesting peer review (from Eloy please).

        I have fixed the problem by making it add entries to the backup_ids table for the rows, then only updating ones that were placed in that table. It is maybe a teensy bit hacky, but appears to work fine.

        I did not remove the after_restore or put this back into a separate step. Maybe that would be better - to me it seems more sensible to have the code in the same place, i.e. all the functions to do with restoring this data ought to be in the same class - but maybe two steps could be right below each other anyway so it wouldn't make much difference... Anyhow, basically, not too sure how to set up new steps and this is a minimal change that fixes the practical problem, so maybe good enough.

        Show
        Sam Marshall added a comment - Requesting peer review (from Eloy please). I have fixed the problem by making it add entries to the backup_ids table for the rows, then only updating ones that were placed in that table. It is maybe a teensy bit hacky, but appears to work fine. I did not remove the after_restore or put this back into a separate step. Maybe that would be better - to me it seems more sensible to have the code in the same place, i.e. all the functions to do with restoring this data ought to be in the same class - but maybe two steps could be right below each other anyway so it wouldn't make much difference... Anyhow, basically, not too sure how to set up new steps and this is a minimal change that fixes the practical problem, so maybe good enough.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Crap, had not seen this. Thanks SamH for pointing to it, sorry SamM... will look in a bunch of hours.

        Show
        Eloy Lafuente (stronk7) added a comment - Crap, had not seen this. Thanks SamH for pointing to it, sorry SamM... will look in a bunch of hours.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I'm not completely sold to the idea of keeping the after_restore() methods for steps there (prone to abuse, sort of workaround for something that should be a step by itself IMO...) ... but as far as they are not documented (keep them that way, plz)... this looks ok... and could have some use in the future... I'm going to straight integrate it after all.

        I hate not being 100% sure of the decision, so let's give it a chance.

        Thanks a lot and sorry for the delay, I had missed this completely!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - I'm not completely sold to the idea of keeping the after_restore() methods for steps there (prone to abuse, sort of workaround for something that should be a step by itself IMO...) ... but as far as they are not documented (keep them that way, plz)... this looks ok... and could have some use in the future... I'm going to straight integrate it after all. I hate not being 100% sure of the decision, so let's give it a chance. Thanks a lot and sorry for the delay, I had missed this completely! Ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
        Hide
        Frédéric Massart added a comment -

        Test successful on master! Cheers

        Show
        Frédéric Massart added a comment - Test successful on master! Cheers
        Hide
        Eloy Lafuente (stronk7) added a comment -

        We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

        Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

        Many thanks for your collaboration!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3. Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish. Many thanks for your collaboration! Ciao
        Hide
        Dan Poltawski added a comment - - edited

        Hi Sam,

        I was just reviewing some code close to this and noticed the line:

        $this->set_mapping('course_sections_availability', $newid, $newid);
        

        I think that this is wrong - $newid, $newid?

        (I'm holding off creating a new issue because i'm not 100% certain)

        Show
        Dan Poltawski added a comment - - edited Hi Sam, I was just reviewing some code close to this and noticed the line: $ this ->set_mapping('course_sections_availability', $newid, $newid); I think that this is wrong - $newid, $newid? (I'm holding off creating a new issue because i'm not 100% certain)
        Hide
        Sam Marshall added a comment -

        I looked up in my patch for this issue. The code you quote is intentional, it would help if you also quote the comment directly above it?

                // We do not need to map between old and new id but storing a mapping
                // means it gets added to the backup_ids table to record which ones
                // need updating. The mapping is stored with $newid => $newid for
                // convenience.
                $this->set_mapping('course_sections_availability', $newid, $newid);
        
        Show
        Sam Marshall added a comment - I looked up in my patch for this issue. The code you quote is intentional, it would help if you also quote the comment directly above it? // We do not need to map between old and new id but storing a mapping // means it gets added to the backup_ids table to record which ones // need updating. The mapping is stored with $newid => $newid for // convenience. $ this ->set_mapping('course_sections_availability', $newid, $newid);
        Hide
        Dan Poltawski added a comment -

        Lol. Doh, that'll teach me for looking at code with the blinkers on!

        Show
        Dan Poltawski added a comment - Lol. Doh, that'll teach me for looking at code with the blinkers on!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: