Skip to content

Add RSS buttons if the feature is enabled on the rest side#1444

Merged
tdonohue merged 16 commits intoDSpace:mainfrom
atmire:shared-feature-rss-viewer
May 13, 2022
Merged

Add RSS buttons if the feature is enabled on the rest side#1444
tdonohue merged 16 commits intoDSpace:mainfrom
atmire:shared-feature-rss-viewer

Conversation

@ConfusionOrb221
Copy link
Copy Markdown
Contributor

@ConfusionOrb221 ConfusionOrb221 commented Dec 9, 2021

References

Add references/links to any related issues or PRs. These may include:

Description

Adds rss feed button that will take you to an rss page based on the current search and if you aren't on the search page it'll base it off of scope e.g on community page the scope is that community pages uuid. Also adds the rss href link as a <link> tag to the Head element on any page the rss button exists.

Instructions for Reviewers

List of changes in this PR:

  • Adds ds-rss component to any page
  • Only adds the buttons to the page if the config websvc.opensearch.enable is set to true on the rest side and requires that pull listed above.
  • Edits test files to pass down providers so that those test files don't fail due to no provider for ds-rss
  • Adds link-head.service to add the link tag to the HEAD element.

Need DSpace running with the other pr with objects indexed then click on the rss button on any page it exists on. Try some searches to verify that the rss respects what you type into search.

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
Copy link
Copy Markdown

lgtm-com Bot commented Dec 9, 2021

This pull request introduces 2 alerts when merging 0a9b4d7 into efe51aa - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@ConfusionOrb221 ConfusionOrb221 marked this pull request as ready for review December 10, 2021 18:07
@tdonohue tdonohue added component: Discovery related to discovery search or browse system low priority new feature labels Dec 10, 2021
@tdonohue tdonohue added this to the 7.2 milestone Dec 10, 2021
@tdonohue tdonohue mentioned this pull request Dec 16, 2021
Copy link
Copy Markdown
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.

@ConfusionOrb221 : First off thanks for this submission. I've tested this locally today, and it's not quite working as expected. I'm testing this using yarn start (production / SSR mode), and here's what I see:

  1. With default settings (defaults in dspace.cfg/local.cfg), I'm seeing the RSS feed icon on the homepage & Community/Collection pages, but if I click it, I get an error: "OpenSearch Service is disabled". It appears this might be because websvc.opensearch.enable = false by default.. and even though this PR checks that value, it's still displaying the RSS icon when it's disabled. It looks like the issue might be that there are two configurations here... there's webui.feed.enable = true (so feeds enabled by default), but OpenSearch is disabled by default.
  2. Once I set websvc.opensearch.enable = true, everything works properly.

A few more notes inline...primarily some code questions & requests to add required TypeDocs / comments (Please keep in mind that we require minimal TypeDoc/JavaDoc comments both for frontend & backend. I noticed neither PR has any code comments.)

Comment thread src/app/shared/rss-feed/rss.component.ts Outdated
Comment thread src/app/core/services/link-head.service.ts
Comment thread src/app/shared/rss-feed/rss.component.ts Outdated
@tdonohue tdonohue removed this from the 7.2 milestone Jan 20, 2022
@tdonohue
Copy link
Copy Markdown
Member

@ConfusionOrb221 : I've moved this over onto our 7.3 board (along with the corresponding backend PR DSpace/DSpace#8064), in the hopes that you'll find time to get this updated for our 7.3 release. Currently, these PRs have been stalled for over a month & have merge conflicts...but if you can find time to fix them up, I'll make sure it gets reviewed. Thanks

@tdonohue
Copy link
Copy Markdown
Member

@ConfusionOrb221 : Somehow this PR is in an odd state where none of the test ran the last time you pushed changes. It also has code conflicts. If you could find time to resolve the conflicts & push the updates to this PR, hopefully that'll help restart the tests. Once that's done we can make sure to get this reassigned to reviewers. Thanks!

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Apr 18, 2022

This pull request introduces 4 alerts when merging 0d3e7fb into 4588715 - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class

@ConfusionOrb221
Copy link
Copy Markdown
Contributor Author

Hmm so I rebased this code to prevent the merge conflicts and all my tests are still passing now but I am getting some tests failures in BrowseByComponent due to being unable to find a provider for GroupDataService.

NullInjectorError: R3InjectorError(DynamicTestModule)[GroupDataService -> GroupDataService]: NullInjectorError: No provider for GroupDataService! error properties: Object({ ngTempTokenPath: null, ngTokenPath: [ 'GroupDataService', 'GroupDataService' ] }) NullInjectorError: R3InjectorError(DynamicTestModule)[GroupDataService -> GroupDataService]: NullInjectorError: No provider for GroupDataService!

Unsure what is going on here with that seems unrelated to my code but I could be wrong.

@ConfusionOrb221 ConfusionOrb221 force-pushed the shared-feature-rss-viewer branch 3 times, most recently from b22e6ff to c246908 Compare April 18, 2022 18:24
@tdonohue tdonohue self-requested a review April 20, 2022 14:33
@tdonohue
Copy link
Copy Markdown
Member

@ConfusionOrb221 : Thanks for the recent updates here! Just a quick note though, this PR has minor merge conflicts now...likely caused after I merged #1560 (earlier today). If you could find time to resolve them that'd help the final review/testing of this. Thanks!

@ConfusionOrb221 ConfusionOrb221 force-pushed the shared-feature-rss-viewer branch from c246908 to 67fb53a Compare April 22, 2022 13:39
@ConfusionOrb221
Copy link
Copy Markdown
Contributor Author

@tdonohue Everything should be good now fixed up the lint issue and all tests are passing now so should be good to move forward with this.

Copy link
Copy Markdown
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 @ConfusionOrb221 ! This PR now looks good to me, as the RSS feed is appearing ONLY on the homepage & community/collection pages. That said, in testing today, I've found that it's still returning items in reverse order, but that appears to be a backend bug, so I'll note that in your other PR.

@artlowel : Could you give this another look when you get the chance? Keep in mind the feed will only appear on homepage & community/collection pages now.

Copy link
Copy Markdown
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

@ConfusionOrb221 ConfusionOrb221 force-pushed the shared-feature-rss-viewer branch from 75ec837 to 3a61125 Compare May 13, 2022 17:38
Copy link
Copy Markdown
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 again @ConfusionOrb221 ... retested with latest changes and everything is working properly. The feed is now ordered by dc.date.accessioned desc as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: Discovery related to discovery search or browse system medium priority new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RSS/ATOM Feeds

3 participants