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.

Sign up for GitHub

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

Jump to bottom

Scrub bug #470

Merged
merged 19 commits into from
Oct 28, 2022
Merged

Scrub bug #470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
cef0cd9
BUG: not scrubbing with secondary surveys, see #469
wasade Oct 27, 2022
e49b14d
TST: delete_myfoodrepo entry
wasade Oct 27, 2022
86dd409
API: myfoodrepo deletion
wasade Oct 27, 2022
e938594
TST: delete_spain_ffq_entry
wasade Oct 27, 2022
0753a3e
API: spain ffq deletion
wasade Oct 27, 2022
27619d4
TST: delete_polyphenol_ffq_entry
wasade Oct 27, 2022
879d5de
API: polyphenol ffq deletion
wasade Oct 27, 2022
e45fac7
remove _entry as methods may get multiple entries
wasade Oct 27, 2022
77f2567
TST: delete_vioscreen
wasade Oct 27, 2022
fd2a5e1
API: vioscreen ffq deletion
wasade Oct 27, 2022
9cb54a6
Remove extra whitespace
wasade Oct 27, 2022
5f86a1a
Use the new deletion methods
wasade Oct 27, 2022
3b1f7c0
TST: has_external_surveys
wasade Oct 27, 2022
ed9b7c0
TST/API: method to get all vioscreen IDs for a source
wasade Oct 27, 2022
90c16b0
API: has_external_surveys
wasade Oct 27, 2022
a1843c2
BUG: fixes #469
wasade Oct 27, 2022
2947479
lint
wasade Oct 27, 2022
d261d33
addressing @cassidysymon's comments
wasade Oct 27, 2022
373469b
addressing @cassidysymons's comments
wasade Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions microsetta_private_api/admin/admin_impl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,6 +796,7 @@ def delete_account(account_id, token_info):
src_repo = SourceRepo(t)
samp_repo = SampleRepo(t)
sar_repo = SurveyAnswersRepo(t)
template_repo = SurveyTemplateRepo(t)

acct = acct_repo.get_account(account_id)
if acct is None:
Expand All @@ -806,35 +807,43 @@ def delete_account(account_id, token_info):
return None, 204

sample_count = 0
account_has_external = False
sources = src_repo.get_sources_in_account(account_id)

for source in sources:
samples = samp_repo.get_samples_by_source(account_id, source.id)

has_samples = len(samples) > 0
sample_count += len(samples)
has_external = template_repo.has_external_surveys(account_id,
source.id)

if has_external:
account_has_external = True

for sample in samples:
# we scrub rather than disassociate in the event that the
# sample is in our freezers but not with an up-to-date scan
samp_repo.scrub(account_id, source.id, sample.id)

surveys = sar_repo.list_answered_surveys(account_id, source.id)
if has_samples:
# if we have samples, we need to scrub survey / source
# free text
if has_samples or has_external:
# if we have samples or external surveys, we need to scrub
# survey / source free text
for survey_id in surveys:
sar_repo.scrub(account_id, source.id, survey_id)
src_repo.scrub(account_id, source.id)
else:
# if we do not have associated samples, then the source
# is safe to delete

if not has_samples and not has_external:
Copy link
Collaborator

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?

Copy link
Member Author

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?

Copy link
Collaborator

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.

Copy link
Member Author

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

Copy link
Collaborator

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.

# if we do not have associated samples, or external surveys,
# then the source is safe to delete
for survey_id in surveys:
sar_repo.delete_answered_survey(account_id, survey_id)
src_repo.delete_source(account_id, source.id)

