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

feat(directory): molgenis-emx2-directory-client #3863

Merged
merged 19 commits into from
Nov 21, 2024

Conversation

dtroelofsprins
Copy link
Contributor

@dtroelofsprins dtroelofsprins commented Jun 10, 2024

What are the main changes you did:
Created a new tool: molgenis-emx2-directory-client, which is also known as the EMX2 Publish script for BBMRI-ERIC, which combines data from the staging areas, add some extra fields and adds the data to the ERIC tables which are used in the Directory App.
This is initial a copy of molgenis-py-bbmri-eric 1.18.1, which is the EMX1 version of the Publish script.

how to test:

  • On a test-server, create a database based on the biobank_directory template (no demo data)
  • Create one or more databases based on the biobank_directory_staging template (with demo data)
  • Create a script by copy paste the "dev.py"
  • Add the latest version on testPyPI of molgenis-emx2-directory-client to the requirements
  • Run the script
  • Check that the data from the staging area(s) is in the directory database

todo:

  • Adjust the project setup files to get it working in EMX2
    - [ ] Adjust README => separate ticket
  • Replace molgenis-py-client with molgenis-emx2-pyclient
    - [ ] updated docs in case of new feature => separate ticket
  • updated tests

Optional todo: separate ticket

  • test coverage is not 76%, but is not checked before final merge, anyway perhaps some extra tests could be added
  • Refactoring: Some code has become redundant (f.e. convert_to_staging as tables names are now equal between ERIC and staging areas), some methods can be rewritten to make them more efficient/logical

@dtroelofsprins dtroelofsprins changed the title Initial commit: copy of molgenis-py-bbmri-eric feat(directory): molgenis-emx2-directory-client Jun 10, 2024
Copy link

sonarqubecloud bot commented Jul 4, 2024

@dtroelofsprins dtroelofsprins marked this pull request as ready for review November 1, 2024 06:22
Copy link
Member

@esthervanenckevort esthervanenckevort left a comment

Choose a reason for hiding this comment

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

De code ziet er goed gestructureerd uit. Ik zag nog een paar plekken waar code in commentaar stond. Dat zou ik nog even verwijderen.
Ik heb de code niet inhoudelijk getest, dat laat ik aan Hessel over. Als hij de PR goedkeurt kun je mergen.

ONCOLOGY = "oncology"
PAEDIATRICS = "paediatrics"
# PAEDIATRIC = "paediatric_only"
# PAEDIATRIC_INCLUDED = "paediatric_included"
Copy link
Member

Choose a reason for hiding this comment

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

Remove commented out code

schema: str = None,
as_df: bool = False,
) -> list | pd.DataFrame:
# response_data = super().get(table, query_filter, schema, as_df)
Copy link
Member

Choose a reason for hiding this comment

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

Remove commented out code

rows = self.get(
schema=self.ONTOLOGY_SCHEMA,
table=entity_type_id,
# attributes=f"id,{parent_attr},ontology,{','.join(matching_attrs)}",
Copy link
Member

Choose a reason for hiding this comment

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

Remove commented out code


biobank_qualities = self.get(
table="QualityInfoBiobanks",
# attributes="id,biobank,assess_level_bio",
Copy link
Member

Choose a reason for hiding this comment

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

Remove commented out code

)
collection_qualities = self.get(
table="QualityInfoCollections",
# attributes="id,collection,assess_level_col",
Copy link
Member

Choose a reason for hiding this comment

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

Remove commented out code

:param code: node to get by code
:return: ExternalServerNode object
"""
# Query filter on null is not yet possible therefore this workaround
Copy link
Member

Choose a reason for hiding this comment

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

If there is an issue in Github for this, please reference it in the comment

Copy link
Contributor

@hslh hslh left a comment

Choose a reason for hiding this comment

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

Looks good, as far as I can tell. I tested it on my own testserver, with an external node on the bbmri-testserver, and only with a dummy PID service and it worked well. I did have to do some extra work to get it to run, I don't know to what extent we want to include those in the documentation somewhere:

  • create the .env, it would be helpful to have an example .env included
  • add rows to the NationalNodes table in the target directory (1 local, CZ, 1 EU and 1 external, NL)
  • make sure the staging areas followed the naming conventions of prefix + national node code
  • install python-dotenv, which isn't listed as a requirement
  • empty the 'studies'-column in the Collections tables of the staging areas

@dtroelofsprins dtroelofsprins merged commit 6b78eb8 into master Nov 21, 2024
6 checks passed
@dtroelofsprins dtroelofsprins deleted the feat/directory-client branch November 21, 2024 14:55
davidruvolo51 pushed a commit that referenced this pull request Nov 26, 2024
* Initial commit: copy of molgenis-py-bbmri-eric

* chore: adjust tool setup files

* chore: rename some modules and classes and fix imports

* chore: rename tool name folder

* chore: change import paths

* chore: update molgenis-emx2-pyclient

* chore: change email and project URLs

* feat: EMX2 version of the publish script

* chore: fix tests

* chore: added additional packages to setup.cfg

* chore: working example file to run the package

* fix: import pyhandle from forked git repo to solve incompatibility with latest pyclient version

* chore(directory): new version of PYHANDLE available

* chore: remove commented out code

---------

Co-authored-by: Hessel Haagsma <h.haagsma@umcg.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants