Moodle
  1. Moodle
  2. MDL-37430

Resource created in a hidden section does not unhide when section is unhidden.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide
      1. Set the first section in a new course to visible and the second section to hidden.
      2. Add 2 activities to each (using the activity chooser - not drag and drop).
      3. Hide 1 activity in the first section.
      4. Hide the first section
      5. Show the first section
      6. Verify that the activities go back to the visible state they were in before the section was hidden.
      7. Unhide the second section, verify the activities in that section become unhidden
      8. Hide the third section,
      9. Move a visible activity into the third section
      10. Unhide the third section
      11. Verify the activity is made visible.
      12. Hide the fourth section
      13. Drag a hidden activity to the fourth section
      14. Drag a visible activity to the fourth section
      15. Verify both activities are hidden
      16. Unhide the section
      17. Verify only one activity in the fourth section is visible
      18. Hide the fifth section
      19. Drag a hidden activity to the fifth section
      20. Drag a visible activity to the fifth section
      21. Drag both activities to the sixth section
      22. Verify only one of the activities becomes visible when dragged to a visible section.
      23. Repeat all tests with ajaxenabled set to 0 (non drag and drop version).
      Show
      Set the first section in a new course to visible and the second section to hidden. Add 2 activities to each (using the activity chooser - not drag and drop). Hide 1 activity in the first section. Hide the first section Show the first section Verify that the activities go back to the visible state they were in before the section was hidden. Unhide the second section, verify the activities in that section become unhidden Hide the third section, Move a visible activity into the third section Unhide the third section Verify the activity is made visible. Hide the fourth section Drag a hidden activity to the fourth section Drag a visible activity to the fourth section Verify both activities are hidden Unhide the section Verify only one activity in the fourth section is visible Hide the fifth section Drag a hidden activity to the fifth section Drag a visible activity to the fifth section Drag both activities to the sixth section Verify only one of the activities becomes visible when dragged to a visible section. Repeat all tests with ajaxenabled set to 0 (non drag and drop version).
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
      git@github.com:damyon/moodle.git
    • Pull 2.4 Branch:
    • Pull Master Branch:
      MDL-37430-master
    • Rank:
      47063

      Description

      Resources created in a hidden section should have visibleold set to 1.

        Issue Links

          Activity

          Hide
          Damyon Wiese added a comment -

          When writing this patch I thought about modifying set_coursemodule_visible to accept a value for setting visibleold - but that function has had a parameter removed - and adding a different parameter back in would be confusing.

          Show
          Damyon Wiese added a comment - When writing this patch I thought about modifying set_coursemodule_visible to accept a value for setting visibleold - but that function has had a parameter removed - and adding a different parameter back in would be confusing.
          Hide
          Frédéric Massart added a comment -

          The patch looks good Damyon. This whole visibility is getting more and more confusing but I don't see any other way of doing what you achieved.

          1. As you mentioned, adding a parameter to set_coursemodule_visible() would have been neater, but you would need to add 2 to ignore the 3rd one (see MDL-36417 & 00a34234). I don't think it'd be a problem, it's up to you.
          2. I don't think you need to rebuild the course cache in moveto_module() as it'll be done in course_add_cm_to_section().
          3. Looking at forcing visibleold to 1 when adding a module, at first it looked weird to force it, but it makes sense as this will not have any impact, except if the section was already hidden.
          4. I noticed that when moving a visible module to a hidden section, the module will not be dimmed by ajax. Not sure if it has to be taken care of in this issue.

          Thanks!
          Fred

          Show
          Frédéric Massart added a comment - The patch looks good Damyon. This whole visibility is getting more and more confusing but I don't see any other way of doing what you achieved. As you mentioned, adding a parameter to set_coursemodule_visible() would have been neater, but you would need to add 2 to ignore the 3rd one (see MDL-36417 & 00a34234). I don't think it'd be a problem, it's up to you. I don't think you need to rebuild the course cache in moveto_module() as it'll be done in course_add_cm_to_section(). Looking at forcing visibleold to 1 when adding a module, at first it looked weird to force it, but it makes sense as this will not have any impact, except if the section was already hidden. I noticed that when moving a visible module to a hidden section, the module will not be dimmed by ajax. Not sure if it has to be taken care of in this issue. Thanks! Fred
          Hide
          Damyon Wiese added a comment -

          Thanks Fred,

          I think this way is neater for 1. I don't like adding an unused parameter back in.

          For 2. Thanks - one less call to clear that cache the better.

          For 3. Yep I think this makes sense too.

          For 4. Good spotting - this is becoming a can of worms.

          • Dragging an activity back out of a hidden section should really restore the state of visibleold.
          • Dragging a visible activity to a hidden section, then to another hidden section, then to a visible section should end up visible.

          I'll update the patch.

          • Damyon
          Show
          Damyon Wiese added a comment - Thanks Fred, I think this way is neater for 1. I don't like adding an unused parameter back in. For 2. Thanks - one less call to clear that cache the better. For 3. Yep I think this makes sense too. For 4. Good spotting - this is becoming a can of worms. Dragging an activity back out of a hidden section should really restore the state of visibleold. Dragging a visible activity to a hidden section, then to another hidden section, then to a visible section should end up visible. I'll update the patch. Damyon
          Hide
          Damyon Wiese added a comment -

          Note: If you use labels for your testing - be aware of this bug: MDL-37453

          Show
          Damyon Wiese added a comment - Note: If you use labels for your testing - be aware of this bug: MDL-37453
          Hide
          Damyon Wiese added a comment -

          The patch is slightly different for 22 and 23 so needs testing of these versions.

          Show
          Damyon Wiese added a comment - The patch is slightly different for 22 and 23 so needs testing of these versions.
          Hide
          Damyon Wiese added a comment -

          This is ready for another review.

          Show
          Damyon Wiese added a comment - This is ready for another review.
          Hide
          Frédéric Massart added a comment -

          Hi Damyon, I don't have much comments about your patch, just a concern about future regressions.

          I am not sure about the new method invoke_function_once(). I looks like it could randomly use the method from one module or the other, which might work correctly in that case because both module inherit the same method, but what if we decided to extend the method in one of them?

          Also in dragdrop.js you have to hardcoded a.editing_hide/show which are defined as constants in toolbox.js. I think we should avoid hardcoding those selectors as much as we can to prevent unexpected regressions if we change them.

          A possible solution that I'm thinking of, though not convinced that it would be nice and clean, would be to add a method to toolbox such as update_visibility_after_dragdrop or object_has_been_dragged. The tricky part is to get to that function... but once there you'll have access to the right methods and constants. Or, perhaps add an argument to invoke_function to specify the module is possible.

          Food for thoughts . Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Damyon, I don't have much comments about your patch, just a concern about future regressions. I am not sure about the new method invoke_function_once(). I looks like it could randomly use the method from one module or the other, which might work correctly in that case because both module inherit the same method, but what if we decided to extend the method in one of them? Also in dragdrop.js you have to hardcoded a.editing_hide/show which are defined as constants in toolbox.js. I think we should avoid hardcoding those selectors as much as we can to prevent unexpected regressions if we change them. A possible solution that I'm thinking of, though not convinced that it would be nice and clean , would be to add a method to toolbox such as update_visibility_after_dragdrop or object_has_been_dragged . The tricky part is to get to that function... but once there you'll have access to the right methods and constants. Or, perhaps add an argument to invoke_function to specify the module is possible. Food for thoughts . Cheers, Fred
          Hide
          Damyon Wiese added a comment -

          Thanks Fred,

          That is a good suggestion (adding a function to the toolbox).

          Thanks!

          Show
          Damyon Wiese added a comment - Thanks Fred, That is a good suggestion (adding a function to the toolbox). Thanks!
          Hide
          Damyon Wiese added a comment -

          Thanks Fred,

          Changed both of those things (added a new function to the resource toolbox "set_visibility_resource_ui" and removed "invoke_function_once"). That function can then use the CSS.SHOW constants instead of hardcoding the class names.

          All issues sorted - sending for integration.

          Cheers - Damyon

          Show
          Damyon Wiese added a comment - Thanks Fred, Changed both of those things (added a new function to the resource toolbox "set_visibility_resource_ui" and removed "invoke_function_once"). That function can then use the CSS.SHOW constants instead of hardcoding the class names. All issues sorted - sending for integration. Cheers - Damyon
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          Integrated to 24, 23 and master.

          Thanks Damyon!

          Show
          Dan Poltawski added a comment - Integrated to 24, 23 and master. Thanks Damyon!
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Damyon,

          Works as mentioned.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Damyon, Works as mentioned.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao
          Hide
          Dan Poltawski added a comment - - edited

          Just commenting that we didn't do great with this change, 3 regressions have been found from this one change and then another saga of regressions.

          MDL-38700 Duplicating a hidden resource can cause both to become unhidden
          MDL-38703 Moving a hidden item sets the item to visible
          MDL-37939 Moving modules between sections is not properly working (which caused MDL-38173, which caused MDL-38173, which caused MDL-38378 which caused MDL-38382)

          Lets laugh because otherwise we'd cry..

          Show
          Dan Poltawski added a comment - - edited Just commenting that we didn't do great with this change, 3 regressions have been found from this one change and then another saga of regressions. MDL-38700 Duplicating a hidden resource can cause both to become unhidden MDL-38703 Moving a hidden item sets the item to visible MDL-37939 Moving modules between sections is not properly working (which caused MDL-38173 , which caused MDL-38173 , which caused MDL-38378 which caused MDL-38382 ) Lets laugh because otherwise we'd cry..

            People

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

              Dates

              • Created:
                Updated:
                Resolved: