Moodle
  1. Moodle
  2. MDL-31316

Blocks controls on the site page are redirecting to /my when default home is My Moodle

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.7, 2.1, 2.2, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Blocks, My home
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide
      1. Log in as Admin go to Home ▶ Site administration ▶ Appearance ▶ Navigation
      2. Set "Default Home page for users" to 'My Moodle'
      3. Click the 'Site home' link
      4. Check you are presented with http://example/?redirect=0
      5. Click on any editing icon in any block
      6. You are being redirected to corresponding action (not http://example/my)
      Show
      Log in as Admin go to Home ▶ Site administration ▶ Appearance ▶ Navigation Set "Default Home page for users" to 'My Moodle' Click the 'Site home' link Check you are presented with http://example/?redirect=0 Click on any editing icon in any block You are being redirected to corresponding action (not http://example/my )
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31316-master-4

      Description

      As per subject. The reason is that redirect parameter is not being passed over and /index.php redirects to My Moodle.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Ruslan Kabalin added a comment -

            Can be cleanly cherry-picked to all affected branches

            Show
            Ruslan Kabalin added a comment - Can be cleanly cherry-picked to all affected branches
            Hide
            Michael de Raadt added a comment -

            Thanks for providing a solution to this problem.

            Show
            Michael de Raadt added a comment - Thanks for providing a solution to this problem.
            Hide
            Ruslan Kabalin added a comment -

            Made some changes, it appeared that previous fix was not complete. Also changed the title as the bug affects add new block dropdown form as well.

            Show
            Ruslan Kabalin added a comment - Made some changes, it appeared that previous fix was not complete. Also changed the title as the bug affects add new block dropdown form as well.
            Hide
            Rajesh Taneja added a comment -

            Hello Ruslan,

            Patch works fine, but seems we are not attacking the problem in right way. Problem is in logic for redirection (index.php line - 61). It will be nice to check bui_editid and if provided the don't redirect else follow the process.
            So, replacing

            else if ($CFG->defaulthomepage == HOMEPAGE_MY && optional_param('redirect', 1, PARAM_BOOL) === 1) {
            

            with

            else if ($CFG->defaulthomepage == HOMEPAGE_MY && optional_param('redirect', 1, PARAM_BOOL) === 1 && optional_param('bui_editid', -1, PARAM_INT) === -1) {
            

            will solve the issue.

            Show
            Rajesh Taneja added a comment - Hello Ruslan, Patch works fine, but seems we are not attacking the problem in right way. Problem is in logic for redirection (index.php line - 61). It will be nice to check bui_editid and if provided the don't redirect else follow the process. So, replacing else if ($CFG->defaulthomepage == HOMEPAGE_MY && optional_param('redirect', 1, PARAM_BOOL) === 1) { with else if ($CFG->defaulthomepage == HOMEPAGE_MY && optional_param('redirect', 1, PARAM_BOOL) === 1 && optional_param('bui_editid', -1, PARAM_INT) === -1) { will solve the issue.
            Hide
            Ruslan Kabalin added a comment - - edited

            Hi Rajesh, thanks for reply. I was thinking about this approach first, problem is that the bug affects all possible control actions including bui_addblock. One option will obviously be listing them all in the line you suggest, but it is not ideal. From the point of avoiding future problems when someone adds a new control element, the solution I suggest is somewhat better

            Another potential problem is that some actions have two steps and not only redirect from the line you mentioned: when you click on the action, it is accessing the page with the parameters, processing and then redirects from (pagelib.php:1211), I fix this via $PAGE->url definition in /index.php rather than doing another check at the point of redirection.

            Show
            Ruslan Kabalin added a comment - - edited Hi Rajesh, thanks for reply. I was thinking about this approach first, problem is that the bug affects all possible control actions including bui_addblock. One option will obviously be listing them all in the line you suggest, but it is not ideal. From the point of avoiding future problems when someone adds a new control element, the solution I suggest is somewhat better Another potential problem is that some actions have two steps and not only redirect from the line you mentioned: when you click on the action, it is accessing the page with the parameters, processing and then redirects from (pagelib.php:1211), I fix this via $PAGE->url definition in /index.php rather than doing another check at the point of redirection.
            Hide
            Rajesh Taneja added a comment -

            Thanks Ruslan,

            Sorry, I didn't check rest of the options. Although it might be nice to check for posted optional param, and if yes then don't redirect, else redirect.

            If you still wish to go with your solution, then probably you don't need to add a separate check as redirect happens when redirect (optional_param ) is 1. As you are adding "redirect = 0" in block url, you might not require changes in index.php

            Show
            Rajesh Taneja added a comment - Thanks Ruslan, Sorry, I didn't check rest of the options. Although it might be nice to check for posted optional param, and if yes then don't redirect, else redirect. If you still wish to go with your solution, then probably you don't need to add a separate check as redirect happens when redirect (optional_param ) is 1. As you are adding "redirect = 0" in block url, you might not require changes in index.php
            Hide
            Ruslan Kabalin added a comment -

            Did not quite get what you mean by "posted optional param"

            If you still wish to go with your solution, then probably you don't need to add a separate check as redirect happens when redirect (optional_param ) is 1. As you are adding "redirect = 0" in block url, you might not require changes in index.php

            I have just edited my comment above adding an explanation just before you posted this, sorry.

            Show
            Ruslan Kabalin added a comment - Did not quite get what you mean by "posted optional param" If you still wish to go with your solution, then probably you don't need to add a separate check as redirect happens when redirect (optional_param ) is 1. As you are adding "redirect = 0" in block url, you might not require changes in index.php I have just edited my comment above adding an explanation just before you posted this, sorry.
            Hide
            Rajesh Taneja added a comment -

            With optional params, I meant white list of parameters which should not redirect user to home page.
            Anyways, explanation make sense and patch looks spot-on

            Thanks Ruslan for working on it, Pushing it for integration review.

            Show
            Rajesh Taneja added a comment - With optional params, I meant white list of parameters which should not redirect user to home page. Anyways, explanation make sense and patch looks spot-on Thanks Ruslan for working on it, Pushing it for integration review.
            Hide
            Ruslan Kabalin added a comment -

            My pleasure Thanks for revision!

            Show
            Ruslan Kabalin added a comment - My pleasure Thanks for revision!
            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
            Rajesh Taneja added a comment -

            Hello Ruslan,

            I think your fix resolves MDL-30355, can you please confirm this
            Thanks.

            Show
            Rajesh Taneja added a comment - Hello Ruslan, I think your fix resolves MDL-30355 , can you please confirm this Thanks.
            Hide
            Ruslan Kabalin added a comment - - edited

            > I think your fix resolves MDL-30355, can you please confirm this

            Not able to confirm, because I can't replicate it. It works for me correctly without my patch :/

            Show
            Ruslan Kabalin added a comment - - edited > I think your fix resolves MDL-30355 , can you please confirm this Not able to confirm, because I can't replicate it. It works for me correctly without my patch :/
            Hide
            Ruslan Kabalin added a comment -

            Rebased on latest master

            Show
            Ruslan Kabalin added a comment - Rebased on latest master
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Hi Ruslan,

            looking at your changes they look perfect, so you add the redirect=0 where necessary @:

            A- The /index.php $PAGE url.
            B- All the block controls.
            C- The add block drop-down.

            Some comments:

            1) By setting A... doesn't that cause B and C to be 100% unnecessary? As far as B and C get the the $PAGE url... and it already contains "redirect=0" ... do we need the param being checked and added again?

            2) If finally there is some good reason to keep B) and C) implemented... then I guess that the detection should add one more condition, checking that the url is /index.php. Else I can imagine problems with other pages also using "redirect=0" for any purpose (note I don't know if there are more pages using "redirect=0" for anything, just imagining it).

            Thoughts? Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Hi Ruslan, looking at your changes they look perfect, so you add the redirect=0 where necessary @: A- The /index.php $PAGE url. B- All the block controls. C- The add block drop-down. Some comments: 1) By setting A... doesn't that cause B and C to be 100% unnecessary? As far as B and C get the the $PAGE url... and it already contains "redirect=0" ... do we need the param being checked and added again? 2) If finally there is some good reason to keep B) and C) implemented... then I guess that the detection should add one more condition, checking that the url is /index.php. Else I can imagine problems with other pages also using "redirect=0" for any purpose (note I don't know if there are more pages using "redirect=0" for anything, just imagining it). Thoughts? Ciao
            Hide
            Ruslan Kabalin added a comment -

            Hello Eloy,

            1) By setting A... doesn't that cause B and C to be 100% unnecessary? As far as B and C get the the $PAGE url... and it already contains "redirect=0" ... do we need the param being checked and added again?

            Regarding B and C, you are right, they can be removed. Shame on me, I have not spotted it and offered less efficient solution. 'A' was the last change I did and it was against the second redirection when action is performed, but I did not realise that first two changes could be removed. Thanks for pointing that out. I will push updated version in a moment.

            Thanks again, Ruslan

            Show
            Ruslan Kabalin added a comment - Hello Eloy, 1) By setting A... doesn't that cause B and C to be 100% unnecessary? As far as B and C get the the $PAGE url... and it already contains "redirect=0" ... do we need the param being checked and added again? Regarding B and C, you are right, they can be removed. Shame on me, I have not spotted it and offered less efficient solution. 'A' was the last change I did and it was against the second redirection when action is performed, but I did not realise that first two changes could be removed. Thanks for pointing that out. I will push updated version in a moment. Thanks again, Ruslan
            Hide
            Ruslan Kabalin added a comment -

            Branch has been updated, I removed B and C as per Eloy's suggestion. Tested as well.

            Show
            Ruslan Kabalin added a comment - Branch has been updated, I removed B and C as per Eloy's suggestion. Tested as well.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Integrated (21, 22 and master) thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated (21, 22 and master) thanks!
            Hide
            Ruslan Kabalin added a comment -

            My pleasure

            Show
            Ruslan Kabalin added a comment - My pleasure
            Hide
            Pooja Jain added a comment - - edited

            .

            Show
            Pooja Jain added a comment - - edited .
            Hide
            Rajesh Taneja added a comment -

            grrrr...
            Should have spotted that. Thanks Eloy.

            Show
            Rajesh Taneja added a comment - grrrr... Should have spotted that. Thanks Eloy.
            Hide
            Ankit Agarwal added a comment -

            Hi,
            This is working as expected.
            Thanks for fixing this.
            Passing
            Thanks

            Show
            Ankit Agarwal added a comment - Hi, This is working as expected. Thanks for fixing this. Passing Thanks
            Hide
            Ruslan Kabalin added a comment -

            Show
            Ruslan Kabalin added a comment -
            Hide
            Eloy Lafuente (stronk7) added a comment -

            A bit later this week, but finally your changes have been accepted and are now available in all the upstream git/cvs servers.

            Many thanks & ciao

            Show
            Eloy Lafuente (stronk7) added a comment - A bit later this week, but finally your changes have been accepted and are now available in all the upstream git/cvs servers. Many thanks & ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: