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

Content reports ported from DSpace 6.x #2163

Merged
merged 46 commits into from Feb 28, 2024
Merged

Content reports ported from DSpace 6.x #2163

merged 46 commits into from Feb 28, 2024

Conversation

jeffmorin
Copy link
Contributor

References

Description

This pull request includes an initial version of the two content reports ported from DSpace 6.x.

Instructions for Reviewers

This pull request adds a Content Reports section in the Administration menu to provide access to both reports from the Angular web application.

After logging in using an account with administrator permissions, select the Report item from the Administration menu at the left of the page, then select one of the two entries in this section.

In the Filtered Items report, the Export Metadata functionality is not yet implemented, it will be part of another pull request, once this one is all settled and merged.

Please note:

  • I am a beginner when it comes to anything related to Angular. I will need proper guidance to apply any requested change.
  • In the component classes (filtered-collections-component.ts and filtered-items-component.ts), I use a DSpaceRestService instance to send my requests to the REST layer. It may be better to create and use a specific data service similar to CollectionDataService, but I will need help to do so if requested.
  • I wrote a few unit tests for my components, but even after several days I didn't succeed in making my tests in filtered-items-component.spec.ts run properly. I will definitely need help with this.

Checklist

  • 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 TSLint validation using yarn run lint [Not sure]
  • My PR doesn't introduce circular dependencies
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods. [Not sure]
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide. [Not yer, please see above]
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@tdonohue tdonohue added new feature component: administrative tools Related to the admin menu or tools labels Mar 24, 2023
@tdonohue tdonohue added this to the 7.6 milestone Mar 24, 2023
@paulo-graca paulo-graca self-requested a review March 30, 2023 15:02
@github-actions
Copy link

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


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@jeffmorin
Copy link
Contributor Author

jeffmorin commented Apr 20, 2023

I resolved the existing conflicts and re-updated my fork (and this PR). All of them were in i18n files.

@tdonohue
Copy link
Member

@jeffmorin : If it's easier to maintain this PR, you are welcome to remove all the changes to i18n files except the ones in en.json5 (English) and any other you wish to maintain (e.g. French). It is NOT required to modify every i18n file when you add new keys to the en.json5. Translators will update those files the next time they are doing translations. That will also make this PR much smaller in size (as it appears most of your changes are related to those i18n files) and much less likely to encounter future merge conflicts.

@jeffmorin
Copy link
Contributor Author

Thanks for the info, I'll happily keep it in mind for future updates!

@pilasou
Copy link
Contributor

pilasou commented Feb 26, 2024

