-
Notifications
You must be signed in to change notification settings - Fork 512
browse pages should not ignore sort config from back end #3741
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
browse pages should not ignore sort config from back end #3741
Conversation
…t call so default sort option isn't ignored
…ages-ignore-sort-config-fix-UI-main
|
Hi @jensvannerum, |
|
Set the title as desc in dspace.cfg when browse is opened it is ordered correctly in a a descending order. The other browses seem to be working fine too. DSpace.Repository.__.Community.List_converted.mp4 |
|
Dear @jensvannerum |
|
Hi @jensvannerum. This PR looks good, and I'd like to review it. Could you resolve the merge conflicts? |
3ccfabc to
789c614
Compare
nwoodward
left a comment
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 @jensvannerum. With this PR I am able to change the sort direction of the browse indices and see it reflected in the pagination options.
tdonohue
left a comment
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.
@jensvannerum : I gave this a quick test today to try to move it along (as it's at +1). While this code works, when I watch the requests sent to the backend (REST API) I'm now seeing two requests.
For example, I've set this in my dspace.cfg:
webui.browse.index.1 = dateissued:item:dateissued:desc
Now, if I open up my DevTools and go to "Browse by Issue Date", I'll see two requests sent to the REST API:
GET /api/discover/browses/dateissued/items?page=0&size=1&sort=default,ASC
GET /api/discover/browses/dateissued/items?page=0&size=1&sort=default,DESC
Notice how the first request is still wrong. It's sending an ASC request when I've configured it for DESC. But, it's immediately followed by a DESC request which ultimately returns the correct search (and this is what is displayed in the User Interface).
Would it be possible to get rid of the first request here? Overall, this code works, but it seems wasteful to send two requests when we need just one.
|
Hi @jensvannerum, |
|
@jensvannerum : Any chance that you'll find time to get back to this PR at some point to solve the outstanding feedback? There are new comments also on the issue ticket #8651 asking for a status update as well. If you are unable to move this forward, we could also search out a new volunteer. Thanks! |
…es-ignore-sort-config-fix-UI-main
|
Hi @tdonohue, I had considered this PR finished so I missed this feedback, sorry about that. I think the PR does fix the linked issue, the extra you call mention seeing is expected, it's from the data picker component getting the first (asc) and last (desc) date to show the correct ranges of dates, you'll see this doesn't happen on other (non date) browse indices. The same behavior is also visible on the demo instance |
232f184 to
a9a9e97
Compare
tdonohue
left a comment
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 @jensvannerum for revisiting this and explaining the behavior. On retesting, I see that you're correct and this behavior is the same as current. I can verify this PR fixes the bug as described. Looks good to me!
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin dspace-7_x
git worktree add -d .worktree/backport-3741-to-dspace-7_x origin/dspace-7_x
cd .worktree/backport-3741-to-dspace-7_x
git switch --create backport-3741-to-dspace-7_x
git cherry-pick -x 86c30e99b6cdefc9df7c9da46ddbad3e17ef0911 2d64c1cd58a5aead6cc880e232b42b7f16639adc 5b0e7c112b96da23fd06bbc4485db3cc9d40b3c0 f779bbe813e092f4c9ebd7a965f0ae410bbf4391 789c614556c23dcca39f6a53e31d5def83c8fe77 |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin dspace-8_x
git worktree add -d .worktree/backport-3741-to-dspace-8_x origin/dspace-8_x
cd .worktree/backport-3741-to-dspace-8_x
git switch --create backport-3741-to-dspace-8_x
git cherry-pick -x 86c30e99b6cdefc9df7c9da46ddbad3e17ef0911 2d64c1cd58a5aead6cc880e232b42b7f16639adc 5b0e7c112b96da23fd06bbc4485db3cc9d40b3c0 f779bbe813e092f4c9ebd7a965f0ae410bbf4391 789c614556c23dcca39f6a53e31d5def83c8fe77 |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin dspace-9_x
git worktree add -d .worktree/backport-3741-to-dspace-9_x origin/dspace-9_x
cd .worktree/backport-3741-to-dspace-9_x
git switch --create backport-3741-to-dspace-9_x
git cherry-pick -x 86c30e99b6cdefc9df7c9da46ddbad3e17ef0911 2d64c1cd58a5aead6cc880e232b42b7f16639adc 5b0e7c112b96da23fd06bbc4485db3cc9d40b3c0 f779bbe813e092f4c9ebd7a965f0ae410bbf4391 789c614556c23dcca39f6a53e31d5def83c8fe77 |
|
@jensvannerum : It looks like all the backports failed here, and I see it's because of changes on |
References
Description
Introduces a new service method to obtain the configured sort direction of a browse index which is then used in the relevant browse components to do the initial fetch. Manually changing sort direction on page will override the configuration of course.
Instructions for Reviewers
In your
dspace.cfgchange some of the default browse indexes by adding a custom sort order to them ("adding:descat the end as they currently default to asc)Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.