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

          Issue Links

            Activity

            Hide
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            Jason Fowler added a comment -

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

            Show
            Jason Fowler added a comment - Pushing for peer review in the hopes of getting more feed back regarding the validity of this patch
            Hide
            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
            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
            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
            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
            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
            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
            Rajesh Taneja added a comment -

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

            Show
            Rajesh Taneja added a comment - As discussed in scrum, taking it from Jason to look at appropriate solution.
            Hide
            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
            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
            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
            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
            Jason Fowler added a comment -

            Thanks for trying that Raj.

            Show
            Jason Fowler added a comment - Thanks for trying that Raj.
            Hide
            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
            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
            Jason Fowler added a comment -

            All changes and branches made now. Pushing for integration.

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

            Rebased and renamed mis-named branches

            Show
            Jason Fowler added a comment - Rebased and renamed mis-named branches
            Hide
            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
            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
            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
            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
            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
            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
            Dan Poltawski added a comment -

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

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

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

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

            Thanks Jason, i've integrated this now.

            Show
            Dan Poltawski added a comment - Thanks Jason, i've integrated this now.
            Hide
            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
            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
            Dan Poltawski added a comment -

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

            Show
            Dan Poltawski added a comment - OK, no fix has arrived for this, reverting and reopening.
            Hide
            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
            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
            Jason Fowler added a comment -

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

            Show
            Jason Fowler added a comment - Minor change to allow for redirect when turning on AND off ...
            Hide
            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
            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
            Dan Poltawski added a comment -

            Integrated to 22, 23 and master. Thanks

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

            Passes in master and 22

            Show
            David Monllaó added a comment - Passes in master and 22
            Hide
            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
            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: