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

          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: