Moodle
  1. Moodle
  2. MDL-34257

Quiz attempt popup doesn't respect theme's config.php layout array

    Details

    • Testing Instructions:
      Hide

      Please test with one or more of the standard themes (this should work in any theme).

      1. As teacher: Create a quiz with "Browser security" set to "Full screen pop-up with some JavaScript security", and "Show blocks during the attempt" turned off.
      2. As student: Attempt the quiz. It should open in a pop-up window. The layout should be very plain:
        • no header
        • no footer
        • no navbar
        • login information shows who is logged in, but no logout link.
        • no real blocks, just the quiz navigation that shows which questions are in the quiz, and the timer (if required).
      3. Now turn on "Show blocks during the attempt" and attempt the quiz again as a student.
      4. Verify the only change to the layout is that now the real blocks (probably navigation and settings) are shown.
      Show
      Please test with one or more of the standard themes (this should work in any theme). As teacher: Create a quiz with "Browser security" set to "Full screen pop-up with some JavaScript security", and "Show blocks during the attempt" turned off. As student: Attempt the quiz. It should open in a pop-up window. The layout should be very plain: no header no footer no navbar login information shows who is logged in, but no logout link. no real blocks, just the quiz navigation that shows which questions are in the quiz, and the timer (if required). Now turn on "Show blocks during the attempt" and attempt the quiz again as a student. Verify the only change to the layout is that now the real blocks (probably navigation and settings) are shown.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      42606

      Description

      The base theme sets the config.php's $THEME->layouts to have popup layout with the following settings:
      'popup' => array(
      'file' => 'general.php',
      'regions' => array(),
      'options' => array('nofooter'=>true, 'nonavbar'=>true, 'nocustommenu'=>true, 'nologininfo'=>true),
      ),

      A quiz set to browser security full screen popup with some javascript security ignores all options. The header, footer, navigation and custom menu all show.

      To recreate

      1. set to standard theme
      2. Create a quiz and set the browser security to full screen popup with javascript security
      3. Login as a student in a course with the quiz
      4. Attempt the quiz and see all of the parts of the theme.

        Issue Links

          Activity

          Hide
          Jason Hardin added a comment -

          screen shot of standard theme with popup quiz

          Show
          Jason Hardin added a comment - screen shot of standard theme with popup quiz
          Hide
          Jason Hardin added a comment - - edited

          Technically based on the layout options even the fake block quiz navigation should not show up in the layout. Aka a block is being added to a layout that has no block regions and some how it appears.

          It would be nice to have a new layout file for quiz to support its different type of popup compared to other popup pages.

          Show
          Jason Hardin added a comment - - edited Technically based on the layout options even the fake block quiz navigation should not show up in the layout. Aka a block is being added to a layout that has no block regions and some how it appears. It would be nice to have a new layout file for quiz to support its different type of popup compared to other popup pages.
          Hide
          Tim Hunt added a comment -

          Can you go to debugging, and turn on the "Show page information" setting, so we can be sure what page type is really being used to render this page. Thanks.

          Show
          Tim Hunt added a comment - Can you go to debugging, and turn on the "Show page information" setting, so we can be sure what page type is really being used to render this page. Thanks.
          Hide
          Jason Hardin added a comment -

          footer information:
          0.815731 secs
          RAM: 69.5MB
          RAM peak: 69.7MB
          Included 750 files
          Contexts for which filters were loaded: 1
          Filters created: 4
          Pieces of content filtered: 1
          Strings filtered: 0
          get_string calls: 2613
          strings mem cache hits: 2738
          strings disk cache hits: 0
          DB reads/writes: 150/2
          Load average: 0.37
          Session: 14.8KB
          This page is: General type: popup. Context Quiz: Linear Inequalities Quiz (context id 4386). Page type mod-quiz-attempt.

          Show
          Jason Hardin added a comment - footer information: 0.815731 secs RAM: 69.5MB RAM peak: 69.7MB Included 750 files Contexts for which filters were loaded: 1 Filters created: 4 Pieces of content filtered: 1 Strings filtered: 0 get_string calls: 2613 strings mem cache hits: 2738 strings disk cache hits: 0 DB reads/writes: 150/2 Load average: 0.37 Session: 14.8KB This page is: General type: popup. Context Quiz: Linear Inequalities Quiz (context id 4386). Page type mod-quiz-attempt.
          Hide
          Tim Hunt added a comment -

          I don't understand why the theme's config.php would be getting ignored. Adding Sam H as a watcher, in case he has any idea.

          Show
          Tim Hunt added a comment - I don't understand why the theme's config.php would be getting ignored. Adding Sam H as a watcher, in case he has any idea.
          Hide
          Jason Hardin added a comment -

          What is the popup supposed to look like? I had always assumed it was to have no header footer or navigation just the question and I guess the quiz navigation block. I am the most concerned about how a fake block is supposed to appear on a layout file that has no regions.

          Show
          Jason Hardin added a comment - What is the popup supposed to look like? I had always assumed it was to have no header footer or navigation just the question and I guess the quiz navigation block. I am the most concerned about how a fake block is supposed to appear on a layout file that has no regions.
          Hide
          Sam Hemelryk added a comment -

          I did a quick bit of testing this morning.
          The page layout is being set twice within the process of the page.
          First to incourse, and then to popup.
          The problem being we can't change pagelayout once theme and output has been initialised and the second call to set page layout is occurring after it has been.
          Within moodle_page::set_pagelayout there is a commented out debugging notice that when uncommented gives us the following:

          Page layout has already been set and cannot be changed.

          line 1021 of /lib/pagelib.php: call to debugging()
          line 76 of /mod/quiz/accessrule/securewindow/rule.php: call to moodle_page->set_pagelayout()
          line 390 of /mod/quiz/accessmanager.php: call to quizaccess_securewindow->setup_attempt_page()
          line 122 of /mod/quiz/attempt.php: call to quiz_access_manager->setup_attempt_page()

          and if I debug moodle_page::_wherethemewasinitialised I get the following:

          line 637 of /lib/pagelib.php call to moodle_page->initialise_theme_and_output()
          line 766 of /lib/pagelib.php call to moodle_page->magic_get_theme()
          line 79 of /mod/quiz/attempt.php call to moodle_page->get_renderer()

          So within attempt.php looks like the theme is being initialised when the renderer is requested on ln:79 and the page layout isn't being set until ln:122 when some action is occurring.

          At the moment Tim I think its probably in your ball park given it seems to be an ordering thing within Quiz.
          Hope this all helps.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - I did a quick bit of testing this morning. The page layout is being set twice within the process of the page. First to incourse, and then to popup. The problem being we can't change pagelayout once theme and output has been initialised and the second call to set page layout is occurring after it has been. Within moodle_page::set_pagelayout there is a commented out debugging notice that when uncommented gives us the following: Page layout has already been set and cannot be changed. line 1021 of /lib/pagelib.php: call to debugging() line 76 of /mod/quiz/accessrule/securewindow/rule.php: call to moodle_page->set_pagelayout() line 390 of /mod/quiz/accessmanager.php: call to quizaccess_securewindow->setup_attempt_page() line 122 of /mod/quiz/attempt.php: call to quiz_access_manager->setup_attempt_page() and if I debug moodle_page::_wherethemewasinitialised I get the following: line 637 of /lib/pagelib.php call to moodle_page->initialise_theme_and_output() line 766 of /lib/pagelib.php call to moodle_page->magic_get_theme() line 79 of /mod/quiz/attempt.php call to moodle_page->get_renderer() So within attempt.php looks like the theme is being initialised when the renderer is requested on ln:79 and the page layout isn't being set until ln:122 when some action is occurring. At the moment Tim I think its probably in your ball park given it seems to be an ordering thing within Quiz. Hope this all helps. Cheers Sam
          Hide
          Tim Hunt added a comment -

          Right, so Sam correctly identifies one problem, and https://github.com/timhunt/moodle/compare/master...MDL-34257 fixes it.

          However, with that fix, you then get

          Warning: array_key_exists() [function.array-key-exists]: The first argument should be either a string or an integer in /fs1/www_root/tjh238/moodle_head/lib/blocklib.php on line 291

          and no navigation block, because, as Jason pointed out, we cannot add the Navigation 'fake block' if the page layout does not have any block.

          So, really, it comes down to Jason's question "What is the popup supposed to look like?"

          I don't have strong opinions. We don't use this feature at the OU, and I think it is pretty pointless.

          Someone with access to users who use this feature should go and find out what is really required.

          My best guess is that the layout should be minimal, but we do need a block region.

          Note that the quiz already has a setting "Show blocks during quiz attempts" independent of the secure-mode setting, so that suggests that a layout with block is OK. The teachers can control it if the only block they want is the navigation one.

          Show
          Tim Hunt added a comment - Right, so Sam correctly identifies one problem, and https://github.com/timhunt/moodle/compare/master...MDL-34257 fixes it. However, with that fix, you then get Warning: array_key_exists() [function.array-key-exists] : The first argument should be either a string or an integer in /fs1/www_root/tjh238/moodle_head/lib/blocklib.php on line 291 and no navigation block, because, as Jason pointed out, we cannot add the Navigation 'fake block' if the page layout does not have any block. So, really, it comes down to Jason's question "What is the popup supposed to look like?" I don't have strong opinions. We don't use this feature at the OU, and I think it is pretty pointless. Someone with access to users who use this feature should go and find out what is really required. My best guess is that the layout should be minimal, but we do need a block region. Note that the quiz already has a setting "Show blocks during quiz attempts" independent of the secure-mode setting, so that suggests that a layout with block is OK. The teachers can control it if the only block they want is the navigation one.
          Hide
          Jason Hardin added a comment - - edited

          My 2 cents below based on the client tickets we are receiving over this issue.

          When quiz is displayed using the incourse layout and the quiz setting show blocks during quiz attempts is set to no the quiz navigation block still shows. The quiz timmer requires the quiz navigation block to work, without this block a javascript error is displayed.

          My recommendation would be to have a different layout format for quiz popups that allows the theme to determine how to display the quiz popup. This would allow me to add a block region where i want for just the quiz popup.

          From what I understand our clients are fine with no header, footer, navigation or blocks, but Quiz is intimately tied to the quiz navigation fake block so this has to be displayable. No header, footer, navigation or blocks is preferred for the popup.

          What should display in the window is only what is required for all of quiz's features to function properly and no other UI.

          Show
          Jason Hardin added a comment - - edited My 2 cents below based on the client tickets we are receiving over this issue. When quiz is displayed using the incourse layout and the quiz setting show blocks during quiz attempts is set to no the quiz navigation block still shows. The quiz timmer requires the quiz navigation block to work, without this block a javascript error is displayed. My recommendation would be to have a different layout format for quiz popups that allows the theme to determine how to display the quiz popup. This would allow me to add a block region where i want for just the quiz popup. From what I understand our clients are fine with no header, footer, navigation or blocks, but Quiz is intimately tied to the quiz navigation fake block so this has to be displayable. No header, footer, navigation or blocks is preferred for the popup. What should display in the window is only what is required for all of quiz's features to function properly and no other UI.
          Hide
          Tim Hunt added a comment -

          Sam, what happens in we introduce a new layout template 'quizinpopup'? Presumably themes that do not declare that exact layout in their config.php fall back to something else, like 'general'.

          Anyway, we cannot just introduce a new layout time without understanding the implications.

          Show
          Tim Hunt added a comment - Sam, what happens in we introduce a new layout template 'quizinpopup'? Presumably themes that do not declare that exact layout in their config.php fall back to something else, like 'general'. Anyway, we cannot just introduce a new layout time without understanding the implications.
          Hide
          Tim Hunt added a comment -
          Show
          Tim Hunt added a comment - See also this thread: http://moodle.org/mod/forum/discuss.php?d=206590
          Hide
          Jason Hardin added a comment -

          That discussion was started by a Moodlerooms client as part of the issue that brought up this ticket. We have a theme called Express that is actually an enhancement to Moodle themes to allow us to not have to make 700+ client themes and stick the over 10 gigs or themes data into our source code. It also does a bunch of other stuff to simplify look and feel creation for clients. Our manual is http://manuals.moodlerooms.com/display/JOULE2/Express+Manual for it.

          In the Express theme we actually pointed the popup layout to the embedded.php file instead of general.php. We then striped embedded.php to just so the main content, because in every core theme i saw that seemed to be what was being done with the config.php's layout variable for popup. Setting no regions, and nofooter, nonav, nologin and no custom menus seemed to me to be the exact same as embedded.php.

          Phillips issue is caused by express pointing to an embedded.php file that doesn't even try to display anything other than the maincontent. To resolve Phillips issue I would have to allow a block region currently in the popup layout file or point popup to general, which i am loath to do as it could adversely affect every popup window in Moodle. I am a firm believer that popups should not have Moodle navigation including blocks in them.

          Show
          Jason Hardin added a comment - That discussion was started by a Moodlerooms client as part of the issue that brought up this ticket. We have a theme called Express that is actually an enhancement to Moodle themes to allow us to not have to make 700+ client themes and stick the over 10 gigs or themes data into our source code. It also does a bunch of other stuff to simplify look and feel creation for clients. Our manual is http://manuals.moodlerooms.com/display/JOULE2/Express+Manual for it. In the Express theme we actually pointed the popup layout to the embedded.php file instead of general.php. We then striped embedded.php to just so the main content, because in every core theme i saw that seemed to be what was being done with the config.php's layout variable for popup. Setting no regions, and nofooter, nonav, nologin and no custom menus seemed to me to be the exact same as embedded.php. Phillips issue is caused by express pointing to an embedded.php file that doesn't even try to display anything other than the maincontent. To resolve Phillips issue I would have to allow a block region currently in the popup layout file or point popup to general, which i am loath to do as it could adversely affect every popup window in Moodle. I am a firm believer that popups should not have Moodle navigation including blocks in them.
          Hide
          Tim Hunt added a comment -

          I'm waiting to see what Sam H says now.

          Show
          Tim Hunt added a comment - I'm waiting to see what Sam H says now.
          Hide
          Tim Hunt added a comment -

          Just to note that MDL-31365 is reporting the equivalent problem when using Safe Exam Browser. We should probably work on the two bugs together.

          Show
          Tim Hunt added a comment - Just to note that MDL-31365 is reporting the equivalent problem when using Safe Exam Browser. We should probably work on the two bugs together.
          Hide
          Tim Hunt added a comment -

          Another case that needs to be fixed: if the quiz has a password, then startattempt.php may do output in the pop-up window. That is suffering from the same problem.

          Show
          Tim Hunt added a comment - Another case that needs to be fixed: if the quiz has a password, then startattempt.php may do output in the pop-up window. That is suffering from the same problem.
          Hide
          Sam Hemelryk added a comment -

          Ok so first up the idea of adding a new pagelayout.
          A couple of things to know first:

          • If you specify a layout that doesn't exist in the current theme but exists in one of the parent themes then the parent theme layout is used.
          • If you specify a layout that doesn't exist at all then the standard layout is used.
            So if we did look at going down the path of adding a layout for this special case of secure/controlled layouts then we'd need to define it within the base theme and review review+implement it in all other core themes.
            It would then be up to each theme maintainer as to whether they dealt with it or not, although quiz being ultra popular will really mean they will need to review it.
            Backporting it would be another bridge to cross I suppose.

          As for the actual idea, I have very mixed feelings about adding a new layout for this.
          Certainly in the past we've rejected and pushed away many proposals for new layouts, we don't want to clutter that space or theming will become an absolute nightmare. We've kept layouts entirely separate from plugins, essentially keeping all of the layouts generic to page needs rather than plugin/component needs.
          Having stuck to the idea of having minimal generic layouts in the past is enough to swing me towards thinking the idea of adding a new layout "quizinpopup" is not so great.
          I'm not saying no at this point, I'm just brining to the table the idea that this is perhaps the sort of thing we'd avoided in past. Certainly if the arguments are strong enough, all avenues have been explored and there is no better solution then I think we should go with it.

          However I do wonder whether perhaps we could search for a more generic solution here that would meet the needs of this issue as well as any other similar situations that may arrise.
          What we've got:

          • A popup with content
          • A block for navigation
          • A popup layout with only content

          If it were as simple as saying we want a popup with blocks and content but nothing else then I purpose adding a new layout called "advancedpopup" that in the base theme had a single block region + content, nothing else.
          Such a layout would undoubtedly be useful to other areas of code [in time] and could be seen as a required feature rather than a addressing a single page requirement.
          Perhaps it is that simple?

          What it doesn't allow for that adding a quiz popup specific layout would allow for is the ability to decide other factors and design specifically towards the idea of a score quiz popup. But then that was never the intention of layouts and I would really rather not see something like that arrive in core.
          Instead I think if that is really judged to be a feature that we want to offer (the ability to use a completely custom layout for quiz popups) then perhaps one of the following crazy ideas could be used in conjunction with the advancedpopup layout:

          1. Add a new super advanced setting to the quiz that allows the user to enter a custom page layout to use. That way they can enter anything they want there and add support for it themselves in their own theme. If they did enter something but didn't create a layout for it then it would default to standard which wouldn't be great, but hell with great functionality comes great responsibility.
          2. Introduce a means for page to pass options to the theme layouts. Then the quiz could implement settings for the display of page components (header, footer, navbar) and pass those through to the layout. The end result would be that individual admin/teacher could decide for themselves what they wanted to see. The downside to this is that we would have to draw a line for code in general, pages should NOT tinker with layouts any such decision should come from the end user(s).

          In summary, I am presently leaning towards an advanced popup and perhaps a new feature to offer better control.

          At this point I'll throw the ball back to the group and see what others think in response.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok so first up the idea of adding a new pagelayout. A couple of things to know first: If you specify a layout that doesn't exist in the current theme but exists in one of the parent themes then the parent theme layout is used. If you specify a layout that doesn't exist at all then the standard layout is used. So if we did look at going down the path of adding a layout for this special case of secure/controlled layouts then we'd need to define it within the base theme and review review+implement it in all other core themes. It would then be up to each theme maintainer as to whether they dealt with it or not, although quiz being ultra popular will really mean they will need to review it. Backporting it would be another bridge to cross I suppose. As for the actual idea, I have very mixed feelings about adding a new layout for this. Certainly in the past we've rejected and pushed away many proposals for new layouts, we don't want to clutter that space or theming will become an absolute nightmare. We've kept layouts entirely separate from plugins, essentially keeping all of the layouts generic to page needs rather than plugin/component needs. Having stuck to the idea of having minimal generic layouts in the past is enough to swing me towards thinking the idea of adding a new layout "quizinpopup" is not so great. I'm not saying no at this point, I'm just brining to the table the idea that this is perhaps the sort of thing we'd avoided in past. Certainly if the arguments are strong enough, all avenues have been explored and there is no better solution then I think we should go with it. However I do wonder whether perhaps we could search for a more generic solution here that would meet the needs of this issue as well as any other similar situations that may arrise. What we've got: A popup with content A block for navigation A popup layout with only content If it were as simple as saying we want a popup with blocks and content but nothing else then I purpose adding a new layout called "advancedpopup" that in the base theme had a single block region + content, nothing else. Such a layout would undoubtedly be useful to other areas of code [in time] and could be seen as a required feature rather than a addressing a single page requirement. Perhaps it is that simple? What it doesn't allow for that adding a quiz popup specific layout would allow for is the ability to decide other factors and design specifically towards the idea of a score quiz popup. But then that was never the intention of layouts and I would really rather not see something like that arrive in core. Instead I think if that is really judged to be a feature that we want to offer (the ability to use a completely custom layout for quiz popups) then perhaps one of the following crazy ideas could be used in conjunction with the advancedpopup layout: Add a new super advanced setting to the quiz that allows the user to enter a custom page layout to use. That way they can enter anything they want there and add support for it themselves in their own theme. If they did enter something but didn't create a layout for it then it would default to standard which wouldn't be great, but hell with great functionality comes great responsibility. Introduce a means for page to pass options to the theme layouts. Then the quiz could implement settings for the display of page components (header, footer, navbar) and pass those through to the layout. The end result would be that individual admin/teacher could decide for themselves what they wanted to see. The downside to this is that we would have to draw a line for code in general, pages should NOT tinker with layouts any such decision should come from the end user(s). In summary, I am presently leaning towards an advanced popup and perhaps a new feature to offer better control. At this point I'll throw the ball back to the group and see what others think in response. Cheers Sam
          Hide
          Jason Hardin added a comment -

          My preference would be the same for the advancedpopup layout for all the reasons Sam has stated, I agree completely with them from a theme stand point I would prefer to keep the theme in charge of the layout not the module.

          I think this layout would work for a couple modules such as scorm that might want blocks and external tools if it ever used popups and blocks were required some how for the LTI object.

          Show
          Jason Hardin added a comment - My preference would be the same for the advancedpopup layout for all the reasons Sam has stated, I agree completely with them from a theme stand point I would prefer to keep the theme in charge of the layout not the module. I think this layout would work for a couple modules such as scorm that might want blocks and external tools if it ever used popups and blocks were required some how for the LTI object.
          Hide
          Tim Hunt added a comment -

          Yes, advancedpopup layout seems the way to go. Before we go any further, someone needs to post in the themes forum to ask their opinion there.

          Note that, already themes can do much more flexible things with layouts than what the theme system explicitly supports. For example, in general.php you can do something like

          if (strpos('mod-quiz', $PAGE->pagetype) === 0) {
              require_once('general-quiz.php');
          } else if (strpos('mod-forum', $PAGE->pagetype) === 0) {
              require_once('general-forum.php');
          } else {
              require_once('general-default.php');
          }
          

          However, that contrains you to have the same block-regions in each of those variant layouts - unless you do a similarly evil hack in the theme config.php.

          I'm just thinking that sort of evil hack might help Jason in the short term, while we get a proper fix, which might be 2.4-only, into Moodle core.

          Show
          Tim Hunt added a comment - Yes, advancedpopup layout seems the way to go. Before we go any further, someone needs to post in the themes forum to ask their opinion there. Note that, already themes can do much more flexible things with layouts than what the theme system explicitly supports. For example, in general.php you can do something like if (strpos('mod-quiz', $PAGE->pagetype) === 0) { require_once('general-quiz.php'); } else if (strpos('mod-forum', $PAGE->pagetype) === 0) { require_once('general-forum.php'); } else { require_once('general- default .php'); } However, that contrains you to have the same block-regions in each of those variant layouts - unless you do a similarly evil hack in the theme config.php. I'm just thinking that sort of evil hack might help Jason in the short term, while we get a proper fix, which might be 2.4-only, into Moodle core.
          Hide
          Tim Hunt added a comment -

          Note that progress is being made at MDL-31365.

          Show
          Tim Hunt added a comment - Note that progress is being made at MDL-31365 .
          Hide
          Tim Hunt added a comment -

          I think these are the remaining changes required, after MDL-31365, in order to make everything work.

          Show
          Tim Hunt added a comment - I think these are the remaining changes required, after MDL-31365 , in order to make everything work.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Aparup Banerjee added a comment -

          Thanks everyone, this and the commits from MDL-31365 have been secured in integration master now for testing here.

          (i'm guessing Mary has had a review here)

          Show
          Aparup Banerjee added a comment - Thanks everyone, this and the commits from MDL-31365 have been secured in integration master now for testing here. (i'm guessing Mary has had a review here)
          Hide
          Mary Evans added a comment -

          Yep!

          Show
          Mary Evans added a comment - Yep!
          Hide
          David Monllaó added a comment -

          Hi,

          I've tested all the themes with Linux + Chrome and Windows 7 + SafeBrowserExam and I've noticed two problems:

          • Afterburner theme + SafeBrowserExam: I can't see the questions (screenshot attached)
          • (Arialist or Brick themes) and (SafeBrowserExam or popup with some JS security): There is no quiz navigation block

          I'm failing this issue for the second point

          Show
          David Monllaó added a comment - Hi, I've tested all the themes with Linux + Chrome and Windows 7 + SafeBrowserExam and I've noticed two problems: Afterburner theme + SafeBrowserExam: I can't see the questions (screenshot attached) (Arialist or Brick themes) and (SafeBrowserExam or popup with some JS security): There is no quiz navigation block I'm failing this issue for the second point
          Hide
          Martin Vögeli added a comment -

          @David: I think I located the source for the issue with Arialist and Brick in Tim's patch [1]. Move this code [2] in [3] back from line 79 to 123 and it works! Similar changes happened in [4] and [5] but I didn't check, if they have to be reversed, too.

          [1] https://github.com/timhunt/moodle/compare/MDL-31365_master...MDL-34257
          [2] $accessmanager->setup_attempt_page($PAGE);
          [3] mod/quiz/attempt.php
          [4] mod/quiz/review.php
          [5] mod/quiz/summary.php

          Show
          Martin Vögeli added a comment - @David: I think I located the source for the issue with Arialist and Brick in Tim's patch [1] . Move this code [2] in [3] back from line 79 to 123 and it works! Similar changes happened in [4] and [5] but I didn't check, if they have to be reversed, too. [1] https://github.com/timhunt/moodle/compare/MDL-31365_master...MDL-34257 [2] $accessmanager->setup_attempt_page($PAGE); [3] mod/quiz/attempt.php [4] mod/quiz/review.php [5] mod/quiz/summary.php
          Hide
          Martin Vögeli added a comment -

          @David: Ah! Afterburner doesen't have a general.php but is has a default.php in [6]. So if we add this code [7] to [8] it seams to work just fine. I think Mary mentioned this once. But I do not remember when or where... I hope the code is still readable after I post this...

          [6] theme/afterburner/layout
          [7]
          // The pagelayout used for safebrowser and securewindow.
          'secure' => array(
          'file' => 'default.php',
          'regions' => array(),
          // 'regions' => array('side-pre'),
          // 'defaultregion' => 'side-pre',
          'options' => array('nofooter'=>true, 'nonavbar'=>true, 'nocustommenu'=>true, 'nologinlinks'=>true),
          ),

          [8] theme/afterburner/config.php

          Show
          Martin Vögeli added a comment - @David: Ah! Afterburner doesen't have a general.php but is has a default.php in [6] . So if we add this code [7] to [8] it seams to work just fine. I think Mary mentioned this once. But I do not remember when or where... I hope the code is still readable after I post this... [6] theme/afterburner/layout [7] // The pagelayout used for safebrowser and securewindow. 'secure' => array( 'file' => 'default.php', 'regions' => array(), // 'regions' => array('side-pre'), // 'defaultregion' => 'side-pre', 'options' => array('nofooter'=>true, 'nonavbar'=>true, 'nocustommenu'=>true, 'nologinlinks'=>true), ), [8] theme/afterburner/config.php
          Hide
          Mary Evans added a comment -

          I can fix Afterburner, as Martin says it needs secure in config.php
          Please don't fail it because of the theme issues I can fix those.

          Show
          Mary Evans added a comment - I can fix Afterburner, as Martin says it needs secure in config.php Please don't fail it because of the theme issues I can fix those.
          Hide
          Mary Evans added a comment -

          @Martin
          Are you sure this is correct for the layout you have it with NO block regions. According to Tim Quiz needs block regions.

          Show
          Mary Evans added a comment - @Martin Are you sure this is correct for the layout you have it with NO block regions. According to Tim Quiz needs block regions.
          Hide
          Martin Vögeli added a comment -

          @Mary: Thanks for your support! As you can see in my code snippet, I tried with and without block regions. Both worked just fine for me. In such cases, I tend to use the version with less code... If one uses block regions the solution should be aware if the theme is left-hand or right-hand oriented.

          Show
          Martin Vögeli added a comment - @Mary: Thanks for your support! As you can see in my code snippet, I tried with and without block regions. Both worked just fine for me. In such cases, I tend to use the version with less code... If one uses block regions the solution should be aware if the theme is left-hand or right-hand oriented.
          Hide
          Mary Evans added a comment -

          Martin:

          When you tested yours code, had you made any changes to the files that Tim is changing now? If not, then why is Tim changing them?

          Show
          Mary Evans added a comment - Martin: When you tested yours code, had you made any changes to the files that Tim is changing now? If not, then why is Tim changing them?
          Hide
          Martin Vögeli added a comment -

          @Mary: Initially, I just tested my own patch. There I can only reproduce the issues with Afterburner. But when David reported the issue with Arialist and Brick, I locally added Tim's code to my branch "MDL-31365_master" which led to "no quiz navigation block" (and a possible solution mentioned earlier).

          Show
          Martin Vögeli added a comment - @Mary: Initially, I just tested my own patch. There I can only reproduce the issues with Afterburner. But when David reported the issue with Arialist and Brick, I locally added Tim's code to my branch " MDL-31365 _master" which led to "no quiz navigation block" (and a possible solution mentioned earlier).
          Hide
          Mary Evans added a comment - - edited

          @Martin

          In that case the BUG, in this particular tracker issue where "Quiz attempt popup doesn't respect theme's config.php layout array" must still be wrong, because it must still be using pop-up, and not the Secure layout which has Blocks. In that case then Tim's code is incorrect so this can fail and get fixed at another time. Your patch works independently and does not rely on MDL-34257 to work, so your patch in MDL-31365 should be passed.

          I think that MDL-31365 should be tested again, for what the Patch is fixing, and in your case it is to correct a different bug than this one.

          As you kept saying your Patch in MDL-31365 actually fixed MDL-34257. And for what it's worth, I think you are right.

          Show
          Mary Evans added a comment - - edited @Martin In that case the BUG, in this particular tracker issue where "Quiz attempt popup doesn't respect theme's config.php layout array" must still be wrong, because it must still be using pop-up, and not the Secure layout which has Blocks. In that case then Tim's code is incorrect so this can fail and get fixed at another time. Your patch works independently and does not rely on MDL-34257 to work, so your patch in MDL-31365 should be passed. I think that MDL-31365 should be tested again, for what the Patch is fixing, and in your case it is to correct a different bug than this one. As you kept saying your Patch in MDL-31365 actually fixed MDL-34257 . And for what it's worth, I think you are right.
          Hide
          Martin Vögeli added a comment -

          @Mary: I'd be great if the patch got a 2nd chance! I wrote an answer back at MDL-31365.

          Show
          Martin Vögeli added a comment - @Mary: I'd be great if the patch got a 2nd chance! I wrote an answer back at MDL-31365 .
          Show
          David Monllaó added a comment - I commented in http://tracker.moodle.org/browse/MDL-31365?focusedCommentId=177231&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-177231
          Hide
          Aparup Banerjee added a comment -

          this issue is going to be reverted here based on Martin's observation above about causing no quiz navigation blocks.. and Mary's too

          Show
          Aparup Banerjee added a comment - this issue is going to be reverted here based on Martin's observation above about causing no quiz navigation blocks.. and Mary's too
          Hide
          Aparup Banerjee added a comment -

          reverted 0e708c3.

          Show
          Aparup Banerjee added a comment - reverted 0e708c3.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Tim Hunt added a comment -

          I am sure this is a good bug fix, and that it does not cause the problems that were reported before when it was last integrated and tested.

          http://tracker.moodle.org/browse/MDL-34257?focusedCommentId=168003&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-168003 explains the problem, and why the code should be re-ordered to set up $PAGE sooner.

          Show
          Tim Hunt added a comment - I am sure this is a good bug fix, and that it does not cause the problems that were reported before when it was last integrated and tested. http://tracker.moodle.org/browse/MDL-34257?focusedCommentId=168003&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-168003 explains the problem, and why the code should be re-ordered to set up $PAGE sooner.
          Hide
          Tim Hunt added a comment -

          I should add, I have re-based, and re-tested, before re-submitting this.

          Show
          Tim Hunt added a comment - I should add, I have re-based, and re-tested, before re-submitting this.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks Tim, the reordering fix has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Tim, the reordering fix has been integrated now.
          Hide
          Adrian Greeve added a comment -

          Sorry. I can't test this until MDL-35754 is resolved. The pop up windows don't work and generate a JavaScript error.

          Show
          Adrian Greeve added a comment - Sorry. I can't test this until MDL-35754 is resolved. The pop up windows don't work and generate a JavaScript error.
          Hide
          Andrew Davis added a comment -

          MDL-35754 was integrated around 10:30am on Wednesday. If you are currently occupying a later point in the space time continuum it is possible to test this issue.

          Show
          Andrew Davis added a comment - MDL-35754 was integrated around 10:30am on Wednesday. If you are currently occupying a later point in the space time continuum it is possible to test this issue.
          Hide
          Dan Poltawski added a comment -

          Oh, could this issue be the cause of a warning I testing the popup on another issue:
          'Warning: array_key_exists(): The first argument should be either a string or an integer in /Users/danp/git/integration/lib/blocklib.php on line 291'

          Show
          Dan Poltawski added a comment - Oh, could this issue be the cause of a warning I testing the popup on another issue: 'Warning: array_key_exists(): The first argument should be either a string or an integer in /Users/danp/git/integration/lib/blocklib.php on line 291'
          Hide
          Adrian Greeve added a comment -

          How far back into the space time continuum will I have to go? MDL-35754 was fixing a javascript popup issue and I noticed that my current master branch has problems with popups. Does someone know when pop ups were working?

          Show
          Adrian Greeve added a comment - How far back into the space time continuum will I have to go? MDL-35754 was fixing a javascript popup issue and I noticed that my current master branch has problems with popups. Does someone know when pop ups were working?
          Hide
          Dan Poltawski added a comment -

          Adrian, have you purged caches, updated master, etc?

          Show
          Dan Poltawski added a comment - Adrian, have you purged caches, updated master, etc?
          Hide
          Adrian Greeve added a comment -

          Stupid purge caches >_> The bane of my testing career.

          Show
          Adrian Greeve added a comment - Stupid purge caches >_> The bane of my testing career.
          Hide
          Adrian Greeve added a comment -

          Tested on Master, which works fine, but branches 2.3 and 2.4 provide a warning and doesn't display any (quiz navigation, Navigation, Settings) regardless of whether allow blocks is enabled or not.
          Screen shot soon to be attached.

          Show
          Adrian Greeve added a comment - Tested on Master, which works fine, but branches 2.3 and 2.4 provide a warning and doesn't display any (quiz navigation, Navigation, Settings) regardless of whether allow blocks is enabled or not. Screen shot soon to be attached.
          Hide
          Tim Hunt added a comment -

          Adrian, it is much easier if you copy and paste error messages like that as text.

          I assume you mean "2.3 and 2.2".

          Ah, so the problem is that, on stable branches, the fix for MDL-31365 was not integrated.

          Therefore, 'secure' window is using popup layout, not secure layout. popup layout does not have blocks, so the quiz code that tries to find a block region to add the fake block to ends up with null, and that causes the exception.

          This was not a problem in the past, because of the buggy page initialisation order in attempt.php. Require-login would set the layout to in-course, which has blocks. The navigation UI would be added to one of those block regions without error. Then the pagelayout would change to popup, and the page would render without blocks.

          I guess the best way to fix this is to make a new commit on stable branches which abort trying to add the fake block if there is no region to add it to.

          Show
          Tim Hunt added a comment - Adrian, it is much easier if you copy and paste error messages like that as text. I assume you mean "2.3 and 2.2". Ah, so the problem is that, on stable branches, the fix for MDL-31365 was not integrated. Therefore, 'secure' window is using popup layout, not secure layout. popup layout does not have blocks, so the quiz code that tries to find a block region to add the fake block to ends up with null, and that causes the exception. This was not a problem in the past, because of the buggy page initialisation order in attempt.php. Require-login would set the layout to in-course, which has blocks. The navigation UI would be added to one of those block regions without error. Then the pagelayout would change to popup, and the page would render without blocks. I guess the best way to fix this is to make a new commit on stable branches which abort trying to add the fake block if there is no region to add it to.
          Hide
          Adrian Greeve added a comment -

          Sorry Tim,

          Yes. I did mean 2.2 and 2.3. For some reason I couldn't copy and paste any of the text off that page on firefox or chrome, so I went with a screen shot instead which also showed that the navigation wasn't present.

          Show
          Adrian Greeve added a comment - Sorry Tim, Yes. I did mean 2.2 and 2.3. For some reason I couldn't copy and paste any of the text off that page on firefox or chrome, so I went with a screen shot instead which also showed that the navigation wasn't present.
          Hide
          Tim Hunt added a comment -

          Right, new commits added:

          https://github.com/timhunt/moodle/commit/c9e06a0 (2.2)
          https://github.com/timhunt/moodle/commit/35f8c67 (2.3)

          on the MDL-34257_2x branches

          Note I intentionally did not apply this on master. The fact that the question navigation block was not showing up in the secure window was a serious bug. It is right that if people's themes are wrong, then this should blow-up with an exception when they try to upgrade to 2.4, so they know to fix it by adding a proper secure layout to their theme. (Except that in master we added a new layout to the base theme to cover this, so actually it will work by default.)

          However, we cannot cause exceptions like that in stable, so we need the fix-up there.

          Show
          Tim Hunt added a comment - Right, new commits added: https://github.com/timhunt/moodle/commit/c9e06a0 (2.2) https://github.com/timhunt/moodle/commit/35f8c67 (2.3) on the MDL-34257 _2x branches Note I intentionally did not apply this on master. The fact that the question navigation block was not showing up in the secure window was a serious bug. It is right that if people's themes are wrong, then this should blow-up with an exception when they try to upgrade to 2.4, so they know to fix it by adding a proper secure layout to their theme. (Except that in master we added a new layout to the base theme to cover this, so actually it will work by default.) However, we cannot cause exceptions like that in stable, so we need the fix-up there.
          Hide
          Tim Hunt added a comment -

          Adrian. Sorry. Of course, the whole point of the 'secure' window is so that masochistic teachers can stop students using copy and paste during the quiz attempt.

          However, you probably could have copy-and-pasted the text out of Firebug or equivalent.

          Show
          Tim Hunt added a comment - Adrian. Sorry. Of course, the whole point of the 'secure' window is so that masochistic teachers can stop students using copy and paste during the quiz attempt. However, you probably could have copy-and-pasted the text out of Firebug or equivalent.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          With extra commits in 22/23 stables the PHP warning seems to be out, but using standard theme (purged caches) is leading to popup with zero blocks (not fake navigation one nor the rest).

          So I'm not sure if that's the expected final behavior (sounds strange in my mind). Tim is investigating...

          Show
          Eloy Lafuente (stronk7) added a comment - With extra commits in 22/23 stables the PHP warning seems to be out, but using standard theme (purged caches) is leading to popup with zero blocks (not fake navigation one nor the rest). So I'm not sure if that's the expected final behavior (sounds strange in my mind). Tim is investigating...
          Hide
          Tim Hunt added a comment -

          I think the only really feasible option is to revert this on the stable branches, and leave well enough along. Combined we MDL-31365 we then have a proper fix in master.

          Show
          Tim Hunt added a comment - I think the only really feasible option is to revert this on the stable branches, and leave well enough along. Combined we MDL-31365 we then have a proper fix in master.
          Hide
          Dan Poltawski added a comment -

          Hi Tim,

          Was just about to do that revert, but we've some confusion due to a bad bug ID and the branch changes too I think.

          To be absolutely clear, I think we need to revert these two on 23 and 22:

          http://git.moodle.org/gw?p=integration.git;a=commit;h=d205afe662ba306833a525d87714d1994467dba7

          http://git.moodle.org/gw?p=integration.git;a=commit;h=e65f54f289f8debb5e78d59cd20b949b239351fd

          Show
          Dan Poltawski added a comment - Hi Tim, Was just about to do that revert, but we've some confusion due to a bad bug ID and the branch changes too I think. To be absolutely clear, I think we need to revert these two on 23 and 22: http://git.moodle.org/gw?p=integration.git;a=commit;h=d205afe662ba306833a525d87714d1994467dba7 http://git.moodle.org/gw?p=integration.git;a=commit;h=e65f54f289f8debb5e78d59cd20b949b239351fd
          Hide
          Tim Hunt added a comment -

          I think you should also revert the fixup commits.

          So, revert

          In both cases, the parents are the commits you identify.

          Once you are done, I would expect

          git diff

          {last week's weekly}

          {this week's weekly}

          mod/quiz/attempt.php mod/quiz/summary.php mod/quiz/review.php

          to show no changes - unless there is some other completely unrelated bug fix from this week that I have forgotten about.

          Show
          Tim Hunt added a comment - I think you should also revert the fixup commits. So, revert https://github.com/timhunt/moodle/commit/MDL-34257_23 and parent on the 2.3 branch https://github.com/timhunt/moodle/commit/MDL-34257_22 and parent on the 2.2 branch In both cases, the parents are the commits you identify. Once you are done, I would expect git diff {last week's weekly} {this week's weekly} mod/quiz/attempt.php mod/quiz/summary.php mod/quiz/review.php to show no changes - unless there is some other completely unrelated bug fix from this week that I have forgotten about.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Reverted the 2 commits (the fixups were not integrated) and confirmed that:

          • 22_STABLE: git diff c5575c63b8a2b0ce7828ea7aee17def84d9d65b5 mod/quiz/attempt.php mod/quiz/summary.php mod/quiz/review.php
          • 23_STABLE: git diff 9273bd1abb534cbcc02cd5baf1482165fcbae2b5 mod/quiz/attempt.php mod/quiz/summary.php mod/quiz/review.php

          (differences from last week's weekly in those files, once reverted, are non-existent)

          Show
          Eloy Lafuente (stronk7) added a comment - Reverted the 2 commits (the fixups were not integrated) and confirmed that: 22_STABLE: git diff c5575c63b8a2b0ce7828ea7aee17def84d9d65b5 mod/quiz/attempt.php mod/quiz/summary.php mod/quiz/review.php 23_STABLE: git diff 9273bd1abb534cbcc02cd5baf1482165fcbae2b5 mod/quiz/attempt.php mod/quiz/summary.php mod/quiz/review.php (differences from last week's weekly in those files, once reverted, are non-existent)
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Sam, Tim, I'm reopening this, coz reverting sounds to me like that. It's not clear to me if you are going to abandon any solution for 22 and 23 stables. Please, perform accordingly (closing this as fixed, continue development and resending to integration... whatever).

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Sam, Tim, I'm reopening this, coz reverting sounds to me like that. It's not clear to me if you are going to abandon any solution for 22 and 23 stables. Please, perform accordingly (closing this as fixed, continue development and resending to integration... whatever). TIA and ciao
          Hide
          Tim Hunt added a comment -

          We are giving up on stable. This will be 2.4 only.

          Show
          Tim Hunt added a comment - We are giving up on stable. This will be 2.4 only.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing as fixed, many thanks for your awesome collaboration.

          Show
          Eloy Lafuente (stronk7) added a comment - Closing as fixed, many thanks for your awesome collaboration.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: