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 Sub-task
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      41145

      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.

        Issue Links

          Activity

          Hide
          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 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 Glancy added a comment -

          David, added you as a watcher

          Show
          Marina Glancy added a comment - David, added you as a watcher
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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 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 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 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 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
          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
          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
          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
          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
          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
          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 Glancy added a comment -

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

          Show
          Marina Glancy added a comment - I created an issue MDL-33805 for creating a new filearea
          Hide
          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
          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 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 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
          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
          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 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 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
          Adrian Greeve added a comment -

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

          Show
          Adrian Greeve added a comment - Ugh, such silly mistakes. Thanks again Marina. Putting forward for integration review.
          Hide
          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
          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
          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
          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
          Aparup Banerjee added a comment -

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

          Show
          Aparup Banerjee added a comment - please reset mdlqa after testing. or test it! :-p ciao
          Hide
          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
          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
          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
          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
          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
          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

            People

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

              Dates

              • Created:
                Updated:
                Resolved: