Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-40971

Importing Outcomes breaks Clean theme

    Details

    • Testing Instructions:
      Hide
      1. Switched to Clean theme
      2. Reset theme Cache
      3. Enabled Outcomes
      4. Created some scales & outcomes
      5. Go to Course and select Grades from Course administration menu.
      6. Select "Import outcomes" in drop-down menu
      7. Make sure you don't see any error.

      In 2.4 do this in Standard theme and make sure the blocks are displayed on the page.

      Show
      Switched to Clean theme Reset theme Cache Enabled Outcomes Created some scales & outcomes Go to Course and select Grades from Course administration menu. Select "Import outcomes" in drop-down menu Make sure you don't see any error. In 2.4 do this in Standard theme and make sure the blocks are displayed on the page.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-40971_M26

      Description

      1. Updated to 2.5.1
      2. Switched to Clean them
      3. Reset theme Cache
      4. Enabled Outcomes
      5. Created some scales & outcomes
      6. Went to gradebook
      7. Clicked 'Add outcome item'
      8. Got error:

      Coding error detected, it must be fixed by a programmer: Trying to reference an unknown block region side-pre

        Gliffy Diagrams

        1. OutcomesImport.png
          92 kB
        2. screenshot-1.jpg
          158 kB
        3. screenshot-2.jpg
          223 kB

          Issue Links

            Activity

            Hide
            chriskl Chris Kings-Lynne added a comment -

            MDL-40065 claims the issue is fixed in 2.5.1, but I'm still experiencing it when adding outcome items.

            Show
            chriskl Chris Kings-Lynne added a comment - MDL-40065 claims the issue is fixed in 2.5.1, but I'm still experiencing it when adding outcome items.
            Hide
            chriskl Chris Kings-Lynne added a comment -

            Prior to clicking 'add outcome item'

            Show
            chriskl Chris Kings-Lynne added a comment - Prior to clicking 'add outcome item'
            Hide
            hernan43 Ray Hernandez added a comment - - edited

            I'm still seeing this issue when trying to import outcomes as well. I am also seeing it when using the Ad-hoc reporting tool. I'm on Moodle 2.5.2 currently.

            Show
            hernan43 Ray Hernandez added a comment - - edited I'm still seeing this issue when trying to import outcomes as well. I am also seeing it when using the Ad-hoc reporting tool. I'm on Moodle 2.5.2 currently.
            Hide
            adamann2 Ann Adamcik added a comment -

            We're also seeing this when attempting to import outcomes, with 2.5.2+. The issue exists in 2.6 as well.

            I've attached a screenshot from qa.moodle.net.

            Show
            adamann2 Ann Adamcik added a comment - We're also seeing this when attempting to import outcomes, with 2.5.2+. The issue exists in 2.6 as well. I've attached a screenshot from qa.moodle.net.
            Hide
            hernan43 Ray Hernandez added a comment -

            We've found that adding:

            $PAGE->set_pagelayout('admin');

            to grade/edit/outcome/import.php made the error go away.

            Show
            hernan43 Ray Hernandez added a comment - We've found that adding: $PAGE->set_pagelayout('admin'); to grade/edit/outcome/import.php made the error go away.
            Hide
            lazydaisy Mary Evans added a comment -

            What was the layout set to prior to you changing it? We can't just keep changing CORE code to make Bootstrap themes work, we need to fix Bootstrapbase theme to stop it playing silly games!

            Show
            lazydaisy Mary Evans added a comment - What was the layout set to prior to you changing it? We can't just keep changing CORE code to make Bootstrap themes work, we need to fix Bootstrapbase theme to stop it playing silly games!
            Hide
            hernan43 Ray Hernandez added a comment -

            It was not set to anything. I added that line.

            Show
            hernan43 Ray Hernandez added a comment - It was not set to anything. I added that line.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            Well it appears to the a problem in Standard themes too, at least I am getting errors in Standard theme.

            Show
            lazydaisy Mary Evans added a comment - - edited Well it appears to the a problem in Standard themes too, at least I am getting errors in Standard theme.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            Adding Michael de Raadt is there any chance we can get this fixed?
            Eloy thinks it's related to MDL-34311 and so told me to PING you

            Show
            lazydaisy Mary Evans added a comment - - edited Adding Michael de Raadt is there any chance we can get this fixed? Eloy thinks it's related to MDL-34311 and so told me to PING you
            Hide
            lazydaisy Mary Evans added a comment -

            I think I figured it out...just need to test it in Clean. I got standard to work OK.

            Show
            lazydaisy Mary Evans added a comment - I think I figured it out...just need to test it in Clean. I got standard to work OK.
            Hide
            lazydaisy Mary Evans added a comment -

            Michael, if you have time can you Peer Review this please or assign it to someone else?

            Thanks
            Mary

            Show
            lazydaisy Mary Evans added a comment - Michael, if you have time can you Peer Review this please or assign it to someone else? Thanks Mary
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for the patch Mary, Patch looks spot-on. Only thing which I can suggest is the git commit message. It seems too long http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages

            [y] Syntax
            [y] Whitespace
            [y] Output
            [-] Language
            [-] Databases
            [y] Testing (instructions and automated tests)
            [-] Security
            [-] Documentation
            [y] Git
            [-] Third party code
            [y] Sanity check

            Feel free to push it for integration when ready.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for the patch Mary, Patch looks spot-on. Only thing which I can suggest is the git commit message. It seems too long http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages [y] Syntax [y] Whitespace [y] Output [-] Language [-] Databases [y] Testing (instructions and automated tests) [-] Security [-] Documentation [y] Git [-] Third party code [y] Sanity check Feel free to push it for integration when ready.
            Hide
            lazydaisy Mary Evans added a comment -

            Thanks Rajesh I realised that the commit was a bit too long, I'll clean it up thanks.

            Show
            lazydaisy Mary Evans added a comment - Thanks Rajesh I realised that the commit was a bit too long, I'll clean it up thanks.
            Hide
            marina Marina Glancy added a comment -

            Hello Mary.
            Thanks a lot for working on it. Patch looked a little suspicious for me because it has nothing to do with Clean theme. The same page having incorrect layout in Standard theme does not throw an error. So I started digging and this is what I found.

            1. Your change in setType() is absolutely correct

            2. Your change in setting pagelayout is half-correct. Function print_grade_page_head() sets the page layout but it is called too late. The form constructor accesses the $OUTPUT and pagelayout must not be changed after that. Correct would be to move print_grade_page_head() above the form constructor and not to not add another set_pagelayout . At the same time I searched the code calling print_grade_page_head() and absolutely everywhere we do the same - instead of calling the function early we add another set_pagelayout in the beginning, or even worse, do this:
            https://github.com/moodle/moodle/blob/master/grade/edit/letter/index.php#L60

            3. But still - in case of calling set_pagelayout too late Clean should not die. I looked at how layouts process block regions in base/standard and made a similar correction. Except that I added this check inside the function core_renderer::blocks() and not inside the layout files like base/standard/etc do:

            diff --git a/lib/outputrenderers.php b/lib/outputrenderers.php
            index 56b6f56..5a0bf27 100644
            --- a/lib/outputrenderers.php
            +++ b/lib/outputrenderers.php
            @@ -3199,7 +3199,12 @@ EOD;
                         'data-blockregion' => $displayregion,
                         'data-droptarget' => '1'
                     );
            -        return html_writer::tag($tag, $this->blocks_for_region($region), $attributes);
            +        if ($this->page->blocks->region_has_content($region, $this)) {
            +            $content = $this->blocks_for_region($region);
            +        } else {
            +            $content = '';
            +        }
            +        return html_writer::tag($tag, $content, $attributes);
                 }
             
                 /**

            I'm going to integrate this patch as it is but I recommend you to consider making some patch for Clean theme so it does not vomit when we set page layout too late.

            Show
            marina Marina Glancy added a comment - Hello Mary. Thanks a lot for working on it. Patch looked a little suspicious for me because it has nothing to do with Clean theme. The same page having incorrect layout in Standard theme does not throw an error. So I started digging and this is what I found. 1. Your change in setType() is absolutely correct 2. Your change in setting pagelayout is half-correct. Function print_grade_page_head() sets the page layout but it is called too late. The form constructor accesses the $OUTPUT and pagelayout must not be changed after that. Correct would be to move print_grade_page_head() above the form constructor and not to not add another set_pagelayout . At the same time I searched the code calling print_grade_page_head() and absolutely everywhere we do the same - instead of calling the function early we add another set_pagelayout in the beginning, or even worse, do this: https://github.com/moodle/moodle/blob/master/grade/edit/letter/index.php#L60 3. But still - in case of calling set_pagelayout too late Clean should not die. I looked at how layouts process block regions in base/standard and made a similar correction. Except that I added this check inside the function core_renderer::blocks() and not inside the layout files like base/standard/etc do: diff --git a/lib/outputrenderers.php b/lib/outputrenderers.php index 56b6f56..5a0bf27 100644 --- a/lib/outputrenderers.php +++ b/lib/outputrenderers.php @@ -3199,7 +3199,12 @@ EOD; 'data-blockregion' => $displayregion, 'data-droptarget' => '1' ); - return html_writer::tag($tag, $this->blocks_for_region($region), $attributes); + if ($this->page->blocks->region_has_content($region, $this)) { + $content = $this->blocks_for_region($region); + } else { + $content = ''; + } + return html_writer::tag($tag, $content, $attributes); } /** I'm going to integrate this patch as it is but I recommend you to consider making some patch for Clean theme so it does not vomit when we set page layout too late.
            Hide
            marina Marina Glancy added a comment -

            Thanks, integrated in 2.4, 2.5, 2.6

            Show
            marina Marina Glancy added a comment - Thanks, integrated in 2.4, 2.5, 2.6
            Hide
            marina Marina Glancy added a comment -

            Tested, all good

            Show
            marina Marina Glancy added a comment - Tested, all good
            Hide
            lazydaisy Mary Evans added a comment -

            Hi Marina,

            I'm not sure how to do what you ask, are you suggesting I make a change to theme/bootstrap/renderers/core_renderer.php similar to your code (3) above? Or are you telling me I need to add something in theme/clean/layouts?

            Show
            lazydaisy Mary Evans added a comment - Hi Marina, I'm not sure how to do what you ask, are you suggesting I make a change to theme/bootstrap/renderers/core_renderer.php similar to your code (3) above? Or are you telling me I need to add something in theme/clean/layouts?
            Hide
            marina Marina Glancy added a comment -

            Hi Mary, I'm not 100% sure. I created an issue MDL-40971 and maybe Sam can help there

            Show
            marina Marina Glancy added a comment - Hi Mary, I'm not 100% sure. I created an issue MDL-40971 and maybe Sam can help there
            Hide
            poltawski Dan Poltawski added a comment -

            Congratulations - this issue has been included in Moodle and is now available on our git mirrors and shortly will become available on the download servers shortly.

            Show
            poltawski Dan Poltawski added a comment - Congratulations - this issue has been included in Moodle and is now available on our git mirrors and shortly will become available on the download servers shortly.

              People

              • Votes:
                6 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Nov/13