-
Notifications
You must be signed in to change notification settings - Fork 433
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
Added search facets to community & collection pages #2732
Conversation
4299ed4
to
80fd90f
Compare
@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: 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: 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. |
@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. |
@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:
<ds-configuration-search-page [showScopeSelector]="false"></ds-configuration-search-page>
Currently PR #2275 doesn't work correctly on communities & collections because the scope isn't given to the 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:
|
@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:
I'll reach out to @GauravD2t in PR #2275 and see if we can use the approach you suggested. |
Hi @alexandrevryghem, |
@tdonohue: When I tested that PR I used a custom |
@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. |
@alexandrevryghem : Friendly reminder to please update/rebase this against latest |
@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 |
…ntribute-7.6' into added-recently-added-section-to-community-page_contribute-7.6
80fd90f
to
e73b493
Compare
There was a problem hiding this 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.
-
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.
- Install this PR with latest
-
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)
-
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.
src/app/shared/comcol/sections/comcol-search-section/comcol-search-section.component.ts
Show resolved
Hide resolved
… clickedObject Also fixed minor CSS issue in mobile mode where sidebar is still visible
…y filter when none is configured
…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
…te-7.6' into added-recently-added-section-to-community-page_contribute-main
e73b493
to
5590ef4
Compare
… search event when trackStatistics is false
…te-7.6' into added-recently-added-section-to-community-page_contribute-main
@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 |
There was a problem hiding this 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).
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 newcomcol
config group.Instructions for Reviewers
List of changes in this PR:
CollectionRecentlyAddedComponent
ComcolSearchSectionComponent
, this is the tab with the search form. It is possible to hide the sidebar facets using the new configcomcol.searchSection.showSidebar
ComcolSearchSectionComponent
section as the default community/collection tabSubComColSectionComponent
section under thesubcoms-cols
route on community pages (should maybe be renamed depending on community feedback)Bug fixes:
clickedObject
's value (it contained the queryParameters)SearchComponent
'strackStatistics
still sendssearchevents
requests when false/server/api/submission/vocabularyEntryDetails/search/top?vocabulary=undefined
when the search facet has typehierarchy
(indiscovery.xml
), but you don't configure any vocabularies in youconfig.yml
fileChecklist
yarn lint
yarn check-circ-deps
)package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.