Moodle
  1. Moodle
  2. MDL-35276

Section return no longer works with single page course views

    Details

    • Testing Instructions:
      Hide

      The main idea of the tests is to make sure that in course editing mode the "Back" button returns you to the same view you've been before, and in case of multiple sections per page it returns to the section anchor. This should work for all editing buttons:

      • with Javascript enabled
      • with Javascript disabled
        when course display is set as
      • 'show all sections on one page'
      • 'show one section per page' and teacher was on course overview page before clicking link
      • 'show one section per page' and teacher was on section page before clicking link

      Please note: Exceptions are sections editing buttons on the right of the section in non-js mode, the functions to display them do not have argument for sectionreturn.

      1. Create a course with several sections and activities in them (Moodle Features Demo works good).
      2. Set course display to 'show all sections on one page'
      3. Enter course editing mode
      4. Click all editing buttons next to the module, perform an action and click 'return to course' and/or 'Cancel'. Make sure you return to the course page to the section where this activity is (url looks like /course/view.php?id=1#section-3). Note that in JS mode with AJAX enabled page is often not reloaded so this check is not always applicable.
      5. Add activity/resource and save it returning to course, make sure you have the right url
      6. Start adding activity/resource and press cancel, make sure you have the right url
      7. Disable JS and repeat #4-6. When you move activity from one section to another you should be returned to the target section.
      8. Enable JS
      9. Set course display to 'show one section per page'
      10. Open course overview page and enter edit mode. Even in this course display mode in editing mode you still can see all activities in the course on one page.
      11. Repeat #4-8
      12. Click on one of the sections header to view/edit just one section on page
      13. Click all editing buttons next to the module, perform an action and click 'return to course' and/or 'Cancel'. Make sure you return to the SECTION page (url looks like /course/view.php?id=1&section=3).
        Even when you are on the page for section 3 but edit activities from header (section 0), you return to the page for section 3.
      14. Add and save activity, start adding and cancel activity, check the return url
      15. Disable JS and repeat the same
      Show
      The main idea of the tests is to make sure that in course editing mode the "Back" button returns you to the same view you've been before, and in case of multiple sections per page it returns to the section anchor. This should work for all editing buttons: with Javascript enabled with Javascript disabled when course display is set as 'show all sections on one page' 'show one section per page' and teacher was on course overview page before clicking link 'show one section per page' and teacher was on section page before clicking link Please note: Exceptions are sections editing buttons on the right of the section in non-js mode, the functions to display them do not have argument for sectionreturn. Create a course with several sections and activities in them (Moodle Features Demo works good). Set course display to 'show all sections on one page' Enter course editing mode Click all editing buttons next to the module, perform an action and click 'return to course' and/or 'Cancel'. Make sure you return to the course page to the section where this activity is (url looks like /course/view.php?id=1#section-3). Note that in JS mode with AJAX enabled page is often not reloaded so this check is not always applicable. Add activity/resource and save it returning to course, make sure you have the right url Start adding activity/resource and press cancel, make sure you have the right url Disable JS and repeat #4-6. When you move activity from one section to another you should be returned to the target section. Enable JS Set course display to 'show one section per page' Open course overview page and enter edit mode. Even in this course display mode in editing mode you still can see all activities in the course on one page. Repeat #4-8 Click on one of the sections header to view/edit just one section on page Click all editing buttons next to the module, perform an action and click 'return to course' and/or 'Cancel'. Make sure you return to the SECTION page (url looks like /course/view.php?id=1&section=3). Even when you are on the page for section 3 but edit activities from header (section 0), you return to the page for section 3. Add and save activity, start adding and cancel activity, check the return url Disable JS and repeat the same
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-MDL-35276-master
    • Rank:
      43931

      Description

      When you edit or add modules on single page course views and click Save and Return to course, you are always taken to section-0, not the section the item was added on.

        Issue Links

          Activity

          Hide
          Eric Merrill added a comment -

          The change in coding made for MDL-33775 made return to section solely dependent on 'sr', even though only multi-page needs that.

          Show
          Eric Merrill added a comment - The change in coding made for MDL-33775 made return to section solely dependent on 'sr', even though only multi-page needs that.
          Hide
          Eric Merrill added a comment -

          Frederic and Jason,

          I've added you to this ticket as this is a regression from MDL-33775, and you two are the dev and reviewer of that ticket.

          If you guys can review my proposed patch, it would be much appreciated.

          Show
          Eric Merrill added a comment - Frederic and Jason, I've added you to this ticket as this is a regression from MDL-33775 , and you two are the dev and reviewer of that ticket. If you guys can review my proposed patch, it would be much appreciated.
          Hide
          Jason Fowler added a comment -

          Looks good to me, will get Fred to look over it too

          Show
          Jason Fowler added a comment - Looks good to me, will get Fred to look over it too
          Hide
          Frédéric Massart added a comment -

          Hi Eric, thanks for working on this with us.

          You patch definitely does the work, but I wonder if it is wise to complexify things even more. Having worked on MDL-33775, I was aware of the regression created but we decided to push it through anyway. The whole sectionreturn thing becomes really hacky-like and I am not sure if it's a good idea to keep on doing such things. I totally agree that your patch is not adding a lot more complexity than there is already, but it might be a good time to consider changing things. IMO sectionreturn should be enough to know where the user is supposed to be redirected.

          Marina is currently working on the refactoring of the course format (MDL-35218). If this new system handles the sectionreturn in its own proper and clean way then I don't see why we should not integrate your patch as it would benefit 2.3 users.

          I will push your issue for integration to gather more feedback from integrators.

          (Added Dan as a watcher as he is mysteriously linked with the single section thing)

          Show
          Frédéric Massart added a comment - Hi Eric, thanks for working on this with us. You patch definitely does the work, but I wonder if it is wise to complexify things even more. Having worked on MDL-33775 , I was aware of the regression created but we decided to push it through anyway. The whole sectionreturn thing becomes really hacky-like and I am not sure if it's a good idea to keep on doing such things. I totally agree that your patch is not adding a lot more complexity than there is already, but it might be a good time to consider changing things. IMO sectionreturn should be enough to know where the user is supposed to be redirected. Marina is currently working on the refactoring of the course format ( MDL-35218 ). If this new system handles the sectionreturn in its own proper and clean way then I don't see why we should not integrate your patch as it would benefit 2.3 users. I will push your issue for integration to gather more feedback from integrators. (Added Dan as a watcher as he is mysteriously linked with the single section thing)
          Hide
          Eric Merrill added a comment -

          I agree this is not necessarily the optimal solution. Really course_get_url could be refactored, but it would require updating all calls (something like 20 of them), and any non-core code would possibly break, so this seemed like the 'best' solution giving the current position.

          My main argument though is that this was added as a feature, and because of that, reasonable effort should be used to maintain it - I know our users have noticed it is missing - unless there is an intentional loss of functionality. If there is a future refactor coming that will have a better remedy, I am all for that, but feel this should land until that point, as it will fix 2.3.x (which a refactor won't), and if the refactor doesn't land for 2.4, it will be patched there too.

          Show
          Eric Merrill added a comment - I agree this is not necessarily the optimal solution. Really course_get_url could be refactored, but it would require updating all calls (something like 20 of them), and any non-core code would possibly break, so this seemed like the 'best' solution giving the current position. My main argument though is that this was added as a feature, and because of that, reasonable effort should be used to maintain it - I know our users have noticed it is missing - unless there is an intentional loss of functionality. If there is a future refactor coming that will have a better remedy, I am all for that, but feel this should land until that point, as it will fix 2.3.x (which a refactor won't), and if the refactor doesn't land for 2.4, it will be patched there too.
          Hide
          Dan Poltawski added a comment -

          Hmm, well I dunno. Maybe we should just get rid of the $course->display logic in get_course_url alltogether. It seems like its causing more problems than its solving :/

          Show
          Dan Poltawski added a comment - Hmm, well I dunno. Maybe we should just get rid of the $course->display logic in get_course_url alltogether. It seems like its causing more problems than its solving :/
          Hide
          Frédéric Massart added a comment -

          I think one of the main issue is that the general section is identified by 0, and each section in a multipage mode gets a sectionreturn of 0 as well... hence you won't have the expected behaviour getting rid of it. Except if the expected behaviour is redefined lol.

          Show
          Frédéric Massart added a comment - I think one of the main issue is that the general section is identified by 0, and each section in a multipage mode gets a sectionreturn of 0 as well... hence you won't have the expected behaviour getting rid of it. Except if the expected behaviour is redefined lol.
          Hide
          Marina Glancy added a comment -

          I'm taking this issue to suggest the solution compatible with course formats refactoring (MDL-35218)

          Show
          Marina Glancy added a comment - I'm taking this issue to suggest the solution compatible with course formats refactoring ( MDL-35218 )
          Hide
          Marina Glancy added a comment -

          Please note that branch also contains commit from MDL-34824 because they make changes to the same function

          Show
          Marina Glancy added a comment - Please note that branch also contains commit from MDL-34824 because they make changes to the same function
          Hide
          Dan Poltawski added a comment -

          Adding dependency relationship based on last comment.

          Show
          Dan Poltawski added a comment - Adding dependency relationship based on last comment.
          Hide
          Marina Glancy added a comment -

          Dan, can you look at this issue please

          Show
          Marina Glancy added a comment - Dan, can you look at this issue please
          Hide
          Dan Poltawski added a comment -

          Hi Marina,

          • I've read it over and over but the 'navigation' and 'sr' parameters doesn't really sound understandable to me. There is something about it which really doesn't 'make sense' in my brain. I'm far away from the problem they are solving, but its not a good sign if i'm getting lost. It seems like the params names are related to their use, rather than the function and so I find it hard to understand what they do. I don't have any suggestions for better names though. It just isn't making sense in my head.
          • That course_get_url is getting really complex logic now, it'd i'd like to see unit tests there.
          • What will the implocations of changing the sectionreturn params to default null to existing course formats etc?

          dan

          Show
          Dan Poltawski added a comment - Hi Marina, I've read it over and over but the 'navigation' and 'sr' parameters doesn't really sound understandable to me. There is something about it which really doesn't 'make sense' in my brain. I'm far away from the problem they are solving, but its not a good sign if i'm getting lost. It seems like the params names are related to their use, rather than the function and so I find it hard to understand what they do. I don't have any suggestions for better names though. It just isn't making sense in my head. That course_get_url is getting really complex logic now, it'd i'd like to see unit tests there. What will the implocations of changing the sectionreturn params to default null to existing course formats etc? dan
          Hide
          Marina Glancy added a comment - - edited

          Hi Dan,
          there is a phpdoc explaining the options:

           * @param array $options options for view URL. At the moment core uses:
           *     'navigation' (bool) if true and section has no separate page, the function returns null
           *     'sr' (int) used by multipage formats to specify to which section to return
          

          we can rename them to anything else, i.e. 'fornavigation' and 'sectionreturn' if it helps

          The reason why I took this issue is that function course_get_url() in 2.4 will redirect to the function from course format. So all the logic will be inside the particular format. Social and scorm formats will have it very simple. The current implementation will be copied to topics and weeks formats. Contributed formats with nested sections may decide that some sections have their own page, some are displayed inline. Actually the 'coursedisplay' is no longer going to be a standard course option, it will be an optional argument used by topics and weeks formats.

          So I'm not sure about unit tests

          Show
          Marina Glancy added a comment - - edited Hi Dan, there is a phpdoc explaining the options: * @param array $options options for view URL. At the moment core uses: * 'navigation' (bool) if true and section has no separate page, the function returns null * 'sr' ( int ) used by multipage formats to specify to which section to return we can rename them to anything else, i.e. 'fornavigation' and 'sectionreturn' if it helps The reason why I took this issue is that function course_get_url() in 2.4 will redirect to the function from course format. So all the logic will be inside the particular format. Social and scorm formats will have it very simple. The current implementation will be copied to topics and weeks formats. Contributed formats with nested sections may decide that some sections have their own page, some are displayed inline. Actually the 'coursedisplay' is no longer going to be a standard course option, it will be an optional argument used by topics and weeks formats. So I'm not sure about unit tests
          Hide
          Dan Poltawski added a comment -

          Hi Marina,

          I read the phpdoc, but it still wasn't clear to me - maybe its just me

          Re: the unit tests, you'll be able to setup the course with a format for a unit test, so I don't think it should be a prblem

          Show
          Dan Poltawski added a comment - Hi Marina, I read the phpdoc, but it still wasn't clear to me - maybe its just me Re: the unit tests, you'll be able to setup the course with a format for a unit test, so I don't think it should be a prblem
          Hide
          Dan Poltawski added a comment -

          As commented, I don't have a better suggestion for these params, so i'm happy to send it for integration - with my feedback from peer review being that I am not sure they are that understandable. (As I don't have a better suggestion to move this forward).

          We'll see what the integrator thinks about the param naming.

          Show
          Dan Poltawski added a comment - As commented, I don't have a better suggestion for these params, so i'm happy to send it for integration - with my feedback from peer review being that I am not sure they are that understandable. (As I don't have a better suggestion to move this forward). We'll see what the integrator thinks about the param naming.
          Hide
          Marina Glancy added a comment -

          I removed dependency and those branches are now based over master/MOODLE_23_STABLE

          Show
          Marina Glancy added a comment - I removed dependency and those branches are now based over master/MOODLE_23_STABLE
          Hide
          Sam Hemelryk added a comment -

          Thanks for splitting this Marina, it has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks for splitting this Marina, it has been integrated now
          Hide
          Adrian Greeve added a comment -

          I tested this in 2.3 and master integration branches.

          I only came across one problem:

          Duplicating an activity
          Steps to replicate:

          1. The course layout needs to be set to Show one section per page.
          2. Select one section for a single page view.
          3. Duplicate an activity.
          4. Click continue and then edit the new copy.
          5. Clicking 'cancel' or 'save and return to course' will bring you out to a full display of the course and all the section (Expected to return to the single section view)
          • This happens regardless of whether JavaScript is enabled or not. Both 2.3 and master.
          Show
          Adrian Greeve added a comment - I tested this in 2.3 and master integration branches. I only came across one problem: Duplicating an activity Steps to replicate: The course layout needs to be set to Show one section per page. Select one section for a single page view. Duplicate an activity. Click continue and then edit the new copy. Clicking 'cancel' or 'save and return to course' will bring you out to a full display of the course and all the section ( Expected to return to the single section view ) This happens regardless of whether JavaScript is enabled or not. Both 2.3 and master.
          Hide
          Dan Poltawski added a comment -

          I've integrated the fix which Marina had produced for this. Please re-test.

          Show
          Dan Poltawski added a comment - I've integrated the fix which Marina had produced for this. Please re-test.
          Hide
          Marina Glancy added a comment -

          Thanks Adrian, that's a very throughout testing! Fixed

          Show
          Marina Glancy added a comment - Thanks Adrian, that's a very throughout testing! Fixed
          Hide
          Adrian Greeve added a comment -

          Re-tested on 2.3 and master.
          Every thing is now working as expected.
          Thanks Marina for the quick patch.

          Show
          Adrian Greeve added a comment - Re-tested on 2.3 and master. Every thing is now working as expected. Thanks Marina for the quick patch.
          Hide
          Dan Poltawski added a comment -

          Congratulations, you've done it!

          Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc

          Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

          Show
          Dan Poltawski added a comment - Congratulations, you've done it! Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: