Jkarni/feature/chunkedupload

[jkarni]

Adds ability to import courses over 100MB.

[jkarni]

@singingwolfboy @cahrens

[singingwolfboy]

Can you add a comment indicating that this is intended to parse the HTTP_CONTENT_RANGE header? Regular expressions are always difficult to understand, and comments can go a long way towards making it easier.

[chrisndodge]

High level question/comment before I start reading the code. Does this approach use multiple POST backs to the server, with each POST sending one chunk? If so, we can't guarantee that each POST will be routed to the same production server through the ELB.

[jkarni]

Ugh... yeah, that's exactly what it does. Is there a way of getting the requests routed to the same server? The other option I can think of is to use the DB instead of the filesystem for temporary storage (of the tar file), but that looks like it'll be a lot uglier...

[chrisndodge]

Can you ping DevOps with this question? I can't remember if we have sticky ELB sessions or not. Even if we do, it's not guaranteed.

[jkarni]

Looks like we do. Waiting to see if stickiness lasts for the entire
session.
On Aug 14, 2013 7:10 PM, "chrisndodge" notifications@github.com wrote:

Can you ping DevOps with this question? I can't remember if we have sticky
ELB sessions or not. Even if we do, it's not guaranteed.

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

[singingwolfboy]

This still seems confusing/unclear. I'd suggest something more like:

def get_all_files(directory):
    """
    For each file in the directory, yield a 2-tuple of (file-name, directory-path)
    """
    for dirpath, dirnames, filenames in os.walk(directory):
        for filename in filenames:
            yield (filename, dirpath)

def get_dir_for_fname(directory, filename):
    """
    Returns the dirpath for the first file found in the directory with the given name.
    If there is no file in the directory with the specified name, return None.
    """
    for fname, dirpath in get_all_files(directory):
        if fname == filename:
            return dirpath
    return None

fname = "course.xml"
dirpath = get_dir_for_fname(course_dir, fname)
if not dirpath:
    # return JsonResponse
# continue

It's longer, but it's much clearer. Also, if you move those two functions outside of the view function, they're reusable and the view is easier to unit test.

[chrisndodge]

This might be out-of-scope to the immediate work here, but could you:

Separate out the import/export functions in assets.py into a new view file, say import_export.py (be sure to also import in init.py in that folder and change the _ all _ variable in both assets.py and the new import_export.py).

This separation should have been done when we initially spit up the view files, and it must have been an oversight that these functions are grouped with the asset library.

If you'll do this I can close a PR which did this, but unfortunately wasn't merged before you did this work.

[jkarni]

Yeah, that makes sense, and should be a quick fix.
On Aug 15, 2013 9:10 PM, "chrisndodge" notifications@github.com wrote:

This might be out-of-scope to the immediate work here, but could you:

Separate out the import/export functions in assets.py into a new view
file, say import_export.py (be sure to also import in init.py in that
folder and change the all variable).

This separation should have been done when we initially spit up the view
files, and it must have been an oversight that these functions are grouped
with the asset library.

If you'll do this I can close a PR which did this, but unfortunately
wasn't merged before you did this work.

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

[cahrens]

Shouldn't this line (and CONTENT_RE below) be deleted?

[jkarni]

I'm using content_re in the multiple assets pr, but could delete both here
and then add again afterwards so the pr makes sense atomically
On Aug 19, 2013 10:57 AM, "Christina Roberts" notifications@github.com
wrote:

In cms/djangoapps/contentstore/views/assets.py:

+MAX_UP_LENGTH = 20000352 # Max chunk size

Shouldn't this line (and CONTENT_RE below) be deleted?

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/674/files#r5844227
.

[cahrens]

I hit a strange case when I tried uploading something that wasn't a tar.gz file. I got a couple errors (both notification bar "Studio couldn't save" bar and the error dialog) when I selected a proper tar.gz file AFTER I had selected a file with a different extension.

The JavaScript/'HTML changes in here are straightforward. I wanted to ensure that when an import fails, things don't "blow up". Julian says he has tested that case.

[jkarni]

@cahrens that was definitely a regression - am pushing a fix now.

[cahrens]

A couple odd things--

1. If I choose a file that is not supported, and then choose one that IS supported, after the 2nd successful upload I get 2 "your upload was successful" dialogs.

2. On master, we replace the "Replace my course with the one above" with the progress bar. In your PR, the button stays. This looks odd (progress bar is not centered, you wonder if you should click the button again).

[jkarni]

@cahrens - looks like it was because I wasn't removing the submit callbacks. New commit fixes the issue. But just to be clear - you get one "your upload was successful" and one "your upload failed", right?

(2) also fixed.
As a side note, there was a hidden bug where when uploading a tarfile without a 'course.xml', the user would get "upload was successful" alert, and be redirected, even though the upload was aborted.

[cahrens]

@jkarni This looks good-- 👍 for interactive testing and JavaScript/HTML, though note that I never tested when the upload actually failed (I only tested unsupported extensions). I assume you have tested the case of the upload failing, with the final version of the code.

Please update the BDD manal testing spec as appropriate, including failure cases: https://edx-wiki.atlassian.net/wiki/display/STU/Import

@chrisndodge We still need your Python code review, and testing this on staging would be splendid.

[chrisndodge]

I can push to stage tonight. Since the release is out migrations have already been run there.

Sent from my iPhone

On Aug 20, 2013, at 2:54 PM, Christina Roberts notifications@github.com wrote:

@jkarni This looks good-- for interactive testing and JavaScript/HTML, though note that I never tested when the upload actually failed (I only tested unsupported extensions). I assume you have tested the case of the upload failing, with the final version of the code.

Please update the BDD manal testing spec as appropriate, including failure cases: https://edx-wiki.atlassian.net/wiki/display/STU/Import

@chrisndodge We still need your Python code review, and testing this on staging would be splendid.

—
Reply to this email directly or view it on GitHub.

[chrisndodge]

rebased to pick up the test fix

[chrisndodge]

Is this used? It seems like this should be in the new import_export.py file...

[chrisndodge]

Not sure why this is getting marked as unstable. There's 18 code lines in violation.

[jkarni]

@chrisndodge switched to a with-context so that all the files are sure to be deleted.
I was originally thinking of having the status file also be a lock, but the studio team should probably discuss what we want before - a lock just to make sure nothing goes wrong, or also a lock that will notify/prevent users from overwriting one another's changes before seeing them. Anyhow, this is probably in the scope of the status notification PR rather than this one, so for now I switched the name from lock to status

[chrisndodge]

That with-context is a nifty trick!

+1