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

Toggling section visibility does not affect modules visibility any more

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      Test pre-requisites

      • A course with sections
      • Some activities, resources within that section
      • Some of those activities are visible, and hidden
      • Refresh the page when testing with JS on, to make sure the Ajax didn't fool you

      Test steps

      1. Navigate to the course and hide the section
      2. Make sure the visible activities are hidden>
      3. Unhide the section
      4. Make sure the previously visible activities are displayed
      5. Repeat with JS off

      Test 2

      1. Hide/show some modules and make sure that works
      2. Repeat with JS off
      Show
      Test pre-requisites A course with sections Some activities, resources within that section Some of those activities are visible, and hidden Refresh the page when testing with JS on, to make sure the Ajax didn't fool you Test steps Navigate to the course and hide the section Make sure the visible activities are hidden> Unhide the section Make sure the previously visible activities are displayed Repeat with JS off Test 2 Hide/show some modules and make sure that works Repeat with JS off
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36417-master

      Description

      • A section with activities hidden and visible
      • Hide the section

      Expected

      • The visible modules are hidden

      Actual

      • The modules keep the same state

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Show
              fred Frédéric Massart added a comment - Before MDL-35339 : https://github.com/moodle/moodle/blob/4ede27b253ca58f08d1b6f0c8823c76ad38d433a/course/lib.php#L2820 After MDL-35339 : https://github.com/moodle/moodle/blob/38b19bbca12d06b5072618bb0b8b016ea2a40c53/course/lib.php#L2832 The call to update the visibility was skipped.
              Hide
              fred Frédéric Massart added a comment -

              Pushing for peer review.

              Following the chat I had with Petr about that, it appeared that it would much better to remove the logic generated by $prevstateoverrides and place it in the only place it was used, in set_section_visible().

              Also, I am not entirely convinced about removing the argument entirely but Petr +10'd it, so there it is. I guess in the future we would classify the whole thing instead of adding more parameters to this function.

              I thought of not updating the module's visibility if it was already set to whatever we were going to set, but I guess that's better to keep the current logic and reprocess what's in set_coursemodule_visible().

              All yours! Thanks.

              Show
              fred Frédéric Massart added a comment - Pushing for peer review. Following the chat I had with Petr about that, it appeared that it would much better to remove the logic generated by $prevstateoverrides and place it in the only place it was used, in set_section_visible(). Also, I am not entirely convinced about removing the argument entirely but Petr +10'd it, so there it is. I guess in the future we would classify the whole thing instead of adding more parameters to this function. I thought of not updating the module's visibility if it was already set to whatever we were going to set, but I guess that's better to keep the current logic and reprocess what's in set_coursemodule_visible(). All yours! Thanks.
              Hide
              skodak Petr Skoda added a comment -

              I like this because the section hiding/showing logic is now easier to understand. It should also eliminate problems with race conditions if you had multiple windows open. I supposed few ppl understood the $prevstateoverrides parameter and moodle will be a better place without it.

              +2, thanks a lot Fred

              Show
              skodak Petr Skoda added a comment - I like this because the section hiding/showing logic is now easier to understand. It should also eliminate problems with race conditions if you had multiple windows open. I supposed few ppl understood the $prevstateoverrides parameter and moodle will be a better place without it. +2, thanks a lot Fred
              Hide
              skodak Petr Skoda added a comment -

              I suppose it would be nice to have some unit tests for section and module hiding. Was it agreed during the hackfest that all bug fixes should have unit tests?

              Show
              skodak Petr Skoda added a comment - I suppose it would be nice to have some unit tests for section and module hiding. Was it agreed during the hackfest that all bug fixes should have unit tests?
              Hide
              fred Frédéric Massart added a comment -

              Pushing for integration after added some Unit Tests.

              Show
              fred Frédéric Massart added a comment - Pushing for integration after added some Unit Tests.
              Hide
              poltawski Dan Poltawski added a comment -

              Integrated, thanks Fred.

              Show
              poltawski Dan Poltawski added a comment - Integrated, thanks Fred.
              Hide
              abgreeve Adrian Greeve added a comment -

              Sorry, David and I hit the button at about the same time to assign the tester. He was a fraction faster. Giving this back to Ankit.

              Show
              abgreeve Adrian Greeve added a comment - Sorry, David and I hit the button at about the same time to assign the tester. He was a fraction faster. Giving this back to Ankit.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Works Great Fred,

              Thanks for fixing this

              Show
              rajeshtaneja Rajesh Taneja added a comment - Works Great Fred, Thanks for fixing this
              Hide
              mina Nicolas Martignoni added a comment -

              Sorry to spam here, but the pointer doesn't change to an arrow (and remains a hand) when hovering the disabled show/hide icons in activities in disabled sections, though the icons are indeed disabled. Not sure if I should open another bug for this.

              Show
              mina Nicolas Martignoni added a comment - Sorry to spam here, but the pointer doesn't change to an arrow (and remains a hand) when hovering the disabled show/hide icons in activities in disabled sections, though the icons are indeed disabled. Not sure if I should open another bug for this.
              Hide
              fred Frédéric Massart added a comment -

              Thanks for the report Nicolas, but the actions of an activity do not change, even if the section is hidden. And also, clicking on those show/hide should not have any effect on the module activity. (Known issue: it is possible to make an activity visible again by editing its general settings).

              Show
              fred Frédéric Massart added a comment - Thanks for the report Nicolas, but the actions of an activity do not change, even if the section is hidden. And also, clicking on those show/hide should not have any effect on the module activity. (Known issue: it is possible to make an activity visible again by editing its general settings).
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Many, many thanks for your effort!

              Millions of people will enjoy the results of your work, yay!

              Closing as fixed. Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Many, many thanks for your effort! Millions of people will enjoy the results of your work, yay! Closing as fixed. Ciao
              Hide
              poltawski Dan Poltawski added a comment -

              Hi,

              We've got lots of problems with section visiblity and cm visiblity in 2.3. I have a feeling its related to this change not being backported and other code being changed to work liks this is working.

              Show
              poltawski Dan Poltawski added a comment - Hi, We've got lots of problems with section visiblity and cm visiblity in 2.3. I have a feeling its related to this change not being backported and other code being changed to work liks this is working.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    3/Dec/12