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.

Sorry, something went wrong.

Jean-François Morin and others added 11 commits November 30, 2022 15:41

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@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!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@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
@tdonohue
Copy link
Member

Documentation for this feature exists in https://wiki.lyrasis.org/pages/viewpage.action?pageId=325255348 Removing the "needs documentation" flag.

@tdonohue tdonohue removed the needs documentation PR is missing documentation. All new features and config changes require documentation. label May 31, 2024
@tdonohue tdonohue mentioned this pull request Jun 18, 2024
8 tasks
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 new feature
Projects
No open 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