Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-32999 META: Files UI Stage 2 polishing in master
  3. MDL-33297

File picker doesn't display some recently uploaded files as a student.

    Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Files API
    • Labels:
    • Testing Instructions:
      Hide

      Set up
      Create the following in a course:

      • Database
      • Workshop
      • Forum
      • Glossary

      Do the following:

      1. Enrol a student in the course.
      2. Upload a picture or file to the activity.
      3. Go to a different activity.
      4. Upload a file or picture, but this time select the file from the recent files section.
        [Test]
        Make sure that you can see the file that you recently uploaded.
      5. Log in as the admin / teacher and browse to an activity.
      6. Upload a file to the activity but browse through the server files section and ensure that you can reach the same file that was uploaded by the student.
        [Test]
        Make sure that you can navigate to the file that was uploaded by the student.
      7. Make sure to test each of the different modules specified.
      Show
      Set up Create the following in a course: Database Workshop Forum Glossary Do the following: Enrol a student in the course. Upload a picture or file to the activity. Go to a different activity. Upload a file or picture, but this time select the file from the recent files section. [Test] Make sure that you can see the file that you recently uploaded. Log in as the admin / teacher and browse to an activity. Upload a file to the activity but browse through the server files section and ensure that you can reach the same file that was uploaded by the student. [Test] Make sure that you can navigate to the file that was uploaded by the student. Make sure to test each of the different modules specified.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-33297-master

      Description

      When uploading files in Forum or Blog, when revisiting the file picker, those files will not be present in the recent files area.
      Loading files for an assignment submission will result in the file being available in the recent files area. Loading files in the my private files area also works.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            marina Marina Glancy added a comment -

            this check was introduced in MDL-31675
            https://github.com/moodle/moodle/blob/master/mod/forum/lib.php#L4084
            https://github.com/moodle/moodle/commit/9a1c075119f60d67d0fbbf597bc7cc75b604dcf9#mod/forum/lib.php

            I understand where it comes from - we decided that only people with course:managefiles capability can see Server files. But somehow we did not think about recent files.

            Maybe we need to remove the capability check in some modules (replace it with ownership check?), such as forum and blog

            Show
            marina Marina Glancy added a comment - this check was introduced in MDL-31675 https://github.com/moodle/moodle/blob/master/mod/forum/lib.php#L4084 https://github.com/moodle/moodle/commit/9a1c075119f60d67d0fbbf597bc7cc75b604dcf9#mod/forum/lib.php I understand where it comes from - we decided that only people with course:managefiles capability can see Server files. But somehow we did not think about recent files. Maybe we need to remove the capability check in some modules (replace it with ownership check?), such as forum and blog
            Hide
            marina Marina Glancy added a comment -

            David, added you as a watcher

            Show
            marina Marina Glancy added a comment - David, added you as a watcher
            Hide
            abgreeve Adrian Greeve added a comment -

            I tested different areas around the site. Blog at the moment doesn't have a method for retrieving recent files. An issue has been created and linked above. Other areas effected are: Forums, database, and glossary modules.

            Show
            abgreeve Adrian Greeve added a comment - I tested different areas around the site. Blog at the moment doesn't have a method for retrieving recent files. An issue has been created and linked above. Other areas effected are: Forums, database, and glossary modules.
            Hide
            mudrd8mz David Mudrak added a comment -

            Negative. Things like

            if ($storedfile->get_userid() != $USER->id) {
                return null;
            }

            are not acceptable. That would put the whole file browsing machinery into a dark smelly corner where users can browse their files only. This would completely change the behaviour of the browsing.

            There is the only right way to support recent files - have a proper callback into all components. Component is the one that can decide what files should be offered to the given $userid (as it is the one that interprets the userid in the files table). All other solutions, including the current hack, are conceptually wrong, even if TheySeemToWork(TM).

            Show
            mudrd8mz David Mudrak added a comment - Negative. Things like if ($storedfile->get_userid() != $USER->id) { return null; } are not acceptable. That would put the whole file browsing machinery into a dark smelly corner where users can browse their files only. This would completely change the behaviour of the browsing. There is the only right way to support recent files - have a proper callback into all components. Component is the one that can decide what files should be offered to the given $userid (as it is the one that interprets the userid in the files table). All other solutions, including the current hack, are conceptually wrong, even if TheySeemToWork(TM).
            Hide
            dougiamas Martin Dougiamas added a comment -

            I'm not sure about the details of the patches here, but as I've repeatedly said I see NOTHING wrong with just searching by userid for recent files.

            The user had control of the file when they put it there, and they should be able to re-use it for another purpose if they choose. It's one reason we store the userid in fact.

            While I get that there is a model in some people's heads that Moodle "takes control" of the file, in reality (where I like to live) users expect to see everything they've uploaded recently in this list, and we will NEVER get all components to implement a callback to make this work.

            The idea that Moodle will not let people access a file they've just uploaded themselves is too ridiculous to allow, sorry.

            Can you even think of ONE security issue with giving people read-only access to files that they themselves created?

            Show
            dougiamas Martin Dougiamas added a comment - I'm not sure about the details of the patches here, but as I've repeatedly said I see NOTHING wrong with just searching by userid for recent files. The user had control of the file when they put it there, and they should be able to re-use it for another purpose if they choose. It's one reason we store the userid in fact. While I get that there is a model in some people's heads that Moodle "takes control" of the file, in reality (where I like to live) users expect to see everything they've uploaded recently in this list, and we will NEVER get all components to implement a callback to make this work. The idea that Moodle will not let people access a file they've just uploaded themselves is too ridiculous to allow, sorry. Can you even think of ONE security issue with giving people read-only access to files that they themselves created?
            Hide
            dougiamas Martin Dougiamas added a comment -

            As a thought experiment, imagine that we simply had a new "recentfiles" area for each user where we made a second copy of every file they uploaded. Then the beauty and wonder of the files structure is retained AND we get the functionality required. And for the user it's EXACTLY the same as just using userid in the first place.

            Show
            dougiamas Martin Dougiamas added a comment - As a thought experiment, imagine that we simply had a new "recentfiles" area for each user where we made a second copy of every file they uploaded. Then the beauty and wonder of the files structure is retained AND we get the functionality required. And for the user it's EXACTLY the same as just using userid in the first place.
            Hide
            poltawski Dan Poltawski added a comment -

            As a solution to this specific issue, if Adrian just changes the managefiles check to managefiles OR is owner of the file, does that work?

            Show
            poltawski Dan Poltawski added a comment - As a solution to this specific issue, if Adrian just changes the managefiles check to managefiles OR is owner of the file, does that work?
            Hide
            dougiamas Martin Dougiamas added a comment -

            I totally agree with David about the callbacks for Server Files browsing however. That should not be checking userid at all.

            Show
            dougiamas Martin Dougiamas added a comment - I totally agree with David about the callbacks for Server Files browsing however. That should not be checking userid at all.
            Hide
            abgreeve Adrian Greeve added a comment -

            I'd love to continue to work on this issue, but unfortunately (or perhaps fortunately) I can't work on this past today (I'm heading to Japan). I'm removing myself from this issue for someone else to complete it in the remaining time before release.

            Show
            abgreeve Adrian Greeve added a comment - I'd love to continue to work on this issue, but unfortunately (or perhaps fortunately) I can't work on this past today (I'm heading to Japan). I'm removing myself from this issue for someone else to complete it in the remaining time before release.
            Hide
            dougiamas Martin Dougiamas added a comment - - edited

            The choice is basically this:

            1) Easiest/some risk: Dan's idea above: we changes the managefiles check to (managefiles OR is owner of the file). I don't think it's too unreasonable, though it does break the idea a little that the modules own the files and there might be repercussions of that down the track (can't think of scenarios though).

            2) Easy/no risk: we modify get_listing() in repository/recent/lib.php so that it does NOT call the file browser, and uses some other way to build $fileinfo more directly. I can't see a good function for this so we might need to make one. At least all the hackiness is one contained place rather than all through the file browser machinery.

            3) Harder/riskier: We do what I mentioned above, which is to create a new filearea called "recentfiles" under the user, and every time new files are added to Moodle from any external source, we create a clone copy of the file in this area. This will DOUBLE the files table, yes, but it is "correct" according to the current Files API design.

            I need some votes on this from senior people so we can proceed.

            Show
            dougiamas Martin Dougiamas added a comment - - edited The choice is basically this: 1) Easiest/some risk: Dan's idea above: we changes the managefiles check to (managefiles OR is owner of the file). I don't think it's too unreasonable, though it does break the idea a little that the modules own the files and there might be repercussions of that down the track (can't think of scenarios though). 2) Easy/no risk: we modify get_listing() in repository/recent/lib.php so that it does NOT call the file browser, and uses some other way to build $fileinfo more directly. I can't see a good function for this so we might need to make one. At least all the hackiness is one contained place rather than all through the file browser machinery. 3) Harder/riskier: We do what I mentioned above, which is to create a new filearea called "recentfiles" under the user, and every time new files are added to Moodle from any external source, we create a clone copy of the file in this area. This will DOUBLE the files table, yes, but it is "correct" according to the current Files API design. I need some votes on this from senior people so we can proceed.
            Hide
            dougiamas Martin Dougiamas added a comment - - edited

            4) Define recent files as "show all the existing draft files with my userid". Anything you had added in an filemanager recently would show up,as well as files you own that you had just looked at in a filemanager. But it would disappear after a few days.

            Show
            dougiamas Martin Dougiamas added a comment - - edited 4) Define recent files as "show all the existing draft files with my userid". Anything you had added in an filemanager recently would show up,as well as files you own that you had just looked at in a filemanager. But it would disappear after a few days.
            Hide
            marina Marina Glancy added a comment -

            I like the idea of separate filearea. Though we might not need to copy the files but pretend there is an area user/recent

            to get the file info we will invoke:
            file_browser::get_file_info(get_user_context(), 'user', 'recent', ...)
            and implement a function
            file_info_context_user::get_area_user_recent
            and agree that we pass/return in $filepath not only the path but fullpath (contextid/component/filearea/itemid/filepath)

            but it looks like a hack.
            Maybe better would be:

            create a new context level and new class extending file_info, i.e.
            class file_info_context_recent extends file_info
            and call
            file_browser::get_file_info($userrecentcontext, ...)

            Show
            marina Marina Glancy added a comment - I like the idea of separate filearea. Though we might not need to copy the files but pretend there is an area user/recent to get the file info we will invoke: file_browser::get_file_info(get_user_context(), 'user', 'recent', ...) and implement a function file_info_context_user::get_area_user_recent and agree that we pass/return in $filepath not only the path but fullpath (contextid/component/filearea/itemid/filepath) but it looks like a hack. Maybe better would be: create a new context level and new class extending file_info, i.e. class file_info_context_recent extends file_info and call file_browser::get_file_info($userrecentcontext, ...)
            Hide
            marina Marina Glancy added a comment -

            I looked more close at the related code while working on MDL-33409

            The function repository::copy_to_area is re-written in repository_recent so it does not use get_file_info and use file_storage::get_file instead plus checks that file belongs to current user.

            But the function repository::get_file_size is not re-written and also repository_recent::get_listing uses file_browser::get_file_info to check file accessibility.

            So there are attempts already in the code to deal with exceptions of file_info for repository_recent the same as in Martin's option #2 - just don't use file_browser::get_file_info.

            Show
            marina Marina Glancy added a comment - I looked more close at the related code while working on MDL-33409 The function repository::copy_to_area is re-written in repository_recent so it does not use get_file_info and use file_storage::get_file instead plus checks that file belongs to current user. But the function repository::get_file_size is not re-written and also repository_recent::get_listing uses file_browser::get_file_info to check file accessibility. So there are attempts already in the code to deal with exceptions of file_info for repository_recent the same as in Martin's option #2 - just don't use file_browser::get_file_info.
            Hide
            mudrd8mz David Mudrak added a comment -

            So there are attempts already in the code to ... not use file_browser::get_file_info()

            Yes, and those have always been a little bit hacky IMHO. If we made a copy into the user's recentfiles area, then we can use clean approach in a way that the API is supposed to be used. In long term period it also gives us more flexibility (eg keep files for one month even if the original is already deleted etc).

            Show
            mudrd8mz David Mudrak added a comment - So there are attempts already in the code to ... not use file_browser::get_file_info() Yes, and those have always been a little bit hacky IMHO. If we made a copy into the user's recentfiles area, then we can use clean approach in a way that the API is supposed to be used. In long term period it also gives us more flexibility (eg keep files for one month even if the original is already deleted etc).
            Hide
            dougiamas Martin Dougiamas added a comment -

            I think the new filearea under user/recentfiles is the cleanest overall:

            • Create new filearea user/recentfiles
            • Create some function to copy files there
            • Call this function at all the places in the code where we want files to be considered recent files
            • Modify recentfiles to use this filearea, with no references allowed
            • Add settings to control lifetime of these files
            • Add cron job to clean out these files older than lifetime

            ...but this is all too much work for 2.3 and too risky.

            Adrian, could you proceed with the Solution #1 hack (allow filebrowsing of files that you have access to OR that have your userid) so we can see how that looks? I think it will be sufficient for now and we can do this better in 2.4 or even a 2.3.x release as no new tables are involved.

            Show
            dougiamas Martin Dougiamas added a comment - I think the new filearea under user/recentfiles is the cleanest overall: Create new filearea user/recentfiles Create some function to copy files there Call this function at all the places in the code where we want files to be considered recent files Modify recentfiles to use this filearea, with no references allowed Add settings to control lifetime of these files Add cron job to clean out these files older than lifetime ...but this is all too much work for 2.3 and too risky. Adrian, could you proceed with the Solution #1 hack (allow filebrowsing of files that you have access to OR that have your userid) so we can see how that looks? I think it will be sufficient for now and we can do this better in 2.4 or even a 2.3.x release as no new tables are involved.
            Hide
            ichklaus Klaus Steitz added a comment -

            Probably this has to be tested by a developer respectively a freshly installed Master-Version. I just tested at http://qa.moodle.net and students still don´t see recent files (Testing step 4). Teachers see the files uploaded by students (step 6).

            Show
            ichklaus Klaus Steitz added a comment - Probably this has to be tested by a developer respectively a freshly installed Master-Version. I just tested at http://qa.moodle.net and students still don´t see recent files (Testing step 4). Teachers see the files uploaded by students (step 6).
            Hide
            marina Marina Glancy added a comment -

            I created an issue MDL-33805 for creating a new filearea

            Show
            marina Marina Glancy added a comment - I created an issue MDL-33805 for creating a new filearea
            Hide
            abgreeve Adrian Greeve added a comment -

            I've provided a hack that will allow a user to access files that they recently uploaded, and to a user that has manage file access. Marina has created a new issue to provide a more elegant solution at another date.

            Up for peer review.

            Show
            abgreeve Adrian Greeve added a comment - I've provided a hack that will allow a user to access files that they recently uploaded, and to a user that has manage file access. Marina has created a new issue to provide a more elegant solution at another date. Up for peer review.
            Hide
            marina Marina Glancy added a comment -

            Adrian, my couple comments from peer review
            1. we decided not to use prefix mod_ for callback names at the moment
            2. can you say in comment:
            // TODO MDL-33805 do not use userid here and move capability check above

            Show
            marina Marina Glancy added a comment - Adrian, my couple comments from peer review 1. we decided not to use prefix mod_ for callback names at the moment 2. can you say in comment: // TODO MDL-33805 do not use userid here and move capability check above
            Hide
            abgreeve Adrian Greeve added a comment -

            Thanks Marina,

            1. I don't know how I ended up editing the function name, but thanks for spotting it.
            2. I have altered the comment to say as you suggested.
            Show
            abgreeve Adrian Greeve added a comment - Thanks Marina, I don't know how I ended up editing the function name, but thanks for spotting it. I have altered the comment to say as you suggested.
            Hide
            marina Marina Glancy added a comment -

            Adrian, there is one empty line in mod/data/lib.php line 2835, it will unlink phpdocs from the function
            and you did not change one function name back
            otherwise looks good!
            Thanks

            Show
            marina Marina Glancy added a comment - Adrian, there is one empty line in mod/data/lib.php line 2835, it will unlink phpdocs from the function and you did not change one function name back otherwise looks good! Thanks
            Hide
            abgreeve Adrian Greeve added a comment -

            Ugh, such silly mistakes. Thanks again Marina. Putting forward for integration review.

            Show
            abgreeve Adrian Greeve added a comment - Ugh, such silly mistakes. Thanks again Marina. Putting forward for integration review.
            Hide
            salvetore Michael de Raadt added a comment -

            Just a friendly reminder that there is a QA test that needs to be reset and re-run when this is integrated.

            Show
            salvetore Michael de Raadt added a comment - Just a friendly reminder that there is a QA test that needs to be reset and re-run when this is integrated.
            Hide
            nebgor Aparup Banerjee added a comment -

            Hi Adrian,
            i've looked at assign , assignment and the other mods not being changed too, seems like the modules being affected are the only ones needing it.

            ps: i like option #4 although it seems slightly abusive of the draft area. perhaps 'draft' should be renamed to recent work area :-p , anyway i've commented in MDL-33805.

            up for testing now.

            Show
            nebgor Aparup Banerjee added a comment - Hi Adrian, i've looked at assign , assignment and the other mods not being changed too, seems like the modules being affected are the only ones needing it. ps: i like option #4 although it seems slightly abusive of the draft area. perhaps 'draft' should be renamed to recent work area :-p , anyway i've commented in MDL-33805 . up for testing now.
            Hide
            nebgor Aparup Banerjee added a comment -

            please reset mdlqa after testing. or test it! :-p ciao

            Show
            nebgor Aparup Banerjee added a comment - please reset mdlqa after testing. or test it! :-p ciao
            Hide
            poltawski Dan Poltawski added a comment -

            Testers: one of my concerns for this change is that there might be some things with the wrong user id. So keep a special eye out for checking that you are not seeing things you shouldn't be

            Show
            poltawski Dan Poltawski added a comment - Testers: one of my concerns for this change is that there might be some things with the wrong user id. So keep a special eye out for checking that you are not seeing things you shouldn't be
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Works Grt,

            Thanks for fixing this Adrian.

            FYI: Not sure how changing user will allow access to other user recent files. Although tried different combinations and it seems to be working as expected.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Works Grt, Thanks for fixing this Adrian. FYI: Not sure how changing user will allow access to other user recent files. Although tried different combinations and it seems to be working as expected.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay!

            Many, many thanks for your hard work!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay! Many, many thanks for your hard work! Ciao
            Hide
            marina Marina Glancy added a comment -

            Adrian Greeve, please note that this issue is closed as Fixed but there is a remaining link to this issue in moodle core:

            $ git grep MDL-33297
            repository/lib.php:        // TODO MDL-33297 remove this function completely?
            

            Will be very appreciated if you can remove it too. TIA

            Show
            marina Marina Glancy added a comment - Adrian Greeve , please note that this issue is closed as Fixed but there is a remaining link to this issue in moodle core: $ git grep MDL-33297 repository/lib.php: // TODO MDL-33297 remove this function completely? Will be very appreciated if you can remove it too. TIA
            Hide
            abgreeve Adrian Greeve added a comment -

            I've created MDL-50272 to deprecate that function. That should get rid of the TODO comment.

            Show
            abgreeve Adrian Greeve added a comment - I've created MDL-50272 to deprecate that function. That should get rid of the TODO comment.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12