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-3817: Support the DSpace Cover page functionality in the bitstream endpoint #1939

Merged
merged 2 commits into from Feb 16, 2018

Conversation

@YanaDePauw
Copy link

YanaDePauw commented Jan 29, 2018

https://jira.duraspace.org/browse/DS-3817

Added the DSpace 6 coverpage implementation to DSpace 7. Also added a JUnit test.
When verifying the functionality, make sure the configuration property citation-page.enable_globally is enabled.

… content endpoint
private Pair<InputStream, Long> getBitstreamInputStreamAndSize(Context context, Bitstream bit) throws SQLException, IOException, AuthorizeException {

if (citationDocumentService.isCitationEnabledForBitstream(bit, context)) {
return generateBitstreamWithCitation(context, bit);

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 1, 2018

Member

This PR was discussed in today's DSpace 7 meeting. As discussed there (with @abollini and others), we need to have a way to still download the original bitstream, even when the Cover Page feature is enabled. For example, an Administrative user needs to be able to see the original bitstream from the Admin UI, but they may want to also optionally see how the file will look with the Cover Page applied.

We haven't yet decided on the best way to implement this...if the cover page should be a separate endpoint, or maybe there's some sort of parameter that Administrative users can use to still get the original bitstream? Other thoughts welcome.

Meeting notes at: https://wiki.duraspace.org/display/DSPACE/2018-02-01+DSpace+7+Working+Group+Meeting+notes

This comment has been minimized.

Copy link
@tomdesair

tomdesair Feb 8, 2018

Contributor

This was ported from DSpace 6: https://github.com/DSpace/DSpace/blob/dspace-6.2/dspace-xmlui/src/main/java/org/dspace/app/xmlui/cocoon/BitstreamReader.java#L369

The CitationDocumentService will not add a cover page for admins: https://github.com/DSpace/DSpace/blob/dspace-6.2/dspace-api/src/main/java/org/dspace/disseminate/CitationDocumentServiceImpl.java#L240

So while I agree with the above use case, I think it is an optional nice to have. This PR complies with the DSpace 6 behaviour. So I think it can be merged.

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 9, 2018

Member

I see...looks like it was a misunderstanding around what the isCitationEnabledForBitstream() method does. It never returns true for Admin users.

So, I agree with @tomdesair that the original response here need to be rethought. We should review this as-is. This PR is in line with DSpace 6 behavior.

Copy link
Member

abollini left a comment

tested it looks good

@abollini abollini merged commit 724e4cb into DSpace:master Feb 16, 2018
14 checks passed
14 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 24.889%
Details
security/snyk - dspace-api/pom.xml No dependency changes
Details
security/snyk - dspace-oai/pom.xml No dependency changes
Details
security/snyk - dspace-rdf/pom.xml No dependency changes
Details
security/snyk - dspace-rest/pom.xml No dependency changes
Details
security/snyk - dspace-services/pom.xml No dependency changes
Details
security/snyk - dspace-solr/pom.xml No dependency changes
Details
security/snyk - dspace-spring-rest/pom.xml No dependency changes
Details
security/snyk - dspace-sword/pom.xml No dependency changes
Details
security/snyk - dspace-swordv2/pom.xml No dependency changes
Details
security/snyk - dspace/modules/pom.xml No dependency changes
Details
security/snyk - dspace/pom.xml No dependency changes
Details
security/snyk - pom.xml No dependency changes
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.