Moodle
  1. Moodle
  2. MDL-35568

Creating new resources via DND in a hidden section sets visibility incorrectly

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide
      • Drag a new file into a visible section
      • Confirm that the new file is visible
      • Hide the section
      • Confirm that both the new file and the section as a whole appear hidden
      • Drag another new file into the now hidden section
      • Confirm that the new file appears hidden
      • Toggle the section visibility
      • Confirm that both new files and the section as a whole appear visible
      Show
      Drag a new file into a visible section Confirm that the new file is visible Hide the section Confirm that both the new file and the section as a whole appear hidden Drag another new file into the now hidden section Confirm that the new file appears hidden Toggle the section visibility Confirm that both new files and the section as a whole appear visible
    • Workaround:
      Hide

      Unhide the section, toggle the resource and then hide the section again.

      Edit the resource's settings and change the visibility there.

      Show
      Unhide the section, toggle the resource and then hide the section again. Edit the resource's settings and change the visibility there.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-35568_dndupload_hidden

      Description

      If a section is hidden, and you drag/drop a new resource into it, the new resource is created with visible of 1.
      When you unhide the section, the resource visibility goes from 1 (visible) to 0 (hidden).

      Since it's not easy to change the visibility of a resource within a hidden section, this could lead to confusion, chaos and eventually anarchy.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Hi, Andrew.

            Could you please expand on this? I tried creating a resource in a hidden section and its default visibility was hidden.

            Or is this a double-negative thing?: "incorrectly sets visibility incorrectly"

            I did note that when using drag-and-drop, resources are always set to visible. Is that what you are referring to?

            Show
            Michael de Raadt added a comment - Hi, Andrew. Could you please expand on this? I tried creating a resource in a hidden section and its default visibility was hidden. Or is this a double-negative thing?: "incorrectly sets visibility incorrectly" I did note that when using drag-and-drop, resources are always set to visible. Is that what you are referring to?
            Hide
            Andrew Nicols added a comment -

            Hi, Michael.

            Sorry about that - being a bit rusty at the moment as I've been really busy.
            I've updated the title and description, and have provided test instructions for when the issue is fixed.

            Show
            Andrew Nicols added a comment - Hi, Michael. Sorry about that - being a bit rusty at the moment as I've been really busy. I've updated the title and description, and have provided test instructions for when the issue is fixed.
            Hide
            Michael de Raadt added a comment -

            Thanks for the additional information.

            Show
            Michael de Raadt added a comment - Thanks for the additional information.
            Hide
            Rex Lorenzo added a comment -

            Shouldn't this be raised a security issue? With us promoting drag and drop as the best way to upload files, we have run into problems in which a user can drag and drop files into a hidden section via drag and drop, but the files are visible in the resource listing page: /mod/resource/index.php.

            This caused some sensitive content being released.

            Show
            Rex Lorenzo added a comment - Shouldn't this be raised a security issue? With us promoting drag and drop as the best way to upload files, we have run into problems in which a user can drag and drop files into a hidden section via drag and drop, but the files are visible in the resource listing page: /mod/resource/index.php. This caused some sensitive content being released.
            Hide
            Michael de Raadt added a comment -

            Hi, Rex.

            This is a bug in behaviour, but I don't think this is a security issue.

            Thanks for launching the linked issue.

            Show
            Michael de Raadt added a comment - Hi, Rex. This is a bug in behaviour, but I don't think this is a security issue. Thanks for launching the linked issue.
            Hide
            Davo Smith added a comment -

            The attached patch checks the section visibility and uses it to set the module visibility (but it always sets the 'visibleold' to '1', so that the resource will unhide when the section is unhidden).

            BTW, whilst working on this, I noticed that creating activities/resources in a hidden section defaults to 'hidden' with 'visibleold' set to hidden as well - there doesn't seem to be a way to create a resource in a hidden section that will unhide when the section is unhidden (other than, with this patch, via drag and drop).

            Show
            Davo Smith added a comment - The attached patch checks the section visibility and uses it to set the module visibility (but it always sets the 'visibleold' to '1', so that the resource will unhide when the section is unhidden). BTW, whilst working on this, I noticed that creating activities/resources in a hidden section defaults to 'hidden' with 'visibleold' set to hidden as well - there doesn't seem to be a way to create a resource in a hidden section that will unhide when the section is unhidden (other than, with this patch, via drag and drop).
            Hide
            Damyon Wiese added a comment -

            This patch cleanly cherry-picks back to 23 (no drag and drop in 22).

            Show
            Damyon Wiese added a comment - This patch cleanly cherry-picks back to 23 (no drag and drop in 22).
            Hide
            Damyon Wiese added a comment -

            I linked a new issue for the 'visibleold' problem you found Davo (I agree it should work like it does in this patch).

            Show
            Damyon Wiese added a comment - I linked a new issue for the 'visibleold' problem you found Davo (I agree it should work like it does in this patch).
            Hide
            Damyon Wiese added a comment -

            Peer Review Checklist:

            [W] Syntax - Very minor comment issue - comments missing fullstops.
            [-] Output
            [Y] Whitespace
            [-] Language
            [-] Databases
            [Y] Testing
            [-] Security
            [-] Documentation
            [Y] Git - (Cleanly cherry picks back to 23)
            [Y] Sanity check

            Thanks Davo - Sending for integration review.

            Show
            Damyon Wiese added a comment - Peer Review Checklist: [W] Syntax - Very minor comment issue - comments missing fullstops. [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git - (Cleanly cherry picks back to 23) [Y] Sanity check Thanks Davo - Sending for integration review.
            Hide
            Damyon Wiese added a comment -

            Sorry - pushed wrong branch.

            Show
            Damyon Wiese added a comment - Sorry - pushed wrong branch.
            Hide
            Damyon Wiese added a comment -

            Actually - there is a problem with this.

            It only adds the dimmed class to the resource - it does not update the show/hide links etc.

            See toggle_hide_resource_ui in course/yui/toolboxes/toolboxes.js for the full list of what needs to be done (really should call that function).

            Regards, Damyon

            Show
            Damyon Wiese added a comment - Actually - there is a problem with this. It only adds the dimmed class to the resource - it does not update the show/hide links etc. See toggle_hide_resource_ui in course/yui/toolboxes/toolboxes.js for the full list of what needs to be done (really should call that function). Regards, Damyon
            Hide
            Davo Smith added a comment -

            Damyon - I'm not quite sure what is missing from my code.

            As far as I can tell the steps that are needed are:

            1. Add the 'dimmed' class - done by my javascript
            2. Change the link + alt tag on the show/hide icon - not needed as the the action icons were generated on the server with the correct status
            3. Cope with conditional hiding of icons - not an issue as a newly drag & dropped resource can never have conditional hiding applied (as there has been no opportunity to open up the settings and apply this)
            4. Toggle the availability icon status - again, not an issue as custom availability could not have been configured for a newly drag and dropped icon.

            Please let me know if I've missed some subtlety in the code that I haven't already accounted for.

            Show
            Davo Smith added a comment - Damyon - I'm not quite sure what is missing from my code. As far as I can tell the steps that are needed are: 1. Add the 'dimmed' class - done by my javascript 2. Change the link + alt tag on the show/hide icon - not needed as the the action icons were generated on the server with the correct status 3. Cope with conditional hiding of icons - not an issue as a newly drag & dropped resource can never have conditional hiding applied (as there has been no opportunity to open up the settings and apply this) 4. Toggle the availability icon status - again, not an issue as custom availability could not have been configured for a newly drag and dropped icon. Please let me know if I've missed some subtlety in the code that I haven't already accounted for.
            Hide
            Damyon Wiese added a comment -

            MDL-37430 includes a change to make toggle_hide_resource_ui easier to call. Suggest we wait for that (or base this on top of that patch).

            Show
            Damyon Wiese added a comment - MDL-37430 includes a change to make toggle_hide_resource_ui easier to call. Suggest we wait for that (or base this on top of that patch).
            Hide
            Damyon Wiese added a comment -

            Sorry Davo - I thought the show/hide icons would need updating but you're right it does work.

            Thanks - sending for integration now.

            Show
            Damyon Wiese added a comment - Sorry Davo - I thought the show/hide icons would need updating but you're right it does work. Thanks - sending for integration now.
            Hide
            Eloy Lafuente (stronk7) 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
            Eloy Lafuente (stronk7) 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
            Sam Hemelryk added a comment -

            Thanks guys this has been integrated now.

            Show
            Sam Hemelryk added a comment - Thanks guys this has been integrated now.
            Hide
            Frédéric Massart added a comment -

            Test successful on master. Thanks!

            Show
            Frédéric Massart added a comment - Test successful on master. Thanks!
            Hide
            Dan Poltawski added a comment -

            Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            Show
            Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: