Moodle

Toggling section visibility does not affect modules visibility any more

Details

  • Type: Bug Bug
  • Status: Closed 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
  • Pull Master Diff URL:

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

blocks

Functional Test - A test of a single Moodle feature MDLQA-4946 CLONE - In Safari the show/hide section icon functions correctly.

  • Passed - The test passed
caused a regression

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-36692 Toggling activity/resource visibility does not change link visibility until after reload

  • Blocker - Blocks development and/or testing, prevents Moodle from running (applicable to bugs only)
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
has been marked as being related by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-38703 Moving a hidden item to visible section sets the item to visible in 2.3

  • Major - Major loss of function, incorrect output
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
is a regression caused by

Sub-task - Issues are sometimes broken into multiple sub-tasks. MDL-35339 Improvements to course-related functions

  • Critical - Crashes server, loss of data, severe memory leak
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
is duplicated by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-36555 Incorrect styles after show / hide sections using Safari

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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.

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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Dan Poltawski added a comment -

Integrated, thanks Fred.

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

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: