Coar Notify Integration - Administer/Log #251
Conversation
added a new contract for ldnmessages endpoints Approved-by: Stefano Maffei
refactoring Approved-by: Stefano Maffei
tdonohue
added
new feature
integration: COAR Notify / LDN
|
@steph-ieffam and @frabacche : Please update this based on the latest main now that the initial COAR Notify work is merged. This PR currently seems to have overlap with the COAR Notify REST Contract #249 that was already merged. |
ldnmessages.md
Outdated
| * 405 Method Not Allowed | ||
|
|
||
| ## enqueueRetry | ||
| **/api/ldn/ldnmessages/enqueueretry/<:id>** |
There was a problem hiding this comment.
@abollin & @steph-ieffam i : Please correct me if I'm wrong, but this doesn't seem to align with our REST Contract? https://github.com/DSpace/RestContract/blob/main/README.md#on-sub-path-of-a-single-resource-endpoint-associations
This looks like it should be an "association" style endpoint, but it's not structured as such. I don't recall any other endpoints that have a structure of /api/[category]/[endpoint-name]/[subpath]/[uuid]. Am I misunderstanding this endpoint?
I also find this design odd that you have to send a GET request to perform an "action" (trying to add back into the queue). In other endpoints, this seems to be a POST or PUT style endpoint.
There was a problem hiding this comment.
should the path be **/api/ldn/ldnmessages/<:id>/enqueueretry** instead of **/api/ldn/ldnmessages/enqueueretry/<:id>** as it currently is?
There was a problem hiding this comment.
@frabacche : I'd recommending asking that question to @abollini . I'm not sure I understand this endpoint completely yet. But, it seems incorrectly designed to me. I think making it /api/ldn/ldnmessages/<:id>/enqueueretry seems more correct... but I still don't know that it should be a GET request either. It's very odd looking to me and I'm not sure I have a recommendation yet.
There was a problem hiding this comment.
@tdonohue the GET is an awful typo, sorry for that it should be POST actually and also the URL should be changed as noted.
There was a problem hiding this comment.
@steph-ieffam : The current implementation in LDNMessageRestController is using GET. You can see that in the LDNMessageRestControllerIT in DSpace/DSpace#9268 So, the implementation would also require an update as it appears to be using GET as far as I can tell.
There was a problem hiding this comment.
@steph-ieffam : Please update this PR on the latest main. It's very difficult to review as it contains all the changes made in the first COAR Notify Contract PR. If you update this against main, it should be much smaller than 1,000+ lines
|
There was a problem hiding this comment.
👍 @steph-ieffam and @frabacche : Thanks for updating this. This looks good/correct to me. I'll wait to merge though until the backend is ready to merge.
References
Related to:
Description
Developments based on these PRs:
Instructions for Reviewers
Documentation for this PR: https://wiki.lyrasis.org/pages/viewpage.action?pageId=325255444
Technical implementations for part one documented here : https://wiki.lyrasis.org/pages/viewpage.action?pageId=319815713