Moodle
  1. Moodle
  2. MDL-33032

META: Files UI Stage 1 integrate branch into core

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Files API
    • Labels:
      None

      Description

      I hope you don't mind, but I thought it'd be easier to combine the two issues into one integration task here so we don't overtake the original issues discussion.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Poltawski added a comment -

            Adding some watchers here.

            Show
            Dan Poltawski added a comment - Adding some watchers here.
            Hide
            Dan Poltawski added a comment -

            Hi,

            I've finished my first review of all the code in the wip-files23-branch and so here are my comments..

            First - I spotted and creates patches for some small things as I went, you can decide to pick them into your branch:

            Dongsheng:

            1. Use PARAM_BOOL for booleans:
              https://github.com/danpoltawski/moodle/commit/7abbfcf3522da66d18f7eceda77055b3fad4c527
              https://github.com/danpoltawski/moodle/commit/2cfc8ee15353fff491da5e31630cb939c91d479c
            2. Remove unused globals: https://github.com/danpoltawski/moodle/commit/185e9eba5100af28f4318924b3fa42dcad56bc5b

            Marina:

            1. Js coding style stuff (removng inline ifs, adding ;'s, spacing etc): https://github.com/danpoltawski/moodle/commit/c8c8eeb045f0c2c92e176d0a37b3630e623e5980
              https://github.com/danpoltawski/moodle/commit/109fb286f8fe732cf1105f065a4bae7d8bdaf857

            Now onto comments (mostly they are in the order I read in diff, sorry):

            files/renderer.php

            1. There are a number of trailing whitespace errors in this file, it'd be great if they could be cleaned up
            2. I really think that using a more verbose name than 'fm' would help here. There is fm_ function names, and $fm argument which could be confused between form and file manager.
            3. static $filemanagertemplateloaded; Hmm, doesn't $PAGE->requires help us with doing stuff like this?
            4. Why do we have all this {!}

              stuff, which we then strip out with preg_replace all over the show? I assume it has been used for development somehow and collaboration between Marina and Barbra, but really we should be removing this before going to production? Else we are just wasting CPU cycles on every site!

            5. fm_print_restrictions has some commented out maxfilesizetuff

            lib/filelib.php

            1. Looks like this needs a bug number // XXX: This is a hack for file manager
            2. file_restore_source_field_from_draft_file - seems like it'd be more robust if we threw an exception if $source turned out not be an object.

            lib/filestorage/stored_file.php

            1. delete_reference I see we are unsetting the file record reference fields, but I see nothing which actually commits that to the DB?

              unset($this->file_record->reference);
              unset($this->file_record->referencelastsync);
              unset($this->file_record->referencelifetime);
              

            2. There has been some discussion about the visibility of the functions here, I just wanted to confirm that we are all agreed that rename and replace_content_with are safe to be made public

            repository/draftfiles_ajax.php

            # Just noting that its not immediately clear to me where the access control is in this file. (I'm sure its there because it hasn't really changes)

            repository/lib.php

            1. repository::send_file() surely that should be declared abstract or at least throw an exception or something if called, no?
            2. repository::prepare_listing could benefit from more explanation - what is _f and _f_s?

            theme/base/style/filemanager.css

            1. Trailing whitespace errors

            repository/filepicker.js

            1. Various Y.Cookie.set's - we really need to get rid of these, they break the law in Europe for example (see MDL-28631) use M.util.set_user_preference

            Unit tests

            1. It'd be great if we could increase coverage here (at some point)
            2. I'm pretty sure that test_file_renaming is not working correctly because I recall you can only expect an exception once in a function and then the execution is ended, so the test of the tests are not being run

            phpdoc

            1. Dongsheng, you've put your copyright statements across the code, even where you didn't own the copyright (see, for example googledocs, where you remove my copyright line and replace it with yours).
            2. Adding stub phpdoc without content is even less useful than none at all, because our checker no longer points at places where we need to add it. Its disappointing to see this in new code. I would've much preferred to see phpdoc ommited and then added properly later.
            Show
            Dan Poltawski added a comment - Hi, I've finished my first review of all the code in the wip-files23-branch and so here are my comments.. First - I spotted and creates patches for some small things as I went, you can decide to pick them into your branch: Dongsheng: Use PARAM_BOOL for booleans: https://github.com/danpoltawski/moodle/commit/7abbfcf3522da66d18f7eceda77055b3fad4c527 https://github.com/danpoltawski/moodle/commit/2cfc8ee15353fff491da5e31630cb939c91d479c Remove unused globals: https://github.com/danpoltawski/moodle/commit/185e9eba5100af28f4318924b3fa42dcad56bc5b Marina: Js coding style stuff (removng inline ifs, adding ;'s, spacing etc): https://github.com/danpoltawski/moodle/commit/c8c8eeb045f0c2c92e176d0a37b3630e623e5980 https://github.com/danpoltawski/moodle/commit/109fb286f8fe732cf1105f065a4bae7d8bdaf857 Now onto comments (mostly they are in the order I read in diff, sorry): files/renderer.php There are a number of trailing whitespace errors in this file, it'd be great if they could be cleaned up I really think that using a more verbose name than 'fm' would help here. There is fm_ function names, and $fm argument which could be confused between form and file manager. static $filemanagertemplateloaded; Hmm, doesn't $PAGE->requires help us with doing stuff like this? Why do we have all this {!} stuff, which we then strip out with preg_replace all over the show? I assume it has been used for development somehow and collaboration between Marina and Barbra, but really we should be removing this before going to production? Else we are just wasting CPU cycles on every site! fm_print_restrictions has some commented out maxfilesizetuff lib/filelib.php Looks like this needs a bug number // XXX: This is a hack for file manager file_restore_source_field_from_draft_file - seems like it'd be more robust if we threw an exception if $source turned out not be an object. lib/filestorage/stored_file.php delete_reference I see we are unsetting the file record reference fields, but I see nothing which actually commits that to the DB? unset($this->file_record->reference); unset($this->file_record->referencelastsync); unset($this->file_record->referencelifetime); There has been some discussion about the visibility of the functions here, I just wanted to confirm that we are all agreed that rename and replace_content_with are safe to be made public repository/draftfiles_ajax.php # Just noting that its not immediately clear to me where the access control is in this file. (I'm sure its there because it hasn't really changes) repository/lib.php repository::send_file() surely that should be declared abstract or at least throw an exception or something if called, no? repository::prepare_listing could benefit from more explanation - what is _f and _f_s? theme/base/style/filemanager.css Trailing whitespace errors repository/filepicker.js Various Y.Cookie.set's - we really need to get rid of these, they break the law in Europe for example (see MDL-28631 ) use M.util.set_user_preference Unit tests It'd be great if we could increase coverage here (at some point) I'm pretty sure that test_file_renaming is not working correctly because I recall you can only expect an exception once in a function and then the execution is ended, so the test of the tests are not being run phpdoc Dongsheng, you've put your copyright statements across the code, even where you didn't own the copyright (see, for example googledocs, where you remove my copyright line and replace it with yours). Adding stub phpdoc without content is even less useful than none at all, because our checker no longer points at places where we need to add it. Its disappointing to see this in new code. I would've much preferred to see phpdoc ommited and then added properly later.
            Hide
            Dan Poltawski added a comment -

            By the way, I have just reviewed the code - but wanted to say overall - looking good.

            Show
            Dan Poltawski added a comment - By the way, I have just reviewed the code - but wanted to say overall - looking good.
            Hide
            Dongsheng Cai added a comment - - edited

            Hi, Dan

            I fixed the issue related to my code, commit: 725e71c64f2b3be30b7051744ce48473a9004175, here is a change list:

            1. Added tracker issue number in comment
            2. Fixed stored_file::delete_reference(), fixed the db record
            3. repository::send_file() will throw exception if not implemented by subclass
            4. Fixed renaming unit test, added one unit test for deleting original file, I am going to add more tests
            5. Fixed copyright statement for googledoc and picasa repository plugin, this must be a mistake when I fixing the docblock boilerplate, it's just googledoc and picasa repository, am I right?
            6. Implemented stored_file::set_filesize()

            I don't think I have enough time to review all phpdoc and fix them all, let fix them by creating another issue.

            Show
            Dongsheng Cai added a comment - - edited Hi, Dan I fixed the issue related to my code, commit: 725e71c64f2b3be30b7051744ce48473a9004175, here is a change list: 1. Added tracker issue number in comment 2. Fixed stored_ file::delete_reference( ), fixed the db record 3. repository::send_file() will throw exception if not implemented by subclass 4. Fixed renaming unit test, added one unit test for deleting original file, I am going to add more tests 5. Fixed copyright statement for googledoc and picasa repository plugin, this must be a mistake when I fixing the docblock boilerplate, it's just googledoc and picasa repository, am I right? 6. Implemented stored_ file::set_filesize( ) I don't think I have enough time to review all phpdoc and fix them all, let fix them by creating another issue.
            Hide
            Dan Poltawski added a comment -

            Great - Thanks!

            Show
            Dan Poltawski added a comment - Great - Thanks!
            Hide
            Dan Poltawski added a comment -

            Marina - whats the status, are we good to go?

            Show
            Dan Poltawski added a comment - Marina - whats the status, are we good to go?
            Hide
            Martin Dougiamas added a comment -

            Marina is currently working on the files/mimetypes in https://github.com/marinaglancy/moodle/tree/wip-MDL-32247-files23 which should get merged in the main branch with https://github.com/barbararamiro/moodle/tree/wip-MDL-32900-master on Monday.

            Show
            Martin Dougiamas added a comment - Marina is currently working on the files/mimetypes in https://github.com/marinaglancy/moodle/tree/wip-MDL-32247-files23 which should get merged in the main branch with https://github.com/barbararamiro/moodle/tree/wip-MDL-32900-master on Monday.
            Hide
            Dongsheng Cai added a comment -

            Martin, is that the UI changes or this one MDL-33082?

            Show
            Dongsheng Cai added a comment - Martin, is that the UI changes or this one MDL-33082 ?
            Hide
            David Mudrak added a comment - - edited

            Just commenting on various things spotted while playing with the branch:

            • I am experiencing MDL-32914 (session terminates in Firefox)
            • The tree view (the 3rd view mode) replaces the realthumbnail with the default mimetype thumbnail when collapsing/expanding the tree
            • The timemodified field in the mdl_file table is not updated when a file is overwritten via the filemanager (MDL-33096)
            Show
            David Mudrak added a comment - - edited Just commenting on various things spotted while playing with the branch: I am experiencing MDL-32914 (session terminates in Firefox) The tree view (the 3rd view mode) replaces the realthumbnail with the default mimetype thumbnail when collapsing/expanding the tree The timemodified field in the mdl_file table is not updated when a file is overwritten via the filemanager ( MDL-33096 )
            Hide
            David Mudrak added a comment -

            Updating a file via the filemanager does not work correctly:

            • Modifying licence and author: The new author does not seem to be stored while still in the draft file area. When changes are saved, the new author is stored in the license field, making it appear as "Other" in the selector.
            • None of these modification does not affect timemodified field. Should it? See MDL-33096
            Show
            David Mudrak added a comment - Updating a file via the filemanager does not work correctly: Modifying licence and author: The new author does not seem to be stored while still in the draft file area. When changes are saved, the new author is stored in the license field, making it appear as "Other" in the selector. None of these modification does not affect timemodified field. Should it? See MDL-33096
            Hide
            Marina Glancy added a comment -

            thanks David, I noticed author bug too, but fix in another branch. I merged your pull request completely.
            Changing of author/license do affect timemodified. It was only with overwriting, not any more, thanks to you.

            I'll check treeview realicons

            Show
            Marina Glancy added a comment - thanks David, I noticed author bug too, but fix in another branch. I merged your pull request completely. Changing of author/license do affect timemodified. It was only with overwriting, not any more, thanks to you. I'll check treeview realicons
            Hide
            David Mudrak added a comment -

            Changing of author/license do affect timemodified.

            Not really. Changing the author/license was changing the timemodified of the file in draftarea. But then this new timestamp was not transferred to the real file record. It was a regression of Dongshen's patch, see my fixture patch in wip-file23-fixes for full description.

            Show
            David Mudrak added a comment - Changing of author/license do affect timemodified. Not really. Changing the author/license was changing the timemodified of the file in draftarea. But then this new timestamp was not transferred to the real file record. It was a regression of Dongshen's patch, see my fixture patch in wip-file23-fixes for full description.
            Hide
            Dan Poltawski added a comment -

            OK, this is integrated now. But we also have a problem that some depdency is breaking both CLI and web installers.

            Show
            Dan Poltawski added a comment - OK, this is integrated now. But we also have a problem that some depdency is breaking both CLI and web installers.
            Hide
            Dan Poltawski added a comment -

            Notes from my review:

            1. Transaction support is required in some of the file functions -particularly now we are dealing with two tables, e.g. create_file_from_reference delete_reference
              delete
            2. class cache_file looks, odd to me. Need more investigation
            3. stored_file::$repository is public. This seems wrong.
            Show
            Dan Poltawski added a comment - Notes from my review: Transaction support is required in some of the file functions -particularly now we are dealing with two tables, e.g. create_file_from_reference delete_reference delete class cache_file looks, odd to me. Need more investigation stored_ file::$repository is public. This seems wrong.
            Hide
            Dan Poltawski added a comment -
            Show
            Dan Poltawski added a comment - I have committed a very ugly fix to the broken installer: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=b99065be0491f6c12032f676bbbfaebfb44d771c
            Hide
            Martin Dougiamas added a comment -

            This is not the usual QA situation since there are known bugs in Stage 1, however it works ENOUGH (and things like databases have been reviewed enough) so I'm going to pass the testing on this.

            All further bugs should go as subtasks for Stage 2 for proper resolution: MDL-32999

            Show
            Martin Dougiamas added a comment - This is not the usual QA situation since there are known bugs in Stage 1, however it works ENOUGH (and things like databases have been reviewed enough) so I'm going to pass the testing on this. All further bugs should go as subtasks for Stage 2 for proper resolution: MDL-32999
            Hide
            Martin Dougiamas added a comment -

            Dan, I'll make issues under MDL-32999 for the things you found in that last quick review so they don't get forgotten.

            Show
            Martin Dougiamas added a comment - Dan, I'll make issues under MDL-32999 for the things you found in that last quick review so they don't get forgotten.
            Hide
            Dan Poltawski added a comment -

            Need to talk with Marina about preg_replace parsing of files/render.php tomorrow.

            Show
            Dan Poltawski added a comment - Need to talk with Marina about preg_replace parsing of files/render.php tomorrow.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

            Thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: