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

DS-3904 - Add support for startsWith parameters in the browse endpoint #2173

Merged
merged 14 commits into from Dec 12, 2018

Conversation

Projects
None yet
4 participants
@ppmdo
Copy link
Contributor

commented Aug 16, 2018

Support for startsWith implemented for Items and Entries under the Browse endpoint.

Integration Tests pending.

@abollini

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

@ppmdo please ping me when the PR is ready to be reviewed. The code changes seem obvious so we only need additional integration tests to approve it. A PR to update the REST contract will be appreciated as well

@ppmdo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

@abollini Integration tests written.

Pablo Prieto added some commits Aug 17, 2018

Pablo Prieto
Checkstyle errors fixed
Added ITs for startsWith + Scope
Pablo Prieto

@tdonohue tdonohue changed the title Ds 3904 - Add support for startsWith parameters in the browse endpoint DS-3904 - Add support for startsWith parameters in the browse endpoint Aug 22, 2018

@abollini

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

This PR has been discussed in the 23rd Aug DSpace 7 meeting https://wiki.duraspace.org/display/DSPACE/2018-08-23+DSpace+7+Working+Group+Meeting @ppmdo has kindly accepted to further investigate about the use of the different Browse methods in the existent UIs

@ppmdo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

@abollini After further inspection I've found that the problem may reside with the paging implementation and how it relates to browse results., not with the methods of the browse repositories.

Here's what I've found:
https://wiki.duraspace.org/pages/viewpage.action?pageId=104562967

If this is indeed related to paging. I don't know if fixing that should be part of another PR.

@tomdesair

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2018

@ppmdo The Browse endpoint works via the generic RestResourceController which does not take into account any query parameters when creating the pagination links. You'll need to extend that class to also re-use any existing query parameters.

To help you get started, I think you need to modify the following line

Link link = linkTo(this.getClass(), apiCategory, English.plural(model)).slash(id)
with

                UriComponentsBuilder uriComponentsBuilder = linkTo(this.getClass(), apiCategory, English.plural(model)).slash(id)
                        .slash(rel).toUriComponentsBuilder();
                
                //TODO we need to strip any existing paging information from the query string
                uriComponentsBuilder.query(request.getQueryString());
                
                Link link = new Link(Link.REL_SELF, uriComponentsBuilder.toUriString());

But as the TODO mentions, it's important that we don't re-use the pagination query parameters because then they will be added twice.

@ppmdo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

@tomdesair Thanks for helping me out. I will try your suggestion and report back.

@ppmdo ppmdo closed this Oct 5, 2018

@ppmdo ppmdo reopened this Oct 5, 2018

ppmdo added some commits Oct 5, 2018

@ppmdo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2018

Should be ready to merge.

@tdonohue
Copy link
Member

left a comment

👍 The code here looks good to me. I admit, I haven't had a chance to test it, but it has its own integration tests that also look good.

@abollini
Copy link
Member

left a comment

can you also add a test to document the expected behavior when both the startsWith and the page parameter are present? which one has the precedence? is this a valid request or will be refused?
I'm fine with both solutions so I just recommend to document the implemented behavior

Pablo Prieto added some commits Nov 14, 2018

@ppmdo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

@abollini Fixed the empty or null querystring handling, separated item and entry browses tests and added a new test to verify when both startsWith and Page parameters are submitted by the user.

@abollini abollini merged commit 7ccd99c into DSpace:master Dec 12, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 29.097%
Details
@abollini

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

code looks good and it comes with the required ITs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.