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

      Description

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

        Issue Links

          Activity

          Ruslan Kabalin created issue -
          Ruslan Kabalin made changes -
          Field Original Value New Value
          Assignee moodle.com [ moodle.com ] Ruslan Kabalin [ kabalin ]
          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
          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
          Ruslan Kabalin made changes -
          Link This issue has been marked as being related by MDL-30222 [ MDL-30222 ]
          Michael de Raadt made changes -
          Link This issue duplicates MDL-30877 [ MDL-30877 ]
          Michael de Raadt made changes -
          Link This issue duplicates MDL-31198 [ MDL-31198 ]
          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.
          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 ]
          Michael de Raadt made changes -
          Labels triaged patch triaged
          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.
          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
          Ruslan Kabalin made changes -
          Status Open [ 1 ] Waiting for peer review [ 10012 ]
          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
          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.
          Rajesh Taneja made changes -
          Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
          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.
          Rajesh Taneja made changes -
          Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
          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.
          Rajesh Taneja made changes -
          Link This issue will help resolve MDL-30355 [ MDL-30355 ]
          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
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator stronk7
          Eloy Lafuente (stronk7) made changes -
          Link This issue will help resolve MDL-30355 [ MDL-30355 ]
          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
          made changes -
          Status Integration review in progress [ 10004 ] Reopened [ 4 ]
          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
          Eloy Lafuente (stronk7) made changes -
          Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          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.
          made changes -
          Status Integration review in progress [ 10004 ] Reopened [ 4 ]
          Eloy Lafuente (stronk7) made changes -
          Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          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!
          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
          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.
          Ankit Agarwal made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Tester ankit_frenz
          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
          Ankit Agarwal made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          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
          Eloy Lafuente (stronk7) made changes -
          Status Tested [ 10006 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Currently in integration Yes [ 10041 ]
          Integration date 13/Feb/12
          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: