Moodle
  1. Moodle
  2. MDL-36453

Database preset import doesn't work on Windows

    Details

    • Testing Instructions:
      Hide

      Test pre-requisites

      • A windows server to test this on.

      Test steps

      1. Log in as admin/teacher.
      2. Navigate to a course.
      3. Create a Database activity.
      4. Click on the Preset tab.
      5. Upload a preset zip file.
      6. Work through the import process.
        • The import should succeed with no errors.
      • Test again with Ubuntu or some other operating system to ensure there are no regressions.
      Show
      Test pre-requisites A windows server to test this on. Test steps Log in as admin/teacher. Navigate to a course. Create a Database activity. Click on the Preset tab. Upload a preset zip file. Work through the import process. The import should succeed with no errors. Test again with Ubuntu or some other operating system to ensure there are no regressions.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-36453-master
    • Rank:
      45284

      Description

      When trying to import a Database activity preset file on a Windows server, the import fails as the importer filters away part of the imported file's filename part way through.

      To replicate this, you will need a server on Windows.

      Replication steps:

      1. Log in as admin/teacher
      2. Navigate to a course
      3. Create a Database activity
      4. Click on the Preset tab
      5. Upload a preset zip file (I will attach an example)
      6. Work through the import process

      Expected result: The import should complete successfully

      Actual result: The import fails in the final stage and reports a 'cannotimport' error (with little detail).

      On a Windows server, the preset file is passed to the server with a .tmp extension (eg. tem2464.tmp). This is able to be extracted to a directory (eg. tem2464.tmp_extracted) without trouble, but on line 219, the extracted preset directory is received as a form field and the PARAM_ALPHANUMEXT filter is applied, which removes dots (eg. tem2464tmp_extracted). When the script checks that the directory exists, this fails and the import fails.

      $presetdir = $CFG->tempdir.'/forms/'.required_param('directory', PARAM_ALPHANUMEXT);
      

      Replacing this filter with the more appropriate PARAM_FILE filter allows dots through and allows the preset import to complete.

        Activity

        Hide
        Andrew Davis added a comment -

        [Y] Syntax
        [NA] Output
        [Y] Whitespace
        [NA] Language
        [NA] Databases
        [Y] Testing
        [NA] Security
        [Y] Documentation
        [Y] Git
        [Y] Sanity check

        re the commit message I believe we should be using frankenstyle component names ie mod_data instead of mod/data. I could easily be wrong however.

        Otherwise, looks good. You are go for integration.

        Show
        Andrew Davis added a comment - [Y] Syntax [NA] Output [Y] Whitespace [NA] Language [NA] Databases [Y] Testing [NA] Security [Y] Documentation [Y] Git [Y] Sanity check re the commit message I believe we should be using frankenstyle component names ie mod_data instead of mod/data. I could easily be wrong however. Otherwise, looks good. You are go for integration.
        Hide
        Adrian Greeve added a comment -

        Thanks for the review Andrew.
        I'm not sure exactly what the correct method for listing the component is. I can't seem to find any documentation about it. Maybe a discussion in the dev chat might get some definitive answers about that.

        Show
        Adrian Greeve added a comment - Thanks for the review Andrew. I'm not sure exactly what the correct method for listing the component is. I can't seem to find any documentation about it. Maybe a discussion in the dev chat might get some definitive answers about that.
        Hide
        Dan Poltawski added a comment -

        Integrated to 22, 23 and master.

        Thanks guys.

        Show
        Dan Poltawski added a comment - Integrated to 22, 23 and master. Thanks guys.
        Hide
        Petr Škoda added a comment -

        Works for me fine on Win and OSX. Thanks.

        Show
        Petr Škoda added a comment - Works for me fine on Win and OSX. Thanks.
        Hide
        Dan Poltawski added a comment -

        Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

        ciao
        Dan

        Show
        Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan
        Hide
        Aaricia Thorgalson added a comment -

        When giving instructions about how to fix the bug, it is important that you mention that the file you need to modify is:

        moodle core directory/mod/data/preset.php and then in line 200 (approximately) replace the aforementioned code ($presetdir = $CFG->tempdir.'/forms/'.required_param('directory', PARAM_ALPHANUMEXT) by this:

        $presetdir = $CFG->tempdir.'/forms/'.required_param('directory', PARAM_FILE);

        The explanation may be obvious to you, but not to non-technical people who are trying to solve the problem.

        As I stated in the Moodle Moot UK/Ireland 2013, the main problem of the Moodle Tracker is that the patches or solution proposed are difficult to understand for many moodlers who would be happy to implement the amendments if they are given clearer instructions on how to do it. Helen Foster, who was attending my session, agreed with me on that.

        Show
        Aaricia Thorgalson added a comment - When giving instructions about how to fix the bug, it is important that you mention that the file you need to modify is: moodle core directory/mod/data/preset.php and then in line 200 (approximately) replace the aforementioned code ($presetdir = $CFG->tempdir.'/forms/'.required_param('directory', PARAM_ALPHANUMEXT) by this: $presetdir = $CFG->tempdir.'/forms/'.required_param('directory', PARAM_FILE); The explanation may be obvious to you, but not to non-technical people who are trying to solve the problem. As I stated in the Moodle Moot UK/Ireland 2013, the main problem of the Moodle Tracker is that the patches or solution proposed are difficult to understand for many moodlers who would be happy to implement the amendments if they are given clearer instructions on how to do it. Helen Foster, who was attending my session, agreed with me on that.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: