Recent Item Submission showing at home page

[GauravD2t]

References

#667

• Fixes "Recently Added Items" doesn't exist on homepage #667

Description

Displaying Recent Submissions on the Repository's homepage with option of configuring number of items required to be displayed.

Instructions for Reviewers

Update the "rpp" variable in the config.example.yml with the item count that needs to be displayed in the Recent Submission section.
If you do not want to display any item under the Recent Submission section, keep the "rpp" count = 0.
Restart the server for the change to take effect, and no need to re-build the application.
Home page will show the Recent Submission section below the community list with the latest items submitted.
Upon clicking the Load more button, users will be redirected to the search results page, and items will list in descending order based on the submission date.
If there is no item submitted in the repository or rpp = 0, then Recent Submission should not appear.

Include guidance for how to test or review your PR. This may include: steps to reproduce a bug, screenshots or description of a new feature, or reasons behind specific changes.

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 TSLint validation using yarn run lint
• 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.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• 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.

[lgtm-com]

This pull request introduces 8 alerts when merging b84fdd7 into 618a7f4 - view on LGTM.com

new alerts:

• 8 for Unused variable, import, function or class

[tdonohue]

@GauravD2t : thanks for the PR! Just a quick note that this is showing lint errors (run yarn lint from commandline to see them) which causes our automated tests to fail. Also the automated review (from the LGTM bot) shows some minor alerts for unused imports, see this comment #1632 (comment)

In the meantime, I'll go ahead and assign this to reviewers, but we need to ensure those automated checks also succeed.

[MarieVerdonck]

Thanks, @GauravD2t . This seems to mostly be working as described, only your rpp config doesn't work as you described/nor as other config. That and some minor suggestions in review below.

I wouldn't put the new "rpp" variable, indicating how many results should be shown under the "Recent submissions" on homepage as a general environment variable. Example alternative: environment.homepage.recentSubmissionsRpp

Would require a src/config/homepage-config.interface.ts with var recentSubmissionsRpp: number & add this homepage: HomepageConfig to AppConfig instead of directly rpp

The (new) way to introduce a new config is:

• Add the default value in src/config/default-app-config.ts
• Add in config/config.example.yml as example on how to override
• Optionally for tests: config value in src/environments/environment.test.ts

Additionally, it doesn't look this this config is used in the same way as I expect. Changing it in config.yml doesn't change it, neither would adding it to config.example.yml (this is an example file on how you would override default config in config.yml|config.dev.yml|config.prod.yml). It does change if the value is changed in default-app-config.ts; but this is, as stated above, the backup value, and should be able to be overrided with a config in yml file.

Fix: Instead of creating new DefaultAppConfig in src/app/home-page/recent-item-list/recent-item-list.component.ts => Use environment.rpp (or for example environment.homepage.recentSubmissionsRpp if above suggestion is applied)

[MarieVerdonck]

