Section return no longer works with single page course views

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

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.

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

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)

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.

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 :/

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.

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

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

Adding dependency relationship based on last comment.

Dan, can you look at this issue please

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

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

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

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.

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

Thanks for splitting this Marina, it has been integrated now

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.

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

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

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

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.