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 scope @Input() to the SearchComponent & fixed related bugs #2794

Conversation

alexandrevryghem
Copy link
Member

References

Description

I added an @Input() scope to the SearchComponent in order to easily be able to defined change the default scope without having to defined it in the url as a query parameter. This PR also fixes a bug caused by a timing issue with the currentScope$ causing invalid request to be sent to the backend when you changed scope that use a different DiscoveryConfiguration. This PR also fixes an issue where the scope was ignored when retrieving the sort options.

Instructions for Reviewers

List of changes in this PR:

  • Added the @Input() scope: string to the SearchComponent and passed it down to the SearchFilterComponent & SearchFacetFilterComponent in order to retrieve the facets with the correct scope.
  • Refactored the way the the currentScope$ is used in order to prevent invalid request to be send when you switch between 2 scopes who have different DiscoveryConfiguration searchFilters.
  • Passed the scope to the searchConfigService#getConfigurationSearchConfig in order to retrieve the correct sort options

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 Feb 3, 2024
@alexandrevryghem alexandrevryghem added bug component: Discovery related to discovery search or browse system claimed: Atmire Atmire team is working on this issue & will contribute back labels Feb 3, 2024
@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge label Feb 5, 2024
@tdonohue tdonohue added this to the 8.0 milestone Feb 5, 2024
@tdonohue tdonohue added the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label Feb 5, 2024
@alanorth
Copy link
Contributor

alanorth commented Feb 6, 2024

@alexandrevryghem would this also fix #2555? I tried to add a ds-themed-search-form on my community and collection pages but the results were always from the entire repository.

@alexandrevryghem
Copy link
Member Author

@alanorth: Yes this will fix that issue I will also add the ds-themed-search-form on the community & collection pages in PR #2732. If you still see results from the whole repository it probably means that you forgot to add [scope]="collection.id" to the ds-themed-search-form tag on your collection page.

@alanorth
Copy link
Contributor

alanorth commented Feb 6, 2024

@alexandrevryghem thanks, apologies as I don't want to derail the conversation.

I had tried the custom ds-themed-search-form some weeks ago before this PR. Now I tried again with [scope]="collection.id" but it doesn't seem to work: the scope is added as a query parameter but the results return immediately and contain all items in the collection. URL with query becomes:

http://localhost:4000/collections/5990226f-7268-4097-a39b-eb2da4d2f367?spc.page=1&query=energy&scope=5990226f-7268-4097-a39b-eb2da4d2f367

@alexandrevryghem
Copy link
Member Author

@alanorth: Sry I just realised that you said ds-themed-search-form and not ds-themed-search 😅, this fix is for the ds-themed-search component. I made an example on how this fix works for collection pages: alexandrevryghem@683c279

@alanorth
Copy link
Contributor

alanorth commented Feb 7, 2024

Thanks @alexandrevryghem. I verified your example. It is indeed working with ds-themed-search on collection pages on DSpace 7.6.2-next. I was hoping there would be a similar fix for the scope with ds-themed-search-form too because it's more simple than the full search with filters.

@alexandrevryghem
Copy link
Member Author

@alanorth: That's because the recent submission section on collection pages ignores those url parameters, but this component will be replaced with the search component, so I don't think it's worth it to add that functionality for dspace-7.6.2 only. But if you want to hide the search facets & settings on your community/collection pages you should be able to achieve this by adding [showSidebar]="false" on the ds-themed-search-form tag.

@tdonohue tdonohue self-requested a review February 15, 2024 18:48
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 ! I tested this today and verified that both bugs are fixed. The community/collection scope is now taken into account in the facets & sort options. I couldn't find any side effects to this change, so I'm merging it immediately & auto-porting to 7.x

@tdonohue tdonohue merged commit 4c3d374 into DSpace:main Feb 15, 2024
13 checks passed
@dspace-bot
Copy link
Contributor

Successfully created backport PR for dspace-7_x:

@alexandrevryghem alexandrevryghem removed the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label Apr 19, 2024
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 bug claimed: Atmire Atmire team is working on this issue & will contribute back component: Discovery related to discovery search or browse system
Projects
No open projects
Status: ✅ Done
4 participants