Skip to content
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.

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

Fix var name issue #762

Merged
merged 1 commit into from
Aug 21, 2013
Merged

Fix var name issue #762

merged 1 commit into from
Aug 21, 2013

Conversation

jkarni
Copy link

@jkarni jkarni commented Aug 21, 2013

Typo that was preventing >10MB asset uploads

@rocha
Copy link
Contributor

rocha commented Aug 21, 2013

Reviewed in #753

@@ -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

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

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

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

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

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.

rocha added a commit that referenced this pull request Aug 21, 2013
@rocha rocha merged commit e01ea88 into release Aug 21, 2013
@rocha rocha deleted the jkarni/hotfix/tempfile-varname branch August 21, 2013 18:16
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
itsjeyd pushed a commit to open-craft/edx-platform that referenced this pull request Feb 10, 2017
caesar2164 pushed a commit to caesar2164/edx-platform that referenced this pull request Feb 16, 2018
* stv/unfork/superuser:
  Split superuser_login_as into djangoapp
andrey-canon pushed a commit to eduNEXT/edx-platform that referenced this pull request Jun 21, 2018
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
…edx#762)

squash!: remove unnecessary styling and migrate to bootstrap and other review feedback
johanseto pushed a commit to nelc/edx-platform that referenced this pull request Jan 22, 2024
Merge pull request openedx#32032 from raccoongang/sagirov/tCRIL_GA-18

[FC-0014] Add GA 4 support to edX platform

Co-authored-by: Brian Mesick <112640379+bmtcril@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants