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

          Issue Links

            Activity

            Hide
            mudrd8mz David Mudrak added a comment -

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

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