Skip to content
This repository has been archived by the owner on Nov 20, 2018. It is now read-only.

Allow custom error message in S3 signature response #1724

Merged

Conversation

mrdziuban
Copy link
Contributor

Brief description of the changes [REQUIRED]

This adds logic to the S3 request signer to use the error property on the response (if set) as the error message displayed to the user. If the error property is not set, the logic will work the same way it currently does.

What browsers and operating systems have you tested these changes on? [REQUIRED]

On Mac OS X 10.11.6:

  • Chrome 55.0.2883.95
  • Firefox 50.1.0
  • Safari 9.1.2

On Windows 7:

  • Internet Explorer 10
  • Internet Explorer 11

On Windows 10:

  • Edge 38.14393.0.0

Are all automated tests passing? [REQUIRED]

Yes

Is this pull request against develop or some other non-master branch? [REQUIRED]

Yes

@CLAassistant
Copy link

CLAassistant commented Jan 20, 2017

CLA assistant check
All committers have signed the CLA.

@mrdziuban
Copy link
Contributor Author

Not sure why tests failed on Travis, but they passed when I ran them locally.

@rnicholus
Copy link
Member

rnicholus commented Jan 20, 2017

Not sure what happened - re-running the build results in green.

This looks good, but two additional requests, if you could:

  • Write a unit test to cover this new behavior
  • Update the documentation to let future integrators know of this possibility

@mrdziuban
Copy link
Contributor Author

@rnicholus thanks, I updated the documentation. I would be happy to add a unit test, but I looked into the existing S3 tests and wasn't able to find a way to tap into the S3 request signer from the test. Let me know if there's an example of this, or what you think the best way to proceed is.

@rnicholus
Copy link
Member

Thanks for the doc update. There are some existing integration tests for the S3 uploader. One tests a number of different failure scenarios. Perhaps you could copy that logic and simplify it a bit to form a new test that focuses on ensuring this results in a failure of the upload.

I realize this does not test for the presence of the custom message after the failure. Another (possibly better?) option would be to test the handleSignatureReceived method in isolation. If you new qq.s3.RequestSigner({...}), you should be able to access the handleSignatureReceived implementation through the onComplete property on the constructed instance. You could then perhaps mock the parameters as appropriate, and test for the rejected promise value.

@mrdziuban
Copy link
Contributor Author

Thanks again @rnicholus. I was able to add some tests by specifying the onError callback to the uploader.

@rnicholus
Copy link
Member

Awesome, this looks good. Thanks for your work. I'll release this as 5.13.0 sometime soon, possibly even this weekend.

@rnicholus rnicholus added this to the 5.13.0 milestone Jan 28, 2017
@rnicholus rnicholus merged commit a513777 into FineUploader:develop Jan 28, 2017
@rnicholus
Copy link
Member

Just released as 5.13.0.

@mrdziuban
Copy link
Contributor Author

Thanks!

@mrdziuban mrdziuban deleted the custom-s3-signature-error-msg branch January 28, 2017 15:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants