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

Versioning #74

Merged
merged 25 commits into from
Nov 12, 2019
Merged

Versioning #74

merged 25 commits into from
Nov 12, 2019

Conversation

elizoller
Copy link
Member

GitHub Issue: (Islandora/documentation#740)

What does this Pull Request do?

Fires off a call to the chullo Fedora API to create a version when the version event type is received from Alpaca

What's new?

This method reads the event type header sent in from Alpaca on the POST node stream to the saveNode method. It passes that header value to the service's saveNode method which then looks at the value. If the event type is "Version" then it invokes chullo's fedoraApi methods for creating a version.
Todo: I need to write tests. I will do so once I'm sure this is the right approach to take.

How should this be tested?

  • update Milliner with this PR
  • probably want to have the Alpaca PR and Islandora PR mentioned above in place, otherwise you could hard code the value of $event_type = "Version" in the saveNode method (but that means you won't get node updates in fedora just so you know...)
  • make sure you get a version created in Fedora (note that when i did this, i got several versions within a few milliseconds of each other which i'll assume has to do with how many times the node update hook is being fired in Drupal...)

Additional Notes:

As I mentioned in the PR for Alpaca, I could easily see this being an entire separate end point in Milliner which a separate route and a separate method from the create/update POST endpoint. If that is what would be considered, I can make modifications for that. The way it's written right now a request to milliner's POST node/uuid route does 1 of 3 things: create node, create version of node, or update node. It might be slightly less tangled if it was its own endpoint that didn't have to read from a header.

Interested parties

@Islandora-CLAW/committers

@codecov
Copy link

codecov bot commented Jul 30, 2019

Codecov Report

Merging #74 into dev will increase coverage by 0.12%.
The diff coverage is 97.14%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev      #74      +/-   ##
============================================
+ Coverage     94.64%   94.77%   +0.12%     
- Complexity      160      168       +8     
============================================
  Files             9        9              
  Lines           654      689      +35     
============================================
+ Hits            619      653      +34     
- Misses           35       36       +1
Impacted Files Coverage Δ Complexity Δ
Milliner/src/Controller/MillinerController.php 94.66% <100%> (+1.01%) 19 <3> (+3) ⬆️
Milliner/src/Service/MillinerService.php 99.2% <95.65%> (-0.36%) 54 <5> (+5)

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 d41371b...2bcc1b6. Read the comment docs.

@elizoller
Copy link
Member Author

I think the tests are failing because it doesn't have the most recent version of chullo? because the tests pass locally after composer updating crayfish-commons and chullo

whikloj
whikloj previously requested changes Aug 6, 2019
Copy link
Member

@whikloj whikloj left a comment

Choose a reason for hiding this comment

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

At minimum the header name needs to be tweaked, but I think a new endpoint is probably better. I will defer if any other @Islandora-CLAW/committers want to weigh in though.

Milliner/src/Controller/MillinerController.php Outdated Show resolved Hide resolved
Milliner/src/Service/MillinerService.php Outdated Show resolved Hide resolved
@seth-shaw-unlv
Copy link
Contributor

Works for me. 👍

@elizoller
Copy link
Member Author

is there a way to get codecov to re-run?

@whikloj
Copy link
Member

whikloj commented Oct 24, 2019

@elizoller only if you add a commit which changes something, like the tests. Did you change something?

@elizoller
Copy link
Member Author

I guess I just need to write better tests then. Thanks

@whikloj
Copy link
Member

whikloj commented Oct 24, 2019

Ohhh you wrote tests for MillinerService::createVersion but not MillinerController::createVersion

@elizoller
Copy link
Member Author

I wrote a createVersionTest. I think I missed adding tests for the MillinerController into the MillinerControllerTest. I'll work on that

@elizoller
Copy link
Member Author

@whikloj is the try/catch in MillinerService::createVersion overkill?

@whikloj
Copy link
Member

whikloj commented Oct 25, 2019

Probably, you are catching your own exception and swallowing it. You probably (though I'm reading this on my phone) want to do the logging and throw the Exception out of the function so it causes the correct response.

Copy link
Contributor

@seth-shaw-unlv seth-shaw-unlv left a comment

Choose a reason for hiding this comment

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

:shipit:

$token = null
) {
$urls = $this->gemini->getUrls($uuid, $token);
$fedora_url = $urls['fedora'];
Copy link
Member

Choose a reason for hiding this comment

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

Maybe an edge case, but if something is deleted it should disappear from Gemini. A subsequent request will return null, probably want to do something like
https://github.com/Islandora/Crayfish/pull/74/files#diff-a3c5561a4953e4787761a32c6ce42b77R471
and throw an error if the item is gone.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, what would it return if $urls is empty? like a new Response(404)?

Copy link
Member Author

Choose a reason for hiding this comment

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

if this is the right approach, can you verify the test accurately tests this?

@elizoller
Copy link
Member Author

ok... i'll work on the tests more tomorrow

@seth-shaw-unlv
Copy link
Contributor

seth-shaw-unlv commented Nov 8, 2019

Well, Travis says the tests pass.

@whikloj, can you either approve or dismiss your review? After that I will pull down all the relevant PRs for a final test and then we can have a merge party! 🥳🎊

Update: I tested everything again. We are ready when you are, @whikloj.

@seth-shaw-unlv seth-shaw-unlv merged commit 8ccf1e4 into Islandora:dev Nov 12, 2019
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.

3 participants