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
    • Rank:
      40209

      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.

        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: