Moodle
  1. Moodle
  2. MDL-38700

Duplicating a hidden resource can cause both to become unhidden

    Details

    • Testing Instructions:
      Hide
      1. Run unit tests
      2. Create an url resource and set it to hidden in the moodle form
      3. Duplicate the newly created url with the x2 icon
      4. Save and return to course
      5. VERIFY: that both modules are hidden.
      6. Add three resources into a single section
      7. Hide 2 of them
      8. Hide the section
      9. VERIFY that all resources are hidden
      10. Unhide the section
      11. VERIFY that the same two resources are hidden as above and the other one is unhidden
      12. Hide a section with no resources in it
      13. Move an unhidden resource tot hat section
      14. VERIFY: the resource is hidden
      15. Move the resource into a visible section
      16. VERIFY: the resource is visible
      Show
      Run unit tests Create an url resource and set it to hidden in the moodle form Duplicate the newly created url with the x2 icon Save and return to course VERIFY: that both modules are hidden. Add three resources into a single section Hide 2 of them Hide the section VERIFY that all resources are hidden Unhide the section VERIFY that the same two resources are hidden as above and the other one is unhidden Hide a section with no resources in it Move an unhidden resource tot hat section VERIFY: the resource is hidden Move the resource into a visible section VERIFY: the resource is visible
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
    • Pull Master Branch:
      MDL-38700-master
    • Rank:
      48749

      Description

      When duplicating a hidden module within a visible section both the module and its duplicate become unhidden. Steps to reproduce 2.3:

      1. Hide an item with the eyeball icon or create it as hidden
      2. Duplicate it with the x2 icon
      3. Save and return to course
      4. Refresh the course page

      Steps to reproduce in 2.4 and current master:

      1. Create an item as hidden
      2. Duplicate it with the x2 icon
      3. Save and return to course
      4. Refresh the course page

      Both modules are now visible. I believe this is a regression introduced by MDL-37430, which didn't take into account that moveto_module() is called by modduplicate.php.

        Issue Links

          Activity

          Hide
          Willy Lee added a comment -

          We've confirmed this issue at Carleton. Upon duplication of a hidden item, the db shows both the original item and the duplicate as visible in mdl_course_modules. AJAX doesn't see that it's been changed, so it looks OK to you until you do some non-AJAX page refresh.

          Show
          Willy Lee added a comment - We've confirmed this issue at Carleton. Upon duplication of a hidden item, the db shows both the original item and the duplicate as visible in mdl_course_modules. AJAX doesn't see that it's been changed, so it looks OK to you until you do some non-AJAX page refresh.
          Hide
          Kevin Wiliarty added a comment -

          Confirmed also at Wesleyan.

          Show
          Kevin Wiliarty added a comment - Confirmed also at Wesleyan.
          Hide
          Charles Fulton added a comment -

          Proposed code solution checks if the module is actually changing sections before updating visibility.

          Show
          Charles Fulton added a comment - Proposed code solution checks if the module is actually changing sections before updating visibility.
          Hide
          Dan Poltawski added a comment -

          Thanks a lot for the clear report and the solution. As I commented in the linked issue, this saga has been a complete nightmare of regressions. We've not done well with.

          Clearly, we really need some phpunit and behat tests here desperately. I'll try and spend a bit of time on the former today.

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [N] Testing
          [Y] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Show
          Dan Poltawski added a comment - Thanks a lot for the clear report and the solution. As I commented in the linked issue, this saga has been a complete nightmare of regressions. We've not done well with. Clearly, we really need some phpunit and behat tests here desperately. I'll try and spend a bit of time on the former today. [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [N] Testing [Y] Security [-] Documentation [Y] Git [Y] Sanity check
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this, guys.

          Show
          Michael de Raadt added a comment - Thanks for working on this, guys.
          Hide
          Dan Poltawski added a comment -

          Whilst writing unit tests for this I struggled to reproduce it because you need visibleold to be set to 1 when the cm is hidden - this should only really happen when the section is hidden (and then this code wouldn't run because of the logic). So whilst this fix is a good fix, there is also a problem that creating the cm using the form is causing visible and visisbleold to be inconsistently set.

          Show
          Dan Poltawski added a comment - Whilst writing unit tests for this I struggled to reproduce it because you need visibleold to be set to 1 when the cm is hidden - this should only really happen when the section is hidden (and then this code wouldn't run because of the logic). So whilst this fix is a good fix, there is also a problem that creating the cm using the form is causing visible and visisbleold to be inconsistently set.
          Hide
          Dan Poltawski added a comment -

          Reported that problem as MDL-38713.

          Show
          Dan Poltawski added a comment - Reported that problem as MDL-38713 .
          Hide
          Dan Poltawski added a comment -

          Ok, i've updated this issue with extensive tests demonstrating this problem and also testing the other functionality.

          The tests are actually failing on 2.3, which I think is exposing the problem being discussed in the linked issue.

          Show
          Dan Poltawski added a comment - Ok, i've updated this issue with extensive tests demonstrating this problem and also testing the other functionality. The tests are actually failing on 2.3, which I think is exposing the problem being discussed in the linked issue.
          Hide
          Dan Poltawski added a comment - - edited

          TO INTEGRATOR: this blocks MDL-38703, and to get 100% passes on 2.3 it needs the fix in that issue to be integrated. (The tests I added here show the bug in MDL-38703)

          Show
          Dan Poltawski added a comment - - edited TO INTEGRATOR: this blocks MDL-38703 , and to get 100% passes on 2.3 it needs the fix in that issue to be integrated. (The tests I added here show the bug in MDL-38703 )
          Hide
          Dan Poltawski added a comment -

          Hi Charles et al.

          I hope i've finally got control of this mess with the fixes in this issue, MDL-38703 and MDL-38713.

          Show
          Dan Poltawski added a comment - Hi Charles et al. I hope i've finally got control of this mess with the fixes in this issue, MDL-38703 and MDL-38713 .
          Hide
          Charles Fulton added a comment -

          Hi Dan,

          Thank you for all your efforts on this!

          Show
          Charles Fulton added a comment - Hi Dan, Thank you for all your efforts on this!
          Hide
          Dan Poltawski added a comment -

          No reviewers found Sending for integration so this bug doesn't persist.

          Show
          Dan Poltawski added a comment - No reviewers found Sending for integration so this bug doesn't persist.
          Hide
          Damyon Wiese 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.

          Thanks!

          Show
          Damyon Wiese 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. Thanks!
          Hide
          Willy Lee added a comment -

          We've run the functional tests here at Carleton and they pass.

          Show
          Willy Lee added a comment - We've run the functional tests here at Carleton and they pass.
          Hide
          Damyon Wiese added a comment -

          Thanks Dan I tested/reviewed MDL-38700, MDL-38703 and MDL-38713 together and they have all been integrated now.

          Show
          Damyon Wiese added a comment - Thanks Dan I tested/reviewed MDL-38700 , MDL-38703 and MDL-38713 together and they have all been integrated now.
          Hide
          Adrian Greeve added a comment -

          Tested on the 2.3, 2.4 and master integration branches.
          No problems encountered.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on the 2.3, 2.4 and master integration branches. No problems encountered. Test passed.
          Hide
          Dan Poltawski added a comment -

          Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking.

          line 1289 of \lib\changes.php: call to debugging()
          line 281 of \lib\are.php: call to moodleform->detectMissingThanks()
          line 202 of \lib\now.php: call to moodleform->_is_poor_form()
          line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

          Show
          Dan Poltawski added a comment - Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking. line 1289 of \lib\changes.php: call to debugging() line 281 of \lib\are.php: call to moodleform->detectMissingThanks() line 202 of \lib\now.php: call to moodleform->_is_poor_form() line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

            People

            • Votes:
              18 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: