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
    • Rank:
      38817

      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'.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          I was able to replicate this behaviour.

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

          Hi, Sam.

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

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

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

          Show
          Kris Roger added a comment - Thanks. This is with our systems team for testing, I'll get back to you with feedback ASAP.
          Hide
          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
          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
          Iurii Kucherov added a comment - - edited
          Show
          Iurii Kucherov added a comment - - edited Please have a look at it https://github.com/yuyokk/moodle/commit/15fae6f5351809382cc28bf8a4675d0f5cd3e4d2 Thanks iurii
          Hide
          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
          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
          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
          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
          Chris Fryer added a comment -

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

          Show
          Chris Fryer added a comment - What is this bug's relationship to MDL-27278 ?
          Hide
          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
          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
          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
          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
          Andrew Davis added a comment -

          I think you're ready to go for integration.

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

          Thanks Andrew.

          Submitting for integration.

          Show
          Adrian Greeve added a comment - Thanks Andrew. Submitting for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

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

          TIA and ciao

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

          Thanks 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
          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
          Aparup Banerjee added a comment -

          passed!

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

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

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

          http://youtu.be/n64CdfDRnZY

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao
          Hide
          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
          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
          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
          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
          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
          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: