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.

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

Allow data access from full Firebase path #22

Open
wants to merge 55 commits into
base: main
Choose a base branch
from
Open

Conversation

AdritRao
Copy link
Member

@AdritRao AdritRao commented Jul 23, 2024

Allow data access from full Firebase path

♻️ Current situation & Problem

Currently, fetch_data can only access data using a specified collection and subcollection. In some use cases, a user's HealthKit or survey data may be deeper in the database structure. Allowing developers to specify a full Firebase path can allow them to run access/flattening in any part of the database.

Data can currently be sorted by date after being fetched. Adding a query filter (through Firebase indexing) can allow for sorting data before it is streamed and reduce time required to fetch data.

⚙️ Release Notes

  • Added a fetch_data_path function that takes in a full string database path (full_path)
  • Made subject_id optional for _fetch_user_resources and flattener to allow single user data access through fetch_data_path
  • Added optional start_date and end_date parameter to run query filter in fetch_data_path

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

Vicbi and others added 30 commits July 7, 2024 21:09
Copy link

codecov bot commented Jul 28, 2024

Codecov Report

Attention: Patch coverage is 37.93103% with 18 lines in your changes missing coverage. Please review.

Project coverage is 82.04%. Comparing base (edc0605) to head (ba7bf8c).

Files Patch % Lines
..._pipeline/data_access/firebase_fhir_data_access.py 21.74% 18 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
- Coverage   82.87%   82.04%   -0.83%     
==========================================
  Files          16       16              
  Lines        1582     1609      +27     
==========================================
+ Hits         1311     1320       +9     
- Misses        271      289      +18     
Flag Coverage Δ
unittests 82.04% <37.94%> (-0.83%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...peline/data_flattening/fhir_resources_flattener.py 88.11% <100.00%> (+0.24%) ⬆️
..._pipeline/data_access/firebase_fhir_data_access.py 70.81% <21.74%> (-8.47%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edc0605...ba7bf8c. Read the comment docs.

@AdritRao
Copy link
Member Author

Thank you for reviewing the PR @Vicbi! I have updated the README with usage information for fetch_data_path and query filtering using dates. The unit tests are also working now.

@AdritRao
Copy link
Member Author

I noticed that "effectivePeriod.start" is not consistent across different FHIR resource types so I added a index_name parameter to specify the Firebase query index filter (e.g. for HealthKit data it is "effectivePeriod.start" but for survey responses it is "authored")

@PSchmiedmayer PSchmiedmayer requested review from vishnuravi and removed request for Vicbi and PSchmiedmayer August 3, 2024 01:30
@PSchmiedmayer PSchmiedmayer added the enhancement New feature or request label Aug 4, 2024
@PSchmiedmayer
Copy link
Member

@AdritRao thank you for the PR; I asked @vishnuravi to take a look at it while @Vicbi is on vacation; we already discussed some changes like moving the arguments in a separate type that contains the filter configuration to address the lint issues. We should ensure that we can make this well applicable for the future extension of the package.

@Vicbi Vicbi self-requested a review September 29, 2024 21:57
@Vicbi
Copy link
Collaborator

Vicbi commented Oct 18, 2024

Hi @AdritRao, thank you for the code updates! :) Did you have the chance to review and set up the commit signing? It might also make sense to fix any lint issues before proceeding with any further modifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants