Coar Notify Integration - Administer/Log

[FrancescoMolinaro]

References

Related to

1. Coar Notify Integration - Administer/Log  DSpace#9268
2. Coar Notify Integration - Administer/Log RestContract#251

Fixes part of #2842

Description

Implementation/improvements of COAR based functionalities.

Developments based on these PRs:

backend DSpace/DSpace#9218 ;
frontend #2681 ;
rest contract DSpace/RestContract#249 ;

Instructions for Reviewers

Technical implementations documented here : https://wiki.lyrasis.org/pages/viewpage.action?pageId=325255444

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes ESLint validation using yarn lint
• My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
• My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
• If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
• If my PR fixes an issue ticket, I've linked them together.

[FrancescoMolinaro]

@FrancescoMolinaro & @frabacche : Please update this based on the latest main now that the initial COAR Notify work is merged. Please also ensure that all tests succeed & documentation is updated. Let me know when this is ready for review.

If you wish, you can also add the fixes for #2842 into this PR. Those bugs will need to be resolved either in a small, separate PR, or as part of this larger PR

Hi @tdonohue , I have addressed the issue from #2842 in this PR.
I hope this works for you, if we need to port it in a separate smaller PR just let me know, thanks.

[tdonohue]

Thanks @FrancescoMolinaro ! Overall this feature works well for me. I've only run into a few bugs & have some suggestions inline as well.

1. When LDN is disabled (ldn.enable=false in local.cfg) the COAR Notify -> Dashboard menu item is still available. If LDN is disabled, I expect that entire menu to go away. I noted inline below where that can be fixed.
2. I'm encountering a weird error on the backend if I click between the "Logs/Inbound" and "Logs/Outbound" tabs on the dashboard. In my Browser's DevTools, I see a 400 Bad Request for every facet option displayed on that page. The underlying error looks like this:

   org.dspace.app.rest.exception.DSpaceBadRequestException: ldn_service is not a valid search facet
   

   • To reproduce this, login as an Admin & go to COAR Notify -> Dashboard
   • Open your browser DevTools on the Network tab
   • Click on the "Logs/Inbound" tab. No errors should occur.
   • Now click on the "Logs/Outbound" tab and you'll see the 400 response errors.
   • If you do this in the opposite order, the errors again occur. Logout, login again, but click on "Logs/Outbound" tab first (no errors). Then click on "Logs/Inbound" and you'll see the errors again.
   • It's unclear to me whether this is a front or backend error. Since it doesn't impact the UI, we could simply create a ticket for this issue if it's not easy to fix.
3. Should there be an option to delete LDN requests? If this is out-of-scope, we can move this to a ticket. But, I noticed in my testing I had several invalid LDN Requests. It'd be nice if I could delete them from the Admin UI, but that doesn't seem possible yet.

Many other inline comments below.

[FrancescoMolinaro]

Thanks @FrancescoMolinaro ! Overall this feature works well for me. I've only run into a few bugs & have some suggestions inline as well.

1. When LDN is disabled (ldn.enable=false in local.cfg) the COAR Notify -> Dashboard menu item is still available. If LDN is disabled, I expect that entire menu to go away. I noted inline below where that can be fixed.

2. I'm encountering a weird error on the backend if I click between the "Logs/Inbound" and "Logs/Outbound" tabs on the dashboard. In my Browser's DevTools, I see a 400 Bad Request for every facet option displayed on that page. The underlying error looks like this:

   org.dspace.app.rest.exception.DSpaceBadRequestException: ldn_service is not a valid search facet
   

   • To reproduce this, login as an Admin & go to COAR Notify -> Dashboard
   • Open your browser DevTools on the Network tab
   • Click on the "Logs/Inbound" tab. No errors should occur.
   • Now click on the "Logs/Outbound" tab and you'll see the 400 response errors.
   • If you do this in the opposite order, the errors again occur. Logout, login again, but click on "Logs/Outbound" tab first (no errors). Then click on "Logs/Inbound" and you'll see the errors again.
   • It's unclear to me whether this is a front or backend error. Since it doesn't impact the UI, we could simply create a ticket for this issue if it's not easy to fix.

3. Should there be an option to delete LDN requests? If this is out-of-scope, we can move this to a ticket. But, I noticed in my testing I had several invalid LDN Requests. It'd be nice if I could delete them from the Admin UI, but that doesn't seem possible yet.

Many other inline comments below.

Hello @tdonohue , I have addressed point 1 and 2, they should be ok now.
For point three I have been told that the possibility to delete an LDN request was not in scope.
Would be ok for you to handle that in a separate ticket?

[github-actions]

Hi @FrancescoMolinaro,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

[tdonohue]

@FrancescoMolinaro and @frabacche : Apologies, but this PR now has code conflicts after merging #2749. If you could rebase this quickly, then this will be the next PR that I test/review (along with its backend PR). Thanks

[tdonohue]

@FrancescoMolinaro and @frabacche : Overall, this looks good now & I'm nearly a +1. One small issue that I found in testing the application when ldn.enabled = false (disabled) is that an error occurs behind the scenes on every Item page. Here's how to reproduce:

1. Disable Notify (ldn.enabled = false). Restart backend & frontend (if needed)
2. Startup UI in prod mode (yarn build:prod; yarn serve:ssr)
3. Open your browser DevTools in the Network tab
4. As an anonymous user, browse to an Item display page and you'll see this error in your DevTools console:

   ERROR TypeError: You provided 'undefined' where a stream was expected. You can provide an Observable, Promise, ReadableStream, Array, AsyncIterable, or Iterable.
   at t (main.fa2f35c7e30eca0d.js:1:1817092)
   at v (main.fa2f35c7e30eca0d.js:1:1796840)
   at l.subscribe.g (main.fa2f35c7e30eca0d.js:1:1807303)
   at o._next (main.fa2f35c7e30eca0d.js:1:1799389)
   at o.next (main.fa2f35c7e30eca0d.js:1:1788183)
   at main.fa2f35c7e30eca0d.js:1:1803362
   at o._next (main.fa2f35c7e30eca0d.js:1:1799389)
   at o.next (main.fa2f35c7e30eca0d.js:1:1788183)
   at subscribe.h (main.fa2f35c7e30eca0d.js:1:1807339)
   at o._next (main.fa2f35c7e30eca0d.js:1:1799389)
   

I do not see this error when ldn.enabled=true. I also don't see this error on the main branch of code. So, it seems like something in this PR is still trying to run on Item pages when it should be disabled?

UPDATE: I found this error also does occur on main, but again only when ldn.enabled=false (or commented out). So, it appears to be an LDN/Notify related error, possibly similar to the TypeError noted in #2842

Beyond that, everything else works well. If this is not a bug that can be easily solved, I'm OK with moving it into a bug ticket to be solved by the 4Science team. (As a sidenote, the request to delete messages can also be moved to a future ticket.)

[tdonohue]

👍 @FrancescoMolinaro and @frabacche : On further analysis, I've decided to merge this as-is, because the remaining TypeError issue looks (to me) like it may be similar to the one described in #2842. Therefore, I'll keep issue #2842 open until that issue can also be resolved. Please send a small follow-up PR to fix that remaining issue when you can.

Thanks for your hard work on this! It works very well other than that TypeError

[tdonohue]

Adding the needs documentation label until docs can be moved to the DSDOC8x space: https://wiki.lyrasis.org/display/DSDOC8x/

[tdonohue]

Removing "Needs documentation" label as they now exist at https://wiki.lyrasis.org/display/DSDOC8x/COAR+Notify