Skip to content

Fix var name issue #762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rocha merged 1 commit into from
Aug 21, 2013
Merged

Fix var name issue #762

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cms/djangoapps/contentstore/views/assets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ def upload_asset(request, org, course, coursename):
sc_partial = partial(StaticContent, content_loc, filename, mime_type)
if chunked:
content = sc_partial(upload_file.chunks())
temp_filepath = upload_file.temporary_file_path()
tempfile_path = upload_file.temporary_file_path()
Copy link
Contributor

@nedbat nedbat Aug 21, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that there's no test that covers this line of code?

Copy link
Contributor

@rocha rocha Aug 21, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. @jkarni can you address?

Copy link
Author

@jkarni jkarni Aug 21, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the tests were being skipped since something was causing them to fail on Jenkins (though not locally - Jay might have the details). I have another PR [ #766 ] that tries to skip conditionally (if on Jenkins).

Copy link
Contributor

@nedbat nedbat Aug 22, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the tests could be run locally, were they skipping this line, or were you not running them, or did you not notice that they failed?

Copy link
Author

@jkarni jkarni Aug 22, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it was worse than that. They were both not testing this line (the if-clause) and being skipped. The commit that introduced this bug (mine) changed a whole lot of things, so I didn't notice they were being skipped, nor that this line wasn't being tested. The PR I mentioned adds a test for this line, and makes the test not be skipped locally. Jonah is looking into why exactly the test fails on Jenkins to see if there's a better solution.

else:
content = sc_partial(upload_file.read())
tempfile_path = None
Expand Down