Hi @tdonohue , hi @artlowel, hi @paulo-graca, I just want to confirm that @jeffmorin has completed all the work we agreed upon during February 8th Dev group meeting: https://wiki.lyrasis.org/display/DSPACE/2024-02-08+DSpace+Developers+Meeting.
The report feature can be tested. To help with the test, I have prepared a document:
tests-DSpace8-reports-beta-release.docx

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@jeffmorin : Overall, this looks great! I've found a few minor bugs though, and I have some minor feedback. I don't think any of this should take long to address, but let me know.

  1. First, a bug I noticed is that pagination on the "Metadata Query" report isn't working properly? Or maybe the numbers are just wrong? Here's what I see:
    • Using the "Metadata Query Report", run an Item query across the whole repository with no filters (or anything that returns several pages of results)
    • Page 1 of the results will show items 1-10
    • Page 2 of the results will show items 101-110 (it's possible these numbers are just wrong?)
    • Page 3 of the results will show items 201-210
    • Essentially, the pagination shows only 10 items at a time, but counts by 100. So, something is off.
  2. I found a few minor issues & small code cleanup as well. See the notes inline below.

Overall though, this appears to work well. Not noticing anything majorly concerning. I've also verified that it can easily be turned on/off via the configuration flag. Thanks!

@tdonohue
Copy link
Member

@jeffmorin and @pilasou : I just stumbled on some very odd behavior. With these "Content Reports" frontend & backend PRs installed, I'm finding that "Browse by Issue Date" and "Browse by Title" no longer work at all. They both are throwing 500 errors for me & no results will load in the UI. The 500 exception on the backend shows this error:

2024-02-26 23:12:24,727 ERROR 023c6910-8f69-45fd-bfa1-5e2b69e0d5c2 11becc14-b20e-40f3-bf46-686839365be2 org.dspace.app.rest.exception.DSpaceApiExceptionControllerAdvice @ An exception has occurred (status:500)
 java.lang.IllegalStateException: The requested browse haven't metadata entries
   at org.dspace.app.rest.repository.BrowseEntryLinkRepository.listBrowseEntries(BrowseEntryLinkRepository.java:85) ~[dspace-server-webapp-8.0-SNAPSHOT.jar:8.0-SNAPSHOT]

Are you seeing this same behavior with these PRs? I cannot reproduce this error on main, but as soon as I install these PRs the error occurs (regardless of whether the Content Reports are enabled or disabled). The cause is not obvious to me & I haven't had a chance to dig into this further.

@jeffmorin
Copy link
Contributor Author

About the bug in the Metadata Query report:

First of all, I cannot reproduce the behaviour you are describing, I navigate between pages without any gap in the numbering.

Besides, you wrote “with no filters (or anything that returns several pages of results)” — how can you navigate between pages without setting anything that returns several pages of results? I am not sure I understand what you mean: did you get paginated results or not?

@jeffmorin
Copy link
Contributor Author

I couldn't reproduce the erratic behaviour in the Browse by Title and Browse by Publication Date functionalities.

@jeffmorin
Copy link
Contributor Author

I finally reproduced the results numbering bug (1–10 then 101–110, and so on). An important repro step I was missing is that I must query a repository containing more than 100 items. I will check this too.

@jeffmorin
Copy link
Contributor Author

The navigation bug is fixed. It was an inconsistency in parameter naming that I didn't see when switching to GET requests.

@tdonohue
Copy link
Member

@artlowel : If you have a moment this week, could you see if you are able to reproduce the issue with Browse by Title / Date that I've run into with this PR? I've found that pulling this PR onto latest main branches results in those pages no longer working. More details in this comment on the backend PR.

Basically, I'm able to reliably reproduce this bug anytime I test these PRs (both yesterday and today). But, @jeffmorin isn't able to reproduce it. If you or someone on your team has time to just test whether you see it, that might be helpful here.

Copy link

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

@artlowel
Copy link
Member

@tdonohue I can't reproduce those browse issues. I tried while logged out, and logged in (having first used the content reports) for both the global and collection browse lists, all seem to work. Perhaps it's something specific to (one of) the items returned by those pages on your backend?

Does it work if you go straight to a different page? /browse/title?bbm.page=2 or /browse/dateissued?bbm.page=2

@jeffmorin
Copy link
Contributor Author

Hi @artlowel. Which version of the code did you use to try to reproduce the bug Tim ran into?

Yesterday, I refactored the FilteredItemRest class so that it doesn't extend ItemRest anymore, so if that inheritance relationship is actually the reason the bug occurred, it's quite possible you won't reproduce it anymore if you retrieved my code yesterday night or later.

@artlowel
Copy link
Member

@jeffmorin I used the latest version

OptionVO.item('50', '50'),
OptionVO.item('100', '100'),
OptionVO.item('250', '250'),
OptionVO.item('1000', '1000')
Copy link
Member

Choose a reason for hiding this comment

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

@jeffmorin : Could we consider removing this 1,000 option (and perhaps 250 as well). I mistakenly tried using it and my entire system froze while the backend attempted to respond with 1,000 items.

If possible, I'd like us to consider placing limits on these page sizes to 100 or less. It'd also be ideal to have the DEFAULT value be simply 10. (It appears the current default is 100...which also responds very slowly for me.)

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @jeffmorin ! This frontend is now working for me and looks good. That said, I had a very minor comment that I submitted just a few minutes ago: https://github.com/DSpace/dspace-angular/pull/2163/files#r1506309146

I did notice the the pagesize options include a 1,000. I mistakenly tried that, and my system hung for several minutes. I think we should consider removing anything under pagesize 100 and set the default to 10. However, I'll leave this as optional based on your feedback.

@tdonohue
Copy link
Member

@jeffmorin : I've decided to merge this as-is just to get it onto main. I think the issue about page size that I mentioned above might be a nice "bug fix" PR to add to this later, if you have time.

@tdonohue tdonohue merged commit ff05094 into DSpace:main Feb 28, 2024
19 of 21 checks passed
@tdonohue
Copy link
Member

Flagging this as needs documentation until Documentation can be added to DSDOC8x. This REST Reports area needs to be either updated or replaced: https://wiki.lyrasis.org/display/DSDOC8x/REST+Based+Quality+Control+Reports

@tdonohue tdonohue added the needs documentation PR is missing documentation. All new features and config changes require documentation. label Feb 28, 2024
@tdonohue tdonohue added this to the 8.0 milestone Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: administrative tools Related to the admin menu or tools needs documentation PR is missing documentation. All new features and config changes require documentation. new feature
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[DS-4301] Ensure that DSpace REST Report capabilities are part of the DSpace 8 REST API
4 participants