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-3511 Fix HTTP 500 errors on REST API bitstream updates #1840

Closed
wants to merge 1 commit into from

Conversation

minurmin
Copy link
Contributor

https://jira.duraspace.org/browse/DS-3511 and
https://groups.google.com/forum/#!topic/dspace-tech/ZEvYkjqbx8s

When trying to update the content of a bitstream or delete a bitstream policy HTTP 500 error occurs (Dspace 6.0-6.2). Fixed by adding context.complete() to the end of the respective methods (adapted the original patch submitted to DS-3511).

@tdonohue tdonohue added bug quick win Pull request is small in size & should be easy to review and/or merge labels Oct 5, 2017
@tdonohue tdonohue added this to the 6.3 milestone Oct 5, 2017
@tdonohue
Copy link
Member

Please rebase this PR against the latest master branch. The master branch now requires adherence to our new Java Code Style documented at: https://wiki.duraspace.org/display/DSPACE/Code+Style+Guide

If you have any questions, feel free to ask in this PR or on our Slack: https://wiki.duraspace.org/display/DSPACE/Slack

@terrywbrady
Copy link
Contributor

@minurmin , thank you for submitting this PR. I believe your intention was to apply this fix to DSpace 6.x. If that is correct, could you update the base branch to dspace-6_x? That will remove the error that is currently in place and will allow a review to take place.

@AndrewBennet
Copy link
Contributor

I've cherry-picked this commit onto dspace-6_x and created another pull request: #1996

…treamPolicy to prevent HTTP 500 response on REST updates
@minurmin minurmin changed the base branch from master to dspace-6_x March 23, 2018 22:34
@minurmin minurmin changed the base branch from dspace-6_x to master March 23, 2018 22:35
@minurmin minurmin changed the base branch from master to dspace-6_x March 23, 2018 22:43
@minurmin
Copy link
Contributor Author

minurmin commented Mar 23, 2018

Yes, the PR is supposed to be used for Dspace 6. Sorry to have missed the comments, originally I wasn't sure what branch to merge into since the code apparently works on master branch as well. For what it's worth, this PR should now be mergeable with current 6.x branch as well (fixed by force push).

Copy link
Member

@kshepherd kshepherd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
Thanks @minurmin for the fix, and thanks @AndrewBennet for the cherrypick to 6.x
@minurmin , i think we'll still want another rebased PR for master (as per @tdonohue 's comments)

@tdonohue
Copy link
Member

The code changes look fine to me, however, Travis CI seems to having issues with the PR. But, the alternative/cherry-picked PR (#1996). Seems to be perfectly fine. Should we be concentrating on reviewing/merging #1996?

@minurmin
Copy link
Contributor Author

By all means, please proceed with #1996. Perhaps the force push caused some problems, I have no idea what could cause the problem considering that this is just a 2-line change.

@tdonohue
Copy link
Member

I'll give my general thumbs up here, 👍 , code looks fine.. I suspect the Travis issues in this PR are actually a Travis hiccup, and no fault of the PR itself. As noted, the same code in #1996 seems to be 100% fine in Travis. I'll leave it up to @kshepherd which one of these to merge...but one should get merged and the other closed.

@minurmin
Copy link
Contributor Author

minurmin commented Mar 27, 2018

I suggest to close this one and use #1996 for 6.3. I have submitted another PR #2001 rebased to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug quick win Pull request is small in size & should be easy to review and/or merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants