-
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
Scrub bug #470
Scrub bug #470
Conversation
# if we do not have associated samples, then the source | ||
# is safe to delete | ||
|
||
if not has_samples and not has_external: |
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 looks like sources that don't have samples but do have external surveys (if not has samples and has_external
) is being missed here. Should line 830 be changed to if has_samples or has_external
?
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.
I think the current logic is correct? We only want to delete a source if and only if it does not have samples and does not have an external survey?
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.
If the source has samples (line 830), it gets scrubbed (on line 835). If the source has neither samples nor external surveys (line 837), it gets deleted (on line 842). There's no condition for a source that does not have samples but does have external surveys, so it would be neither deleted nor scrubbed.
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.
ah i see, done, good spot
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.
Great, thanks. I'll push it to staging for testing whenever Jeff get the filesystem issues resolved.
|
||
This method is idempotent | ||
|
||
This method deletes ALL surveys associated with an account and source |
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.
This comment in each of the delete_[remote survey] functions should probably be re-worded since these functions only delete specific surveys, rather than all surveys associated with the account/source.
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.
Done
Fixes #469
To do so, we needed to add admin-intended deletion methods for the external surveys in order support the testing framework.
If a user has any external surveys, their account is scrubbed rather than deleted so the relationships can be preserved.