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

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

    Details

    • 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

            kabalin Ruslan Kabalin created issue -
            kabalin Ruslan Kabalin made changes -
            Field Original Value New Value
            Assignee moodle.com [ moodle.com ] Ruslan Kabalin [ kabalin ]
            Hide
            kabalin Ruslan Kabalin added a comment -

            Can be cleanly cherry-picked to all affected branches

            Show
            kabalin Ruslan Kabalin added a comment - Can be cleanly cherry-picked to all affected branches
            kabalin Ruslan Kabalin made changes -
            Pull Master Diff URL https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=257cd76ee659ee207ee8357ee8ded0f8f4307442
            Pull Master Branch MDL-31316-master-1
            Priority Minor [ 4 ] Major [ 3 ]
            Pull from Repository git://git.luns.net.uk/moodle.git
            kabalin Ruslan Kabalin made changes -
            Link This issue has been marked as being related by MDL-30222 [ MDL-30222 ]
            salvetore Michael de Raadt made changes -
            Link This issue duplicates MDL-30877 [ MDL-30877 ]
            salvetore Michael de Raadt made changes -
            Link This issue duplicates MDL-31198 [ MDL-31198 ]
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for providing a solution to this problem.

            Show
            salvetore Michael de Raadt added a comment - Thanks for providing a solution to this problem.
            salvetore Michael de Raadt made changes -
            Fix Version/s STABLE backlog [ 10463 ]
            Priority Major [ 3 ] Critical [ 2 ]
            Labels triaged
            Affects Version/s 2.3 [ 10657 ]
            Component/s My Moodle [ 10433 ]
            Component/s General [ 10049 ]
            salvetore Michael de Raadt made changes -
            Labels triaged patch triaged
            Hide
            kabalin 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
            kabalin 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.
            kabalin Ruslan Kabalin made changes -
            Pull Master Diff URL https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=257cd76ee659ee207ee8357ee8ded0f8f4307442 https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=df0eb4b2231bdca7ce42c44d6cfc4e26924d9e45
            Summary Blocks control links on the site page are redirecting to /my when default home is My Moodle Blocks controls on the site page are redirecting to /my when default home is My Moodle
            Pull Master Branch MDL-31316-master-1 MDL-31316-master-2
            kabalin Ruslan Kabalin made changes -
            Status Open [ 1 ] Waiting for peer review [ 10012 ]
            rajeshtaneja Rajesh Taneja made changes -
            Original Estimate 0 minutes [ 0 ]
            Remaining Estimate 0 minutes [ 0 ]
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Peer reviewer rajeshtaneja
            Hide
            rajeshtaneja 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
            rajeshtaneja 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.
            rajeshtaneja Rajesh Taneja made changes -
            Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
            Hide
            kabalin 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
            kabalin 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
            rajeshtaneja 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
            rajeshtaneja 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
            kabalin 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
            kabalin 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
            rajeshtaneja 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
            rajeshtaneja 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.
            rajeshtaneja Rajesh Taneja made changes -
            Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
            Hide
            kabalin Ruslan Kabalin added a comment -

            My pleasure Thanks for revision!

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

            Hello Ruslan,

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

            Show
            rajeshtaneja Rajesh Taneja added a comment - Hello Ruslan, I think your fix resolves MDL-30355 , can you please confirm this Thanks.
            rajeshtaneja Rajesh Taneja made changes -
            Link This issue will help resolve MDL-30355 [ MDL-30355 ]
            Hide
            kabalin 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
            kabalin 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
            kabalin Ruslan Kabalin added a comment -

            Rebased on latest master

            Show
            kabalin Ruslan Kabalin added a comment - Rebased on latest master
            stronk7 Eloy Lafuente (stronk7) made changes -
            Currently in integration Yes [ 10041 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator stronk7
            stronk7 Eloy Lafuente (stronk7) made changes -
            Link This issue will help resolve MDL-30355 [ MDL-30355 ]
            Hide
            stronk7 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
            stronk7 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
            Anonymous made changes -
            Status Integration review in progress [ 10004 ] Reopened [ 4 ]
            Hide
            kabalin 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
            kabalin 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
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Hide
            kabalin Ruslan Kabalin added a comment -

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

            Show
            kabalin Ruslan Kabalin added a comment - Branch has been updated, I removed B and C as per Eloy's suggestion. Tested as well.
            Anonymous made changes -
            Status Integration review in progress [ 10004 ] Reopened [ 4 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (21, 22 and master) thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (21, 22 and master) thanks!
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Affects Version/s 2.3 [ 10657 ]
            Fix Version/s 2.1.5 [ 11553 ]
            Fix Version/s 2.2.2 [ 11552 ]
            Fix Version/s STABLE backlog [ 10463 ]
            Hide
            kabalin Ruslan Kabalin added a comment -

            My pleasure

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

            .

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

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

            Show
            rajeshtaneja Rajesh Taneja added a comment - grrrr... Should have spotted that. Thanks Eloy.
            ankit_frenz Ankit Agarwal made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Tester ankit_frenz
            Hide
            ankit_frenz Ankit Agarwal added a comment -

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

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi, This is working as expected. Thanks for fixing this. Passing Thanks
            ankit_frenz Ankit Agarwal made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            kabalin Ruslan Kabalin added a comment -

            Show
            kabalin Ruslan Kabalin added a comment -
            Hide
            stronk7 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
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            Integration date 13/Feb/12
            salvetore Michael de Raadt made changes -
            Link This issue is duplicated by MDL-32216 [ MDL-32216 ]

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Mar/12