Moodle
  1. Moodle
  2. MDL-31365

Safe Exam Browser window is not safe

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.4
    • Fix Version/s: 2.4
    • Component/s: Quiz
    • Labels:
    • Rank:
      37882

      Description

      When a quiz is accessed in the mode of Safe Exam Browser, there is the Logout-Link and the Breadcrumb.
      These links should disappear if the quiz is only accessible with Safe Exam Browser.

      1. afterburner.png
        23 kB
      2. moodle-24-afterburner-2012-09-06.png
        44 kB
      3. moodle-24-securewindow-2012-20-08.png
        38 kB

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Marking this 'not a security issue' since there is no reason to hide the details of this bug report.

          Thanks for the report.

          Show
          Tim Hunt added a comment - Marking this 'not a security issue' since there is no reason to hide the details of this bug report. Thanks for the report.
          Hide
          Marco Lehre added a comment -

          Dear Tim

          We have got a fix for this bug for Moodle v. 2.2.x. The breadcrumb and the logout-link is no longer visible while accessing a quiz with Safe Exam Browser.

          The added code is marked with
          // Voma add start
          ...
          // Voma add end

          Could you please add the fix to the core and to the next release of Moodle

          Best regards
          Marco Lehre

          Show
          Marco Lehre added a comment - Dear Tim We have got a fix for this bug for Moodle v. 2.2.x. The breadcrumb and the logout-link is no longer visible while accessing a quiz with Safe Exam Browser. The added code is marked with // Voma add start ... // Voma add end Could you please add the fix to the core and to the next release of Moodle Best regards Marco Lehre
          Hide
          Tim Hunt added a comment -

          Would you be able to follow these instructions, and send me a patch file? http://docs.moodle.org/dev/How_to_create_a_patch

          (Of even better, submit your change using git.)

          Show
          Tim Hunt added a comment - Would you be able to follow these instructions, and send me a patch file? http://docs.moodle.org/dev/How_to_create_a_patch (Of even better, submit your change using git.)
          Hide
          Marco Lehre added a comment -

          Dear Tim

          Please find the patch file at
          https://github.com/birdy1976/moodle/commit/ed642d61ac5a39a1fbf20953f88c677cba010c65

          (git)

          Does this suit your needs?

          Best regards
          Marco

          Show
          Marco Lehre added a comment - Dear Tim Please find the patch file at https://github.com/birdy1976/moodle/commit/ed642d61ac5a39a1fbf20953f88c677cba010c65 (git) Does this suit your needs? Best regards Marco
          Hide
          Tim Hunt added a comment -

          Thanks, that makes the change much easier to review.

          I can see that the change will work, but it is really not suitable code to include in the standard Moodle release.

          For release-quality code, if you want to change the HTML that Moodle outputs, we really need to get Moodle to output the right HTML in the first place, an not use JavaScript to fix the wrong HTML later.

          Also, instead of hard-coding Safe-Exam-Browser-specific code in the quiz scripts, it would be better to put the necessary code in the setup_attempt_page method in /mod/quiz/accessrule/safebrowser/rule.php.

          Finally, whatever changes you make, it would probably be good to make the changes in both accessrule/securewindow and accessrule/safebrowser.

          Show
          Tim Hunt added a comment - Thanks, that makes the change much easier to review. I can see that the change will work, but it is really not suitable code to include in the standard Moodle release. For release-quality code, if you want to change the HTML that Moodle outputs, we really need to get Moodle to output the right HTML in the first place, an not use JavaScript to fix the wrong HTML later. Also, instead of hard-coding Safe-Exam-Browser-specific code in the quiz scripts, it would be better to put the necessary code in the setup_attempt_page method in /mod/quiz/accessrule/safebrowser/rule.php. Finally, whatever changes you make, it would probably be good to make the changes in both accessrule/securewindow and accessrule/safebrowser.
          Hide
          Marco Lehre added a comment -

          Dear Tim

          Please find the new patch file at
          https://github.com/birdy1976/moodle/commit/418b7743a13fe31d2e11686ebc9dd7296a090652

          We tried to write the code in the way you described above.

          Does this suit your needs?

          Best regards
          Marco

          Show
          Marco Lehre added a comment - Dear Tim Please find the new patch file at https://github.com/birdy1976/moodle/commit/418b7743a13fe31d2e11686ebc9dd7296a090652 We tried to write the code in the way you described above. Does this suit your needs? Best regards Marco
          Hide
          Tim Hunt added a comment -

          Will try to look soon. Sorry, busy now.

          Show
          Tim Hunt added a comment - Will try to look soon. Sorry, busy now.
          Hide
          Tim Hunt added a comment -

          It seems that the corresponding problem with 'Secure' window has been noticed and reported separately. It seems that setting the page type is not working properly.

          I think we should probably merge these two bugs, and try to get them fixed together in a good way.

          Show
          Tim Hunt added a comment - It seems that the corresponding problem with 'Secure' window has been noticed and reported separately. It seems that setting the page type is not working properly. I think we should probably merge these two bugs, and try to get them fixed together in a good way.
          Hide
          Marco Lehre added a comment -

          Dear Tim

          Please find here the patch for Moodle 2.3.x
          https://github.com/birdy1976/moodle/commit/35c3650672b33f8b388cc7611817fdabe8d1e9e2

          This code could be used for the "securewindow" as well.

          Best regards
          Marco

          Show
          Marco Lehre added a comment - Dear Tim Please find here the patch for Moodle 2.3.x https://github.com/birdy1976/moodle/commit/35c3650672b33f8b388cc7611817fdabe8d1e9e2 This code could be used for the "securewindow" as well. Best regards Marco
          Hide
          Tim Hunt added a comment -

          You cannot just introduce a new page layout type like that. Doing so will require everyone who has made a Moodle theme to change their theme.

          Show
          Tim Hunt added a comment - You cannot just introduce a new page layout type like that. Doing so will require everyone who has made a Moodle theme to change their theme.
          Hide
          Martin Vögeli added a comment - - edited

          It works out of the box with the standard themes because there is no additional CSS. Moreover, it affects only those who actually use the Safe Exam Browser (SEB) or the secure window (if we use the same layout) in the first place. So if there is an issue with a custom theme one resorts to the standard.

          Before I created the layout "safebrowser" (we better call it "secure", as it also works for the secure window) I tried to use the existing ones. But (for example) there was no way to rid the user's name from the profile link or to remove the logout. With a heavy heart, I decided to create a new layout.

          But if there really is no way this patch can be part of Moodle I'd like a nudge in the right direction. Because we really want this loophole fixed for the ever growing SEB community. So, if necessary I'll deliver a third version of the patch which will consider both past and future inputs of your part.

          PS: With this patch [1] all you have to do is change the layout from 'popup' to 'safebrowser' in [2].

          [1] https://github.com/birdy1976/moodle/commit/35c3650672b33f8b388cc7611817fdabe8d1e9e2
          [2] mod/quiz/accessrule/securewindow/rule.php

          Show
          Martin Vögeli added a comment - - edited It works out of the box with the standard themes because there is no additional CSS. Moreover, it affects only those who actually use the Safe Exam Browser (SEB) or the secure window (if we use the same layout) in the first place. So if there is an issue with a custom theme one resorts to the standard. Before I created the layout "safebrowser" (we better call it "secure", as it also works for the secure window) I tried to use the existing ones. But (for example) there was no way to rid the user's name from the profile link or to remove the logout. With a heavy heart, I decided to create a new layout. But if there really is no way this patch can be part of Moodle I'd like a nudge in the right direction. Because we really want this loophole fixed for the ever growing SEB community. So, if necessary I'll deliver a third version of the patch which will consider both past and future inputs of your part. PS: With this patch [1] all you have to do is change the layout from 'popup' to 'safebrowser' in [2] . [1] https://github.com/birdy1976/moodle/commit/35c3650672b33f8b388cc7611817fdabe8d1e9e2 [2] mod/quiz/accessrule/securewindow/rule.php
          Hide
          Tim Hunt added a comment -

          Well, you need to stop talking exclusively to me about it. The people this will affect most is people who create themes. You need to find out what they think. (http://moodle.org/mod/forum/view.php?id=46)

          Show
          Tim Hunt added a comment - Well, you need to stop talking exclusively to me about it. The people this will affect most is people who create themes. You need to find out what they think. ( http://moodle.org/mod/forum/view.php?id=46 )
          Hide
          Martin Vögeli added a comment - - edited

          All right, I've started a discussion [3] back at moodle.org in the "Themes" forum B)

          [3] http://moodle.org/mod/forum/discuss.php?d=209556

          Show
          Martin Vögeli added a comment - - edited All right, I've started a discussion [3] back at moodle.org in the "Themes" forum B) [3] http://moodle.org/mod/forum/discuss.php?d=209556
          Hide
          Martin Vögeli added a comment - - edited

          Mary Evans also suggested [3] to have a new page layout for our purpose. So I guess the patch [1] is back in business. Tim, do you have inputs concerning code quality or naming of the new page layout or can we use the patch as it is? It's really great we get some momentum into this issue!

          Show
          Martin Vögeli added a comment - - edited Mary Evans also suggested [3] to have a new page layout for our purpose. So I guess the patch [1] is back in business. Tim, do you have inputs concerning code quality or naming of the new page layout or can we use the patch as it is? It's really great we get some momentum into this issue!
          Hide
          Mary Evans added a comment - - edited

          @Tim

          If you think this is OK to run, I don't mind taking on the work, so if you want to assign it to me I'll put it in for integration this weekend ready for Mondays trawl.

          Cheers
          Mary

          Show
          Mary Evans added a comment - - edited @Tim If you think this is OK to run, I don't mind taking on the work, so if you want to assign it to me I'll put it in for integration this weekend ready for Mondays trawl. Cheers Mary
          Hide
          John Stabinger added a comment -

          Here are my comments from the forum reposted:

          This is going to produce a bug in literally hundreds of custom themes. I just can't see a new layout for such a small (no offense intended) piece of Moodle.

          I personally would be against this unless it was the absolute last resort, after you have exhausted every other avenue imaginable. Please, please rethink this.

          Show
          John Stabinger added a comment - Here are my comments from the forum reposted: This is going to produce a bug in literally hundreds of custom themes. I just can't see a new layout for such a small (no offense intended) piece of Moodle. I personally would be against this unless it was the absolute last resort, after you have exhausted every other avenue imaginable. Please, please rethink this.
          Hide
          Daniel Wahl added a comment -

          @John - I don't think it'll break custom themes, though it'll probably put some debug info at a warning level (or should...)

          There have been extra layouts added in the past and as far as I remember it falls back to the "general" or "default" layout if it hasn't been defined in the custom theme.

          Show
          Daniel Wahl added a comment - @John - I don't think it'll break custom themes, though it'll probably put some debug info at a warning level (or should...) There have been extra layouts added in the past and as far as I remember it falls back to the "general" or "default" layout if it hasn't been defined in the custom theme.
          Hide
          Martin Vögeli added a comment - - edited

          Wow, thanks for the action! @Daniel: That's exactly what happened. We tested first on localhost, then on moodle.ch and finally on moodle.let.ethz.ch. It worked fine with the standard themes. We had to tweak the ETHZ theme, but only because we want to use the SEB... @John: No offence taken!

          Show
          Martin Vögeli added a comment - - edited Wow, thanks for the action! @Daniel: That's exactly what happened. We tested first on localhost, then on moodle.ch and finally on moodle.let.ethz.ch. It worked fine with the standard themes. We had to tweak the ETHZ theme, but only because we want to use the SEB... @John: No offence taken!
          Hide
          Mary Evans added a comment -

          I find it strange that no-one queried BASE layout when that got changed in Canvas theme? As anyone noticed? Does anyone care?

          Show
          Mary Evans added a comment - I find it strange that no-one queried BASE layout when that got changed in Canvas theme? As anyone noticed? Does anyone care?
          Hide
          Martin Vögeli added a comment - - edited

          @Mary: I'm not sure what you mean. Is there a connection to the SEB issue? I can not find a Canvas theme in my Moodle 2.3.1+

          Show
          Martin Vögeli added a comment - - edited @Mary: I'm not sure what you mean. Is there a connection to the SEB issue? I can not find a Canvas theme in my Moodle 2.3.1+
          Hide
          Mary Evans added a comment -

          Sorry Martin, I should have addressed that at John. It has nothing to do with SEB, I was just pointing out that one of the main default layouts for Moodle is called 'base' the other is called 'standard'. These are the default pages used in Moodle when no layout can be found. However in some cases, 'base' is used as the actual layout. If you notice in Base theme's config.php the layout for that is defined as follows:

              // Most backwards compatible layout without the blocks - this is the layout used by default
              'base' => array(
                  'file' => 'general.php',
                  'regions' => array(),
              ),
          

          Whereas in Canvas theme (parent theme for most of Moodle CORE themes) defines is thus:

              'base' => array(
                  'file' => 'general.php',
                  'regions' => array('side-pre', 'side-post'),
                  'defaultregion' => 'side-post',
              ),
          

          Notice the block regions have been added.

          This is OK in most cases, however when using Plagiarism plugin like "Turnitin", which uses the 'base' layout, we are finding that this layout breaks themes that use Canvas as a parent. So in effect this could have been SEB's fate had you not defined a special layout.

          So what I was pointing out, albeit subliminally, was that it is better to have a special layout rather than a 'fallback' layout which may be flawed!

          Show
          Mary Evans added a comment - Sorry Martin, I should have addressed that at John. It has nothing to do with SEB, I was just pointing out that one of the main default layouts for Moodle is called 'base' the other is called 'standard'. These are the default pages used in Moodle when no layout can be found. However in some cases, 'base' is used as the actual layout. If you notice in Base theme's config.php the layout for that is defined as follows: // Most backwards compatible layout without the blocks - this is the layout used by default 'base' => array( 'file' => 'general.php', 'regions' => array(), ), Whereas in Canvas theme (parent theme for most of Moodle CORE themes) defines is thus: 'base' => array( 'file' => 'general.php', 'regions' => array('side-pre', 'side-post'), 'defaultregion' => 'side-post', ), Notice the block regions have been added. This is OK in most cases, however when using Plagiarism plugin like "Turnitin", which uses the 'base' layout, we are finding that this layout breaks themes that use Canvas as a parent. So in effect this could have been SEB's fate had you not defined a special layout. So what I was pointing out, albeit subliminally, was that it is better to have a special layout rather than a 'fallback' layout which may be flawed!
          Hide
          Mary Evans added a comment -

          @Martin
          The reason you do not see Canvas theme in the theme selector is because it is hidden. If you enable Theme Designer Mode in Site Administration > Appearance > Themes > Theme settings you will see it. Alternatively, checkout https://github.com/moodle/moodle/tree/master/theme/canvas

          Show
          Mary Evans added a comment - @Martin The reason you do not see Canvas theme in the theme selector is because it is hidden. If you enable Theme Designer Mode in Site Administration > Appearance > Themes > Theme settings you will see it. Alternatively, checkout https://github.com/moodle/moodle/tree/master/theme/canvas
          Hide
          Martin Vögeli added a comment -

          @Mary: Thx, one never stops learning! So how do we proceed with the patch? I have to admit I'm a bit at a loss here...

          Show
          Martin Vögeli added a comment - @Mary: Thx, one never stops learning! So how do we proceed with the patch? I have to admit I'm a bit at a loss here...
          Hide
          Mary Evans added a comment - - edited

          Normally, with straight forward BUG fixes, the Assignee would commit all the files necessary for the fix to be applied to the relevant branch of Moodle. In this case it would be Moodle (master).

          So what you need to do, if you have not already done so, is:

          1. Update your GITHUB Moodle and then Fast-Forward local Moodle master branch (latest version 2.4Alpha which was updated 23/08/2012)
          2. then on your Local repository with 'master' as your active branch create a new branch and call it MDL-31365_master
          3. with MDL-31365_master as your active branch amend all the files in this new branch as necessary
          4. when completed commit adding a message something like: 'MDL-31365 mod/quiz/accessrule/safebrowser: adds new page layout to base theme + fix for Safe Exam Browser'
          5. push MDL-31365_master to your GITHUB

          Once all the above has been done, then we can set this for Integration Review (I can do this part) - the next PULL is on Monday (Perth Auatralia - Sunday night in UK).

          The main thing is that you MUST make sure you are working on the current version of Moodle in your local GIT repository and then push this to your GITHUB so the actual URL will be something like:

          https://github.com/birdy1976/moodle/compare/master...MDL-31365_master
          Show
          Mary Evans added a comment - - edited Normally, with straight forward BUG fixes, the Assignee would commit all the files necessary for the fix to be applied to the relevant branch of Moodle. In this case it would be Moodle (master). So what you need to do, if you have not already done so, is: Update your GITHUB Moodle and then Fast-Forward local Moodle master branch (latest version 2.4Alpha which was updated 23/08/2012) then on your Local repository with 'master' as your active branch create a new branch and call it MDL-31365 _master with MDL-31365 _master as your active branch amend all the files in this new branch as necessary when completed commit adding a message something like: ' MDL-31365 mod/quiz/accessrule/safebrowser: adds new page layout to base theme + fix for Safe Exam Browser' push MDL-31365 _master to your GITHUB Once all the above has been done, then we can set this for Integration Review (I can do this part) - the next PULL is on Monday (Perth Auatralia - Sunday night in UK). The main thing is that you MUST make sure you are working on the current version of Moodle in your local GIT repository and then push this to your GITHUB so the actual URL will be something like: https: //github.com/birdy1976/moodle/compare/master...MDL-31365_master
          Hide
          Martin Vögeli added a comment - - edited

          @Mary: Thx again... I've followed your instructions for the new version of the patch [4] in compare view. I've come up with a less dramatic solution without additional file for the "secure" layout. But I had to add some code to the "general.php" layout. I'm looking forward to the integration review!

          [4] https://github.com/birdy1976/moodle/compare/master...MDL-31365_master

          Show
          Martin Vögeli added a comment - - edited @Mary: Thx again... I've followed your instructions for the new version of the patch [4] in compare view. I've come up with a less dramatic solution without additional file for the "secure" layout. But I had to add some code to the "general.php" layout. I'm looking forward to the integration review! [4] https://github.com/birdy1976/moodle/compare/master...MDL-31365_master
          Hide
          Martin Vögeli added a comment -

          @Tim: The same patch [4] takes care of MDL-34257 (securewindow). With the introduction of the layout "secure" all I had to do in [5] was to change one line from [6] to [7]. You will observe that the examinee's log-in info is still visible but devoid of profile link and log-out.

          [5] mod/quiz/accessrule/securewindow/rule.php
          [6] $page->set_pagelayout('popup');
          [7] $page->set_pagelayout('secure');

          Show
          Martin Vögeli added a comment - @Tim: The same patch [4] takes care of MDL-34257 (securewindow). With the introduction of the layout "secure" all I had to do in [5] was to change one line from [6] to [7] . You will observe that the examinee's log-in info is still visible but devoid of profile link and log-out. [5] mod/quiz/accessrule/securewindow/rule.php [6] $page->set_pagelayout('popup'); [7] $page->set_pagelayout('secure');
          Hide
          Mary Evans added a comment -

          @Tim
          I've just added the links to the new commit in Martin Vögeli's GitHub.

          I am worried now as the alterations to general.php is going to be a pain adding this to all themes, where as a single layout (secure.php) file in Base would have worked better, almost similar to embedded.php

          If you can't Peer Review this can you suggest someone who can?

          Show
          Mary Evans added a comment - @Tim I've just added the links to the new commit in Martin Vögeli's GitHub. I am worried now as the alterations to general.php is going to be a pain adding this to all themes, where as a single layout (secure.php) file in Base would have worked better, almost similar to embedded.php If you can't Peer Review this can you suggest someone who can?
          Hide
          Martin Vögeli added a comment -

          @Mary: If you want, I add those changes to the affected themes (anomaly, arialist, binarius, boxxie, brick, canvas, formal_white, formfactor, fusion, leatherbound, magazine, mymobile, nimble, overlay, serenity, sky_high, splash) and remake the patch! Luckily, standard and standardold are okay

          Show
          Martin Vögeli added a comment - @Mary: If you want, I add those changes to the affected themes (anomaly, arialist, binarius, boxxie, brick, canvas, formal_white, formfactor, fusion, leatherbound, magazine, mymobile, nimble, overlay, serenity, sky_high, splash) and remake the patch! Luckily, standard and standardold are okay
          Hide
          Mary Evans added a comment -

          Hi Martin,
          Thanks, but no thanks. If this gets approved as it is, which I am beginning to doubt, I will sett up a separate tracker issue to deal with the changes. I must admit it would have been simpler with a separate layout file which would be called from Base theme, and as most quizes are styled so nicely out of the box, there is little need to style the secure layout. As I see it, it does not even need side columns, so in effect it would just be a "content-only" page, wouldn't it? Otherwise the blocks in the SEB could have links outside the page which would not make it particularly secure.

          Just some thoughts.

          Show
          Mary Evans added a comment - Hi Martin, Thanks, but no thanks. If this gets approved as it is, which I am beginning to doubt, I will sett up a separate tracker issue to deal with the changes. I must admit it would have been simpler with a separate layout file which would be called from Base theme, and as most quizes are styled so nicely out of the box, there is little need to style the secure layout. As I see it, it does not even need side columns, so in effect it would just be a "content-only" page, wouldn't it? Otherwise the blocks in the SEB could have links outside the page which would not make it particularly secure. Just some thoughts.
          Hide
          Martin Vögeli added a comment - - edited

          @Mary: Thanks again for your input. You're right! Why do it the hard way? I've remade the patch (still [4]) once more but this time I've fixed the root of all evil... Now it should work with the majority of themes (custom & standard) without adaptation. Self-praise warning: That's a solution of striking simplicity!

          [4] https://github.com/birdy1976/moodle/compare/master...MDL-31365_master

          Show
          Martin Vögeli added a comment - - edited @Mary: Thanks again for your input. You're right! Why do it the hard way? I've remade the patch (still [4] ) once more but this time I've fixed the root of all evil... Now it should work with the majority of themes (custom & standard) without adaptation. Self-praise warning: That's a solution of striking simplicity! [4] https://github.com/birdy1976/moodle/compare/master...MDL-31365_master
          Hide
          Martin Vögeli added a comment -

          PS: Actually, there is a quiz navigation which (depending on the theme) can be found in either the left or right side column. Furthermore, because there is a setting called "Show blocks during quiz attempts" teachers may decide for themselves if they want to have additional blocks in the side columns or not.

          Show
          Martin Vögeli added a comment - PS: Actually, there is a quiz navigation which (depending on the theme) can be found in either the left or right side column. Furthermore, because there is a setting called "Show blocks during quiz attempts" teachers may decide for themselves if they want to have additional blocks in the side columns or not.
          Hide
          Martin Vögeli added a comment -

          @Tim: Are there any questions left? Can I do something more to help? How do we start the peer review process?

          Show
          Martin Vögeli added a comment - @Tim: Are there any questions left? Can I do something more to help? How do we start the peer review process?
          Hide
          Tim Hunt added a comment -

          This is looking very good. A new layout is clearly the way to go. Thanks Martin and Mary for working that out. Just one problems:

          1. Hard-coding one particular layout name in lib/outputrenderers.php is wrong.

          Instead, add a new option that can be added to theme config.php: nologinlinks => true / false. If true, then it shows who is logged in, but without the links.

          2. Hmm, actually, the way you have implemented this, using strip tags, is evil. Can't you just get it to build the login information HTML without links in the first place?

          3. This is something of an API change. I thin you should document the new layout option, and the new page type, in theme/upgrade.txt

          Show
          Tim Hunt added a comment - This is looking very good. A new layout is clearly the way to go. Thanks Martin and Mary for working that out. Just one problems: 1. Hard-coding one particular layout name in lib/outputrenderers.php is wrong. Instead, add a new option that can be added to theme config.php: nologinlinks => true / false. If true, then it shows who is logged in, but without the links. 2. Hmm, actually, the way you have implemented this, using strip tags, is evil. Can't you just get it to build the login information HTML without links in the first place? 3. This is something of an API change. I thin you should document the new layout option, and the new page type, in theme/upgrade.txt
          Hide
          Tim Hunt added a comment - - edited

          Actually, I was not quite right in what I said in the chat after the developer meeting.

          Even fdoing $this->page->layout_options['nologinlinks'] in lib/outputrenderers.php is a bit of an evil hack. It is a necessary hack, because of backwards-compatibility of themes, but we should do it slightly differently.

          Add a new argument to the function, and only use $this->page->layout_options['nologinlinks'] as a fall-back if nothing is passed in:

           
              /**
               * Return the standard string that says whether you are logged in (and switched
               * roles/logged in as another user).
               * @param bool $withlinks if false, then don't include any links in the HTML produced.
               * If not set, the default is the nologinlinks option from the theme config.php file,
               * and if that is not set, then links are included.
               * @return string HTML fragment.
               */
              public function login_info($withlinks = null) {
                  global $USER, $CFG, $DB, $SESSION;
          
                  if (during_initial_install()) {
                      return '';
                  }
          
                  if (is_null($withlinks)) {
                      $withlinks = !array_key_exists('nologinlinks', $this->page->layout_options)
                              || !$this->page->layout_options['nologinlinks']; 
                  }
          
          Show
          Tim Hunt added a comment - - edited Actually, I was not quite right in what I said in the chat after the developer meeting. Even fdoing $this->page->layout_options ['nologinlinks'] in lib/outputrenderers.php is a bit of an evil hack. It is a necessary hack, because of backwards-compatibility of themes, but we should do it slightly differently. Add a new argument to the function, and only use $this->page->layout_options ['nologinlinks'] as a fall-back if nothing is passed in: /** * Return the standard string that says whether you are logged in (and switched * roles/logged in as another user). * @param bool $withlinks if false , then don't include any links in the HTML produced. * If not set, the default is the nologinlinks option from the theme config.php file, * and if that is not set, then links are included. * @ return string HTML fragment. */ public function login_info($withlinks = null ) { global $USER, $CFG, $DB, $SESSION; if (during_initial_install()) { return ''; } if (is_null($withlinks)) { $withlinks = !array_key_exists('nologinlinks', $ this ->page->layout_options) || !$ this ->page->layout_options['nologinlinks']; }
          Hide
          Martin Vögeli added a comment -

          Thx Mary and Tim for the chat after the developer meeting - it helped a lot! I've implemented [4] the $withlinks check slightly different with empty() [5] (the way they do it in general.php). It's easier to read... Furthermore, I've built the log-in information HTML without links in the first place

          [4] https://github.com/birdy1976/moodle/compare/master...MDL-31365_master
          [5] http://php.net/manual/en/function.empty.php

          Finally, I've documented the parameter $withlinks for login_info(), the layout option "nologinlinks" and the page layout "secure" in theme/upgrade.txt. Testing: create a quiz with enabled securewindow or safebrowser. The latter requires an installed and configured Safe Exam Browser to access the quiz.

          Show
          Martin Vögeli added a comment - Thx Mary and Tim for the chat after the developer meeting - it helped a lot! I've implemented [4] the $withlinks check slightly different with empty() [5] (the way they do it in general.php). It's easier to read... Furthermore, I've built the log-in information HTML without links in the first place [4] https://github.com/birdy1976/moodle/compare/master...MDL-31365_master [5] http://php.net/manual/en/function.empty.php Finally, I've documented the parameter $withlinks for login_info(), the layout option "nologinlinks" and the page layout "secure" in theme/upgrade.txt. Testing: create a quiz with enabled securewindow or safebrowser. The latter requires an installed and configured Safe Exam Browser to access the quiz.
          Hide
          Mary Evans added a comment -

          @Martin

          I want to try and test this out, but having problems. Do I need a SEB installed? My OS is Windows 7. Moodle is installed in my Localhost WAMP2 Server.

          I have not added your code yet as I want to see what messages I get without it.

          This is what I am seeing when trying to attempt a a Quiz that is set to use a SEB.

          This quiz has been configured so that students may only attempt it using the Safe Exam Browser.
          
          Grading method: Highest grade
          
          This quiz has been set up so that it may only be attempted using the Safe Exam Browser. You cannot attempt it from this web browser.
          
          Show
          Mary Evans added a comment - @Martin I want to try and test this out, but having problems. Do I need a SEB installed? My OS is Windows 7. Moodle is installed in my Localhost WAMP2 Server. I have not added your code yet as I want to see what messages I get without it. This is what I am seeing when trying to attempt a a Quiz that is set to use a SEB. This quiz has been configured so that students may only attempt it using the Safe Exam Browser. Grading method: Highest grade This quiz has been set up so that it may only be attempted using the Safe Exam Browser. You cannot attempt it from this web browser.
          Hide
          Mary Evans added a comment - - edited

          @Martin
          I have just been checking the code and find you missed off theme/base/config.php. Surely the layout for secure still needs coding under $Theme->layouts doesn't it?

          Show
          Mary Evans added a comment - - edited @Martin I have just been checking the code and find you missed off theme/base/config.php. Surely the layout for secure still needs coding under $Theme->layouts doesn't it?
          Hide
          Martin Vögeli added a comment -

          @Mary I: You may test it without SEB. But then you have to set "Full screen pop-up with some JavaScript security" for the "Browser security" option in the quiz settings. Thus you'll use the "securewindow" feature instead of "safebrowser" which looks exactly the same once you attempt the quiz.

          @Mary II: Oops! Sorry, it worked locally when I tested it but I forgot "git add theme/base/config.php" in my commit. Such things happen all the time if one tries to fix bugs in the dead of night... Thanks for pointing it out to me. I hope everything is okay now. Otherwise, you know how to reach me B)

          Show
          Martin Vögeli added a comment - @Mary I: You may test it without SEB. But then you have to set "Full screen pop-up with some JavaScript security" for the "Browser security" option in the quiz settings. Thus you'll use the "securewindow" feature instead of "safebrowser" which looks exactly the same once you attempt the quiz. @Mary II: Oops! Sorry, it worked locally when I tested it but I forgot "git add theme/base/config.php" in my commit. Such things happen all the time if one tries to fix bugs in the dead of night... Thanks for pointing it out to me. I hope everything is okay now. Otherwise, you know how to reach me B)
          Hide
          Tim Hunt added a comment -

          That is looking pretty good. My only concern is that you have not fully implemented the $withlinks = false case. I don't think it is good enough to only deal with the situation that you care about. We need a complete implementation. Try not to make the code too much more complex than it already is when you do that.

          Show
          Tim Hunt added a comment - That is looking pretty good. My only concern is that you have not fully implemented the $withlinks = false case. I don't think it is good enough to only deal with the situation that you care about. We need a complete implementation. Try not to make the code too much more complex than it already is when you do that.
          Hide
          Martin Vögeli added a comment - - edited

          @Tim: I'm not sure if I got you right. Did you mean something along those lines [8]? It's only my preliminary code. Hope, that's what you meant! If I'm on the right track I'll test it more thoroughly after a good night's sleep and I'll publish it once more here [4]. I guess you know this link by heart now...

          [8] https://github.com/birdy1976/moodle/compare/master...mytest

          Show
          Martin Vögeli added a comment - - edited @Tim: I'm not sure if I got you right. Did you mean something along those lines [8] ? It's only my preliminary code. Hope, that's what you meant! If I'm on the right track I'll test it more thoroughly after a good night's sleep and I'll publish it once more here [4] . I guess you know this link by heart now... [8] https://github.com/birdy1976/moodle/compare/master...mytest
          Hide
          Tim Hunt added a comment -

          Well, code using ? : operator is never going to win any beauty contests. The alternative would be to build two strings, $loggedinas and $loginlink in the main, mulit-branch link. Then at the end, do

          if ($withlinks)

          { $loggedinas = $loggedinas . ' (' . $loginlink . ')'; }

          That does not quite handle everything. You need a few extra ifs elsewhere. Anwyay, taht is my best thought for not making this code too much more complex.

          Show
          Tim Hunt added a comment - Well, code using ? : operator is never going to win any beauty contests. The alternative would be to build two strings, $loggedinas and $loginlink in the main, mulit-branch link. Then at the end, do if ($withlinks) { $loggedinas = $loggedinas . ' (' . $loginlink . ')'; } That does not quite handle everything. You need a few extra ifs elsewhere. Anwyay, taht is my best thought for not making this code too much more complex.
          Hide
          Martin Vögeli added a comment -

          @Tim: Thx for your input! In place of $loginlink I had to decide from if to if whether or not I add links or leave it be. Here [4] you'll find my renewed patch.

          [4] https://github.com/birdy1976/moodle/compare/master...MDL-31365_master

          Show
          Martin Vögeli added a comment - @Tim: Thx for your input! In place of $loginlink I had to decide from if to if whether or not I add links or leave it be. Here [4] you'll find my renewed patch. [4] https://github.com/birdy1976/moodle/compare/master...MDL-31365_master
          Hide
          Tim Hunt added a comment -

          Yes, that is much better. Just one more thing: http://docs.moodle.org/dev/Coding_style#If_.2F_else. It needs to be "if ($withlinks) {" not "if($withlinks){". The integrators care about that (there is an automated tool https://github.com/moodlehq/moodle-local_codechecker for checking it.)

          But, with the addition of a few spaces, this should be ready for integration.

          Show
          Tim Hunt added a comment - Yes, that is much better. Just one more thing: http://docs.moodle.org/dev/Coding_style#If_.2F_else . It needs to be "if ($withlinks) {" not "if($withlinks){". The integrators care about that (there is an automated tool https://github.com/moodlehq/moodle-local_codechecker for checking it.) But, with the addition of a few spaces, this should be ready for integration.
          Hide
          Martin Vögeli added a comment -

          I've installed the checker, added missing and removed additional spaces but only in the code we've created or modified. You'll find the purified patch here [4]. Thanks Mary and Tim for your endurance! I learned a lot about Git, themes and coding-style during the making of this patch B)

          [4] https://github.com/birdy1976/moodle/compare/master...MDL-31365_master

          Show
          Martin Vögeli added a comment - I've installed the checker, added missing and removed additional spaces but only in the code we've created or modified. You'll find the purified patch here [4] . Thanks Mary and Tim for your endurance! I learned a lot about Git, themes and coding-style during the making of this patch B) [4] https://github.com/birdy1976/moodle/compare/master...MDL-31365_master
          Hide
          Mary Evans added a comment - - edited

          Thanks Martin, you have done a brilliant job.

          It looks OK to me.

          My only worry now is that with Moodle being updated, MDL-31365_master will need to be rebased, but I hope I am wrong as I know re-basing can open-up a nest of vipers, with conflicts and such.

          We don't like to rebase often, although in most cases it's easy, however, sometimes it can be a nightmare of a thing, which I tend to abort if ever I encounter conflicts.

          I have set it for Integration review which will be this weekend.

          Show
          Mary Evans added a comment - - edited Thanks Martin, you have done a brilliant job. It looks OK to me. My only worry now is that with Moodle being updated, MDL-31365 _master will need to be rebased, but I hope I am wrong as I know re-basing can open-up a nest of vipers, with conflicts and such. We don't like to rebase often, although in most cases it's easy, however, sometimes it can be a nightmare of a thing, which I tend to abort if ever I encounter conflicts. I have set it for Integration review which will be this weekend.
          Hide
          Tim Hunt added a comment -

          Thanks for sticking with it. Hopefully it will be easier next time.

          Show
          Tim Hunt added a comment - Thanks for sticking with it. Hopefully it will be easier next time.
          Hide
          Mary Evans added a comment -

          @ Martin and/or Tim

          Can you suggest a few testing instructions for me to add to what I started at the top of the page?

          Thanks

          Show
          Mary Evans added a comment - @ Martin and/or Tim Can you suggest a few testing instructions for me to add to what I started at the top of the page? Thanks
          Hide
          Tim Hunt added a comment -

          Good point.

          Actually, it is tricky to test this in Moodle core before MDL-34257 is integrated. Perhaps I should try to get that done today.

          Show
          Tim Hunt added a comment - Good point. Actually, it is tricky to test this in Moodle core before MDL-34257 is integrated. Perhaps I should try to get that done today.
          Hide
          Mary Evans added a comment -

          It would help.

          Show
          Mary Evans added a comment - It would help.
          Hide
          Martin Vögeli added a comment - - edited

          @Tim: It looks as if I've fixed MDL-34257 as well:

          All I had to do in [5] was to change one line from [6] to [7].

          [5] mod/quiz/accessrule/securewindow/rule.php
          [6] $page->set_pagelayout('popup');
          [7] $page->set_pagelayout('secure');

          Show
          Martin Vögeli added a comment - - edited @Tim: It looks as if I've fixed MDL-34257 as well: All I had to do in [5] was to change one line from [6] to [7] . [5] mod/quiz/accessrule/securewindow/rule.php [6] $page->set_pagelayout('popup'); [7] $page->set_pagelayout('secure');
          Hide
          Tim Hunt added a comment -

          Hang on. This patch does not work.

          Did you test with developer debug turned on?

          The quiz requires there to be at least one block region on the attempt page, so it can display the navigation. Therefore, we need something like:

          diff --git a/theme/base/config.php b/theme/base/config.php
          index 1316822..cafa7fb 100644
          --- a/theme/base/config.php
          +++ b/theme/base/config.php
          @@ -162,7 +162,8 @@ $THEME->layouts = array(
               // The pagelayout used for safebrowser and securewindow.
               'secure' => array(
                   'file' => 'general.php',
          -        'regions' => array(),
          +        'regions' => array('side-pre'),
          +        'defaultregion' => 'side-pre',
                   'options' => array('nofooter'=>true, 'nonavbar'=>true, 'nocustommenu'=>     ),
           );
          

          in addition to the other changes.

          Show
          Tim Hunt added a comment - Hang on. This patch does not work. Did you test with developer debug turned on? The quiz requires there to be at least one block region on the attempt page, so it can display the navigation. Therefore, we need something like: diff --git a/theme/base/config.php b/theme/base/config.php index 1316822..cafa7fb 100644 --- a/theme/base/config.php +++ b/theme/base/config.php @@ -162,7 +162,8 @@ $THEME->layouts = array( // The pagelayout used for safebrowser and securewindow. 'secure' => array( 'file' => 'general.php', - 'regions' => array(), + 'regions' => array('side-pre'), + 'defaultregion' => 'side-pre', 'options' => array('nofooter'=> true , 'nonavbar'=> true , 'nocustommenu'=> ), ); in addition to the other changes.
          Hide
          Tim Hunt added a comment -

          Actually, I will submit that change, along with the rest of the fix to MDL-34257.

          Show
          Tim Hunt added a comment - Actually, I will submit that change, along with the rest of the fix to MDL-34257 .
          Hide
          Martin Vögeli added a comment -

          @Mary: Test Instructions:

          Login as Admin

          1. IF using Safe Exam Browser (SEB), go to Site Administration > Development > Experimental > Experimental settings and set 'Enable Safe Exam Browser' to YES (put tick to box).
          2. Go to the course page create a quiz with a few questions and set "Browser security" in the quiz settings to "Require the use of Safe Exame Browser" and save the settings.

          Login as Student

          1. Install and configure SEB for your Moodle quiz on the student's desktop or notebook. Otherwise access to the quiz will be denied on behalf of Moodle.
          2. Start SEB and login with your student's credentials. Now the quiz should open automatically. Check if there is no footer, no navbar, no custom menu and no login links.

          Shortcut test without SEB

          1. Teacher: Set the "Browser security" option in the quiz settings to "Full screen pop-up with some JavaScript security" (i.e. without SEB).
          2. Student: Now students may use a regular browser to access the quiz, which will open in a new popup window with the exact same page layout (secure).
          Show
          Martin Vögeli added a comment - @Mary: Test Instructions: Login as Admin IF using Safe Exam Browser (SEB), go to Site Administration > Development > Experimental > Experimental settings and set 'Enable Safe Exam Browser' to YES (put tick to box). Go to the course page create a quiz with a few questions and set "Browser security" in the quiz settings to "Require the use of Safe Exame Browser" and save the settings. Login as Student Install and configure SEB for your Moodle quiz on the student's desktop or notebook. Otherwise access to the quiz will be denied on behalf of Moodle. Start SEB and login with your student's credentials. Now the quiz should open automatically. Check if there is no footer, no navbar, no custom menu and no login links. Shortcut test without SEB Teacher: Set the "Browser security" option in the quiz settings to "Full screen pop-up with some JavaScript security" (i.e. without SEB). Student : Now students may use a regular browser to access the quiz, which will open in a new popup window with the exact same page layout (secure).
          Hide
          Martin Vögeli added a comment -

          @Tim: I set "debug messages" to "developer" and enabled "display debug messages" but there was no error message. Besides, the quiz navigation was there during my tests in both safebrowser and securewindow. I think, that's why it's good to test patches on several independent machines...

          Show
          Martin Vögeli added a comment - @Tim: I set "debug messages" to "developer" and enabled "display debug messages" but there was no error message. Besides, the quiz navigation was there during my tests in both safebrowser and securewindow. I think, that's why it's good to test patches on several independent machines...
          Hide
          Tim Hunt added a comment -

          Did you test standard theme? The I don't understand how the block appeard with the list of block regions in theme/base/config.php being empty.

          Show
          Tim Hunt added a comment - Did you test standard theme? The I don't understand how the block appeard with the list of block regions in theme/base/config.php being empty.
          Hide
          Martin Vögeli added a comment -

          Yes, I also tried "formal white", "sky hight" and other themes.

          Show
          Martin Vögeli added a comment - Yes, I also tried "formal white", "sky hight" and other themes.
          Hide
          Tim Hunt added a comment -

          Well, I definitely got the error, and my changes in the other bug fix it, so no harm done.

          Show
          Tim Hunt added a comment - Well, I definitely got the error, and my changes in the other bug fix it, so no harm done.
          Hide
          Mary Evans added a comment -

          @Martin

          In your previous version of 'secure' layout in theme/base/config.php I am sure the regions were specified. I queried it if you recall? Tim is correct in saying that the regions need to be specified in layout in config.php. Did you change them when you re-added config.php to MDL-31365_master?

          Something strange is happening here. Do either of you have Theme Designer Mode enabled at all? Or perhaps Tim Purged caches and Martin didn't after making changes? Or visa versa?

          Show
          Mary Evans added a comment - @Martin In your previous version of 'secure' layout in theme/base/config.php I am sure the regions were specified. I queried it if you recall? Tim is correct in saying that the regions need to be specified in layout in config.php. Did you change them when you re-added config.php to MDL-31365 _master? Something strange is happening here. Do either of you have Theme Designer Mode enabled at all? Or perhaps Tim Purged caches and Martin didn't after making changes? Or visa versa?
          Hide
          Martin Vögeli added a comment -

          Hm, I didn't define the regions in the last three patch versions. Maybe I didn't get your point back then... I've tried again with "theme designer mode" enabled and I purged my browser cache. Everything works perfectly without the regions. I use the latest version of my patch [4] on localhost.

          Show
          Martin Vögeli added a comment - Hm, I didn't define the regions in the last three patch versions. Maybe I didn't get your point back then... I've tried again with "theme designer mode" enabled and I purged my browser cache. Everything works perfectly without the regions. I use the latest version of my patch [4] on localhost.
          Hide
          Mary Evans added a comment -

          If it works then no need to fix it!

          Show
          Mary Evans added a comment - If it works then no need to fix it!
          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
          Martin Vögeli added a comment -

          @Eloy: Thanks for the information! I've completed the rebase of my PULL branch and tested the patch with the latest modifications - works like a charm B)

          Show
          Martin Vögeli added a comment - @Eloy: Thanks for the information! I've completed the rebase of my PULL branch and tested the patch with the latest modifications - works like a charm B)
          Hide
          Aparup Banerjee added a comment -

          Thanks everyone, this and the commit from MDL-34257 has been secured in integration master now.

          Show
          Aparup Banerjee added a comment - Thanks everyone, this and the commit from MDL-34257 has been secured in integration master now.
          Hide
          Aparup Banerjee added a comment -

          ah:
          There was 1 error:

          1) core_course_external_testcase::test_duplicate_course
          Declaration of theme_mymobile_core_renderer::login_info() should be compatible with core_renderer::login_info($withlinks = NULL)

          i've added a commit fixing up that in http://git.moodle.org/gw?p=integration.git;a=commit;h=82fb0aa57635e7742f295270988142fec2f7d14a

          I'm not sure if we need any change to mymobile here (added John Stabinger just in case he see's any need)

          Show
          Aparup Banerjee added a comment - ah: There was 1 error: 1) core_course_external_testcase::test_duplicate_course Declaration of theme_mymobile_core_renderer::login_info() should be compatible with core_renderer::login_info($withlinks = NULL) i've added a commit fixing up that in http://git.moodle.org/gw?p=integration.git;a=commit;h=82fb0aa57635e7742f295270988142fec2f7d14a I'm not sure if we need any change to mymobile here (added John Stabinger just in case he see's any need)
          Hide
          Martin Vögeli added a comment -

          @Aparup: As far as "safebrowser" is concerned, no further work is needed, because there is no mobile version of the Safe Exam Browser (SEB)...

          Show
          Martin Vögeli added a comment - @Aparup: As far as "safebrowser" is concerned, no further work is needed, because there is no mobile version of the Safe Exam Browser (SEB)...
          Hide
          David Monllaó added a comment -

          Copied from MDL-34257

          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
          Show
          David Monllaó added a comment - Copied from MDL-34257 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
          Hide
          Martin Vögeli added a comment -

          @David: I wrote a comment about this issue back at MDL-34257. I hope it helps to solve this issue!

          Show
          Martin Vögeli added a comment - @David: I wrote a comment about this issue back at MDL-34257 . I hope it helps to solve this issue!
          Hide
          Martin Vögeli added a comment -

          @Aparup and David: I've tested the patch [4] once more with all standard themes. In the process I've fixed the issue with Afterburner (the way I mentioned back at MDL-34257), added the "$withlinks = null" in MyMobile and fixed an additional bug in Overlay ($hasfooter wasn't implemented at the end).

          [4] https://github.com/birdy1976/moodle/compare/master...MDL-31365_master

          Show
          Martin Vögeli added a comment - @Aparup and David: I've tested the patch [4] once more with all standard themes. In the process I've fixed the issue with Afterburner (the way I mentioned back at MDL-34257 ), added the "$withlinks = null" in MyMobile and fixed an additional bug in Overlay ($hasfooter wasn't implemented at the end). [4] https://github.com/birdy1976/moodle/compare/master...MDL-31365_master
          Hide
          David Monllaó added a comment -

          Hi Martin,

          Thanks for working on this again, I've applied your patch but I'm receiving a warning using afterburner theme

          Warning: array_key_exists(): The first argument should be either a string or an integer in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/blocklib.php on line 291 Call Stack: 0.0006 774736 
          1. {main}() /home/davidm/Desktop/moodlecode/INTEGRATION/master/mod/quiz/review.php:0 0.2790 58106832 
          2. block_manager->add_fake_block() /home/davidm/Desktop/moodlecode/INTEGRATION/master/mod/quiz/review.php:255 0.2790 58106832 
          3. block_manager->is_known_region() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/blocklib.php:426 0.2790 58106912 
          4. array_key_exists() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/blocklib.php:291 
          

          Reverting Tim's last commit (https://github.com/timhunt/moodle/commit/941f12401ea7386f3714ee27c4b4edd39dba8e62) there is no navigation block but the warning is not thrown so as Mary said (http://tracker.moodle.org/browse/MDL-34257?focusedCommentId=177153&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-177153) IMO this issue should pass and MDL-34257 should fail

          Show
          David Monllaó added a comment - Hi Martin, Thanks for working on this again, I've applied your patch but I'm receiving a warning using afterburner theme Warning: array_key_exists(): The first argument should be either a string or an integer in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/blocklib.php on line 291 Call Stack: 0.0006 774736 1. {main}() /home/davidm/Desktop/moodlecode/INTEGRATION/master/mod/quiz/review.php:0 0.2790 58106832 2. block_manager->add_fake_block() /home/davidm/Desktop/moodlecode/INTEGRATION/master/mod/quiz/review.php:255 0.2790 58106832 3. block_manager->is_known_region() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/blocklib.php:426 0.2790 58106912 4. array_key_exists() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/blocklib.php:291 Reverting Tim's last commit ( https://github.com/timhunt/moodle/commit/941f12401ea7386f3714ee27c4b4edd39dba8e62 ) there is no navigation block but the warning is not thrown so as Mary said ( http://tracker.moodle.org/browse/MDL-34257?focusedCommentId=177153&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-177153 ) IMO this issue should pass and MDL-34257 should fail
          Hide
          Martin Vögeli added a comment -

          @David: Thanks for reconsidering! When I also apply my 2nd commit to [4] (from yesterday) the navigation block is visible in Afterburner (screenshot). I never get to see your warning though (debug messages, display debug messages and theme designer mode are on). Maybe I'm looking in all the wrong places?

          Show
          Martin Vögeli added a comment - @David: Thanks for reconsidering! When I also apply my 2nd commit to [4] (from yesterday) the navigation block is visible in Afterburner (screenshot). I never get to see your warning though (debug messages, display debug messages and theme designer mode are on). Maybe I'm looking in all the wrong places?
          Hide
          David Monllaó added a comment -

          Well, after investigating a bit more, with your last patch applied:

          • I can't see the quiz navigation block in arialist, binarius, brick, fusion and nimble

          MDL-31365 and MDL-34257 are sharing testing instructions and I'm not sure where finishes one issue and begins the another.

          • From a dev. point of view, for the commits I understand that, this issue, solving the afterburner issue of course, could pass, but I'm not 100% sure since the last commit includes modifications that seems to belong to the first one.
          • From a tester point of view IMO both issues should fail, because "Test using popup with JavaScript" nor "Test using SEB" (see MDL-34257 testing instructions) are completely satisfied due to the missing quiz navigation block
          Show
          David Monllaó added a comment - Well, after investigating a bit more, with your last patch applied: It seems the afterburner warning is displayed when there are no regions in the layout, In theme/afterburner/config.php -> layouts -> secure there are no regions, so quiz navigation can not be added. With 'regions' => array('side-pre') and 'defaultregion' => 'side-pre' (like base theme does) there is no warning. It's the same commented by Tim in http://tracker.moodle.org/browse/MDL-31365?focusedCommentId=176089&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-176089 but applied to afterburner. What I can not explain is why you are not seeing this warning, maybe your debug level is not set at max. I can't see the quiz navigation block in arialist, binarius, brick, fusion and nimble MDL-31365 and MDL-34257 are sharing testing instructions and I'm not sure where finishes one issue and begins the another. From a dev. point of view, for the commits I understand that, this issue, solving the afterburner issue of course, could pass, but I'm not 100% sure since the last commit includes modifications that seems to belong to the first one. From a tester point of view IMO both issues should fail, because "Test using popup with JavaScript" nor "Test using SEB" (see MDL-34257 testing instructions) are completely satisfied due to the missing quiz navigation block
          Hide
          Mary Evans added a comment -

          I added a fix for Afterburner MDL-35269 which adds the block regions to the secure layout, as Tim and David point out, it needs the block regions to be able to see the 'false block' of the navigation in the Quiz. However, I have just closed it as it can be dealt with in your commit here Martin, you just need to add the block regions.

          I'm not so good with complex PHP as can be found in most of Moodle core files, but perhaps there should be a setting in blocklib.php that allows no 'real' blocks, but makes an exception for this 'false' block in the quiz. Is that possible? Then the secure layout would not need block regions as Martin has it in his latest commit https://github.com/birdy1976/moodle/commit/b1985cf382d6bdb0a7156a11f991da068da0ae73 or is that a silly idea?

          Show
          Mary Evans added a comment - I added a fix for Afterburner MDL-35269 which adds the block regions to the secure layout, as Tim and David point out, it needs the block regions to be able to see the 'false block' of the navigation in the Quiz. However, I have just closed it as it can be dealt with in your commit here Martin, you just need to add the block regions. I'm not so good with complex PHP as can be found in most of Moodle core files, but perhaps there should be a setting in blocklib.php that allows no 'real' blocks, but makes an exception for this 'false' block in the quiz. Is that possible? Then the secure layout would not need block regions as Martin has it in his latest commit https://github.com/birdy1976/moodle/commit/b1985cf382d6bdb0a7156a11f991da068da0ae73 or is that a silly idea?
          Hide
          Mary Evans added a comment -

          @David

          The reason you cannot see the navigation in arialist, binarius, brick, fusion and nimble is because these are all side-post only themes, and as such cannot use a side pre only layout. This is an easy fix as this group just need the secure layout adding to their respective config.php, but change the order of the default region to side-post.

          Show
          Mary Evans added a comment - @David The reason you cannot see the navigation in arialist, binarius, brick, fusion and nimble is because these are all side-post only themes, and as such cannot use a side pre only layout. This is an easy fix as this group just need the secure layout adding to their respective config.php, but change the order of the default region to side-post.
          Hide
          Martin Vögeli added a comment -

          @David: My debug level is "developer" (Ubuntu 12.04, PHP 5.3.10). I've now added the regions to the pagelayout. Arialist, Binarius, Brick, Fusion and Nimble use both Base and Canvas as parents. So I've added the new pagelayout to Canvas for good measure. Still the quiz navigation block shows, no warnings.

          Now that the regions are included one may test MDL-31365 independently of MDL-34257. Once we've cleaned MDL-31365 further work can be added on top of it (if need be). But it is still my opinion, that it fixes both issues (and an small additional one for Overlay ignoring $hasfooter in layout/general.php).

          Show
          Martin Vögeli added a comment - @David: My debug level is "developer" (Ubuntu 12.04, PHP 5.3.10). I've now added the regions to the pagelayout. Arialist, Binarius, Brick, Fusion and Nimble use both Base and Canvas as parents. So I've added the new pagelayout to Canvas for good measure. Still the quiz navigation block shows, no warnings. Now that the regions are included one may test MDL-31365 independently of MDL-34257 . Once we've cleaned MDL-31365 further work can be added on top of it (if need be). But it is still my opinion, that it fixes both issues (and an small additional one for Overlay ignoring $hasfooter in layout/general.php).
          Hide
          Martin Vögeli added a comment -

          @Mary: I've added the regions like this [9]. This seems to solve it. Now branch MDL-31365_master has three commits which all contribute to fix the bug. I wouldn't prohibit other blocks, because teachers want to see the course navigation and there is the quiz setting "Show blocks during quiz attempts"...

          [9]

                  'regions' => array('side-pre', 'side-post'),
                  'defaultregion' => 'side-pre',
          
          Show
          Martin Vögeli added a comment - @Mary: I've added the regions like this [9] . This seems to solve it. Now branch MDL-31365 _master has three commits which all contribute to fix the bug. I wouldn't prohibit other blocks, because teachers want to see the course navigation and there is the quiz setting "Show blocks during quiz attempts"... [9] 'regions' => array('side-pre', 'side-post'), 'defaultregion' => 'side-pre',
          Hide
          Aparup Banerjee added a comment -

          ok if thats all, then (as discussed with David too) , i'll integrate the further fixes and mark this as passed.

          Show
          Aparup Banerjee added a comment - ok if thats all, then (as discussed with David too) , i'll integrate the further fixes and mark this as passed.
          Hide
          Martin Vögeli added a comment -

          @Aparup: Wow, cool, thx!!!

          Show
          Martin Vögeli added a comment - @Aparup: Wow, cool, thx!!!
          Hide
          Mary Evans added a comment -

          @Martin, good move with Canvas theme, that's one theme we tend to forget.

          Show
          Mary Evans added a comment - @Martin, good move with Canvas theme, that's one theme we tend to forget.
          Show
          Aparup Banerjee added a comment - so just to be clear the further fixes are : https://github.com/birdy1976/moodle/commit/b1985cf382d6bdb0a7156a11f991da068da0ae73 and https://github.com/birdy1976/moodle/commit/5f7be7a46cdb3be86643838c13d400c978c32227
          Hide
          Martin Vögeli added a comment -

          @Mary: I didn't know it existed until you told me so B)

          Show
          Martin Vögeli added a comment - @Mary: I didn't know it existed until you told me so B)
          Hide
          Martin Vögeli added a comment -

          @Aparup: Yes!

          Show
          Martin Vögeli added a comment - @Aparup: Yes!
          Hide
          Mary Evans added a comment - - edited

          @Martin

          Have you tested the side-post only themes again as these all exclude pagelayout.css from both canvas & base theme so have no means of styling the page for a side-pre. For those themes we nee to make default side-post.

          If need be I can set up a tracker to fix that by adding the layouts in each themes' config.php as mentioned earlier.

          Show
          Mary Evans added a comment - - edited @Martin Have you tested the side-post only themes again as these all exclude pagelayout.css from both canvas & base theme so have no means of styling the page for a side-pre. For those themes we nee to make default side-post. If need be I can set up a tracker to fix that by adding the layouts in each themes' config.php as mentioned earlier.
          Hide
          Martin Vögeli added a comment -

          @Mary: Yes, I've tested it for several themes. Here [10] are a few examples. They look okay and and I get no warnings B)

          [10] moodle-24-theme-shots-2012-09-06.zip

          Show
          Martin Vögeli added a comment - @Mary: Yes, I've tested it for several themes. Here [10] are a few examples. They look okay and and I get no warnings B) [10] moodle-24-theme-shots-2012-09-06.zip
          Show
          Aparup Banerjee added a comment - ok https://github.com/birdy1976/moodle/commit/b1985cf382d6bdb0a7156a11f991da068da0ae73 and https://github.com/birdy1976/moodle/commit/5f7be7a46cdb3be86643838c13d400c978c32227 have been integrated.
          Hide
          Aparup Banerjee added a comment -

          passing here for everyone's pleasure and for David (also based on enough test - it works)

          thanks for the hard work here!

          Show
          Aparup Banerjee added a comment - passing here for everyone's pleasure and for David (also based on enough test - it works) thanks for the hard work here!
          Hide
          Aparup Banerjee added a comment -

          Martin: just a note for the future, if possible, it would really help to have any patches during integration time rebased against integration.git/master or other branch to foresee any fixes . This time no complications

          Show
          Aparup Banerjee added a comment - Martin: just a note for the future, if possible, it would really help to have any patches during integration time rebased against integration.git/master or other branch to foresee any fixes . This time no complications
          Hide
          Martin Vögeli added a comment -

          @Aparup: Thanks for the advice - I'll keep that in mind!

          Show
          Martin Vögeli added a comment - @Aparup: Thanks for the advice - I'll keep that in mind!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for the hard work.

          These changes have been spread upstream and are already available in the git and cvs repositories.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work. These changes have been spread upstream and are already available in the git and cvs repositories. Ciao
          Hide
          Martin Vögeli added a comment -

          That's it, we're in... Thanks everybody, but my special thanks go to Mary and Tim!

          Show
          Martin Vögeli added a comment - That's it, we're in... Thanks everybody, but my special thanks go to Mary and Tim!
          Hide
          Mary Cooch added a comment -

          Removing the docs_required label as I think it was already documented here http://docs.moodle.org/24/en/Quiz_settings, but not working properly. I just added a screenshot. If I have missed anything vital that is new, please say.

          Show
          Mary Cooch added a comment - Removing the docs_required label as I think it was already documented here http://docs.moodle.org/24/en/Quiz_settings , but not working properly. I just added a screenshot. If I have missed anything vital that is new, please say.
          Hide
          Aparup Banerjee added a comment -

          Thanks Mary, you're right, it was already documented. (tag oops)

          Show
          Aparup Banerjee added a comment - Thanks Mary, you're right, it was already documented. (tag oops)

            People

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

              Dates

              • Created:
                Updated:
                Resolved: