fix: course staff mgmt command #310
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
varshamenon4
force-pushed
the
varshamenon4/fix-course-staff-mgmt-command
branch
2 times, most recently
from
236ed90
to
241fe94
Compare
| ) for row in reader[i:i + batch_size]), | ||
| ignore_conflicts=True, | ||
| ) | ||
| users_dict = {k: v for k, v in [(u.username, u) for (u, c) in [User.objects.get_or_create(username=row.get('username'), email=row.get('email')) for row in reader[i:i + batch_size]]]} |
There was a problem hiding this comment.
Does this look good? I can now use this to get users in the bulk create function as this does include the pk. Need to handle IntegrityError (specifically ran into the error when trying to add a user with an existing username, but a different email). Guessing this can be done with a try/catch?
There was a problem hiding this comment.
My question specifically related to error handling: what do we want to happen in this case? Ideally we'd want to skip the failing entry, right? So maybe the move is to break out the list comprehension (for readability sake) and to handle the error at the get_or_create by having a try/catch?
| for row in reader[i:i + batch_size]: | ||
| try: | ||
| users_list.append(User.objects.get_or_create(username=row.get('username'), email=row.get('email'))) | ||
| except IntegrityError: |
There was a problem hiding this comment.
Took a pass at handling IntegrityError here. This handles the case where there's a duplicate username with a different email. This does not handle when there is a new username with a duplicate email address. I don't know if this is an issue, but this is also something that may need to be handled in the Model/db itself so might be more cumbersome than needed.
There was a problem hiding this comment.
if this is a case we're expecting can we log something or output information about the rows that failed to create when this is run so the failure isn't silent?
varshamenon4
force-pushed
the
varshamenon4/fix-course-staff-mgmt-command
branch
from
ee2fb5c
to
dafdc1a
Compare
JIRA: COSMO-234
Description: Attempting to fix the course staff role mgmt command by separating the loops and removing the atomic transaction. I am unable to replicate locally, but running this command in stage resulted in an error where the User couldn't be found.