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

Correctly process situation when file reference source is missing

    Details

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

      1. Backup resource with references to Private files
      2. Remove the source in Private files
      3. Restore resource
      4. Make sure that error is displayed to user who views resource and teacher is able to edit it

      Test 2
      5. create resource with references to box.net, dropbox, equella repositories
      6. remove originals
      7. make sure you can view/edit the resource

      Show
      1. Backup resource with references to Private files 2. Remove the source in Private files 3. Restore resource 4. Make sure that error is displayed to user who views resource and teacher is able to edit it Test 2 5. create resource with references to box.net, dropbox, equella repositories 6. remove originals 7. make sure you can view/edit the resource
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-MDL-33550-master

      Description

      For internal repositories it happens at the moment when source is deleted between backup and restore (although it should not, there is an issue MDL-33430 for that).

      For external repositories it may happen any time.

      At the moment the whole file manager is not displayed

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              mudrd8mz David Mudrák added a comment -

              Hi Marina. Any plans/ideas on how to address this? Is there an expected behaviour described somewhere?

              Show
              mudrd8mz David Mudrák added a comment - Hi Marina. Any plans/ideas on how to address this? Is there an expected behaviour described somewhere?
              Hide
              mudrd8mz David Mudrák added a comment -

              For internal repositories it happens at the moment when source is deleted between backup and restore (although it should not, there is an issue MDL-33430 for that).

              Please note that in MDLQA-2500 I am actually asking for the expected behaviour. I heard today in HQ chat that all references are equal - regardless whether they link to an external repository (like box.net) or an internal one (like Private files). So the referenced files are not includes in MBZ. Right now, I do not know if it is a feature or a bug.

              Show
              mudrd8mz David Mudrák added a comment - For internal repositories it happens at the moment when source is deleted between backup and restore (although it should not, there is an issue MDL-33430 for that). Please note that in MDLQA-2500 I am actually asking for the expected behaviour. I heard today in HQ chat that all references are equal - regardless whether they link to an external repository (like box.net) or an internal one (like Private files). So the referenced files are not includes in MBZ. Right now, I do not know if it is a feature or a bug.
              Hide
              marina Marina Glancy added a comment -

              Sorry David, not quite sure what you ask here.
              This issue has nothing to do with backup. There are just places in code that assume that source always exists and die with fatal error if not.
              There is also code that set file status to 666 if source is missing and never check it. Also when source appears again the status is not restored (for example somebody deleted file from dropbox and later realised mistake and uploaded it back)

              Show
              marina Marina Glancy added a comment - Sorry David, not quite sure what you ask here. This issue has nothing to do with backup. There are just places in code that assume that source always exists and die with fatal error if not. There is also code that set file status to 666 if source is missing and never check it. Also when source appears again the status is not restored (for example somebody deleted file from dropbox and later realised mistake and uploaded it back)
              Hide
              marina Marina Glancy added a comment -

              MDL-33550 Correctly process situation when file reference source is missing

              • do not die with fatal error if source file in moodle internal repository is missing;
              • moved code duplication for moodle repositories into class repository (functions send_file, get_reference_details, get_file_by_reference, get_file_reference);
              • update file status after repository::sync_external_file so we know that it is missing (or not missing anymore). Do not run this function more than once for file within one request;
              • display readable name for Private Files and Server files with the new format;
              • display broken icon in filemanager if we know that source is missing, display information (for admin) where it was located before: see repository::get_reference_details() and extending classes;
              • removed unnecessary queries in stored_file::sync_external_file();
              • syncronize files before displaying it's size in mod_resource, do not query $DB directly
              Show
              marina Marina Glancy added a comment - MDL-33550 Correctly process situation when file reference source is missing do not die with fatal error if source file in moodle internal repository is missing; moved code duplication for moodle repositories into class repository (functions send_file, get_reference_details, get_file_by_reference, get_file_reference); update file status after repository::sync_external_file so we know that it is missing (or not missing anymore). Do not run this function more than once for file within one request; display readable name for Private Files and Server files with the new format; display broken icon in filemanager if we know that source is missing, display information (for admin) where it was located before: see repository::get_reference_details() and extending classes; removed unnecessary queries in stored_ file::sync_external_file( ); syncronize files before displaying it's size in mod_resource, do not query $DB directly
              Hide
              poltawski Dan Poltawski added a comment -

              Hi,

              Getting phpunit failures after pulling this in:
              Time: 01:53, Memory: 257.25Mb

              There were 2 errors:

              1) filelib_testcase::test_prepare_draft_area
              Undefined property: stdClass::$status

              /Users/danp/git/integration/lib/filestorage/stored_file.php:668
              /Users/danp/git/integration/lib/filestorage/stored_file.php:873
              /Users/danp/git/integration/repository/lib.php:2297
              /Users/danp/git/integration/lib/filestorage/stored_file.php:533
              /Users/danp/git/integration/lib/filestorage/stored_file.php:686
              /Users/danp/git/integration/lib/tests/filelib_test.php:156
              /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76

              To re-run:
              phpunit filelib_testcase lib/tests/filelib_test.php

              2) filelib_testcase::test_delete_original_file_from_draft
              Undefined property: stdClass::$status

              /Users/danp/git/integration/lib/filestorage/stored_file.php:668
              /Users/danp/git/integration/lib/filestorage/stored_file.php:873
              /Users/danp/git/integration/repository/lib.php:2297
              /Users/danp/git/integration/lib/filestorage/stored_file.php:533
              /Users/danp/git/integration/lib/filestorage/stored_file.php:686
              /Users/danp/git/integration/lib/tests/filelib_test.php:254
              /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76

              To re-run:
              phpunit filelib_testcase lib/tests/filelib_test.php

              There was 1 failure:

              1) filestoragelib_testcase::test_create_file_from_reference
              Failed asserting that two strings are equal.
              — Expected
              +++ Actual
              @@ @@
              -'bca20547e94049e1ffea27223581c567022a5774'
              +'da39a3ee5e6b4b0d3255bfef95601890afd80709'

              /Users/danp/git/integration/lib/filestorage/tests/file_storage_test.php:212
              /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76

              To re-run:
              phpunit filestoragelib_testcase lib/filestorage/tests/file_storage_test.php

              FAILURES!
              Tests: 1292, Assertions: 24288, Failures: 1, Errors: 2.

              Show
              poltawski Dan Poltawski added a comment - Hi, Getting phpunit failures after pulling this in: Time: 01:53, Memory: 257.25Mb There were 2 errors: 1) filelib_testcase::test_prepare_draft_area Undefined property: stdClass::$status /Users/danp/git/integration/lib/filestorage/stored_file.php:668 /Users/danp/git/integration/lib/filestorage/stored_file.php:873 /Users/danp/git/integration/repository/lib.php:2297 /Users/danp/git/integration/lib/filestorage/stored_file.php:533 /Users/danp/git/integration/lib/filestorage/stored_file.php:686 /Users/danp/git/integration/lib/tests/filelib_test.php:156 /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76 To re-run: phpunit filelib_testcase lib/tests/filelib_test.php 2) filelib_testcase::test_delete_original_file_from_draft Undefined property: stdClass::$status /Users/danp/git/integration/lib/filestorage/stored_file.php:668 /Users/danp/git/integration/lib/filestorage/stored_file.php:873 /Users/danp/git/integration/repository/lib.php:2297 /Users/danp/git/integration/lib/filestorage/stored_file.php:533 /Users/danp/git/integration/lib/filestorage/stored_file.php:686 /Users/danp/git/integration/lib/tests/filelib_test.php:254 /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76 To re-run: phpunit filelib_testcase lib/tests/filelib_test.php – There was 1 failure: 1) filestoragelib_testcase::test_create_file_from_reference Failed asserting that two strings are equal. — Expected +++ Actual @@ @@ -'bca20547e94049e1ffea27223581c567022a5774' +'da39a3ee5e6b4b0d3255bfef95601890afd80709' /Users/danp/git/integration/lib/filestorage/tests/file_storage_test.php:212 /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76 To re-run: phpunit filestoragelib_testcase lib/filestorage/tests/file_storage_test.php FAILURES! Tests: 1292, Assertions: 24288, Failures: 1, Errors: 2.
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              marina Marina Glancy added a comment -

              Fixed default value for status (apparently it was not used before) and forgotten global $USER in phpunit test

              Show
              marina Marina Glancy added a comment - Fixed default value for status (apparently it was not used before) and forgotten global $USER in phpunit test
              Hide
              poltawski Dan Poltawski added a comment -

              Still failing:
              2) filestoragelib_testcase::test_create_file_from_reference
              Failed asserting that two strings are equal.
              — Expected
              +++ Actual
              @@ @@
              -'bca20547e94049e1ffea27223581c567022a5774'
              +'da39a3ee5e6b4b0d3255bfef95601890afd80709'

              /Users/danp/git/integration/lib/filestorage/tests/file_storage_test.php:212
              /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76

              To re-run:
              phpunit filestoragelib_testcase lib/filestorage/tests/file_storage_test.php

              Show
              poltawski Dan Poltawski added a comment - Still failing: 2) filestoragelib_testcase::test_create_file_from_reference Failed asserting that two strings are equal. — Expected +++ Actual @@ @@ -'bca20547e94049e1ffea27223581c567022a5774' +'da39a3ee5e6b4b0d3255bfef95601890afd80709' /Users/danp/git/integration/lib/filestorage/tests/file_storage_test.php:212 /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76 To re-run: phpunit filestoragelib_testcase lib/filestorage/tests/file_storage_test.php
              Hide
              poltawski Dan Poltawski added a comment -

              Just noting the above problem is occurring when all tests are running, but not a single test. Seems like a problem with the phpunit environment. Will alert Petr.

              Show
              poltawski Dan Poltawski added a comment - Just noting the above problem is occurring when all tests are running, but not a single test. Seems like a problem with the phpunit environment. Will alert Petr.
              Hide
              poltawski Dan Poltawski added a comment -

              False alarm about phpunit problems - Petr helpfully pointed in direction of static caches.

              Show
              poltawski Dan Poltawski added a comment - False alarm about phpunit problems - Petr helpfully pointed in direction of static caches.
              Hide
              poltawski Dan Poltawski added a comment -

              Ok, i've integrated this now.

              I think I would've I thrown an exception if file argument to sync_external_file() was empty/ not a stored file and not reseting.

              Show
              poltawski Dan Poltawski added a comment - Ok, i've integrated this now. I think I would've I thrown an exception if file argument to sync_external_file() was empty/ not a stored file and not reseting.
              Hide
              fred Frédéric Massart added a comment -

              Marina, while testing this I can't get the error you are talking about in the testing instructions. I used a File resource with 3 files in it, 2 of them being link to Private files content. (Haven't tested the repositories yet). Where is the error message supposed to happen? Do I have to use a specific resource/file type to reproduce it?

              Cheers!

              Show
              fred Frédéric Massart added a comment - Marina, while testing this I can't get the error you are talking about in the testing instructions. I used a File resource with 3 files in it, 2 of them being link to Private files content. (Haven't tested the repositories yet). Where is the error message supposed to happen? Do I have to use a specific resource/file type to reproduce it? Cheers!
              Hide
              andyjdavis Andrew Davis added a comment -

              Fred, did you remove the file(s) from your private files before doing the restore?
              What happens if you click on the resource on the course page of the restored course?

              Show
              andyjdavis Andrew Davis added a comment - Fred, did you remove the file(s) from your private files before doing the restore? What happens if you click on the resource on the course page of the restored course?
              Hide
              marina Marina Glancy added a comment -

              Fred,
              for example if you create a Folder resource and put a reference to pdf file in there
              then you backup, remove source of a reference
              then restore
              then as a student you view the Folder resource, click on pdf file but see 'file can not be found' error

              as a teacher you edit the Folder resource, click on file and see error message as "original"

              Show
              marina Marina Glancy added a comment - Fred, for example if you create a Folder resource and put a reference to pdf file in there then you backup, remove source of a reference then restore then as a student you view the Folder resource, click on pdf file but see 'file can not be found' error as a teacher you edit the Folder resource, click on file and see error message as "original"
              Hide
              fred Frédéric Massart added a comment -

              Test passed on master.

              I was using the File resource with a JPEG file which creates a display of the file, so the filenotfound error was not displayed.

              Show
              fred Frédéric Massart added a comment - Test passed on master. I was using the File resource with a JPEG file which creates a display of the file, so the filenotfound error was not displayed.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL Ciao

                People

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

                  Dates

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