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

Turning off course edit while inside an activity or resource redirects (wrongly) to the course's front page

    Details

    • Testing Instructions:
      Hide
      1. Log in as an admin
      2. Create or use a course with an activity
      3. Turn editing on at the course page
      4. Open the Activity
      5. From the course administration menu, select "Turn editing off"
      6. Ensure you remain on the activity page.
      7. From the course administration menu, select "Turn editing on"
      8. Ensure you remain on the activity page.
      Show
      Log in as an admin Create or use a course with an activity Turn editing on at the course page Open the Activity From the course administration menu, select "Turn editing off" Ensure you remain on the activity page. From the course administration menu, select "Turn editing on" Ensure you remain on the activity page.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      wip-MDL-32184-master

      Description

      Minor usability issue.

      When course Edit mode is turned ON and while editing or viewing an Activity or a Resource,
      I click on turn course editing off (setting block > course > editing off) ...
      I am redirected to the course's front page and not to the page i was at when i clicked.

      I am expecting to redirect to the same page i was at.

      It seems, that the $fromurl is not used or has the wrong value.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              kanikagoyal Kanika Goyal added a comment -

              Hi,
              Here is a quick patch for this issue-
              https://github.com/kanikagoyal/moodle/compare/master...master_MDL-32184_turn_on_off_course_edit_inside_activity_redirects_incorrectly

              Please let me know if any changes are required.

              Thanks,
              Kanika

              Show
              kanikagoyal Kanika Goyal added a comment - Hi, Here is a quick patch for this issue- https://github.com/kanikagoyal/moodle/compare/master...master_MDL-32184_turn_on_off_course_edit_inside_activity_redirects_incorrectly Please let me know if any changes are required. Thanks, Kanika
              Hide
              nadavkav Nadav Kavalerchik added a comment -

              I confirm that Kanika Goyal's patch solves this issue!

              Thank you very (very) much for the quick response

              Show
              nadavkav Nadav Kavalerchik added a comment - I confirm that Kanika Goyal's patch solves this issue! Thank you very (very) much for the quick response
              Hide
              phalacee Jason Fowler added a comment -

              I've opted to go with this patch as opposed to the ones discussed in the scrum (passing in just the variables required to build the URL within the redirect code) due to the fact that this looks 1) less complex, 2) cleaner, 3) more moodle-friendly.

              Show
              phalacee Jason Fowler added a comment - I've opted to go with this patch as opposed to the ones discussed in the scrum (passing in just the variables required to build the URL within the redirect code) due to the fact that this looks 1) less complex, 2) cleaner, 3) more moodle-friendly.
              Hide
              phalacee Jason Fowler added a comment -

              Testing this, it doesn't seem to work on the Course Settings menu option, and the button disappears in the activity, so I am unsure what to do about this. Will think it over and look in to it.

              Show
              phalacee Jason Fowler added a comment - Testing this, it doesn't seem to work on the Course Settings menu option, and the button disappears in the activity, so I am unsure what to do about this. Will think it over and look in to it.
              Hide
              phalacee Jason Fowler added a comment - - edited

              Turns out I had applied the patch to the wrong area of the code, now looking at the fix, I am wondering if there is anything wrong with the original behaviour - if you go to the course administration nav block, you should expect to be removed from the activity you are viewing in order to make changes to the course.

              The way this fix works comes across as messy and hacky, the Course Administration is for courses after all.

              Show
              phalacee Jason Fowler added a comment - - edited Turns out I had applied the patch to the wrong area of the code, now looking at the fix, I am wondering if there is anything wrong with the original behaviour - if you go to the course administration nav block, you should expect to be removed from the activity you are viewing in order to make changes to the course. The way this fix works comes across as messy and hacky, the Course Administration is for courses after all.
              Hide
              phalacee Jason Fowler added a comment -

              Pushing for peer review in the hopes of getting more feed back regarding the validity of this patch

              Show
              phalacee Jason Fowler added a comment - Pushing for peer review in the hopes of getting more feed back regarding the validity of this patch
              Hide
              phalacee Jason Fowler added a comment - - edited

              Raj has pointed out that the need to edit blocks may call for a "Block editing on/off" type button akin to the one on the ./admin/user.php page, handled by each activity module, as opposed to the over all "Turn Editing on/off" which is provided by the course

              Show
              phalacee Jason Fowler added a comment - - edited Raj has pointed out that the need to edit blocks may call for a "Block editing on/off" type button akin to the one on the ./admin/user.php page, handled by each activity module, as opposed to the over all "Turn Editing on/off" which is provided by the course
              Hide
              nadavkav Nadav Kavalerchik added a comment -

              @Jason
              I hope you are not considering not fixing this issue, since it would be very annoying to be redirected to the main course page each time I change "editing" mode, while inside an activity or a block page (or even other inner course pages with lower context then the course's context)

              Show
              nadavkav Nadav Kavalerchik added a comment - @Jason I hope you are not considering not fixing this issue, since it would be very annoying to be redirected to the main course page each time I change "editing" mode, while inside an activity or a block page (or even other inner course pages with lower context then the course's context)
              Hide
              fred Frédéric Massart added a comment -

              Hi Jason,

              I am not sure I understood the need to edit blocks, nothing wrong occurred to me. The patch looks good and here are my thoughts:

              • It does not appear like we should distinct the page we are on when setting 'return' as both will generate the exact same behaviour even though they appear different. (navigationlib:3567)
              • Now there are 6 redirects in a row in course/view.php, I guess we could simplify this even though it is not highly required.

              Cheers!

              Show
              fred Frédéric Massart added a comment - Hi Jason, I am not sure I understood the need to edit blocks, nothing wrong occurred to me. The patch looks good and here are my thoughts: It does not appear like we should distinct the page we are on when setting 'return' as both will generate the exact same behaviour even though they appear different. (navigationlib:3567) Now there are 6 redirects in a row in course/view.php, I guess we could simplify this even though it is not highly required. Cheers!
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              As discussed in scrum, taking it from Jason to look at appropriate solution.

              Show
              rajeshtaneja Rajesh Taneja added a comment - As discussed in scrum, taking it from Jason to look at appropriate solution.
              Hide
              poltawski Dan Poltawski added a comment -

              Raj & I discussed this, and I advised to go for a simple 'solve this problem', rather than trying a more extensive solution solving editing mode on pages, interaction with blocks, edit button etc. That is opening a can of worms and I don't think its a worthwhile effort.

              Show
              poltawski Dan Poltawski added a comment - Raj & I discussed this, and I advised to go for a simple 'solve this problem', rather than trying a more extensive solution solving editing mode on pages, interaction with blocks, edit button etc. That is opening a can of worms and I don't think its a worthwhile effort.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              I was going to add a proper solution to add editing button and link (https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-32184) for this solution, but after taking to Dan, it seems Kanika's solution is the way to go.

              As per discussion with Dan, this is not a major issue and not affecting lot of people. Also, in past there have been talks about getting rid of editing button, so adding a proper framework, to do so might not be appropriate at this point. It will be nice to go with Kanika's solution and resolve it for now.
              Passing it back to Jason, as he already have picked Kanika's patch.

              Show
              rajeshtaneja Rajesh Taneja added a comment - I was going to add a proper solution to add editing button and link ( https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-32184 ) for this solution, but after taking to Dan, it seems Kanika's solution is the way to go. As per discussion with Dan, this is not a major issue and not affecting lot of people. Also, in past there have been talks about getting rid of editing button, so adding a proper framework, to do so might not be appropriate at this point. It will be nice to go with Kanika's solution and resolve it for now. Passing it back to Jason, as he already have picked Kanika's patch.
              Hide
              phalacee Jason Fowler added a comment -

              Thanks for trying that Raj.

              Show
              phalacee Jason Fowler added a comment - Thanks for trying that Raj.
              Hide
              phalacee Jason Fowler added a comment -

              got the branch names wrong for the older versions – if that's an issue I can try to fix it.

              Show
              phalacee Jason Fowler added a comment - got the branch names wrong for the older versions – if that's an issue I can try to fix it.
              Hide
              phalacee Jason Fowler added a comment -

              All changes and branches made now. Pushing for integration.

              Show
              phalacee Jason Fowler added a comment - All changes and branches made now. Pushing for integration.
              Hide
              poltawski Dan Poltawski 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
              poltawski Dan Poltawski 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
              phalacee Jason Fowler added a comment -

              Rebased and renamed mis-named branches

              Show
              phalacee Jason Fowler added a comment - Rebased and renamed mis-named branches
              Hide
              poltawski Dan Poltawski added a comment -

              Hi,

              base64_encoding the returnurl like this is not the way.

              Instead, if you use the out_as_local_url() function of moodle_url to get the page url as 'local url' stripped for passing around.

              Then the param should be a PARAM_LOCALURL, you can see this approach used in quiz and oauth2.

              Its a shame that Kanika was not credited in the submitted patch, as she seemed to have created most of it.

              Show
              poltawski Dan Poltawski added a comment - Hi, base64_encoding the returnurl like this is not the way. Instead, if you use the out_as_local_url() function of moodle_url to get the page url as 'local url' stripped for passing around. Then the param should be a PARAM_LOCALURL, you can see this approach used in quiz and oauth2. Its a shame that Kanika was not credited in the submitted patch, as she seemed to have created most of it.
              Hide
              phalacee Jason Fowler added a comment -

              I assumed she would be because I cherry-picked it from her code, I must have rebased it in a way that prevented that. Will make sure she gets the credit when I push the final version next sprint.

              Show
              phalacee Jason Fowler added a comment - I assumed she would be because I cherry-picked it from her code, I must have rebased it in a way that prevented that. Will make sure she gets the credit when I push the final version next sprint.
              Hide
              phalacee Jason Fowler added a comment -

              Actually, Kanika was credited ... the commit reads:

              MDL-32184 - Fixing incorrect redirect when toggling course edit while inside an activity – Patch provided by Kanika Goyal

              Show
              phalacee Jason Fowler added a comment - Actually, Kanika was credited ... the commit reads: MDL-32184 - Fixing incorrect redirect when toggling course edit while inside an activity – Patch provided by Kanika Goyal
              Hide
              poltawski Dan Poltawski added a comment -

              Apologies Jason, I didn't see the [...] expanded view on github and missesd your credit. Thats fine.

              Show
              poltawski Dan Poltawski added a comment - Apologies Jason, I didn't see the [...] expanded view on github and missesd your credit. Thats fine.
              Hide
              phalacee Jason Fowler added a comment -

              All those changes have been made Dan. Pushing for integration again.

              Show
              phalacee Jason Fowler added a comment - All those changes have been made Dan. Pushing for integration again.
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks Jason, i've integrated this now.

              Show
              poltawski Dan Poltawski added a comment - Thanks Jason, i've integrated this now.
              Hide
              dmonllao David Monllaó added a comment -

              It fails both in master and 22.

              When I'm in an activity and I enable editing mode from the course administration menu I'm redirected to the course view page instead of remain on the activity. FYI I've seen that when enabling/disabling editing mode through the course administration menu the &return URL argument is exactly the same.

              Show
              dmonllao David Monllaó added a comment - It fails both in master and 22. When I'm in an activity and I enable editing mode from the course administration menu I'm redirected to the course view page instead of remain on the activity. FYI I've seen that when enabling/disabling editing mode through the course administration menu the &return URL argument is exactly the same.
              Hide
              poltawski Dan Poltawski added a comment -

              OK, no fix has arrived for this, reverting and reopening.

              Show
              poltawski Dan Poltawski added a comment - OK, no fix has arrived for this, reverting and reopening.
              Hide
              phalacee Jason Fowler added a comment -

              I've just figured out why this wasn't working, will patch it in the next sprint and push it then

              Show
              phalacee Jason Fowler added a comment - I've just figured out why this wasn't working, will patch it in the next sprint and push it then
              Hide
              phalacee Jason Fowler added a comment -

              Minor change to allow for redirect when turning on AND off ...

              Show
              phalacee Jason Fowler added a comment - Minor change to allow for redirect when turning on AND off ...
              Hide
              fred Frédéric Massart added a comment - - edited

              Looks good to me Jason. Pushing for integration. Thanks!

              [Y] Syntax
              [Y] Output
              [Y] Whitespace
              [NA] Language
              [NA] Databases
              [Y] Testing
              [Y] Security
              [NA] Documentation
              [Y] Git
              [Y] Sanity check

              Show
              fred Frédéric Massart added a comment - - edited Looks good to me Jason. Pushing for integration. Thanks! [Y] Syntax [Y] Output [Y] Whitespace [NA] Language [NA] Databases [Y] Testing [Y] Security [NA] Documentation [Y] Git [Y] Sanity check
              Hide
              poltawski Dan Poltawski added a comment -

              Integrated to 22, 23 and master. Thanks

              Show
              poltawski Dan Poltawski added a comment - Integrated to 22, 23 and master. Thanks
              Hide
              dmonllao David Monllaó added a comment -

              Passes in master and 22

              Show
              dmonllao David Monllaó added a comment - Passes in master and 22
              Hide
              nebgor Aparup Banerjee added a comment -

              Your issue has dug up some gold.
              It works great i've been told.
              Go forth, be brave, be bold.

              yay! "All your thoughts are belong to everyone."

              Thanks and ciao!

              Show
              nebgor Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!

                People

                • Votes:
                  0 Vote for this issue
                  Watchers:
                  1 Start watching this issue

                  Dates

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