To get override in yml file, use environment.rpp
(with import { environment } from '../../../environments/environment';

[MarieVerdonck]

Might be interesting to have the sort field also configurable, in case someone would for example want to use dc.date.available (recently un-embargoed items would then also show up in recent submissions) or in case this field ever changes in REST
Eg under environment.homepage.recentSubmissionsSortField

Though then the same should be done for recent submission on collection page (in src/app/collection-page/collection-page.component.ts)

[GauravD2t]

added recentSubmissionsSortField in homepageConfig..

[MarieVerdonck]

Putting the explicit search link with dc.date.accessioned, DESC sort explicitly here doesn't seem very resistant to minor changes. Instead of a href, I would use a [routerLink]="" (click)="onLoadMore()"
And in this onLoadMore() use SearchService#getEndpoint(SearchOptions) to navigate with a given sort field/order. (Sort field optionally configurable, see note above)

[GauravD2t]

change as your suggestion

[MarieVerdonck]

Should be a i18n label; eg {{'home.recent-submissions.head' | translate}} & add that label to en.json5

[GauravD2t]

home.recent-submissions.head added into en.jon

[MarieVerdonck]

Thanks for the changes! Config setting still not quite working as I expected though (not overridable):

• Still isn't overridable via setting it in config/config(.prod|dev).yml doesn't change rpp/sort; tested with in config/config.dev.yml / yrsd mode

  homePage:
    recentSubmissionsRpp: 20
    recentSubmissionsSortField: 'dc.date.issued'
  

  • Requires in recent-item-list.component.ts to use environment (import { environment } from '../../../environments/environment';) instead of this.appConfig

• I would add your config to config/config.example.yml as well, since all overridable config is set there with documentation as an example on how/which config can be overrided in config.dev.yml/config.prod.yml/config.yml
  Eg

  # Home Page
  homePage:
  	# The number of item showing in recent submission components
    recentSubmissionsRpp: 5,
  	# Sort record of recent submission
    recentSubmissionsSortField: 'dc.date.accessioned',
  

• Tests currently failing, need newly used SearchService mock injected in RecentItemListComponent spec

[MarieVerdonck]

Change to environment.homePage.recentSubmissionsRpp

[artlowel]

Since @MarieVerdonck is on holiday, I'll take over for her

[artlowel]

Thanks @GauravD2t! I've verified that the problems overriding the config @MarieVerdonck mentioned are gone now.

Two more suggestions of my own though:

• Currently all recent submissions are stuck together, I'd add the same spacing they get on e.g. the search result page, by wrapping each result in a div with the bootstrap class my-4
• It would be good if the load more button used an actual routerLink instead of an onClick handler. Not doing so introduces a11y problems as well. So perhaps create a queryParams property on the component, and in the constructor initialize it, something like (untested):

    this.loadMoreQueryParams = {
      [`${this.searchConfigurationService.paginationID}.sf`]:  environment.homePage.recentSubmissionsSortField,
      [`${this.searchConfigurationService.paginationID}.sd`]:  SortDirection.DESC,
    };

… and then use that object as the queryParams input for the load more link

[tdonohue]

@GauravD2t : I wanted to drop you a quick note about the 7.3 release schedule. Any PRs for the 7.3 release must be fully approved & merged by June 17 (in around two weeks) or else they will be rescheduled for 7.4.

It looks like this PR has had some positive feedback from reviewers, but it still has some minor fixes to be applied. It also appears to be failing tests & missing tests for the new recent-item-list.component.ts (there is a corresponding spec file, but it looks mostly empty & is failing). I'd expect there to be some basic tests of the behavior of this recent items listing...perhaps similar to the tests which verify the "top level" Community lists also on the homepage : https://github.com/DSpace/dspace-angular/blob/main/src/app/home-page/top-level-community-list/top-level-community-list.component.spec.ts (notice how it uses test data and verify that the expected communities are displayed)

In any case, if you have questions let us know! I'd still like to see this get into 7.3, but we are quickly running out of time, so we'd need you to address the feedback & fix the tests in the near future. Thanks!

[tdonohue]

Moving this to 7.4 board/schedule, as it hasn't been updated based on feedback in time for the 7.3 release. I would love to see this make it into 7.4.

[tdonohue]

@GauravD2t or @DSquareTechnologies: This work has been stalled for a very long time. Any chance of it being updated for the 7.4 release? I know others are interested in seeing this move forward, but I haven't seen any movement on this PR in 2+ months. If you are no longer planning on doing this work, please also let me know so that I can reassign it (and someone else might be able to take your partial code & help move it forward for 7.4). Thanks!

[DSquareTechnologies]

Hi Tim, This work is done already and same is functional in recent DSpace sites implemented by us. https://oasis.col.org Let me check the reason and get back soon.

…

________________________________ From: Tim Donohue ***@***.***> Sent: Friday, August 19, 2022 9:52:29 PM To: DSpace/dspace-angular ***@***.***> Cc: DSquare Technologies ***@***.***>; Mention ***@***.***> Subject: Re: [DSpace/dspace-angular] Recent Item Submission showing at home page (PR #1632) @GauravD2t<https://github.com/GauravD2t> or @DSquareTechnologies<https://github.com/DSquareTechnologies>: This work has been stalled for a very long time. Any chance of it being updated for the 7.4 release? I know others are interested in seeing this move forward, but I haven't seen any movement on this PR in 2+ months. If you are no longer planning on doing this work, please also let me know so that I can reassign it (and someone else might be able to take your partial code & help move it forward for 7.4). Thanks! — Reply to this email directly, view it on GitHub<#1632 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABQHNKX5OBNT5GBHMTV2PMDVZ6YELANCNFSM5UVSYB7A>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[tdonohue]

@DSquareTechnologies : It's nice to see that you have a version of this working in your local code. But, obviously it never was added to core DSpace. So, definitely let us know if you still plan to contribute this back to core DSpace -- if so, we'd love to continue working with you to move this PR forward. (First though this PR would need some updates/fixes as it currently has merge conflicts and hasn't been updated/rebased to latest main, so it is failing all tests, etc.)

[tdonohue]

@GauravD2t : It appears you've accidently removed all the commits from this PR... which has confused GitHub into thinking this PR has been "merged". It has not been merged. (I just double checked)

Did you want to maybe create a fresh PR with your changes & we can move the review over to that PR? Part of the issue may be that this PR was created from your own main branch. So, anytime you update your main branch it would update this PR. Usually, we recommend creating a new branch for any PR you create... here's an example workflow we recommend: https://wiki.lyrasis.org/display/DSPACE/Development+with+Git#DevelopmentwithGit-DevelopingfromaForkedRepository This isn't required, but it can make working with GitHub easier.

GitHub also has it's own similar documentation which describe the same process here: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests