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

Ensure structmap isn't included and METS create V2 fails under various error conditions #1455

Merged

Conversation

ross-spencer
Copy link
Contributor

@ross-spencer ross-spencer commented Jul 16, 2019

A number of different error conditions were spotted during testing of
the Archivematica 1.10 release candidate. Rather than failing
silently in the system we introduce logic to make the output more
robust. Example failure conditions for custom structMaps include when
files do not exist, or there are empty attributes for the CONTENTID
field.

Additionally, small sections of the code have been made simpler.

Tests have been added, and all sample structmaps created for this feature will still pass. Those are available from here.

Connected to archivematica/Issues#283

@ross-spencer ross-spencer changed the title Ensure structmap isn't included if no CONTENTIDS WIP: Ensure structmap isn't included if no CONTENTIDS Jul 16, 2019
@ross-spencer ross-spencer force-pushed the dev/issue-283-make-custom-structmap-more-robust branch 3 times, most recently from 0cbc66a to cbef58f Compare July 30, 2019 11:40
@ross-spencer ross-spencer changed the title WIP: Ensure structmap isn't included if no CONTENTIDS Ensure structmap isn't included under various error conditions Jul 30, 2019
@ross-spencer ross-spencer requested a review from a team July 30, 2019 11:52
@ross-spencer ross-spencer added this to the 1.10.0 milestone Jul 30, 2019
@ross-spencer ross-spencer changed the title Ensure structmap isn't included under various error conditions Ensure structmap isn't included and METS create V2 fails under various error conditions Jul 30, 2019
Copy link
Member

@replaceafill replaceafill left a comment

Choose a reason for hiding this comment

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

@ross-spencer This LGTM! 👍

normalized_path = _fixup_path_input_by_user(job, file_path)
if normalized_path in state.fileNameToFileID:
item.set("FILEID", state.fileNameToFileID[normalized_path])
else:
state.error_accumulator.error_count += 1
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to move this line up here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just it looked nicer ^_^;

self.validate_mets(self.mets_xsd_path, broken_structmap_path)
except etree.DocumentInvalid:
assert (
True
Copy link
Member

Choose a reason for hiding this comment

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

Nothing to do about this, but I'm never sure how to best represent these cases: assert True. A comment may have the same effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point. I might try a comment style the next time around; though I hope we move away more from test case and use the pytest style you showed me with the checksum validation microservice.

continue
else:
self.validate_mets(self.mets_xsd_path, structmap_path)
self.validate_mets(self.mets_xsd_path, structmap_path)
Copy link
Member

Choose a reason for hiding this comment

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

Nice cleanup of these broken functionality.

normalized_path = _fixup_path_input_by_user(job, file_path)
if normalized_path in state.fileNameToFileID:
item.set("FILEID", state.fileNameToFileID[normalized_path])
else:
state.error_accumulator.error_count += 1
job.pyprint(
"Custom structmap error: no fileUUID for",
file_path,
normalized_path,
file=sys.stderr,
)
Copy link
Member

Choose a reason for hiding this comment

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

Oh! One thing I just noticed is that part doesn't use the logger like the other errors:

Custom structmap error: no fileUUID for objects/_ferdinand_short_2017_01_27.mp3 objects/_ferdinand_short_2017_01_27.mp3

vs

createmets_v2.0: ERROR     2019-07-31 14:42:04,990  archivematica.mcp.client.createMETS2.include_custom_structmap:925  Mismatch of CONTENTID elements to elements we wish to replace. AIP METS cannot be generated

Copy link
Contributor Author

@ross-spencer ross-spencer Jul 31, 2019

Choose a reason for hiding this comment

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

Hi @replaceafill I've changed it to a logger here: 47af926 can you let me know that's okay, and then I'll merge asap and we can get into regression testing!!

Copy link
Member

Choose a reason for hiding this comment

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

👍 LGTM!

@ross-spencer ross-spencer force-pushed the dev/issue-283-make-custom-structmap-more-robust branch 3 times, most recently from 47af926 to 175646e Compare July 31, 2019 16:25
A number of different error conditions were spotted during testing of
the Archivematica 1.10 release candidate. Rather than failing
silently in the system we introduce logic to make the output more
robust. Example failure conditions for custom structMaps include when
files do not exist, or there are empty attributes for the CONTENTID
field.
@ross-spencer ross-spencer force-pushed the dev/issue-283-make-custom-structmap-more-robust branch from 175646e to e9896ba Compare July 31, 2019 16:31
@ross-spencer ross-spencer merged commit e9896ba into qa/1.x Jul 31, 2019
@ross-spencer ross-spencer deleted the dev/issue-283-make-custom-structmap-more-robust branch July 31, 2019 16:47
@replaceafill replaceafill self-assigned this Jun 8, 2021
@replaceafill replaceafill added the Status: in progress Issue that is currently being worked on. Waffle label. label Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: in progress Issue that is currently being worked on. Waffle label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants