multi ffqs fix #581
multi ffqs fix #581
Conversation
| if source: | ||
| ffq_complete, ffq_taken, _ = \ | ||
| vs_repo.get_ffq_status_by_source(source.id) | ||
| else: | ||
| ffq_complete = False | ||
| ffq_taken = False |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| is_complete = status[0] == 'Finished' | ||
| is_taken = status[0] in ('Started', 'Review', 'Finished') | ||
| results.append((is_complete, is_taken, status[0])) | ||
| return results |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if source: | ||
| ffq_complete, ffq_taken, _ = \ | ||
| vs_repo.get_ffq_status_by_source(source.id) |
There was a problem hiding this comment.
Two comments here:
- These lines are within a block of code that already checked
if source is not None- isn't checkingif sourcehere redundant? - If we successfully obtain a
vio_idwe should check the taken/complete status of that particularvio_idrather than anything associated with the source.
There was a problem hiding this comment.
Gotcha. In my latest commit I've changed it in favor of looking up FFQs status via vio_id. In the query though, since vio_id is not in ag.ag_kit_barcode I first use the vio_id to obtain the source_id from ag.vioscreen_registry. Not sure if that's the best approach.
No description provided.