Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical 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
    • Rank:
      41478

      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

        Issue Links

          Activity

          Hide
          David Mudrak added a comment -

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

          Show
          David Mudrak added a comment - Hi Marina. Any plans/ideas on how to address this? Is there an expected behaviour described somewhere?
          Hide
          David Mudrak 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
          David Mudrak 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 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 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 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 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
          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
          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 added a comment -

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

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          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 Glancy added a comment - Fixed default value for status (apparently it was not used before) and forgotten global $USER in phpunit test
          Hide
          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
          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
          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
          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
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - False alarm about phpunit problems - Petr helpfully pointed in direction of static caches.
          Hide
          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
          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
          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
          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
          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
          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 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 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
          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
          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
          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
          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: