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

Default home page for users: "user preference" not behaving as expected

    Details

    • Testing Instructions:
      Hide
      1. Log in as an admin
      2. Set defaulthomepage to 'User preference' (Settings > Site administration > Appearance > Navigation)
      3. Login as any user type e.g. 'student' and in the navigation browse to 'Home'.
      4. In settings expand My profile settings and check that there is a `Make this my default home page` link. (If not then go to My Home and check the same settings there).
      5. Do not Click this link. We want to leave the default home page to 'My Home'.
      6. Browse to any course.

      [TEST]
      Check that 'My home' is the first item in the navbar and that it links to /my/.

      • Logout and then log back in again as the same student user and check that you are taken to 'my home'.
      • Repeat steps 3 onwards but set the 'Site Home' as the default home page and test again.
      Show
      Log in as an admin Set defaulthomepage to 'User preference' (Settings > Site administration > Appearance > Navigation) Login as any user type e.g. 'student' and in the navigation browse to 'Home'. In settings expand My profile settings and check that there is a `Make this my default home page` link. (If not then go to My Home and check the same settings there). Do not Click this link. We want to leave the default home page to 'My Home'. Browse to any course. [TEST] Check that 'My home' is the first item in the navbar and that it links to /my/. Logout and then log back in again as the same student user and check that you are taken to 'my home'. Repeat steps 3 onwards but set the 'Site Home' as the default home page and test again.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-32108-master

      Description

      Default home page 'user preference' has no effect on the first page displayed after logging in. The site home is always displayed regardless of which page a user has set as their default homepage.

      1. Log in as an admin
      2. Set defaulthomepage to 'User preference' (Settings > Site administration > Appearance > Navigation)
      3. Login as any user type e.g. 'student' and in the navigation browse to 'My home'
      4. In settings expand My profile settings and check that there is a `Make this my default home page` link
      5. Click that link
      6. Browse to any course
      7. Check that 'My home' is now the first item in the navbar and that it links to /my/
      8. Logout and then log back in again as the same student user and you are taken to 'site home', when the expected landing page would be 'My home' due to the previously set 'user preference'.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              salvetore Michael de Raadt added a comment -

              I was able to replicate this behaviour.

              Show
              salvetore Michael de Raadt added a comment - I was able to replicate this behaviour.
              Hide
              yuyokk Iurii Kucherov added a comment -
              Show
              yuyokk Iurii Kucherov added a comment - Please have a look at it https://github.com/yuyokk/moodle/compare/master...MDL-32108-master
              Hide
              salvetore Michael de Raadt added a comment -

              Hi, Sam.

              It would be good to get your review of the attached patch.

              Show
              salvetore Michael de Raadt added a comment - Hi, Sam. It would be good to get your review of the attached patch.
              Hide
              krisr9999 Kris Roger added a comment -

              Thanks. This is with our systems team for testing, I'll get back to you with feedback ASAP.

              Show
              krisr9999 Kris Roger added a comment - Thanks. This is with our systems team for testing, I'll get back to you with feedback ASAP.
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Hi guys,

              I've had a look at this patch now.
              While it may solve the issue in some circumstances in other circumstances it is going to cause more of problem.
              The defaulthomepage setting has four effective states for logged in users all leading to one of two home page options:

              Setting value State Effective home page
              0 (HOMEPAGE_SITE) The default home page is the Site home Site
              1 (HOMEPAGE_MY) The default home page is the My Moodle page My Moodle
              2 (HOMEPAGE_USER) The user can choose and has selected My Moodle or hasn't made a choice My Moodle
              2 (HOMEPAGE_USER) The user can choose and has selected Site home Site

              Of course for a user who is not logged in, or is logged in as guest they don't have a My Moodle page so they have to go to the site home page.

              Having spend a bit of time looking at the code around that setting it appears there are several issues there presently.
              How it should work (not how it presently works obviously) is that when a user initially logs if there isn't a return URL that has been specified they should be taken to the correct default home page based upon the states above.
              At the moment things are a real mess, in that its incorrectly checking those states, and it prematurely redirecting to the users home page before performing session tests.

              At this point I should mention I have not tested I have only read.

              The solution you have provided Lurii looks like it would cause the redirect to occur if $CFG->defaulthomepage wasn't empty and wasn't set HOMEPAGE_MY, which would mean it would redirect to the /my/ page if $CFG->defaulthomepage were set to User preference. However it would redirect there regardless of the users selected preference. My apologies if I've missed something there.

              Certainly that whole area of code looks like its buggy, and it would be nice to solve the premature redirection issue at the same time.
              From memory there is a session variable used to provide a redirection after successful session tests that could be set to the correct default home page for the user.
              One more thing, there is a convenient function for working out the users default home page, I have not clue why this code isn't using it already.

              Cheers
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Hi guys, I've had a look at this patch now. While it may solve the issue in some circumstances in other circumstances it is going to cause more of problem. The defaulthomepage setting has four effective states for logged in users all leading to one of two home page options: Setting value State Effective home page 0 (HOMEPAGE_SITE) The default home page is the Site home Site 1 (HOMEPAGE_MY) The default home page is the My Moodle page My Moodle 2 (HOMEPAGE_USER) The user can choose and has selected My Moodle or hasn't made a choice My Moodle 2 (HOMEPAGE_USER) The user can choose and has selected Site home Site Of course for a user who is not logged in, or is logged in as guest they don't have a My Moodle page so they have to go to the site home page. Having spend a bit of time looking at the code around that setting it appears there are several issues there presently. How it should work (not how it presently works obviously) is that when a user initially logs if there isn't a return URL that has been specified they should be taken to the correct default home page based upon the states above. At the moment things are a real mess, in that its incorrectly checking those states, and it prematurely redirecting to the users home page before performing session tests. At this point I should mention I have not tested I have only read. The solution you have provided Lurii looks like it would cause the redirect to occur if $CFG->defaulthomepage wasn't empty and wasn't set HOMEPAGE_MY, which would mean it would redirect to the /my/ page if $CFG->defaulthomepage were set to User preference. However it would redirect there regardless of the users selected preference. My apologies if I've missed something there. Certainly that whole area of code looks like its buggy, and it would be nice to solve the premature redirection issue at the same time. From memory there is a session variable used to provide a redirection after successful session tests that could be set to the correct default home page for the user. One more thing, there is a convenient function for working out the users default home page, I have not clue why this code isn't using it already. Cheers Sam
              Hide
              yuyokk Iurii Kucherov added a comment - - edited
              Show
              yuyokk Iurii Kucherov added a comment - - edited Please have a look at it https://github.com/yuyokk/moodle/commit/15fae6f5351809382cc28bf8a4675d0f5cd3e4d2 Thanks iurii
              Hide
              krisr9999 Kris Roger added a comment -

              Hi Iurii,

              We've tested your latest patch and this does seem to fix it for non-admin users. I haven't yet spotted any untoward side-effects. Anyone else? I probably need to get a few more people to test it here too.

              Thanks,

              Kris.

              Show
              krisr9999 Kris Roger added a comment - Hi Iurii, We've tested your latest patch and this does seem to fix it for non-admin users. I haven't yet spotted any untoward side-effects. Anyone else? I probably need to get a few more people to test it here too. Thanks, Kris.
              Hide
              chrisf Chris Fryer added a comment -

              Just to confirm, I've tested this patch again with Moodle 2011120503.04 [2.2.3+ (Build: 20120601)] and it works as expected.

              Show
              chrisf Chris Fryer added a comment - Just to confirm, I've tested this patch again with Moodle 2011120503.04 [2.2.3+ (Build: 20120601)] and it works as expected.
              Hide
              chrisf Chris Fryer added a comment -

              What is this bug's relationship to MDL-27278?

              Show
              chrisf Chris Fryer added a comment - What is this bug's relationship to MDL-27278 ?
              Hide
              andyjdavis Andrew Davis added a comment -

              Does this whole section...

              $home_page = get_home_page();
                  /// Go to my-moodle page instead of site homepage if defaulthomepage set to homepage_my
                      if ($home_page == HOMEPAGE_MY && !is_siteadmin() && !isguestuser()) {
                          if ($urltogo == $CFG->wwwroot or $urltogo == $CFG->wwwroot.'/' or $urltogo == $CFG->wwwroot.'/index.php') {
                              $urltogo = $CFG->wwwroot.'/my/';
                          }
                      }

              want to go inside the else clause above it? As I read it it looks like students will always get sent to $CFG->wwwroot.'/my/'; even if their user isn't fully set up or if $SESSION->wantsurl is set as we're overwriting the value in $urltogo.

              Show
              andyjdavis Andrew Davis added a comment - Does this whole section... $home_page = get_home_page(); /// Go to my-moodle page instead of site homepage if defaulthomepage set to homepage_my if ($home_page == HOMEPAGE_MY && !is_siteadmin() && !isguestuser()) { if ($urltogo == $CFG->wwwroot or $urltogo == $CFG->wwwroot.'/' or $urltogo == $CFG->wwwroot.'/index.php') { $urltogo = $CFG->wwwroot.'/my/'; } } want to go inside the else clause above it? As I read it it looks like students will always get sent to $CFG->wwwroot.'/my/'; even if their user isn't fully set up or if $SESSION->wantsurl is set as we're overwriting the value in $urltogo.
              Hide
              abgreeve Adrian Greeve added a comment -

              Hi Andrew,

              The way that the code was doesn't overwrite the $urltogo for the $SESSION->wantsurl, but there might be issues with the user not being fully set up. I've moved the code into the above else clause.

              If you would be so kind as to have another look at it please.

              Show
              abgreeve Adrian Greeve added a comment - Hi Andrew, The way that the code was doesn't overwrite the $urltogo for the $SESSION->wantsurl, but there might be issues with the user not being fully set up. I've moved the code into the above else clause. If you would be so kind as to have another look at it please.
              Hide
              andyjdavis Andrew Davis added a comment -

              I think you're ready to go for integration.

              Show
              andyjdavis Andrew Davis added a comment - I think you're ready to go for integration.
              Hide
              abgreeve Adrian Greeve added a comment -

              Thanks Andrew.

              Submitting for integration.

              Show
              abgreeve Adrian Greeve added a comment - Thanks Andrew. Submitting for integration.
              Hide
              stronk7 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
              stronk7 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
              nebgor Aparup Banerjee added a comment -

              Thanks Adrian, thats been integrated into master and picked into 22 and 23.

              I've tested this and the logging back in stuff works fine for me now.

              Show
              nebgor Aparup Banerjee added a comment - Thanks Adrian, thats been integrated into master and picked into 22 and 23. I've tested this and the logging back in stuff works fine for me now.
              Hide
              nebgor Aparup Banerjee added a comment -

              passed!

              ps: 'Home' in test means 'Site Home'.

              Show
              nebgor Aparup Banerjee added a comment - passed! ps: 'Home' in test means 'Site Home'.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              I'm so proud...of you, many thanks!

              http://youtu.be/n64CdfDRnZY

              Closing as fixed, ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao
              Hide
              chrisf Chris Fryer added a comment - - edited

              I don't think this is fixed. Since the stanza was moved into the "else" clause above it, all users are sent to site home.

              If $SESSION->wantsurl === $CFG->wwwroot . '/' you end up in the "elseif" clause:

              } else if (isset($SESSION->wantsurl) and (strpos($SESSION->wantsurl, $CFG->wwwroot) === 0 or strpos($SESSION->wantsurl, str_replace('http://', 'https://', $CFG->wwwroot)) === 0)) {
                  $urltogo = $SESSION->wantsurl;    /// Because it's an address in this site
                  unset($SESSION->wantsurl);

              because strpos($SESSION->wantsurl, $CFG->wwwroot) === 0 in that case. Something earlier in the sequence is setting $SESSION->wantsurl, possibly require_login() in moodlelib.php because we force login at our site.

              I think there are two possible fixes. Either detect the fact that $SESSION->wantsurl === $CFG->wwwroot . '/' in the "elseif" clause above, or move the whole homepage detecting code outside the if .. elseif .. else and place it in its own clause as below:

              if ($urltogo === $CFG->wwwroot.'/') {
                  $home_page = get_home_page();
                  // Go to my-moodle page instead of site homepage if defaulthomepage set to homepage_my
                  if ($home_page == HOMEPAGE_MY && !is_siteadmin() && !isguestuser()) {
                      if ($urltogo == $CFG->wwwroot or $urltogo == $CFG->wwwroot.'/' or $urltogo == $CFG->wwwroot.'/index.php') {
                          $urltogo = $CFG->wwwroot.'/my/';
                      }
                  }
              }

              Show
              chrisf Chris Fryer added a comment - - edited I don't think this is fixed. Since the stanza was moved into the "else" clause above it, all users are sent to site home. If $SESSION->wantsurl === $CFG->wwwroot . '/' you end up in the "elseif" clause: } else if (isset($SESSION->wantsurl) and (strpos($SESSION->wantsurl, $CFG->wwwroot) === 0 or strpos($SESSION->wantsurl, str_replace('http://', 'https://', $CFG->wwwroot)) === 0)) { $urltogo = $SESSION->wantsurl; /// Because it's an address in this site unset($SESSION->wantsurl); because strpos($SESSION->wantsurl, $CFG->wwwroot) === 0 in that case. Something earlier in the sequence is setting $SESSION->wantsurl , possibly require_login() in moodlelib.php because we force login at our site. I think there are two possible fixes. Either detect the fact that $SESSION->wantsurl === $CFG->wwwroot . '/' in the "elseif" clause above, or move the whole homepage detecting code outside the if .. elseif .. else and place it in its own clause as below: if ($urltogo === $CFG->wwwroot.'/') { $home_page = get_home_page(); // Go to my-moodle page instead of site homepage if defaulthomepage set to homepage_my if ($home_page == HOMEPAGE_MY && !is_siteadmin() && !isguestuser()) { if ($urltogo == $CFG->wwwroot or $urltogo == $CFG->wwwroot.'/' or $urltogo == $CFG->wwwroot.'/index.php') { $urltogo = $CFG->wwwroot.'/my/'; } } }
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Chris - probably best to create a new bug in the tracker and reference this bug in your comments.

              Show
              danmarsden Dan Marsden added a comment - Hi Chris - probably best to create a new bug in the tracker and reference this bug in your comments.
              Hide
              chrisf Chris Fryer added a comment -

              OK, I created a new issue as requested. Watchers of this bug, please see MDL-37983 and vote it up if you want this fixed.

              Show
              chrisf Chris Fryer added a comment - OK, I created a new issue as requested. Watchers of this bug, please see MDL-37983 and vote it up if you want this fixed.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    10/Sep/12