-
Notifications
You must be signed in to change notification settings - Fork 19
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
Resolve external survey account deletion bug. #498
Conversation
@wasade When you have a moment, can you please review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think there is a concern w/ the early returns and commit. Comment inline, if you agree I think the change is minor to make
source_id, | ||
survey_id | ||
) | ||
if not source_repo.scrub(account_id, source_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this check happen on line 124 above the call to has_external_surveys
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's less a check and more of an in-line verification that the scrub function actually works, modeled after the existing functionality to delete a source.
From that standpoint (addressing the other comment), I think it makes sense to not commit the scrub calls on the surveys, as we haven't successfully scrubbed the source.
We could avoid all of that by putting a check on line 124 that verifies the source meets the conditions to be deleted/scrubbed (i.e., the source exists and there's only a single record in the ag.source table with that ID). In that case, the scrub function could still fail if it's not a human source, but that would only happen if someone had somehow associated either a sample or external survey with a non-human source. And if that happened, I'd want it to show up in the exception logs to investigate because that would be rather concerning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'm +1 on path of least resistance which I think is to retain the current code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, code as written would be the simplest path. If you don't see any other issues here, I'm going to merge this, #497, and biocore/microsetta-interface#264 and get them up on staging for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good lets do this
survey_id | ||
) | ||
if not source_repo.scrub(account_id, source_id): | ||
return jsonify(code=404, message=SRC_NOT_FOUND_MSG), 404 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think we need to be careful with short circuit returns. If this return is hit, the calls to scrub
above will not be committed
Co-authored-by: Daniel McDonald <danielmcdonald@ucsd.edu>
#492
This PR addresses a few related issues: