Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-33832

Check files.source field is set even in non-filepicker situations.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Files API, Repositories
    • Labels:
    • Testing Instructions:
      Hide

      With Javascript enabled:
      Create a folder resource and insert the following files:

      1.

      • upload a file
      • upload a file with a name conflict

      2.

      • pick file from Server files and insert it as copy
      • pick file from Server files and insert it as reference
      • pick file from Server files and insert it as copy with a name conflict
      • pick file from Server files and insert it as reference with a name conflict

      3. pick file from Private files

      4. pick file from Course legacy files

      5. pick file from Recent files

      6. repeat #2 for file from Dropbox

      7. pick file from Box.net

      8.

      • pick file from Wikimedia
      • pick file from Wikimedia with a name conflict

      you may try other repositories if you have time

      Save folder.

      Select from DB directly all files in this filearea, make sure the field source is filled human-readable in all cases and it is one of:

      • just filename in case of upload
      • "Admin user Private files: filepath" (in case of Private files)
      • "Server files: category/coursename/...." (in case of Server files and course legacy files)
      • something readable in case of Recent files (not that important actually)
      • full URL (for repositories that have public access to files)
      • repository name + path inside repository (for other external repositories)

      With Javascript disabled:
      Create a folder resource and

      • upload a file
      • upload a file with name conflict
      • pick a file from local repository
      • pick a file from local repository with name conflict
      • pick a file from external repository
      • pick a file from external repository with name conflict

      Save the folder
      Check the files.source field as described above

      Show
      With Javascript enabled: Create a folder resource and insert the following files: 1. upload a file upload a file with a name conflict 2. pick file from Server files and insert it as copy pick file from Server files and insert it as reference pick file from Server files and insert it as copy with a name conflict pick file from Server files and insert it as reference with a name conflict 3. pick file from Private files 4. pick file from Course legacy files 5. pick file from Recent files 6. repeat #2 for file from Dropbox 7. pick file from Box.net 8. pick file from Wikimedia pick file from Wikimedia with a name conflict you may try other repositories if you have time Save folder. Select from DB directly all files in this filearea, make sure the field source is filled human-readable in all cases and it is one of: just filename in case of upload "Admin user Private files: filepath" (in case of Private files) "Server files: category/coursename/...." (in case of Server files and course legacy files) something readable in case of Recent files (not that important actually) full URL (for repositories that have public access to files) repository name + path inside repository (for other external repositories) With Javascript disabled: Create a folder resource and upload a file upload a file with name conflict pick a file from local repository pick a file from local repository with name conflict pick a file from external repository pick a file from external repository with name conflict Save the folder Check the files.source field as described above
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-MDL-33832-master

      Description

      From Marina in MDL-33513:

      I found that for some reason setting of files.source at
      https://github.com/moodle/moodle/blob/master/repository/repository_ajax.php#L269
      is included inside the different parts of "if" statement but not all.

      field source is set:

      • If file is picked by reference (from external or moodle repository) and there is no name conflict
      • If file is picked as copy from external repository

      field source is not set:

      • If file is picked by reference and there is a name conflict (user is asked whether to rename or overwrite an existing file);
      • If file is picked as a copy from internal moodle repository.

      Why don't we just set $record->source before this big "if" statement and it will be always consistent?

      And besides I don't see any usage of get_file_source_info() in non-js filepicker

      Again, I did not test it, this is just from code review.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            marina Marina Glancy added a comment -

            now I tested it. This is how the source field is filled in cases of dnd upload, filepicker upload, picking from local repository and dropbox:

            filename source
            dnd.jpg dnd.jpg
            dnd_2.jpg dnd.jpg
            upload.jpg upload.jpg
            upload_2.jpg upload.jpg
            from_local.pdf NULL
            from_local_ref.pdf YTo2OntzOjk6ImNvbnRleHRpZCI7...
            from_local_ref_2.pdf NULL
            from_local_2.pdf NULL
            dropboxjpg.jpg Dropbox:/dropboxjpg.jpg
            dropboxjpg_2.jpg Dropbox:/dropboxjpg.jpg
            dropboxjpg_ref.jpg Dropbox:/dropboxjpg.jpg
            dropboxjpg_ref_2.jpg NULL

            postfix _ref means that I picked file by reference
            postfix _2 means that I had a name conflict and choose to rename a file

            Show
            marina Marina Glancy added a comment - now I tested it. This is how the source field is filled in cases of dnd upload, filepicker upload, picking from local repository and dropbox: filename source dnd.jpg dnd.jpg dnd_2.jpg dnd.jpg upload.jpg upload.jpg upload_2.jpg upload.jpg from_local.pdf NULL from_local_ref.pdf YTo2OntzOjk6ImNvbnRleHRpZCI7... from_local_ref_2.pdf NULL from_local_2.pdf NULL dropboxjpg.jpg Dropbox:/dropboxjpg.jpg dropboxjpg_2.jpg Dropbox:/dropboxjpg.jpg dropboxjpg_ref.jpg Dropbox:/dropboxjpg.jpg dropboxjpg_ref_2.jpg NULL postfix _ref means that I picked file by reference postfix _2 means that I had a name conflict and choose to rename a file
            Hide
            marina Marina Glancy added a comment -

            Also in case of picking a file from moodle repository, field source should contain readable name
            see repository::get_reference_details()
            and not packed reference. The value of the source field will be used to show a missed-source-warning during restore. For example, when file references to the contextid that does not exist (any more or on new site), we can show that it used to be "Admin User Private files: bla-bla-bla"

            Show
            marina Marina Glancy added a comment - Also in case of picking a file from moodle repository, field source should contain readable name see repository::get_reference_details() and not packed reference. The value of the source field will be used to show a missed-source-warning during restore. For example, when file references to the contextid that does not exist (any more or on new site), we can show that it used to be "Admin User Private files: bla-bla-bla"
            Hide
            poltawski Dan Poltawski added a comment -

            I've integrated this now.

            Not sure I really like think silencing those unserialize calls is a good solution. But time is of the essence.

            Show
            poltawski Dan Poltawski added a comment - I've integrated this now. Not sure I really like think silencing those unserialize calls is a good solution. But time is of the essence.
            Hide
            mudrd8mz David Mudrak added a comment -

            I am afraid there is other way how to hide E_NOTICE produced by PHP. See the unserialize() man page:

            In case the passed string is not unserializeable, FALSE is returned and E_NOTICE is issued.

            AFAIK there is no other way how to check if a string is unserializeable. So using the @ operator seems to be appropriate here. Of course, the returned false value must be handled correctly.

            Show
            mudrd8mz David Mudrak added a comment - I am afraid there is other way how to hide E_NOTICE produced by PHP. See the unserialize() man page: In case the passed string is not unserializeable, FALSE is returned and E_NOTICE is issued. AFAIK there is no other way how to check if a string is unserializeable. So using the @ operator seems to be appropriate here. Of course, the returned false value must be handled correctly.
            Hide
            fred Frédéric Massart added a comment -

            All good!

            Show
            fred Frédéric Massart added a comment - All good!
            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

              People

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

                Dates

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