# an account is safe to delete if there are no associated samples
if sample_count > 0:
# and does not have external surveys
if sample_count > 0 or account_has_external:
acct_repo.scrub(account_id)
else:
acct_repo.delete_account(account_id)
Expand Down
50 changes: 50 additions & 0 deletions microsetta_private_api/api/tests/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from microsetta_private_api.repo.account_repo import AccountRepo
from microsetta_private_api.repo.source_repo import SourceRepo
from microsetta_private_api.repo.survey_answers_repo import SurveyAnswersRepo
from microsetta_private_api.repo.survey_template_repo import SurveyTemplateRepo
from microsetta_private_api.repo.sample_repo import SampleRepo
from microsetta_private_api.repo.vioscreen_repo import (
VioscreenSessionRepo, VioscreenPercentEnergyRepo,
Expand Down Expand Up @@ -311,6 +312,7 @@ def delete_dummy_accts():
survey_answers_repo = SurveyAnswersRepo(t)
sample_repo = SampleRepo(t)
barcode_repo = BarcodeRepo(t)
template_repo = SurveyTemplateRepo(t)

# Delete fake kit and barcode preps
barcode_repo.delete_preparation(BC1, 1234)
Expand All @@ -330,6 +332,17 @@ def delete_dummy_accts():
sample_ids = [x.id for x in source_samples]
all_sample_ids.extend(sample_ids)

# Dissociate any secondary surveys.
# IMPORTANT: the order of operations here matters. It is
# necessary to delete external surveys PRIOR to deleting survey
# answers as the survey answers deletion will set source_id to
# NULL for entries in the registries
template_repo.delete_myfoodrepo(curr_acct_id, curr_source.id)
template_repo.delete_vioscreen(curr_acct_id, curr_source.id)
template_repo.delete_polyphenol_ffq(curr_acct_id,
curr_source.id)
template_repo.delete_spain_ffq(curr_acct_id, curr_source.id)

# Dissociate all samples linked to this source from all
# answered surveys linked to this source, then delete all
# answered surveys
Expand Down Expand Up @@ -1051,6 +1064,43 @@ def test_account_scrub_non_admin(self):
# check response code
self.assertEqual(401, response.status_code)

def test_account_scrub_no_samples_has_secondary_survey_bug(self):
# setup an account with a source and sample
dummy_acct_id, dummy_source_id = create_dummy_source(
"Bo", Source.SOURCE_TYPE_HUMAN, DUMMY_HUMAN_SOURCE,
create_dummy_1=True)

_ = create_dummy_acct(create_dummy_1=False,
iss=ACCT_MOCK_ISS_3,
sub=ACCT_MOCK_SUB_3,
dummy_is_admin=True)

# "take" the polyphenol FFQ
response = self.client.get(
'/api/accounts/%s/sources/%s/survey_templates/%s?language_tag=en_us' % # noqa
(dummy_acct_id, dummy_source_id,
SurveyTemplateRepo.POLYPHENOL_FFQ_ID),
headers=self.dummy_auth)

# now let's scrub it
response = self.client.delete(
'/api/accounts/%s' %
(dummy_acct_id,),
headers=make_headers(FAKE_TOKEN_ADMIN))

# check response code
self.assertEqual(204, response.status_code)

# verify deleting is idempotent. If the account was scrubbed, then
# we get a 204. If the account was deleted, we get a 404
response = self.client.delete(
'/api/accounts/%s' %
(dummy_acct_id,),
headers=make_headers(FAKE_TOKEN_ADMIN))

# check response code
self.assertEqual(204, response.status_code)

# region account view/get tests
def test_account_view_success(self):
"""Successfully view existing account"""
Expand Down
211 changes: 211 additions & 0 deletions microsetta_private_api/repo/survey_template_repo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,38 @@ def set_myfoodrepo_id(self, account_id, source_id, mfr_id):
WHERE account_id=%s AND source_id=%s""",
(mfr_id, account_id, source_id))

def delete_myfoodrepo(self, account_id, source_id):
"""Intended for admin use, remove MyFoodRepo entries

This method is idempotent

This method deletes ALL MyFoodRepo surveys associated with an account
and source

This is a hard delete, we REMOVE rows rather than setting a flag

Parameters
----------
account_id : str, UUID
The account UUID
source_id : str, UUID
The source UUID
"""
with self._transaction.cursor() as cur:
existing, created = self.get_myfoodrepo_id_if_exists(account_id,
source_id)
if existing is not None:
cur.execute("""DELETE FROM ag.ag_login_surveys
WHERE ag_login_id=%s
AND source_id=%s
AND survey_id=%s""",
(account_id, source_id, existing))

cur.execute("""DELETE FROM ag.myfoodrepo_registry
WHERE account_id=%s
AND source_id=%s""",
(account_id, source_id))

def get_myfoodrepo_id_if_exists(self, account_id, source_id):
"""Return a MyFoodRepo ID if one exists

Expand Down Expand Up @@ -467,6 +499,37 @@ def get_polyphenol_ffq_id_if_exists(self, account_id, source_id):
else:
return res

def delete_polyphenol_ffq(self, account_id, source_id):
"""Intended for admin use, remove Polyphenol FFQ entries

This method is idempotent.

This method deletes ALL Polyphenol FFQ surveys associated with an
account and source

This is a hard delete, we REMOVE rows rather than setting a flag

Parameters
----------
account_id : str, UUID
The account UUID
source_id : str, UUID
The source UUID
"""
with self._transaction.cursor() as cur:
existing, _ = self.get_polyphenol_ffq_id_if_exists(account_id,
source_id)
if existing is not None:
cur.execute("""DELETE FROM ag.ag_login_surveys
WHERE ag_login_id=%s
AND source_id=%s
AND survey_id=%s""",
(account_id, source_id, existing))
cur.execute("""DELETE FROM ag.polyphenol_ffq_registry
WHERE account_id=%s
AND source_id=%s""",
(account_id, source_id))

def create_spain_ffq_entry(self, account_id, source_id):
"""Return a newly created Spain FFQ ID

Expand Down Expand Up @@ -530,6 +593,38 @@ def get_spain_ffq_id_if_exists(self, account_id, source_id):
else:
return res[0]

def delete_spain_ffq(self, account_id, source_id):
"""Intended for admin use, remove Spain FFQ entries

This method is idempotent

This method deletes ALL Spain FFQ surveys associated with an account
and source

This is a hard delete, we REMOVE rows rather than setting a flag

Parameters
----------
account_id : str, UUID
The account UUID
source_id : str, UUID
The source UUID
"""
with self._transaction.cursor() as cur:
existing = self.get_spain_ffq_id_if_exists(account_id,
source_id)
if existing is not None:
cur.execute("""DELETE FROM ag.ag_login_surveys
WHERE ag_login_id=%s
AND source_id=%s
AND survey_id=%s""",
(account_id, source_id, existing))

cur.execute("""DELETE FROM ag.spain_ffq_registry
WHERE account_id=%s
AND source_id=%s""",
(account_id, source_id))

def get_vioscreen_sample_to_user(self):
"""Obtain a mapping of sample barcode to vioscreen user"""
with self._transaction.cursor() as cur:
Expand Down Expand Up @@ -605,6 +700,88 @@ def get_vioscreen_id_if_exists(self, account_id, source_id,
else:
return rows[0][0]

def get_vioscreen_all_ids_if_exists(self, account_id, source_id):
"""Obtain all vioscreen IDs for a source

This method captures all IDs including IDs with the "deleted"
flag set.

Parameters
----------
account_id : str, UUID
The account UUID
source_id : str, UUID
The source UUID

Returns
tuple or None
The tuple of IDs or None of there are no associated IDs
"""
with self._transaction.cursor() as cur:
cur.execute("""SELECT vio_id
FROM vioscreen_registry
WHERE account_id = %s
AND source_id = %s""",
(account_id, source_id))
ids = tuple([r[0] for r in cur.fetchall()])
if len(ids) > 0:
return ids
else:
return None

def delete_vioscreen(self, account_id, source_id):
"""Intended for admin use, remove Vioscreen entries from the system

This method is idempotent

This method deletes ALL Vioscreen FFQ surveys associated with an
account and source

This is a hard delete, we REMOVE rows rather than setting a flag

Parameters
----------
account_id : str, UUID
The account UUID
source_id : str, UUID
The source UUID
"""
with self._transaction.cursor() as cur:
# get all vioscreen user names associated with the source
cur.execute("""SELECT vio_id
FROM vioscreen_registry
WHERE account_id=%s
AND source_id=%s""",
(account_id, source_id))
vio_ids = tuple([r[0] for r in cur.fetchall()])

if len(vio_ids) == 0:
return None

# get all sessions associated with the vio_ids
cur.execute("""SELECT sessionid
FROM ag.vioscreen_sessions
WHERE username IN %s""",
(vio_ids, ))
sessions = tuple([r[0] for r in cur.fetchall()])

if len(sessions) > 0:
for tbl in ('dietaryscore', 'eatingpatterns', 'foodcomponents',
'foodconsumption', 'foodconsumptioncomponents',
'percentenergy', 'supplements', 'mpeds'):
tbl = f'ag.vioscreen_{tbl}'
cur.execute("DELETE FROM " + tbl + " " +
"WHERE sessionid IN %s",
(sessions, ))
cur.execute("""DELETE FROM ag.vioscreen_sessions
WHERE sessionid IN %s""",
(sessions, ))

cur.execute("""DELETE FROM ag.vioscreen_registry
WHERE account_id = %s
AND source_id = %s""",
(account_id, source_id))

def fetch_user_basic_physiology(self, account_id, source_id):
"""Given an account and source ID, obtain basic physiology properties

Expand Down Expand Up @@ -718,3 +895,37 @@ def fetch_user_basic_physiology(self, account_id, source_id):
weight = None

return (birth_year, gender, height, weight)

def has_external_surveys(self, account_id, source_id):
"""Test whether a source has any external surveys associated

Parameters
----------
account_id : str, UUID
The account UUID
source_id : str, UUID
The source UUID

Returns
-------
boolean
True indicates the user has an external survey associated, false
otherwise
"""
getters = (self.get_myfoodrepo_id_if_exists,
self.get_polyphenol_ffq_id_if_exists,
self.get_spain_ffq_id_if_exists,
self.get_vioscreen_all_ids_if_exists)

for get in getters:
res = get(account_id, source_id)

# the if_exists methods are inconsistent in return type, yay
if isinstance(res, tuple):
if res[0] is not None:
return True
else:
if res is not None:
return True

return False
Loading