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

ICAT to PaNOSC Data Model Conversion #300

Merged
merged 49 commits into from
Jan 31, 2022

Conversation

VKTB
Copy link
Contributor

@VKTB VKTB commented Jan 7, 2022

This PR will close #265

Description

This PR implements the conversion from ICAT to PaNOSC data models. It uses the PaNOSC - ICAT mappings define in the filter-input-conversion branch which is why that is the base branch. All the commits before b0983bb, with exception of 85c4bfc, are as a result of me merging the changes from the base as well as the feature/icat-to-panosc-data-model-conversion-#265 branch.

The PaNOSC fields that map to id fields in ICAT were set to be of type StrictStr but because they are integers in ICAT, the creation of the PaNOSC models was failing because they were only accepting strings. To deal with this issue, the type of such PaNOSC fields were changed to str instead as this casts integers to strings but still throws a ValidationError if None is passed to it. To keep things consistent and less confusing, the types of all the other fields were made non-strict.

A version bump will be made once this is merged.

Testing Instructions

  • Review code
  • Check GitHub Actions build
  • If icatdb Generator Script Consistency Test CI job fails, is this because of a deliberate change made to the script to change generated data (which isn't actually a problem) or is here an underlying issue with the changes made?
  • Review changes to test coverage
  • Does this change mean a new patch, minor or major version should be made? If so, does one of the commit messages feature fix:, feat: or BREAKING CHANGE: so a release is automatically made via GitHub Actions upon merge?

Agile Board Tracking

Connect to #265

MRichards99 and others added 4 commits January 4, 2022 10:37
- I've moved the code which deals with the parameter value field name list away from this function to make the function more generic/reusable. That code has been moved to applying the WHERE filter, where it'll be used. The mapping will not be reached by an include filter (under a valid API request) because the parameter values are for a field name/attribute, not a related entity
@VKTB VKTB changed the base branch from master to filter-input-conversion January 7, 2022 15:50
MRichards99 and others added 25 commits January 10, 2022 12:26
…ter objects for multiple and nested related entities #261
Copy link
Collaborator

@MRichards99 MRichards99 left a comment

Choose a reason for hiding this comment

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

The overall implementation of this looks to work, although the test failures I came across on my machine suggest there's an issue with the parameter mapping. I've left some comments/suggestions as I've been looking through the code

test/search_api/test_models.py Outdated Show resolved Hide resolved
datagateway_api/src/search_api/models.py Outdated Show resolved Hide resolved
datagateway_api/src/search_api/models.py Outdated Show resolved Hide resolved
datagateway_api/src/search_api/models.py Outdated Show resolved Hide resolved
datagateway_api/src/search_api/models.py Outdated Show resolved Hide resolved
datagateway_api/src/search_api/models.py Show resolved Hide resolved
datagateway_api/src/search_api/models.py Outdated Show resolved Hide resolved
datagateway_api/src/search_api/models.py Outdated Show resolved Hide resolved
datagateway_api/src/search_api/models.py Outdated Show resolved Hide resolved
datagateway_api/src/search_api/models.py Outdated Show resolved Hide resolved
@MRichards99
Copy link
Collaborator

Little bit unsure why the CI hasn't run on this PR 🤔 it's not a problem until we get a new release of Python ICAT because it'll just fail right now, but note to future us to maybe run it manually.

@VKTB
Copy link
Contributor Author

VKTB commented Jan 28, 2022

Little bit unsure why the CI hasn't run on this PR 🤔 it's not a problem until we get a new release of Python ICAT because it'll just fail right now, but note to future us to maybe run it manually.

I am not sure what happened there, but it is running now 😄

@VKTB VKTB requested a review from MRichards99 January 28, 2022 13:47
@MRichards99 MRichards99 force-pushed the feature/icat-to-panosc-data-model-conversion-#265 branch from 298a5b6 to 3f1b2d6 Compare January 31, 2022 17:26
Base automatically changed from filter-input-conversion to master January 31, 2022 17:28
MRichards99
MRichards99 previously approved these changes Jan 31, 2022
@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #300 (358ac22) into master (63f4f17) will increase coverage by 0.58%.
The diff coverage is 93.02%.

❗ Current head 358ac22 differs from pull request most recent head f17038d. Consider uploading reports for the commit f17038d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #300      +/-   ##
==========================================
+ Coverage   90.64%   91.22%   +0.58%     
==========================================
  Files          43       43              
  Lines        2982     3066      +84     
  Branches      273      290      +17     
==========================================
+ Hits         2703     2797      +94     
+ Misses        244      230      -14     
- Partials       35       39       +4     
Impacted Files Coverage Δ
datagateway_api/src/search_api/filters.py 89.61% <80.00%> (+4.61%) ⬆️
datagateway_api/src/search_api/panosc_mappings.py 84.61% <81.25%> (-2.89%) ⬇️
datagateway_api/src/search_api/models.py 96.51% <95.73%> (+9.63%) ⬆️
...gateway_api/src/search_api/query_filter_factory.py 94.95% <100.00%> (+0.17%) ⬆️
...atagateway_api/src/datagateway_api/icat/filters.py 99.37% <0.00%> (-0.04%) ⬇️
datagateway_api/src/common/filters.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

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

@MRichards99 MRichards99 self-requested a review January 31, 2022 19:03
@MRichards99 MRichards99 merged commit 2609aae into master Jan 31, 2022
@MRichards99 MRichards99 deleted the feature/icat-to-panosc-data-model-conversion-#265 branch January 31, 2022 19:04
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.

ICAT to PaNOSC Data Model Conversion
2 participants