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

multi ffqs fix #581

Merged
merged 13 commits into from
Oct 24, 2024
Merged

multi ffqs fix #581

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a223a32
addresses #579
ayobi Aug 31, 2024
8da634e
Merge branch 'biocore:master' into multi_ffqs_fix
ayobi Aug 31, 2024
a8409b7
lint and update tests
ayobi Aug 31, 2024
60a84f1
update source id for vioscreen test
ayobi Sep 10, 2024
321ce4d
update exp
ayobi Sep 10, 2024
6084b95
test
ayobi Sep 10, 2024
b781035
update vioscreen test
ayobi Sep 10, 2024
8ffe0a6
test2
ayobi Sep 10, 2024
a2908e4
test3
ayobi Sep 10, 2024
9102276
test4
ayobi Sep 10, 2024
101667d
fix vioscreen test
ayobi Sep 10, 2024
34cda80
update per_sample based on feedback
ayobi Sep 16, 2024
c0c97c2
use vio_id instead of source, update query
ayobi Sep 17, 2024
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
9 changes: 6 additions & 3 deletions microsetta_private_api/admin/sample_summary.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,12 @@ def per_sample(project, barcodes, strip_sampleid):
sample_date = None
sample_time = None

ffq_complete, ffq_taken, _ = vs_repo.get_ffq_status_by_sample(
sample.id
)
if source:
ffq_complete, ffq_taken, _ = \
vs_repo.get_ffq_status_by_source(source.id)
else:
ffq_complete = False
ffq_taken = False
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the change from sample to source, I think it makes sense to move this code block into the if statement on Line 51 (if source is not None and source_type == Source.SOURCE_TYPE_HUMAN). Additionally, it makes sense to harmonize what they're doing. For example, get_ffq_status_by_source could return True, True but vio_id could be None.

I'd suggest first looking for a match including sample_id since a good chunk of historical data will include that. Failing an exact match with sample_id you can fall back to a match on source.

Copy link
Contributor Author

@ayobi ayobi Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you're saying it'd be better to change if source is not None and source_type == Source.SOURCE_TYPE_HUMAN to first check for a sample_id match then fall back on source? Or when trying to get vio_id? I have the latter to commit now.


summary = {
"sampleid": None if strip_sampleid else barcode,
Expand Down
34 changes: 25 additions & 9 deletions microsetta_private_api/repo/tests/test_vioscreen_sessions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ def _to_dt(mon, day, year):
# in ag_test db, this uuid corresponds to VIOSCREEN_USERNAME1
BARCODE_UUID_FOR_VIOSESSION = '66ec7d9a-400d-4d71-bce8-fdf79d2be554'
BARCODE_UUID_NOTIN_REGISTRY = 'edee4af9-65b2-4ed1-ba66-5bf58383005e'
SOURCE_ID_FOR_VIOSESSION = '6d343527-ab68-4e01-9ef7-16943dc5cee0'
SOURCE_ID_NOTIN_REGISTRY = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa'


today = date.today()
VIOSCREEN_SESSION = VioscreenSession(sessionId='a session',
Expand Down Expand Up @@ -265,7 +268,16 @@ def test_get_ffq_status_by_sample(self):
r.upsert_session(session_copy)
session = r.get_sessions_by_username(VIOSCREEN_USERNAME1)[0]

obs = r.get_ffq_status_by_sample(BARCODE_UUID_NOTIN_REGISTRY)
cur = t.cursor()
source_id_query = """
SELECT source_id
FROM ag.vioscreen_registry
WHERE sample_id = %s
"""
cur.execute(source_id_query, (BARCODE_UUID_FOR_VIOSESSION,))
source_id = cur.fetchone()

obs = r.get_ffq_status_by_source(SOURCE_ID_NOTIN_REGISTRY)
self.assertEqual(obs, (False, False, None))

session.status = 'Finished'
Expand All @@ -274,25 +286,29 @@ def test_get_ffq_status_by_sample(self):

# enumerate the empirically observed states from vioscreen
# (is_complete, has_taken, exact_status)
obs = r.get_ffq_status_by_sample(BARCODE_UUID_FOR_VIOSESSION)
self.assertEqual(obs, (True, True, 'Finished'))
obs = r.get_ffq_status_by_source(source_id)
self.assertEqual(obs, [(True, True, 'Finished'),
(True, True, 'Finished')])

session.status = 'Started'
session.endDate = None
r.upsert_session(session)

obs = r.get_ffq_status_by_sample(BARCODE_UUID_FOR_VIOSESSION)
self.assertEqual(obs, (False, True, 'Started'))
obs = r.get_ffq_status_by_source(source_id)
self.assertEqual(obs, [(False, True, 'Started'),
(False, True, 'Started')])

session.status = 'New'
r.upsert_session(session)
obs = r.get_ffq_status_by_sample(BARCODE_UUID_FOR_VIOSESSION)
self.assertEqual(obs, (False, False, 'New'))
obs = r.get_ffq_status_by_source(source_id)
self.assertEqual(obs, [(False, False, 'New'),
(False, False, 'New')])

session.status = 'Review'
r.upsert_session(session)
obs = r.get_ffq_status_by_sample(BARCODE_UUID_FOR_VIOSESSION)
self.assertEqual(obs, (False, True, 'Review'))
obs = r.get_ffq_status_by_source(source_id)
self.assertEqual(obs, [(False, True, 'Review'),
(False, True, 'Review')])


if __name__ == '__main__':
Expand Down
24 changes: 14 additions & 10 deletions microsetta_private_api/repo/vioscreen_repo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ def upsert_session(self, session):
{doupdateset}
""",
tuple([getattr(session, attr) for attr in self.COLS]))
cur.execute("SELECT * FROM ag.vioscreen_sessions "
"WHERE sessionId = %s", (session.sessionId,))
rows = cur.fetchall()
print("Upserted rows:", rows)
ayobi marked this conversation as resolved.
Show resolved Hide resolved
return cur.rowcount == 1

def get_session(self, sessionId):
Expand Down Expand Up @@ -168,12 +172,12 @@ def get_unfinished_sessions(self):

return not_in_vioscreen_sessions + incomplete_sessions

def get_ffq_status_by_sample(self, sample_uuid):
"""Obtain the FFQ status for a given sample
def get_ffq_status_by_source(self, source_uuid):
"""Obtain the FFQ status for a given source

Parameters
----------
sample_uuid : UUID4
source_uuid : UUID4
The UUID to check the status of

Returns
Expand All @@ -190,17 +194,17 @@ def get_ffq_status_by_sample(self, sample_uuid):
FROM ag.vioscreen_sessions AS vs
JOIN ag.vioscreen_registry AS vr
ON vs.username=vr.vio_id
WHERE sample_id=%s""", (sample_uuid, ))
WHERE source_id=%s""", (source_uuid, ))
res = cur.fetchall()
if len(res) == 0:
return (False, False, None)
elif len(res) == 1:
status = res[0][0]
is_complete = status == 'Finished'
is_taken = status in ('Started', 'Review', 'Finished')
return (is_complete, is_taken, status)
else:
raise ValueError("A sample should not have multiple FFQs")
results = []
for status in res:
is_complete = status[0] == 'Finished'
is_taken = status[0] in ('Started', 'Review', 'Finished')
results.append((is_complete, is_taken, status[0]))
return results
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't changing the return value from a tuple to a list of tuples will break the code that uses this function? The unit tests work fine because they test the function in isolation, but is this still working when you try to run a sample summary report through microsetta-admin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, it does break. So, if the user has taken multiple FFQs for a particular sample, we'd probably need to change the columns for ffq-taken and ffq-complete to indicate which ffq they've taken and if that one was taken/complete?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's overkill for the sake of this particular function. If a sample (or source) is associated with more than one FFQ, let's return the one (and it's taken/complete statuses) that's temporally nearest to the sample collection date.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, in my latest commit I get the closest sample collection data in sample_date and sample_time in ag.ag_kit_barcode to compare in startdate in ag.vioscreen_sessions.


def get_missing_ffqs(self):
"""The set of valid sessions which lack FFQ data
Expand Down
Loading