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

Jump to bottom

Add bp image schema #445

Merged
merged 7 commits into from
Jun 13, 2022
Merged

Add bp image schema #445

merged 7 commits into from
Jun 13, 2022

Conversation

genie9
Copy link
Contributor

@genie9 genie9 commented May 24, 2022 •
edited
Loading

Description

Add schema for BP image

Related issues

Related #446

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update (this needs a follow up PR)

Changes Made

Adds new schema image for validation and database writes

Testing

  • Unit Tests
  • Integration Tests
  • Tests do not apply
  • Needs testing (start an issue or do a follow up PR about it)

Mentions

@genie9 genie9 marked this pull request as draft May 24, 2022 16:30
@genie9 genie9 linked an issue May 24, 2022 that may be closed by this pull request
As a researcher, I want to submit imaging metadata with XML files so they can be findable through descovery service #443
Open
5 tasks
@genie9 genie9 force-pushed the feature/add-bp-image-schema branch 2 times, most recently from 50073dc to 24b5236 Compare May 31, 2022 12:56
csc-jm
csc-jm reviewed Jun 2, 2022
View reviewed changes
Copy link
Collaborator

@csc-jm csc-jm left a comment

Choose a reason for hiding this comment

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

Before refactoring XML parser to parse BP related xml files, it's important we have working schemas and example xml files that validate against the schemas. Here are the problems I found with these current files.

A helpful command line tool for validating these files can be found here, which validates the files in the same way the methods in helpers/validator.py does.

metadata_backend/conf/schemas.json Outdated Show resolved Hide resolved
tests/test_files/bp_image/template_images.xml Outdated Show resolved Hide resolved
tests/test_files/bp_sample/template_samples.xml Outdated Show resolved Hide resolved
metadata_backend/helpers/schemas/BP.bp_ds.xsd Outdated Show resolved Hide resolved
tests/test_files/bp_dataset/template_dataset.xml Outdated Show resolved Hide resolved
@genie9 genie9 force-pushed the feature/add-bp-image-schema branch from 4c86294 to ba95ff0 Compare June 3, 2022 10:48
@csc-jm csc-jm mentioned this pull request Jun 6, 2022
Merged
2 tasks
@genie9 genie9 force-pushed the feature/add-bp-image-schema branch from 6cee759 to 3eafb8a Compare June 8, 2022 13:12
@genie9 genie9 marked this pull request as ready for review June 8, 2022 13:24
csc-jm
csc-jm requested changes Jun 8, 2022
View reviewed changes
Copy link
Collaborator

@csc-jm csc-jm left a comment

Choose a reason for hiding this comment

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

When the parser parses the example image xml, these following changes to the image json schema would validate the json object correctly... almost

metadata_backend/helpers/schemas/bp_image.json Outdated Show resolved Hide resolved
metadata_backend/helpers/schemas/bp_image.json Outdated Show resolved Hide resolved
metadata_backend/helpers/schemas/bp_image.json Show resolved Hide resolved
metadata_backend/helpers/schemas/bp_image.json Outdated Show resolved Hide resolved
@genie9 genie9 force-pushed the feature/add-bp-image-schema branch from 6ed8d60 to eca039c Compare June 9, 2022 19:35
genie9 added 2 commits June 10, 2022 10:21
@genie9
Update wordlist for pyspelling
b0ef810
@genie9
Add image file for integration tests
6020783 
Adds test file with only one image object as there is bug for processing
multiple object XML: #479
@genie9 genie9 force-pushed the feature/add-bp-image-schema branch from eca039c to 6020783 Compare June 10, 2022 07:21
@genie9 genie9 requested a review from csc-jm June 10, 2022 10:56
@genie9 genie9 changed the title Add bp_image schema file and related json configs Add bp image schema file Jun 10, 2022
@genie9 genie9 changed the title Add bp image schema file Add bp image schema Jun 10, 2022
csc-jm
csc-jm approved these changes Jun 10, 2022
View reviewed changes
Copy link
Collaborator

@csc-jm csc-jm left a comment

Choose a reason for hiding this comment

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

This PR is now good for parsing a single BP image from an XML file into JSON and adding it to DB. Added a unit test for parser as well.

@genie9
Copy link
Contributor Author

genie9 commented Jun 13, 2022

Thanks, @csc-jm for the tests!

@genie9 genie9 merged commit fe2add2 into develop Jun 13, 2022
@genie9 genie9 deleted the feature/add-bp-image-schema branch June 13, 2022 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csc-felipe csc-felipe csc-felipe left review comments

@csc-jm csc-jm csc-jm approved these changes

Assignees

@genie9 genie9

@csc-jm csc-jm

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@genie9 @csc-jm @csc-felipe