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

Problems to do with the combination of setting course format set to Weeks and the layout to "Show one section per page"

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Course
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. Create a course of week format, course layout: 'Show one section per page'
      2. Go to the main page of the course with editing set to off
      3. Make sure the week corresponding to today's date is highlighted or has the class 'current'
      4. Turn editing on, hide any other section and turn editing back off.
      5. Make sure you see the hidden section is greyed or has the class 'hidden'
      6. Navigate to one of the sections and create any activity or resource
      7. Make sure you're redirected back to the page you came from
      8. Click to duplicate your resource, 'Continue' and 'Return to course'
      9. Make sure you're redirected back to the page you came from
      10. Click to duplicate your resource, but Cancel the duplicate.
      11. Make sure you're redirected back to the page you came from
      12. Click to delete your resource, and confirm.
      13. Make sure you're redirected back to the page you came from
      14. Click to delete your resource, but cancel.
      15. Make sure you're redirected back to the page you came from
      16. Click to move your resource
      17. Make sure that when navigating between sections the text 'Moving this activity: <activity> (Cancel)' appears and looks nice in the design
      18. Make sure after choosing the destination, you're redirected on the page you came from
      19. Make sure you can't move the section itself (as it's the only one with the main one)
      20. Go the main page of the course and set editing on
      21. Now reproduce steps 3 to 18
      22. Go to the summary page with editing off
      23. While switching to different themes make sure the page looks nice
      Show
      Create a course of week format, course layout: 'Show one section per page' Go to the main page of the course with editing set to off Make sure the week corresponding to today's date is highlighted or has the class 'current' Turn editing on, hide any other section and turn editing back off. Make sure you see the hidden section is greyed or has the class 'hidden' Navigate to one of the sections and create any activity or resource Make sure you're redirected back to the page you came from Click to duplicate your resource, 'Continue' and 'Return to course' Make sure you're redirected back to the page you came from Click to duplicate your resource, but Cancel the duplicate. Make sure you're redirected back to the page you came from Click to delete your resource, and confirm. Make sure you're redirected back to the page you came from Click to delete your resource, but cancel. Make sure you're redirected back to the page you came from Click to move your resource Make sure that when navigating between sections the text 'Moving this activity: <activity> (Cancel)' appears and looks nice in the design Make sure after choosing the destination, you're redirected on the page you came from Make sure you can't move the section itself (as it's the only one with the main one) Go the main page of the course and set editing on Now reproduce steps 3 to 18 Go to the summary page with editing off While switching to different themes make sure the page looks nice
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-33442-master-integration

      Description

      If you go into a section then click the move button for a resource you can see the current section plus the first section (the one that is always visible). The URL is: http://localhost/moodle/int/master/course/view.php?id=2&section=1

      After clicking in one of the available slots you are returned to http://localhost/moodle/int/master/course/view.php?id=2&section=0
      This page displays the contents of all sections.

      After duplicating a resource I'm returned to:
      http://localhost/moodle/int/master/course/view.php?id=2#section-3

      When deleting a resource I started at:
      http://localhost/moodle/int/master/course/view.php?id=2&section=3

      but was returned to:
      http://localhost/moodle/int/master/course/view.php?id=2&section=0

      Quote text is from MDLQA-2298

      "12. Verify the current week is marked visually."
      Not working. If you turn editing mode on (which displays the contents of all the sections) the highlighting works.
      With editing off, so all you have is a list of links to the sections, the current week is not highlighted. My theme is "standard".

      "13. Move a section and verify that the section CANNOT be moved."
      If I turn editing on I can move the sections.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            fred Frédéric Massart added a comment - - edited

            Note to reviewer:
            The course_get_url() function should be improved or reviewed because if we are mostly never redirected to #section-x, but mostly #section-0.

            #section-x should also be used when the course format is one per page but we are editing the main page, which displays all the sections at once.

            I kept the existing behaviour when duplicating, which was redirecting to the corresponding section when in multi sections per page format, that's why I did not use the course_log_url() function.

            We could raise an issue to have consistency and really redirect users to the section they were working in, taking in account the course format and the page they were visiting.

            Thanks! Waiting for your feedback.

            Edit:

            About "13. Move a section and verify that the section CANNOT be moved." If I turn editing on I can move the sections.

            I don't really understand why we expect the sections to be 'frozen'

            Show
            fred Frédéric Massart added a comment - - edited Note to reviewer: The course_get_url() function should be improved or reviewed because if we are mostly never redirected to #section-x, but mostly #section-0. #section-x should also be used when the course format is one per page but we are editing the main page, which displays all the sections at once. I kept the existing behaviour when duplicating, which was redirecting to the corresponding section when in multi sections per page format, that's why I did not use the course_log_url() function. We could raise an issue to have consistency and really redirect users to the section they were working in, taking in account the course format and the page they were visiting. Thanks! Waiting for your feedback. Edit: About "13. Move a section and verify that the section CANNOT be moved." If I turn editing on I can move the sections. I don't really understand why we expect the sections to be 'frozen'
            Hide
            poltawski Dan Poltawski added a comment -

            Regarding course_get_url - I know what you are saying, the behaviour evolved nd the idea of returning the index page anchored by section was when in non-paging mode. In non-paging mode, the url for a section is always the section page. But in paging mode it later evolved that we wanted editing from the front page too, editing on the front page is kinda of an 'unususal' quirk when in editing mode so we just got the right page, do not care so much about the anchor.

            I think it would be better if you went for consistency with the mod duplicate url and used course_get_url ignoring the anchor problem. Maybe we can alter this in the future to return to the right anchor point, but its better we have everything using the same function for it and I think that is relatively minor (since you are at least on the right page).

            About the two QA test instructions:

            "13. Move a section and verify that the section CANNOT be moved." If I turn editing on I can move the sections."

            This is about when "on a section page". i.e. you can't move a section when there is only one section.

            Similarly the highlighting issue is meant to be talking about highlighting on a single section page. Though I agree that highlighting on the 'index page' is a probably a good idea. Though we need to find a better way to style this.

            Show
            poltawski Dan Poltawski added a comment - Regarding course_get_url - I know what you are saying, the behaviour evolved nd the idea of returning the index page anchored by section was when in non-paging mode. In non-paging mode, the url for a section is always the section page. But in paging mode it later evolved that we wanted editing from the front page too, editing on the front page is kinda of an 'unususal' quirk when in editing mode so we just got the right page, do not care so much about the anchor. I think it would be better if you went for consistency with the mod duplicate url and used course_get_url ignoring the anchor problem. Maybe we can alter this in the future to return to the right anchor point, but its better we have everything using the same function for it and I think that is relatively minor (since you are at least on the right page). About the two QA test instructions: "13. Move a section and verify that the section CANNOT be moved." If I turn editing on I can move the sections." This is about when "on a section page". i.e. you can't move a section when there is only one section. Similarly the highlighting issue is meant to be talking about highlighting on a single section page. Though I agree that highlighting on the 'index page' is a probably a good idea. Though we need to find a better way to style this.
            Hide
            fred Frédéric Massart added a comment -

            Thanks for you comments Dan. I updated the patch and added some comment inline on Github. Cheers

            Show
            fred Frédéric Massart added a comment - Thanks for you comments Dan. I updated the patch and added some comment inline on Github. Cheers
            Hide
            poltawski Dan Poltawski added a comment -

            It looks good to me fred, the only issue is that we probably need to do something about the styling on the summary page when an issue is highlighted.

            (try adding some text to a highlighted section summary, it looks a bit ugly)

            Show
            poltawski Dan Poltawski added a comment - It looks good to me fred, the only issue is that we probably need to do something about the styling on the summary page when an issue is highlighted. (try adding some text to a highlighted section summary, it looks a bit ugly)
            Hide
            fred Frédéric Massart added a comment - - edited

            All right, this is an idea. In order to keep the consistency between the themes and classname, I kept 'current' but had to overwrite the background in the base CSS file.
            I noticed an H3 in an A which I fixed, and I set minimal style over the current by enlarging the border.

            Not sure that it's the best way to deal with that. But that looks reasonable. What is your opinion?

            Show
            fred Frédéric Massart added a comment - - edited All right, this is an idea. In order to keep the consistency between the themes and classname, I kept 'current' but had to overwrite the background in the base CSS file. I noticed an H3 in an A which I fixed, and I set minimal style over the current by enlarging the border. Not sure that it's the best way to deal with that. But that looks reasonable. What is your opinion?
            Hide
            poltawski Dan Poltawski added a comment -

            Hmm. No - i'm a fan of it, because it doesn't use the same 'orange style' (or whatever the style people use) as the main section pages.

            I think we could add some additional html elements to help styling this. But I don't have any current clever ideas for this.

            Show
            poltawski Dan Poltawski added a comment - Hmm. No - i'm a fan of it, because it doesn't use the same 'orange style' (or whatever the style people use) as the main section pages. I think we could add some additional html elements to help styling this. But I don't have any current clever ideas for this.
            Hide
            poltawski Dan Poltawski added a comment -

            Adding Martin here to see if he has any clever ideas for styling the current section on the section 'summary page'.

            Show
            poltawski Dan Poltawski added a comment - Adding Martin here to see if he has any clever ideas for styling the current section on the section 'summary page'.
            Hide
            fred Frédéric Massart added a comment -

            This is the last idea I have without having to hack the HTML/CSS too much. I noticed that by default the background was grey and the border too, the standard theme overrides those colours, so I have updated its CSS. I've also found out that the hidden style was ugly too, I used the same style than in the other pages by adding 'dimmed_text' to the title. Upon validation on your side I would update the other styles as well.

            Show
            fred Frédéric Massart added a comment - This is the last idea I have without having to hack the HTML/CSS too much. I noticed that by default the background was grey and the border too, the standard theme overrides those colours, so I have updated its CSS. I've also found out that the hidden style was ugly too, I used the same style than in the other pages by adding 'dimmed_text' to the title. Upon validation on your side I would update the other styles as well.
            Hide
            poltawski Dan Poltawski added a comment -

            I'm not sold on the increasing border idea at all i'm afriad. But I don't have any good ideas myself on how we can style this. I think we need some more input on this.

            The most important thing is that that we have the markup there so themers can theme it.

            Show
            poltawski Dan Poltawski added a comment - I'm not sold on the increasing border idea at all i'm afriad. But I don't have any good ideas myself on how we can style this. I think we need some more input on this. The most important thing is that that we have the markup there so themers can theme it.
            Hide
            poltawski Dan Poltawski added a comment -

            Barbara, i've added you here in case you have any ideas.

            With the course page in 'single section' mode we want to give some indication of the current (e.g. the current week).

            I've attached a screenshot of Freds current solution.

            On normal topic pages we have two divs on left/right which are coloured in. I'm wondering if we need to add some html elements to do this nicely (more important to get that in than styling before release).

            Show
            poltawski Dan Poltawski added a comment - Barbara, i've added you here in case you have any ideas. With the course page in 'single section' mode we want to give some indication of the current (e.g. the current week). I've attached a screenshot of Freds current solution. On normal topic pages we have two divs on left/right which are coloured in. I'm wondering if we need to add some html elements to do this nicely (more important to get that in than styling before release).
            Hide
            dougiamas Martin Dougiamas added a comment -

            My +1 not to change anything about the highlighting in 2.3, do it just like 2.2.

            We can make big changes in 2.4.

            Show
            dougiamas Martin Dougiamas added a comment - My +1 not to change anything about the highlighting in 2.3, do it just like 2.2. We can make big changes in 2.4.
            Hide
            fred Frédéric Massart added a comment -

            Here it is, with similar look and feel than in multi section per page.

            Show
            fred Frédéric Massart added a comment - Here it is, with similar look and feel than in multi section per page.
            Hide
            poltawski Dan Poltawski added a comment - - edited

            OK well this looks good i'm pushing the button.

            One problem is that its now harder to tell you are in summary mode.. But MDL-32767 should help with that.

            Show
            poltawski Dan Poltawski added a comment - - edited OK well this looks good i'm pushing the button. One problem is that its now harder to tell you are in summary mode.. But MDL-32767 should help with that.
            Hide
            salvetore Michael de Raadt added a comment -

            There is probably about 20 QA issues that should be failed and linked to this, but I think the links we have now are a reasonable representative sample.

            Show
            salvetore Michael de Raadt added a comment - There is probably about 20 QA issues that should be failed and linked to this, but I think the links we have now are a reasonable representative sample.
            Hide
            poltawski Dan Poltawski added a comment -

            Raising the priority of this because its causing so many QA test failures.

            Show
            poltawski Dan Poltawski added a comment - Raising the priority of this because its causing so many QA test failures.
            Hide
            nebgor Aparup Banerjee added a comment - - edited

            Fred, can you rebase this on to integration, i'm getting conflicts - seeing some differences in pixels amounts

            Show
            nebgor Aparup Banerjee added a comment - - edited Fred, can you rebase this on to integration, i'm getting conflicts - seeing some differences in pixels amounts
            Hide
            fred Frédéric Massart added a comment -

            Rebased!

            Show
            fred Frédéric Massart added a comment - Rebased!
            Hide
            nebgor Aparup Banerjee added a comment -

            cool, this has been integrated into master.

            ps: Fred, you may want to create an issue about redirects including anchors to handle later.

            ready for testing. please reset blocking MDLQAs when/if passed.

            Show
            nebgor Aparup Banerjee added a comment - cool, this has been integrated into master. ps: Fred, you may want to create an issue about redirects including anchors to handle later. ready for testing. please reset blocking MDLQAs when/if passed.
            Hide
            phalacee Jason Fowler added a comment -

            Works well, thanks Fred

            Show
            phalacee Jason Fowler added a comment - Works well, thanks Fred
            Hide
            poltawski Dan Poltawski added a comment -

            Reset linked QA tests.

            Show
            poltawski Dan Poltawski added a comment - Reset linked QA tests.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12