-
Notifications
You must be signed in to change notification settings - Fork 29
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
Versioning #74
Conversation
… over time and to keep the fedora specific pieces in chullo, not milliner
… versioning functionality in a try catch
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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 |
There was a problem hiding this 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.
…eNode route, to having a separate route
Works for me. 👍 |
is there a way to get codecov to re-run? |
@elizoller only if you add a commit which changes something, like the tests. Did you change something? |
I guess I just need to write better tests then. Thanks |
Ohhh you wrote tests for |
I wrote a createVersionTest. I think I missed adding tests for the MillinerController into the MillinerControllerTest. I'll work on that |
@whikloj is the try/catch in MillinerService::createVersion overkill? |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$token = null | ||
) { | ||
$urls = $this->gemini->getUrls($uuid, $token); | ||
$fedora_url = $urls['fedora']; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
?
There was a problem hiding this comment.
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?
ok... i'll work on the tests more tomorrow |
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?
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