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

Added search facets to community & collection pages #2732

Conversation

alexandrevryghem
Copy link
Member

@alexandrevryghem alexandrevryghem commented Jan 8, 2024

References

Description

Added the search section as the default community page tab and moved the community en collection lists to a separate tab. I replaced the recently added section on the collection page with the same search section. I also moved the comcolSelectionSort config in the new comcol config group.

Instructions for Reviewers

List of changes in this PR:

  • Removed the CollectionRecentlyAddedComponent
  • Created the new ComcolSearchSectionComponent, this is the tab with the search form. It is possible to hide the sidebar facets using the new config comcol.searchSection.showSidebar
  • Set the ComcolSearchSectionComponent section as the default community/collection tab
  • Moved the SubComColSectionComponent section under the subcoms-cols route on community pages (should maybe be renamed depending on community feedback)

Bug fixes:

  • Fixed searchevents request sometimes sending an incorrect UUID as the clickedObject's value (it contained the queryParameters)
  • The SearchComponent's trackStatistics still sends searchevents requests when false
  • Prevented angular from sending the request /server/api/submission/vocabularyEntryDetails/search/top?vocabulary=undefined when the search facet has type hierarchy (in discovery.xml), but you don't configure any vocabularies in you config.yml file
  • Prevent the access status data from being retrieved when it is not enabled

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 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.

@alexandrevryghem alexandrevryghem self-assigned this Jan 8, 2024
@alexandrevryghem alexandrevryghem added component: Discovery related to discovery search or browse system high priority new feature labels Jan 8, 2024
@alexandrevryghem alexandrevryghem force-pushed the added-recently-added-section-to-community-page_contribute-main branch 2 times, most recently from 4299ed4 to 80fd90f Compare January 9, 2024 00:16
@tdonohue tdonohue self-requested a review January 11, 2024 16:00
@tdonohue
Copy link
Member

tdonohue commented Jan 24, 2024

@alexandrevryghem : My apologies, but I've just notices a major conflict between this PR and #2275. They are resolving arguably "different" features, but the end result is mostly the same thing.

#2275 creates a "Search Within" tab on Community/Collection homepages which provides a searchbox & filters, but also defaults to showing recent submissions. See this screenshot of how that PR changes a Community homepage:

facets-search-on-comcol-pages

This "Recent Submissions" PR makes a very similar change to the Community homepage, but instead of the "Search Within" tab it creates a "Recent Submission" tab. Here's a screenshot of the exact same Community with this PR:

recent-submission-community

I don't know how everyone (self included) missed the fact that ticket DSpace/DSpace#8426 and #1901 are essentially the "same feature" described in completely different ways. These should have been merged into one ticket a long time ago.

I'm currently not sure how best to move these forward. Overall, I suspect the usability of #2275 will be preferred by DSpace users, but I also realize you've made some major improvements in #2722 that we don't want to lose. Suggestions/thoughts welcome. Again, my apologies on this mix-up.

@tdonohue
Copy link
Member

@alexandrevryghem : Thanks for your feedback. In that case, I'll review both #2275 and #2722 and leave this PR alone. We'll resolve the minor conflicts between these other PRs once we merge the first one. I'm not sure which will be merged first...it will depend on the reviews, but I'll get both reviewed today/tomorrow.

All this means that this PR likely can be closed, as it appears we'll fix this issue via #2275 and #2722.

@alexandrevryghem
Copy link
Member Author

@tdonohue: I looked at the code of #2275 again and it contains a lot of changes by accident I think, but what it mainly does is:

  • Moving the default tab (community & collection list) to a different component in order to change the default tab (this is already covered by PR Refactored community & collection pages #2722)
  • Making the following code the new default tab on community & collection pages (this is what actually conflicts with this PR):
<ds-configuration-search-page [showScopeSelector]="false"></ds-configuration-search-page>
  • Adding the search component to the home page

Currently PR #2275 doesn't work correctly on communities & collections because the scope isn't given to the ds-configuration-search-page, which means that all the items are visible on all the community/collection pages. Last week I implemented this same search functionality for one of our clients and I found that giving the scope to the SearchComponent wasn't somthing that was very straightforward. The solution to fix the scope problem should simply be to add a scope @Input() to the SearchComponent in order to pass it to the SearchSidebarComponent & SearchFormComponent. This solution does work fine for SearchFormComponent, but it doesn't work for SearchSidebarComponent, because the filter suggestions & sort options look at the scope in the url (scope={uuid}) in order to define in which scope the are and which discovery.xml configuration they should use. But because the scope is actually given through the @Input() and not the url, the suggestions & searchfilters/sort options think that no scope is defined and therefore the default searchfilters & sort options are shown. Because this was a community bug/improvent I received time to work on this bug this Friday, together with another bug I found while working on that functionality related to the defaultSortField field in discovery.xml.

The bug described above should be fixed before PR #2275 is merged, so that's why I tought that it might be a better idea to seperate issue DSpace/DSpace#8426 into 2 seperate issues:

  • Adding the search to the home page configuration (I would suggest that @GauravD2t would create this PR, since he already wrote the code for that here)
  • Adding the search to the community & collection page (Because the scope currently can't be given to the search component and it's children, and that this is required to solve this ticket. I would suggest that I create that bug fix and then I can also add support for search pages with scope to this PR, since it would only require this change if I want to add support for that functionality starting from my branch (together with the new [scope]="collection.id" attribute that I will create))

@tdonohue
Copy link
Member

tdonohue commented Jan 25, 2024

@alexandrevryghem : Your suggestion makes sense that we likely need to break down the tasks in DSpace/DSpace#8426 because we have encountered major conflicts between the code in #2275 and the necessary refactors here and in #2722

I did want to point out a small correction:

  • only add Display Search Facets to the Homepage #2275 does work for community/collection pages. But, appears to only work because it modifies the behavior of collection-page.component.ts and community-page.compontent.ts to hardcode the search scope to the id of the object being viewed.
    • This approach "works". However, I agree that it's less ideal than updating the search components themselves to have a [scope] parameter. The latter is better as it allows search components to be widely reused elsewhere without having to hardcode a scope in every other component that uses them.

I'll reach out to @GauravD2t in PR #2275 and see if we can use the approach you suggested.

Copy link

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

@alexandrevryghem
Copy link
Member Author

@tdonohue: When I tested that PR I used a custom DiscoveryConfiguration for a certain scope to verify that you could configure a custom config for certain collections only. But I was able to verify that this issue was already present on DSpace 7.6 so I created issue #2793 and fixed it in a separate PR (#2794) since this might be something worth back porting to 7.6.2.

@tdonohue
Copy link
Member

@alexandrevryghem : When you get a chance, it looks like this PR could now be updated since #2722 is merged. I suspect that'd also make this much smaller in size / easier to review.

@tdonohue
Copy link
Member

@alexandrevryghem : Friendly reminder to please update/rebase this against latest main so that I can review it again. Our 8.0 feature PR merger deadline is Friday, Feb 23 & I'd love to include this. Thanks!

@alexandrevryghem
Copy link
Member Author

@tdonohue: I'll do this now, but just a heads up, this PR will be dependent on #2794

@tdonohue
Copy link
Member

@alexandrevryghem : Oh, thanks for the note! I didn't realize #2794 was required for this PR. In that case, I'll try to get #2794 reviewed today/tomorrow to help this along

@alexandrevryghem alexandrevryghem force-pushed the added-recently-added-section-to-community-page_contribute-main branch from 80fd90f to e73b493 Compare February 15, 2024 22:33
@alexandrevryghem alexandrevryghem changed the title Added recently added section to community page Added search facets to community & collection pages Feb 15, 2024
@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge label Feb 16, 2024
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 @alexandrevryghem ! At a basic level, this works well & the code looks good. However, I've come across what looks like a few bugs/usability issues. None of these seem major though.

  1. First, when using the search facets on a Community or Collection page, I'm seeing a 422 error on the backend. It's not visible on the frontend, but appears in the logs & in the Network tab of DevTools. Here's what I'm doing:

    • Install this PR with latest main backend
    • Visit a Community or Collection (doesn't matter which)
    • In the Filters, select an Author (doesn't matter who)
    • You'll see this 422 error in logs & in Network tab of DevTools when it attempts to track this search event in statistics via POST /server/api/statistics/searchevents
      Unprocessable or invalid entity (status:422)
         org.dspace.app.rest.exception.UnprocessableEntityException: Error parsing request body
            at org.dspace.app.rest.repository.SearchEventRestRepository.createSearchEvent(SearchEventRestRepository.java:48) ~[dspace-server-webapp-8.0-SNAPSHOT.jar:8.0-SNAPSHOT]
            at org.dspace.app.rest.StatisticsRestController.postSearchEvent(StatisticsRestController.java:97) ~[dspace-server-webapp-8.0-SNAPSHOT.jar:8.0-SNAPSHOT]
      
    • This error is NOT reproducible from the global search page. It only occurs from Community/Collection pages.
  2. I find it a bit odd that, by default, on a Community page the first results are all child Collections. (I have a suggestion of how to fix this in the next bullet)
    all-collections

  3. Similar to the previous point. I find it odd that in both Community/Collection searches, the default is to sort by "Most Relevant". This results in a very odd ordering of the results (almost random).

    • To solve both this point and the previous one, I'd recommend we default this Sort By to "Accessioned Date Descending". That would make the "Search" tab a recently submitted items listing by default. It would also ensure Collections do NOT appear first in the list on a Community page.

I also have a few inline comments below. None of these issues are show stoppers.

config/config.example.yml Outdated Show resolved Hide resolved
config/config.example.yml Outdated Show resolved Hide resolved
… clickedObject

Also fixed minor CSS issue in mobile mode where sidebar is still visible
…d-section-to-community-page_contribute-main

# Conflicts:
#	config/config.example.yml
#	src/app/shared/search-form/search-form.component.ts
#	src/assets/i18n/en.json5
#	src/config/default-app-config.ts
@alexandrevryghem alexandrevryghem force-pushed the added-recently-added-section-to-community-page_contribute-main branch from e73b493 to 5590ef4 Compare February 18, 2024 16:54
…te-7.6' into added-recently-added-section-to-community-page_contribute-main
@alexandrevryghem
Copy link
Member Author

@tdonohue: Thnx for the review, I implemented the feedback and wile looking at the network tab I also fixed 2 other small bugs (see last 3 points of the bug fixes section in the PR description). This PR now also requires the PR DSpace/DSpace#9348. While testing I also found out that the sidebar facets didn't work correctly in mobile mode, but this is caused by #1789.

For the searchevents bug, I fixed it so that it wil now always send the clickedObject without any query parameters (since this should be a UUID). I did however find it strange that this request was even triggered, since trackStatistics is false by default. I also added that bug fix to this PR, so if you want to test the bug again you will need to set trackStatistics to true first

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 @alexandrevryghem ! This looks great now & works well with the new backend PR (DSpace/DSpace#9348).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge component: Discovery related to discovery search or browse system high priority new feature
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Add "Recently Added Items" to Community pages
2 participants