Moodle
  1. Moodle
  2. MDL-36417

Toggling section visibility does not affect modules visibility any more

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical 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
    • Rank:
      45242

      Description

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

      Expected

      • The visible modules are hidden

      Actual

      • The modules keep the same state

        Issue Links

          Activity

          Show
          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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Frédéric Massart added a comment -

          Pushing for integration after added some Unit Tests.

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

          Integrated, thanks Fred.

          Show
          Dan Poltawski added a comment - Integrated, thanks Fred.
          Hide
          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
          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
          Rajesh Taneja added a comment -

          Works Great Fred,

          Thanks for fixing this

          Show
          Rajesh Taneja added a comment - Works Great Fred, Thanks for fixing this
          Hide
          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
          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
          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
          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
          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
          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
          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
          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: