Moodle
  1. Moodle
  2. MDL-25937

Filepicker element does not work with required or disabledif rules

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.1, 2.1.1, 2.2
    • Fix Version/s: 2.2
    • Component/s: Files API, Forms Library
    • Labels:
    • Testing Instructions:
      Hide

      Test 1: Check filepicker validation
      1. Log in as admin
      2. Select a course
      3. Import grades (Settings -> grade -> import -> CSV file)
      4. Click upload grades (Without choosing a file)
      5. You should see validation error.
      6. Refresh the page and click "choose a file" and cancel without selecting any file
      7. Click upload grades
      8. You should see validation error.
      9. Refresh the page and click "choose a file" and select a file
      10. Click upload grades
      11. Form should be submitted.

      Test 2: Check disable if validation
      1. Log in as admin
      2. Select a course
      3. Import grades (Settings -> grade -> import -> XML file)
      4. Enter text in "Remote file URL" and then click outside
      5. "Choose a file" button should be disabled.
      6. Delete text in "Remote file URL" and then click outside
      6a. "Choose a file" button should be enabled
      7. Click "Choose a file" button and cancel
      8. Nothing should happen
      9. Click "Choose a file" button and select a file
      10. "Remote file URL" should be disabled.
      11. Try submit the form and form should be submitted.

      Test 3:
      1. copy attached test.php in moodle installation
      2. Login as admin
      3. access test.php
      4. In "Filepicker serverside validation test" click "choose a file" but don't upload and click "server validation"
      5. You should see validation error in red and url should be changed, adding sesskey and other variables
      6. Now add a file and click "server validation"
      7. On top of page it should say "test passed"
      8. Repeat 5-7 for "Filepicker clientside validation test" and it should work similar to serverside validation.
      Note: There is no client side validation happening, this test make sure that if client or server side is set then validation should happen on serverside for filepicker.
      9. In "Filepicker disableif validation test", enter text and click outside.
      10. Filepicker button should be disabled
      11. Clear text and select a file, make sure text field is disabled
      12. Clicking on validation button should show "test passed"
      13. Repeat 5-8 for "Filemanager server/client side validation test" and it should behave similarly.
      Note;
      Make sure to add files from server area, recent file area, upload a file and private area of filepicker and filemanager to make sure file is picked from all the places.

      Show
      Test 1: Check filepicker validation 1. Log in as admin 2. Select a course 3. Import grades (Settings -> grade -> import -> CSV file) 4. Click upload grades (Without choosing a file) 5. You should see validation error. 6. Refresh the page and click "choose a file" and cancel without selecting any file 7. Click upload grades 8. You should see validation error. 9. Refresh the page and click "choose a file" and select a file 10. Click upload grades 11. Form should be submitted. Test 2: Check disable if validation 1. Log in as admin 2. Select a course 3. Import grades (Settings -> grade -> import -> XML file) 4. Enter text in "Remote file URL" and then click outside 5. "Choose a file" button should be disabled. 6. Delete text in "Remote file URL" and then click outside 6a. "Choose a file" button should be enabled 7. Click "Choose a file" button and cancel 8. Nothing should happen 9. Click "Choose a file" button and select a file 10. "Remote file URL" should be disabled. 11. Try submit the form and form should be submitted. Test 3: 1. copy attached test.php in moodle installation 2. Login as admin 3. access test.php 4. In "Filepicker serverside validation test" click "choose a file" but don't upload and click "server validation" 5. You should see validation error in red and url should be changed, adding sesskey and other variables 6. Now add a file and click "server validation" 7. On top of page it should say "test passed" 8. Repeat 5-7 for "Filepicker clientside validation test" and it should work similar to serverside validation. Note: There is no client side validation happening, this test make sure that if client or server side is set then validation should happen on serverside for filepicker. 9. In "Filepicker disableif validation test", enter text and click outside. 10. Filepicker button should be disabled 11. Clear text and select a file, make sure text field is disabled 12. Clicking on validation button should show "test passed" 13. Repeat 5-8 for "Filemanager server/client side validation test" and it should behave similarly. Note; Make sure to add files from server area, recent file area, upload a file and private area of filepicker and filemanager to make sure file is picked from all the places.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      wip-mdl-25937-disableif
    • Rank:
      15318

      Description

      While peer-reviewing a patch I stumbled upon a bug with the file picker / mforms library.
      I was on a form where I was required to upload a file, however I found I could submit the form and proceed without the file.
      Originally I thought someone had forgot to add a required rule, however upon inspecting the form I see that it does indeed have a required rule on the file picker element.

      To reproduce:

      1. Log in as an admin
      2. Enter a course and click grades in the settings block for that course.
      3. In the settings block again under Grade administration expand Import and click CSV file
      4. On the form you see immediately click Upload grades

      Outcome: You will be taken to the next form in the series, however because the file picker has a required rule I would expect to get an error and see the original form again.
      I tested this by adding a validate method and manually validating the field, doing this worked fine, however it isn't a fix its a work around.

      Yell out if there is anything more I can help with.

      Cheers
      Sam

      1. test.php
        5 kB
        Rajesh Taneja

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment - - edited

          I encountered this problem today.

          Note you cannot call get_file_content (as given in documentation) within the validate function because it recursively will call the validate function.

          Here is my workaround in case it is useful to others:

              /**
               * @param string $elname Name of filepicker form element 
               * @return bool True if the filepicker contains a file, false otherwise
               */
              private function has_file($elname) {
                  global $USER;
                  $values = $this->_form->exportValues($elname);
                  if (empty($values[$elname])) {
                      return false;
                  }
                  $draftid = $values[$elname];
                  $fs = get_file_storage();
                  $context = get_context_instance(CONTEXT_USER, $USER->id);
                  if (!$files = $fs->get_area_files($context->id, 'user', 'draft', $draftid, 'id DESC', false)) {
                      return false;
                  }
                  return true;
              }
          
              function validation($data) {
                  global $DB;
                  $mform = $this->_form;
                  $errors = array();
          
                  if (!$this->has_file('backupfile')) {
                      $errors['backupfile'] = get_string('error_missingfile', 'local_convertoldbackup');
                  }
          
                  return $errors;
              }
          

          Note: If it isn't obvious, the code in has_file was based on copy/paste of the first bit of get_file_content.

          Show
          Sam Marshall added a comment - - edited I encountered this problem today. Note you cannot call get_file_content (as given in documentation) within the validate function because it recursively will call the validate function. Here is my workaround in case it is useful to others: /** * @param string $elname Name of filepicker form element * @ return bool True if the filepicker contains a file, false otherwise */ private function has_file($elname) { global $USER; $values = $ this ->_form->exportValues($elname); if (empty($values[$elname])) { return false ; } $draftid = $values[$elname]; $fs = get_file_storage(); $context = get_context_instance(CONTEXT_USER, $USER->id); if (!$files = $fs->get_area_files($context->id, 'user', 'draft', $draftid, 'id DESC', false )) { return false ; } return true ; } function validation($data) { global $DB; $mform = $ this ->_form; $errors = array(); if (!$ this ->has_file('backupfile')) { $errors['backupfile'] = get_string('error_missingfile', 'local_convertoldbackup'); } return $errors; } Note: If it isn't obvious, the code in has_file was based on copy/paste of the first bit of get_file_content.
          Hide
          Tim Hunt added a comment -

          The same underlying problem also breaks disabledIf.

          To reproduce that, go to Grade -> Import -> XML. Note that the URL field is disabled, even though no file has been selected. If you look at the form definition, you will see that selecting a file should disable the URL box, and entering a URL should disable the filepicker.

          Basically, formslib thinks that a filepicker always has a value set, and that is what needs to be fixed.

          I am taking the libertly of upping the severity, because the Gradebook issue is causing us pain at the OU. (Note for me, Redmine issue #637.)

          Show
          Tim Hunt added a comment - The same underlying problem also breaks disabledIf. To reproduce that, go to Grade -> Import -> XML. Note that the URL field is disabled, even though no file has been selected. If you look at the form definition, you will see that selecting a file should disable the URL box, and entering a URL should disable the filepicker. Basically, formslib thinks that a filepicker always has a value set, and that is what needs to be fixed. I am taking the libertly of upping the severity, because the Gradebook issue is causing us pain at the OU. (Note for me, Redmine issue #637.)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Adding some watchers, this needs dispatching ASAP.

          Show
          Eloy Lafuente (stronk7) added a comment - Adding some watchers, this needs dispatching ASAP.
          Hide
          Tim Hunt added a comment -

          See Eloy's comment. He is right!

          Show
          Tim Hunt added a comment - See Eloy's comment. He is right!
          Hide
          Michael de Raadt added a comment -

          Scheduling this for next sprint.

          Show
          Michael de Raadt added a comment - Scheduling this for next sprint.
          Hide
          Tim Hunt added a comment -

          Yay! Thank you.

          Show
          Tim Hunt added a comment - Yay! Thank you.
          Hide
          Rajesh Taneja added a comment -

          Solution has 2 commits, solving filepicker and filemanager validation:
          1. Server-side validation.
          2. Client-side validation.

          filepicker and filemanager are special elements, hence quickform doesn't know how to handle them and can't apply any validation rule. Which is why validation never happened on these elements. Proposed solution will add a custom validation for these elements in formslib.php which will validate these elements (if they are required fields)

          Client side validation was not possible because filepicker and filemanager add

          <input type="hidden" name="'.$elname.'" id="'.$id.'" value="'.$draftitemid.'" class="filepickerhidden"/>
          

          As we don't pass any specific format to validation rule, current validation is only checking for empty value and in this case draft value is always supplied (To keep in compatible for non-JS browsers). Hence, the proposed solution will remove draft_id on initialization of filepicker/filemanager and will be added back when file is uploaded/selected (in filepicker/filemanager callback). This will not break anything as client side validation will only happen by JS and on-the-fly removing and adding of draft_id will ensure that value is provided in case file is uploaded/selected.

          Show
          Rajesh Taneja added a comment - Solution has 2 commits, solving filepicker and filemanager validation: 1. Server-side validation. 2. Client-side validation. filepicker and filemanager are special elements, hence quickform doesn't know how to handle them and can't apply any validation rule. Which is why validation never happened on these elements. Proposed solution will add a custom validation for these elements in formslib.php which will validate these elements (if they are required fields) Client side validation was not possible because filepicker and filemanager add <input type= "hidden" name= "'.$elname.'" id= "'.$id.'" value= "'.$draftitemid.'" class= "filepickerhidden" /> As we don't pass any specific format to validation rule, current validation is only checking for empty value and in this case draft value is always supplied (To keep in compatible for non-JS browsers). Hence, the proposed solution will remove draft_id on initialization of filepicker/filemanager and will be added back when file is uploaded/selected (in filepicker/filemanager callback). This will not break anything as client side validation will only happen by JS and on-the-fly removing and adding of draft_id will ensure that value is provided in case file is uploaded/selected.
          Hide
          Dongsheng Cai added a comment -

          Thanks Rajesh for fixing this bug

          There are a few things I can think of:

          1. `function validate_draft_files` we don't add '' prefix to function
          2. https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-25937#L6R363 `$fs = get_file_storage();` this line should outside of foreach
          3. https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-25937#L1R162 I am not sure about the way of triggering click event on hidden element, It's better to use YUI simulate() to trigger custom event(or onchange), Rajesh mentioned YUI is not available in lib/form/filepicker.js, and he suggested we better rewriting that script to work like lib/form/filemanger.js, sounds good to me.
          Show
          Dongsheng Cai added a comment - Thanks Rajesh for fixing this bug There are a few things I can think of: `function validate_draft_files` we don't add ' ' prefix to function https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-25937#L6R363 `$fs = get_file_storage();` this line should outside of foreach https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-25937#L1R162 I am not sure about the way of triggering click event on hidden element, It's better to use YUI simulate() to trigger custom event(or onchange), Rajesh mentioned YUI is not available in lib/form/filepicker.js, and he suggested we better rewriting that script to work like lib/form/filemanger.js, sounds good to me.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Dongsheng
          I have updated the branch with your feedback.
          For point 2:
          This has been done intentionally to avoid redundant check of file in draft area. (This should only happen if validation is required.) In current case it will happen once so leaving it in for loop.
          For point 3:
          For backward compatibility, I am keeping filepicker as it is and saving YUI context to be used in callback.

          Can you please review this again...

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Dongsheng I have updated the branch with your feedback. For point 2: This has been done intentionally to avoid redundant check of file in draft area. (This should only happen if validation is required.) In current case it will happen once so leaving it in for loop. For point 3: For backward compatibility, I am keeping filepicker as it is and saving YUI context to be used in callback. Can you please review this again...
          Hide
          Dongsheng Cai added a comment -

          Looks good to me +1

          Show
          Dongsheng Cai added a comment - Looks good to me +1
          Hide
          Rajesh Taneja added a comment -

          Thanks Dongsheng

          Show
          Rajesh Taneja added a comment - Thanks Dongsheng
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Rajesh Taneja added a comment -

          Branches rebased

          Show
          Rajesh Taneja added a comment - Branches rebased
          Hide
          Sam Hemelryk added a comment -

          Thanks Raj, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Raj, this has been integrated now
          Hide
          Michael de Raadt added a comment -

          Test result: test passed. Well done!

          Show
          Michael de Raadt added a comment - Test result: test passed. Well done!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing. Ciao
          Hide
          Petr Škoda added a comment -

          This created a serious regression described MDL-29512, please revert asap, we can not release next weekly without a fix.

          I have tested that reverting of b19539bf44ac82ee4c379712b09f4fe54d8a7bd9 and 1f552b4c48c0cb509387d3046e2b2e5c27865c95 in 21 branch fixes the regression for me.

          I am proposing to revert+reopen this issue and reroll the weeklies today or at least on Monday.

          Show
          Petr Škoda added a comment - This created a serious regression described MDL-29512 , please revert asap, we can not release next weekly without a fix. I have tested that reverting of b19539bf44ac82ee4c379712b09f4fe54d8a7bd9 and 1f552b4c48c0cb509387d3046e2b2e5c27865c95 in 21 branch fixes the regression for me. I am proposing to revert+reopen this issue and reroll the weeklies today or at least on Monday.
          Hide
          Petr Škoda added a comment -

          I suppose the regression is caused by:

          //For client side validation, remove hidden draft_id
          Y.one('#id_'+options.elementname).set('value', '');

          I do not understand this, why would anybody want to throw out the most precious data we have in any file related forms element?

          In anycase changes like this should be tested for some time in master, it should not imo go into stable branches after a few minutes of integration testing.

          Show
          Petr Škoda added a comment - I suppose the regression is caused by: //For client side validation, remove hidden draft_id Y.one('#id_'+options.elementname).set('value', ''); I do not understand this, why would anybody want to throw out the most precious data we have in any file related forms element? In anycase changes like this should be tested for some time in master, it should not imo go into stable branches after a few minutes of integration testing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Reopening this as far as has been reverted completely by MDL-29512 + emergency build on Sunday, 25th September 2011

          Show
          Eloy Lafuente (stronk7) added a comment - Reopening this as far as has been reverted completely by MDL-29512 + emergency build on Sunday, 25th September 2011
          Hide
          Tim Hunt added a comment -

          Petr, can you give us any hints how this bug should be fixed without causing major regressions? Thanks.

          Show
          Tim Hunt added a comment - Petr, can you give us any hints how this bug should be fixed without causing major regressions? Thanks.
          Hide
          Petr Škoda added a comment - - edited

          I do not know anything about filepicker/manager javascript internals, I could help only with the server side validation. For now you can write simple form validation that verifies that file is actually picked in the filepicker (the required rule).

          I have no idea how to do the disabledIf thing properly. I am 100% sure about one thing - we must not remove the draftitemid from the form. It could probably just disable the pick button and should not mess with the files in draftarea at all because the form processing logic should ignore the changes..

          Show
          Petr Škoda added a comment - - edited I do not know anything about filepicker/manager javascript internals, I could help only with the server side validation. For now you can write simple form validation that verifies that file is actually picked in the filepicker (the required rule). I have no idea how to do the disabledIf thing properly. I am 100% sure about one thing - we must not remove the draftitemid from the form. It could probably just disable the pick button and should not mess with the files in draftarea at all because the form processing logic should ignore the changes..
          Hide
          Rajesh Taneja added a comment -

          Quickform client-side validation expects element value to be empty. The solution was implemented, keeping in mind that we should not do any change in quickform. While initializing, Filepicker and Filemanager add draftitemid to element, because of which client-side validation doesn't work.
          Current solution will work for filepicker, although we need to look at how this can be implemented for filemanager.

          Show
          Rajesh Taneja added a comment - Quickform client-side validation expects element value to be empty. The solution was implemented, keeping in mind that we should not do any change in quickform. While initializing, Filepicker and Filemanager add draftitemid to element, because of which client-side validation doesn't work. Current solution will work for filepicker, although we need to look at how this can be implemented for filemanager.
          Hide
          Rajesh Taneja added a comment -

          Looking at this again.

          Show
          Rajesh Taneja added a comment - Looking at this again.
          Hide
          Michael de Raadt added a comment -

          Carrying this over to the current sprint.

          Show
          Michael de Raadt added a comment - Carrying this over to the current sprint.
          Hide
          Tim Hunt added a comment -

          Rajesh, not problem if you need to change Quickforms. That project is no longer maintained by anyone else, so it is not like we will ever get bugfixes from upstream that we need to integrate.

          However, I don't see the need to change quickform (I have not really looked). The filepicker / filemanager element is all our code, and the hidden field is part of that, so we should be in complete control of how it is validated or enabled/disabled.

          Show
          Tim Hunt added a comment - Rajesh, not problem if you need to change Quickforms. That project is no longer maintained by anyone else, so it is not like we will ever get bugfixes from upstream that we need to integrate. However, I don't see the need to change quickform (I have not really looked). The filepicker / filemanager element is all our code, and the hidden field is part of that, so we should be in complete control of how it is validated or enabled/disabled.
          Hide
          Dongsheng Cai added a comment - - edited

          I just talked with Rajesh, I noted here for commenting:

          Files validation

          Server side validation

          It worked fine, validate_draft_files does the job

          Client side validation

          The previous solution is set hidden element's value to empty, then a file picked, file picker callback function will set the value to draft itemid, same to file manager, but it causing a problem when editing an existing draft area.

          Another solution I can think of: registering all file picker/file manager form elements in global name spaces:

          M.form_filepickers = {};
          M.form_filemanagers = {};
          

          When initializing a file picker/file manager, we register it using the form element name, for example:

          M.form_filepickers['backup_file'] = {};
          M.form_filepickers['backup_file'].fileadded = false;
          

          When a file picker or added by file manager, in the form callback function we set the `fileadded` flag to true.

          In moodle form validation javascript, because form knows element name and have access to `M`, so form is able to know the status of picker/manager.

          For editing existing file manager, we need to change the flag if there are files exist. I think file manager itself knows element (I think), if not, the element can be part of file manager options.

          disabledif

          I believe it's caused by the same issue as client validation. So registering in global name space could solve it too.

          Show
          Dongsheng Cai added a comment - - edited I just talked with Rajesh, I noted here for commenting: Files validation Server side validation It worked fine, validate_draft_files does the job Client side validation The previous solution is set hidden element's value to empty, then a file picked, file picker callback function will set the value to draft itemid, same to file manager, but it causing a problem when editing an existing draft area. Another solution I can think of: registering all file picker/file manager form elements in global name spaces: M.form_filepickers = {}; M.form_filemanagers = {}; When initializing a file picker/file manager, we register it using the form element name, for example: M.form_filepickers['backup_file'] = {}; M.form_filepickers['backup_file'].fileadded = false ; When a file picker or added by file manager, in the form callback function we set the `fileadded` flag to true. In moodle form validation javascript, because form knows element name and have access to `M`, so form is able to know the status of picker/manager. For editing existing file manager, we need to change the flag if there are files exist. I think file manager itself knows element (I think), if not, the element can be part of file manager options. disabledif I believe it's caused by the same issue as client validation. So registering in global name space could solve it too.
          Hide
          Rajesh Taneja added a comment -

          Thanks for all the support Tim and DongSheng.

          I think, filepicker and filemanager validation should be fixed in two separate bugs. As I haven't seen any validation issue for filemanager currently, I think we can handle it with much nicer approach later in separate bug.

          @Dongsheng
          Can you please peer review my new fix for the same.

          Show
          Rajesh Taneja added a comment - Thanks for all the support Tim and DongSheng. I think, filepicker and filemanager validation should be fixed in two separate bugs. As I haven't seen any validation issue for filemanager currently, I think we can handle it with much nicer approach later in separate bug. @Dongsheng Can you please peer review my new fix for the same.
          Hide
          Dongsheng Cai added a comment -

          I had a quick look:

          1. Instead of using form_filepickers_fileadded, can we use M.form_filepickers[elementname] = {} ? You can access it via M.form_filepickers[elementname].fileadded. The point is we may need other flags later, we cannot keep adding more flags like form_filepickers_otherflags.
          2. When edit existing files, we already have file in draft area without adding new one, in this case, filepicker element needs to pass option to filepicker.js, and change the `fileadded` flag.
          3. Where is the changes to file manager? Are you planning to fix file manager in another issue?
          Show
          Dongsheng Cai added a comment - I had a quick look: Instead of using form_filepickers_fileadded, can we use M.form_filepickers [elementname] = {} ? You can access it via M.form_filepickers [elementname] .fileadded. The point is we may need other flags later, we cannot keep adding more flags like form_filepickers_otherflags. When edit existing files, we already have file in draft area without adding new one, in this case, filepicker element needs to pass option to filepicker.js, and change the `fileadded` flag. Where is the changes to file manager? Are you planning to fix file manager in another issue?
          Hide
          Rajesh Taneja added a comment -

          Thanks Dongsheng,
          1: I have updated the branch with M.form_filepickers[elementname] and keeping the status of file in M.form_filepickers[elementname].fileadded
          2: Do we load draft files for editing in filepicker? As per my understanding filemanager only upload files in draft area, in such case do we need to update any flag?
          3: Yes, I will prefer handling filemanager in separate bug. As suggested by Tim, we can modify quickform, so I would love to tweak Quickform design to do this in correct way. Saying so, that will go in master only.

          Show
          Rajesh Taneja added a comment - Thanks Dongsheng, 1: I have updated the branch with M.form_filepickers [elementname] and keeping the status of file in M.form_filepickers [elementname] .fileadded 2: Do we load draft files for editing in filepicker? As per my understanding filemanager only upload files in draft area, in such case do we need to update any flag? 3: Yes, I will prefer handling filemanager in separate bug. As suggested by Tim, we can modify quickform, so I would love to tweak Quickform design to do this in correct way. Saying so, that will go in master only.
          Hide
          Dongsheng Cai added a comment -

          Hi Rajesh

          I was wrong on 2), if it doesn't process draft file then you don't need to worry about.

          Show
          Dongsheng Cai added a comment - Hi Rajesh I was wrong on 2), if it doesn't process draft file then you don't need to worry about.
          Hide
          Rajesh Taneja added a comment -

          Thanks Dongsheng,
          Pushing it for integration review.

          @integrators:
          I don't foresee any regression with this new fix. But being on the safer side, please integrate this in master only and probably after a week or two it can be cherry-picked for stable branches

          Show
          Rajesh Taneja added a comment - Thanks Dongsheng, Pushing it for integration review. @integrators: I don't foresee any regression with this new fix. But being on the safer side, please integrate this in master only and probably after a week or two it can be cherry-picked for stable branches
          Hide
          Aparup Banerjee added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and cheers!

          Show
          Aparup Banerjee added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and cheers!
          Hide
          Rajesh Taneja added a comment -

          branch rebased.

          Show
          Rajesh Taneja added a comment - branch rebased.
          Hide
          Aaron Wells added a comment -

          It might be worth noting the ticket MDL-26167 was also supposed to make filepickers work with disabledIf rules. Although, it certainly doesn't work on the grade/import/xml/index.php page.

          Show
          Aaron Wells added a comment - It might be worth noting the ticket MDL-26167 was also supposed to make filepickers work with disabledIf rules. Although, it certainly doesn't work on the grade/import/xml/index.php page.
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks Aaron,
          Current fix has taken care of this

          In this fix, two issues will be solved:
          1. server side validation for filepicker and filemanager (for required rule. grade/import/cvs/index.php)
          2. disableif rule for filepicker (grade/import/xml/index.php)

          Show
          Rajesh Taneja added a comment - - edited Thanks Aaron, Current fix has taken care of this In this fix, two issues will be solved: 1. server side validation for filepicker and filemanager (for required rule. grade/import/cvs/index.php) 2. disableif rule for filepicker (grade/import/xml/index.php)
          Hide
          Sam Hemelryk added a comment -

          Hey guys

          I've been looking at this for a while now going over it with a fine tooth comb (don't want a repeat of the last attempt to get this in).

          In general I am happy with the approach that this patch is taking however I think there are a couple of minor things still to tidy up, particularly the JS within filepicker.js which I really think can be tidied.

          1. filepicker.js The definition for M.form_filepickers doesn't need to check if M.form_filepickers already exists… In general the code here could be improved by not introducing another variable directly off M (ideally we'd like to avoid cluttering our global namespace) by instead using a property of M.form_filepicker like M.core_filepicker does for instances. Also noting there seems to be some confusion about the variable type of form_filepickers as it declared as an object and then used as an array. Works fine as JS is sloppy however it would be nice to tidy this up.

          2. filepicker.js you check M.form_filepickers[options.elementname] to see if it is already set - I don't think this is needed is it - that init call is made once for each element in the mform - its not really possible to have more than one element with the same name so this should never be initialised twice - an in fact if it was to be it wouldn't matter as everything else is gonna break WAY before it. This get my +1 to just initialise to false.

          3. filepicker.php No need to call $fp->options->element name it was added above to the args array which ends up being $fp->options.

          4. Within our JavaScript where we need to hold onto our YUI instance (Y) we always do so as blah.Y = Y. Y is an instance of YUI and storing it as Y is much clearer and consistent so my +1 to change M.form_filepicker.YUI => M.form_filepicker.Y ... At the same time actually you could add Y (or YUI) as a defined NULL property to M.form_filepicker so that people new to the code know to expect it and because its cleaner/clearer.

          5. Testing instructions need to be broadened to include what server/client side validation works and to include testing filepickers and managers with the different rules. I think you could either look for examples of each and with the different rules or you could create a test form script to use instead. Personally I'd look for examples - the more areas we test the lass chance of unexpected regressions like last time.
          We should also create issues to see missing sections like filepicker required rules on the client being implemented at some point (dev tasks)

          In general the JS behind filepicker and filemanager could still be greatly improved.
          During the brief bit of testing an investigation I found a couple of different bugs which I will work out how to reproduce accurately and then report.

          As one suggestion depending upon the path you choose to take here I would consider defining the callback function within the init function to avoid the need to hold onto Y. This would be a major improvement as presently if a form contains more than one filepicker element M.form_filepicker.YUI is replaced each time one is initialised.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hey guys I've been looking at this for a while now going over it with a fine tooth comb (don't want a repeat of the last attempt to get this in). In general I am happy with the approach that this patch is taking however I think there are a couple of minor things still to tidy up, particularly the JS within filepicker.js which I really think can be tidied. 1. filepicker.js The definition for M.form_filepickers doesn't need to check if M.form_filepickers already exists… In general the code here could be improved by not introducing another variable directly off M (ideally we'd like to avoid cluttering our global namespace) by instead using a property of M.form_filepicker like M.core_filepicker does for instances. Also noting there seems to be some confusion about the variable type of form_filepickers as it declared as an object and then used as an array. Works fine as JS is sloppy however it would be nice to tidy this up. 2. filepicker.js you check M.form_filepickers [options.elementname] to see if it is already set - I don't think this is needed is it - that init call is made once for each element in the mform - its not really possible to have more than one element with the same name so this should never be initialised twice - an in fact if it was to be it wouldn't matter as everything else is gonna break WAY before it. This get my +1 to just initialise to false. 3. filepicker.php No need to call $fp->options->element name it was added above to the args array which ends up being $fp->options. 4. Within our JavaScript where we need to hold onto our YUI instance (Y) we always do so as blah.Y = Y. Y is an instance of YUI and storing it as Y is much clearer and consistent so my +1 to change M.form_filepicker.YUI => M.form_filepicker.Y ... At the same time actually you could add Y (or YUI) as a defined NULL property to M.form_filepicker so that people new to the code know to expect it and because its cleaner/clearer. 5. Testing instructions need to be broadened to include what server/client side validation works and to include testing filepickers and managers with the different rules. I think you could either look for examples of each and with the different rules or you could create a test form script to use instead. Personally I'd look for examples - the more areas we test the lass chance of unexpected regressions like last time. We should also create issues to see missing sections like filepicker required rules on the client being implemented at some point (dev tasks) In general the JS behind filepicker and filemanager could still be greatly improved. During the brief bit of testing an investigation I found a couple of different bugs which I will work out how to reproduce accurately and then report. As one suggestion depending upon the path you choose to take here I would consider defining the callback function within the init function to avoid the need to hold onto Y. This would be a major improvement as presently if a form contains more than one filepicker element M.form_filepicker.YUI is replaced each time one is initialised. Cheers Sam
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks for the feedback Sam,
          Have integrated your feedback, can you please review this again for me.

          I am not sure where filemanager required rule is used. Will update testcase soon.

          Show
          Rajesh Taneja added a comment - - edited Thanks for the feedback Sam, Have integrated your feedback, can you please review this again for me. I am not sure where filemanager required rule is used. Will update testcase soon.
          Hide
          Sam Hemelryk added a comment -

          Thanks Raj - its looking better now.
          I'll wait until you get those testing instructions updated and will then look to integrate this.
          Please be sure to include expected result for filemanager + filepicker working with the disabledIf and required rules in regards to client and server as I really think we need to be 100% clear on what works where and when.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Raj - its looking better now. I'll wait until you get those testing instructions updated and will then look to integrate this. Please be sure to include expected result for filemanager + filepicker working with the disabledIf and required rules in regards to client and server as I really think we need to be 100% clear on what works where and when. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Thanks Raj - this has been integrated now.
          Testers Raj will have improved testing instructions/file shortly.

          Show
          Sam Hemelryk added a comment - Thanks Raj - this has been integrated now. Testers Raj will have improved testing instructions/file shortly.
          Hide
          Jason Fowler added a comment -

          Passed with flying colours, no problems encountered at all

          Show
          Jason Fowler added a comment - Passed with flying colours, no problems encountered at all
          Hide
          Eloy Lafuente (stronk7) added a comment -

          git repositories have been updated with your awesome changes, thanks! Closing.

          Show
          Eloy Lafuente (stronk7) added a comment - git repositories have been updated with your awesome changes, thanks! Closing.
          Hide
          Tim Hunt added a comment -

          Above, it says "please integrate this in master only and probably after a week or two it can be cherry-picked for stable branches". (Search for the word 'master' in this page.)

          Now, several weeks have passed since this was integrated. please can we back-port it now?

          Show
          Tim Hunt added a comment - Above, it says "please integrate this in master only and probably after a week or two it can be cherry-picked for stable branches". (Search for the word 'master' in this page.) Now, several weeks have passed since this was integrated. please can we back-port it now?
          Hide
          Tim Hunt added a comment -

          I just tested, and the fix works for me, and back-ports cleanly. Will make a new issue for back-porting it.

          Show
          Tim Hunt added a comment - I just tested, and the fix works for me, and back-ports cleanly. Will make a new issue for back-porting it.
          Hide
          Tim Hunt added a comment -

          MDL-30172 created and submitted for integration.

          Show
          Tim Hunt added a comment - MDL-30172 created and submitted for integration.
          Hide
          Rajesh Taneja added a comment -

          Thanks for pointing this Tim
          Backport issue was already there (MDL-29638) and is up for integration review.
          Please suggest if we should close MDL-29638 as duplicate of MDL-30172.

          Show
          Rajesh Taneja added a comment - Thanks for pointing this Tim Backport issue was already there ( MDL-29638 ) and is up for integration review. Please suggest if we should close MDL-29638 as duplicate of MDL-30172 .

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: