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

Fix error in duplicate PREMIS event ID handling #1978

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

mcantelon
Copy link
Member

@mcantelon mcantelon commented Aug 16, 2024

Attempting to create a UUID object using an existing UUID object, rather than a string, results in an error. Changed duplicate PREMIS event identifier handling to avoid ending up doing this.

@mcantelon mcantelon marked this pull request as draft August 16, 2024 02:38
@mcantelon mcantelon changed the title work Fix error created when a duplicate PREMIS event identifier is encountered Aug 16, 2024
@mcantelon mcantelon added the Type: bug A flaw in the code that causes the software to produce an incorrect or unexpected result. label Aug 16, 2024
@mcantelon mcantelon changed the title Fix error created when a duplicate PREMIS event identifier is encountered Fix error in duplicate PREMIS event ID handling Aug 16, 2024
@replaceafill
Copy link
Member

Thanks for catching and fixing this @mcantelon!

What do you think of updating the test module like this:

 def test_ensure_event_id_is_uuid(mocker, params):
     if params["message_logged"]:
-        mocker.patch("uuid.uuid4", return_value="f4eea76b-1921-4152-b1b4-a93dbbfeaaef")
+        mocker.patch(
+            "uuid.uuid4", return_value=uuid.UUID("f4eea76b-1921-4152-b1b4-a93dbbfeaaef")
+        )
     printfn = mocker.Mock()
     result = load_premis_events_from_xml.ensure_event_id_is_uuid(
         params["event_id"], printfn
     )
+    assert isinstance(result, str)
     if not params["message_logged"]:
         assert result == params["event_id"]
         printfn.assert_not_called()

This updates the test of that specific function test_ensure_event_id_is_uuid and fixes the wrong return value of the mock which should be a uuid.UUID type, not a string. Then it adds an assertion on result similar to yours but using isinstance which is recommended for this use case.

@mcantelon
Copy link
Member Author

@replaceafill Nice! I've updated the PR!

@mcantelon mcantelon marked this pull request as ready for review August 19, 2024 16:28
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.

Looks good @mcantelon, thank you!

Attempting to create a UUID object using an existing UUID object,
rather than a string, results in an error. Changed duplicate PREMIS
event identifier handling to avoid ending up doing this.
@mcantelon mcantelon merged commit 58d2157 into qa/1.x Aug 19, 2024
26 checks passed
@mcantelon mcantelon deleted the dev/event-uuid-dupe-fix branch August 19, 2024 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: bug A flaw in the code that causes the software to produce an incorrect or unexpected result.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants