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 1st set of DspaceMets field methods #191

Merged
merged 4 commits into from
Jun 11, 2024
Merged

Add 1st set of DspaceMets field methods #191

merged 4 commits into from
Jun 11, 2024

Conversation

ehanson8
Copy link
Contributor

@ehanson8 ehanson8 commented Jun 6, 2024

Purpose and background context

Refactors the first set of DspaceMets fields as separate field methods. The absurd line count is from updating the METS fixtures and updating the imports for test_datacite.py to bring it in line with the other unit test modules in the repo. The actual code changes are ~250 lines.

How can a reviewer manually see the effects of these changes?

Run the following command to see that the DspaceMets transform still transforms a source file:

pipenv run transform -i tests/fixtures/dspace/dspace_mets_records.xml -o output/dspace-mets-transformed-records.json -s dspace

Includes new or updated dependencies?

NO

Changes expectations for external applications?

NO

What are the relevant tickets?

Developer

  • All new ENV is documented in README
  • All new ENV has been added to staging and production environments
  • All related Jira tickets are linked in commit message(s)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer(s)

  • The commit message is clear and follows our guidelines (not just this PR message)
  • There are appropriate tests covering any new functionality
  • The provided documentation is sufficient for understanding any new functionality introduced
  • Any manual tests have been performed and verified
  • New dependencies are appropriate or there were no changes

@ehanson8
Update DSpace METS fixtures
db4cc30
@ehanson8
Update imports in test_datacite.py for consistency
e20f948
@ehanson8
Add 1st set of DspaceMets field methods
2534bac 
Why these changes are being introduced:
* Refactor DspaceMets to use field methods

How this addresses that need:
* Add create_dspace_mets_source_record_stub function to DspaceMets test module
* Rename param xml > source_record
* Add field methods and associated private methods for alternate_titles, citation, and content_type
* Add unit tests for new field methods

Side effects of this change:
* None

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/TIMX-286
ghukill
ghukill approved these changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@ghukill ghukill left a comment

Choose a reason for hiding this comment

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

Overall, looks great!

Left a comment about alternate list comprehension, and possible test, but nothing blocking. Really appreciating how these are clicking together, looking familiar.

transmogrifier/sources/xml/dspace_mets.py Outdated Show resolved Hide resolved
tests/sources/xml/test_dspace_mets.py Show resolved Hide resolved
@ehanson8
Updates based on discussion in PR #191
ab5dddb 
* Refactor get_alternate_titles method for more efficient parsing
* Add get_alternate_titles unit test for multiple titles
@ehanson8
Copy link
Contributor Author

ehanson8 commented Jun 7, 2024

Pushed new commit responding to @ghukill 's suggestions!

ghukill
ghukill reviewed Jun 7, 2024
View reviewed changes
tests/sources/xml/test_dspace_mets.py
Comment on lines +251 to +254
assert DspaceMets.get_alternate_titles(source_record) == [
timdex.AlternateTitle(value="Title 2"),
timdex.AlternateTitle(value="Title 3"),
]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is awesome. Crystal clear the logic it's testing.

jonavellecuerdo
jonavellecuerdo approved these changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@jonavellecuerdo jonavellecuerdo left a comment

Choose a reason for hiding this comment

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

@ehanson8 Great first pass! I focused on reviewing the main changes in dspace_mets.py, and changes look good. I was curious what changes you made to the fixtures? It appears that a lot of the changes were directly to the XML files?

@ehanson8
Copy link
Contributor Author

@ehanson8 Great first pass! I focused on reviewing the main changes in dspace_mets.py, and changes look good. I was curious what changes you made to the fixtures? It appears that a lot of the changes were directly to the XML files?

Just editing them to include <records> as the top level element which is how the app would always receive records even if there is only 1 (as well as being consistent with the other sources' fixtures). That shifts the indentation so GH thinks the whole file was edited

@ehanson8 ehanson8 merged commit 679ddff into main Jun 11, 2024
5 checks passed
@ehanson8 ehanson8 deleted the TIMX-286-dspace-mets-fmr-begins branch June 11, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghukill ghukill ghukill approved these changes

@jonavellecuerdo jonavellecuerdo jonavellecuerdo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ehanson8 @ghukill @jonavellecuerdo