Moodle

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
  • Pull Master Diff URL:

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

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: