Fix import errors with unicode filenames

[jkarni]

@nedbat @singingwolfboy - Despite Ned's helpful lesson, I've no clue whether this is the right way about things, but courses with unicode file names can't be imported even though there isn't (to the best of my knowledge) a reason why for our not supporting them.

[jkarni]

@chrisndodge This seems like it's the same issue you were looking into [STUD-680]. With these changes I can import courses with unicode filenames, but I'm worried something else might go wrong.

[nedbat]

This would be better as: log.debug('importing static content %s...', content_path)

[singingwolfboy]

Why? Isn't using str.format() preferable to using the %s old-style string formatting?

[nedbat]

Best practice for log.debug and its ilk is to defer the formatting to the logging module itself. It accepts %-style formatting. In this case, it would also solve the unicode issues without an explicit encode on our part.

[chrisndodge]

Please add some test files with unicode characters to common/test/data test courses. In particular for courses that do an imprt/export/reimport paths. Thx.

I think your suspicion is likely correct in that this is probably a partial fix. Ultimately I think we need to make Location be urlencoded rather than doing the underscore hack.

[jkarni]

Yeah, I think this might be enough to get the import to stop falling, but
likely not enough for the assets and pages to be accessible.
On Aug 30, 2013 3:04 PM, "chrisndodge" notifications@github.com wrote:

Please add some test files with unicode characters to common/test/data
test courses. In particular for courses that do an imprt/export/reimport
paths. Thx.

I think your suspicion is likely correct in that this is probably a
partial fix. Ultimately I think we need to make Location be urlencoded
rather than doing the underscore hack.

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/830#issuecomment-23582712
.

[jkarni]

Wrote some tests cases for various potential unicode issues. I'm finding a number of other similar or related problems ; I guess we shouldn't merge a partial fix if it might mean unknown behavior later, so this probably shouldn't be merged.

[jkarni]

@nedbat @chrisndodge Added some tests that check that courses with unicode filenames still get imported, even if some files don't. And if we decide to surface the errors/exceptions (with or without backing off of import), the exception throwing and catching stuff now works.

[chrisndodge]

Hey, thanks for the sample course content & tests. Hate to be a nit-pick, but it seems like you just copied the toy course and added just a bit of stuff, but most of that course has nothing to do with unicode stuff. Can I suggest you prune away some of the non-relevant content in the "unicode" test course?

[jkarni]

Sure. Only thing I kind of wanted was for there to be non-unicode filenames
loaded and tested for after exceptions were raised for unicode filename. I
can try to figure out the load order so I can prune the course and still
get that.
On Sep 4, 2013 1:52 PM, "chrisndodge" notifications@github.com wrote:

Hey, thanks for the sample course content & tests. Hate to be a nit-pick,
but it seems like you just copied the toy course and added just a bit of
stuff, but most of that course has nothing to do with unicode stuff. Can I
suggest you prune away some of the non-relevant content in the "unicode"
test course?

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/830#issuecomment-23809740
.

[jkarni]

Build failing since diff-cover/diff-quality don't currently support unicode filenames. Opened a PR for diff-cover, and Will is reviewing/updating Jenkins, but merge will have to wait for that.

[singingwolfboy]

This import is no longer necessary

[jkarni]

@chrisndodge @nedbat Made the fixes and cleaned up the test course - let me know if there are other issues or if this is good to merge

[chrisndodge]

@jkarni just noticing that the build failed... :-(

[cahrens]

@chrisndodge It only failed because of the LTI known issue (with the version of oathlib). I'll kick off a manual run.

[chrisndodge]

This type of indexing is always going to break as we add more test courses. I wonder if we should just add a helpful method such as find_test_course_by_name(courses, 'toy').

This is a nice-to-have....

[chrisndodge]

I believe @brianhw also did some unicode compatibility work. Do you want to brush this by him as well?

[brianhw]

I don't think I have much to add to this -- looks good.

[singingwolfboy]

Why does the list of courses in the initdb method contain four courses ('toy', 'simple', 'simple_with_draft', and 'test_unicode') but this test expects there to be five? Can we calculate what the number should be in the initdb method and save it somewhere, rather than hardcoding it into this test? (Is that even a good idea?)

[jkarni]

The fifth course gets imported in a separate call to import_from_xml since its params are different.

Not sure about calculating the number: we obviously shouldn't just do len(courses) since that's what we're trying to test. We could create a wrapper around import_from_xml that adds to a global object; but that wouldn't work if the import_from_xml call is supposed to fail...

[singingwolfboy]

Aside from my one comment about hardcoding the test value, this looks good to me.

[nedbat]

Please follow the docstring formatting guidelines here: https://edx-wiki.atlassian.net/wiki/display/ENG/Python+Guidelines

[jkarni]

In our codebase I most commonly see the first line start at the same line as the triple-quotes, but the link suggests otherwise. Does it matter? If so, which way do I go?

[nedbat]

Do it the way the wiki page shows. We need to be more consistent, it's true.

[chrisndodge]

+1 from me