Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Files API
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      32797

      Description

      All the pages where we have a simple"upload" to get import files should now use a filepicker:

      Please add any others to this list:

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -
          Show
          Jérôme Mouneyrac added a comment - I added Course import ( http://docs.moodle.org/en/Community_hub#Import_course )
          Hide
          Dongsheng Cai added a comment -

          Jerome, course import needs a new repository plugin, is it?

          Show
          Dongsheng Cai added a comment - Jerome, course import needs a new repository plugin, is it?
          Hide
          Dongsheng Cai added a comment -

          Petr,

          I created two funcitons: get_draft_files and delete_draft_files in formslib, get_draft_files will be used to read draft file info. Let me know if there is any problem.

          Thanks

          Show
          Dongsheng Cai added a comment - Petr, I created two funcitons: get_draft_files and delete_draft_files in formslib, get_draft_files will be used to read draft file info. Let me know if there is any problem. Thanks
          Hide
          Petr Škoda added a comment -

          hmm, this is exactly what I tried to prevent in the file api design, instead of reading the draft area you can simply get the file content or save the files somewhere. I agree we need some cleanup function for forms, but I think it should be something more general like $mform->dispose() which actually does all the necessary cleanup for all form elements and it should also clear the submitted flag in _POST.

          The problem is that the draft files would get discarded after the new proposed $mform->dispose().

          Why do you need this get_draft_files()? You just want to get one file, right? The correct way now is to do $mform->get_file_content($elname). As I said before the problem is that it is not suitable for large files, so you might need to actually use $mform->save_file() and store the content into temporary file.

          So in short:
          1/ instead of mform->delete_draft_files($elname) we need mform->dispose() imo
          2/ I do not think we need get_draft_files() when the necessary mform->get_file_content() and mform->save_file() already exists (it is compatible with old style upload and new file pickers btw)
          3/ +1 for the "|| $element instanceof MoodleQuickForm_filemanager"

          Petr

          Show
          Petr Škoda added a comment - hmm, this is exactly what I tried to prevent in the file api design, instead of reading the draft area you can simply get the file content or save the files somewhere. I agree we need some cleanup function for forms, but I think it should be something more general like $mform->dispose() which actually does all the necessary cleanup for all form elements and it should also clear the submitted flag in _POST. The problem is that the draft files would get discarded after the new proposed $mform->dispose(). Why do you need this get_draft_files()? You just want to get one file, right? The correct way now is to do $mform->get_file_content($elname). As I said before the problem is that it is not suitable for large files, so you might need to actually use $mform->save_file() and store the content into temporary file. So in short: 1/ instead of mform->delete_draft_files($elname) we need mform->dispose() imo 2/ I do not think we need get_draft_files() when the necessary mform->get_file_content() and mform->save_file() already exists (it is compatible with old style upload and new file pickers btw) 3/ +1 for the "|| $element instanceof MoodleQuickForm_filemanager" Petr
          Hide
          Dongsheng Cai added a comment -

          Hi, Petr,

          Thanks for review.

          I already used get_file_content for a few places, sometimes, if modules need file handler, save_file function will be used.

          The problem I have got here is question importing, it is trying to get the uploaded file name, in this case, get_file_content and save_file are not enough, I haven't looked into the question plugins yet, but this function can be useful when you want draft file info.

          Show
          Dongsheng Cai added a comment - Hi, Petr, Thanks for review. I already used get_file_content for a few places, sometimes, if modules need file handler, save_file function will be used. The problem I have got here is question importing, it is trying to get the uploaded file name, in this case, get_file_content and save_file are not enough, I haven't looked into the question plugins yet, but this function can be useful when you want draft file info.
          Hide
          Petr Škoda added a comment -

          You can use mform->get_new_filename() to get the file name, right? The mform->get_draft_files() is dangerous because it would encourage bad coding style in most of the cases. IMO we should not add it if there is no valid use case for it.

          Show
          Petr Škoda added a comment - You can use mform->get_new_filename() to get the file name, right? The mform->get_draft_files() is dangerous because it would encourage bad coding style in most of the cases. IMO we should not add it if there is no valid use case for it.
          Hide
          Dongsheng Cai added a comment -

          Thanks get_new_filename will solved the problem, I didn't notice this one.

          I will rename delete_draft_files and add "|| MoodleQuickForm_filemanager".

          Show
          Dongsheng Cai added a comment - Thanks get_new_filename will solved the problem, I didn't notice this one. I will rename delete_draft_files and add "|| MoodleQuickForm_filemanager".
          Hide
          David Mudrak added a comment -

          Hi Dong,

          I just realized that Outcomes import do not work at all, due to an exception thrown. As far as I can see, it is caused because the form submits into index.php which in turn includes import.php. In import.php, all environment (course, contexts, security) is repeated which leads to the exception (re-setting the course and therefore the theme).

          So I propose:
          1. either to copy the code from import.php directly into index.php
          2. or let the import form submit into import.php and after the successful process, redirect to index.php

          Show
          David Mudrak added a comment - Hi Dong, I just realized that Outcomes import do not work at all, due to an exception thrown. As far as I can see, it is caused because the form submits into index.php which in turn includes import.php. In import.php, all environment (course, contexts, security) is repeated which leads to the exception (re-setting the course and therefore the theme). So I propose: 1. either to copy the code from import.php directly into index.php 2. or let the import form submit into import.php and after the successful process, redirect to index.php
          Hide
          Dongsheng Cai added a comment -

          Hi, David
          Thanks, I am going to separate import form and index.php

          Show
          Dongsheng Cai added a comment - Hi, David Thanks, I am going to separate import form and index.php
          Hide
          Dongsheng Cai added a comment -

          the import course from hub will be tracked here: MDL-23148

          Show
          Dongsheng Cai added a comment - the import course from hub will be tracked here: MDL-23148
          Hide
          Dongsheng Cai added a comment -

          Please feel free to reopen it if you find more 'import' button needed to use filepicker

          Show
          Dongsheng Cai added a comment - Please feel free to reopen it if you find more 'import' button needed to use filepicker

            People

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

              Dates

              • Created:
                Updated:
                Resolved: