Fix AssertionError when creating a course via the web UI#1520
Merged
Conversation
set_up_new_course initializes a local `repo = None` and closes it on both the success and failure paths, but git_clone_and_create_course never returned the repo it opened. On the success path `repo` was therefore still None and the `else: assert repo is not None` always fired -- raising AssertionError *after* the course had already been created (content validated, DB row written), which surfaces to the user as "creation succeeded" immediately followed by "creation failed: AssertionError" and a 500 on the new course page. The repo's file handles also leaked. Return the initialized repo from git_clone_and_create_course and assign it in the caller, so the existing close() logic on both branches works as intended.
5e54fe6 to
e59b232
Compare
Owner
|
Thanks! LGTM, in it goes. Sorry about the delay. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Creating a course through Set up new course fails with an
AssertionError, even though the content validates and the course is actually created. The user sees:…and then a 500 when opening the new course.
Traceback:
Cause
set_up_new_courseinitializes a localrepo = Noneand closes it on both the success (else) and failure (except) paths. Butgit_clone_and_create_courseopens its ownrepoviaRepo.init(...)and never returns it, so the caller'srepostaysNone. On the success path,else: assert repo is not Nonetherefore always fires — aftercreate_coursehas already validated content and written theCourserow. The clone's file handles also leak, since the intendedrepo.close()never receives a real repo.Fix
Return the initialized repo from
git_clone_and_create_courseand assign it in the caller, so the existingclose()logic on both branches works as intended. Two-line change plus a return annotation; no behavior change beyond closing the repo as was already intended.Testing
python -m py_compile course/versioning.pypasses.repois now the real repo and is closed; on failure before the assignment,repoisNoneand theif repo is not Noneguard still holds.