Skip to content
This repository has been archived by the owner on Oct 17, 2023. It is now read-only.

Fix improper use of Requests for file upload #35

Closed
wants to merge 1 commit into from

Conversation

jeteon
Copy link

@jeteon jeteon commented Jan 17, 2018

The data structure prepared by format_file_params is supposed to be a list of tuples with a certain structure, not the dict one created currently. See #34 for details of the issue being addressed here.

The data structure prepared by `format_file_params` is supposed to be a list of tuples with a certain structure, not the dict one created currently. See hellosign#34 for details of the issue being addressed here.
@alexmac05
Copy link
Contributor

Hi Jeteon,

Your change passed all of the unit tests, but I'm having trouble making a test that fails on the main branch and passes on your branch.

Here is what I have tried:

response = client.send_signature_request(
test_mode=True,
title="Paperwork with Acme Co.",
subject="we talked about",
message="Please sign these",
signers=[
{'email_address': 'email@heplakd.com', 'name': 'Jack', 'order': 0},
{'email_address': 'jerryExample@yahoo.com', 'name': 'Jill', 'order': 1}
],
cc_email_addresses=['lawyer@hellosign.com', 'lawyer@example.com'],
files=['NDA.pdf', 'code.pdf', 'tomato.pdf', '200.txt'],
metadata={
'client_id': '1234',
'custom_text': 'NDA #9'
}
)

However, on both branches I get the files in this order:
files=['NDA.pdf', 'code.pdf', 'tomato.pdf', '200.txt'],

  1. code.pdf
  2. tomato.pdf
  3. 200.txt
  4. NDA.pdf

I seem to get that order each time on both branches.

What am I missing as far as creating a useful test? Thank you.

@jeteon
Copy link
Author

jeteon commented Jan 21, 2018

Where are you checking for that list? Would you mind adding your current version of the test to the PR? It would seem that both times the order doesn't match the order specified in the requesting code.

I admit I should have put more time into writing a test for the repo. I only got so far as testing that it does what I needed in my application.

@jeteon
Copy link
Author

jeteon commented Jan 21, 2018

I don't yet see a functional test that includes more than one file in the main branch. Am I missing something?

@alexmac05
Copy link
Contributor

I didn't add it as a test because I am still trying to understand the issue you are reporting. I need to understand what you consider success and failure for your pull request, then I can make the test.

I ran this test manually, outside of the test suite:
response = client.send_signature_request(
test_mode=True,
title="Paperwork with Acme Co.",
subject="we talked about",
message="Please sign these",
signers=[
{'email_address': 'email@heplakd.com', 'name': 'Jack', 'order': 0},
{'email_address': 'jerryExample@yahoo.com', 'name': 'Jill', 'order': 1}
],
cc_email_addresses=['lawyer@hellosign.com', 'lawyer@example.com'],
files=['NDA.pdf', 'code.pdf', 'tomato.pdf', '200.txt'],
metadata={
'client_id': '1234',
'custom_text': 'NDA #9'
}
)

However, on both branches I get the files in this order:
files=['NDA.pdf', 'code.pdf', 'tomato.pdf', '200.txt'],

code.pdf
tomato.pdf
200.txt
NDA.pdf
I seem to get that order each time on both branches.

Thank you for taking the time to troubleshoot this and to clarify my questions.

@alexmac05
Copy link
Contributor

This merge request is failing this test:

class TestHSFormat(TestCase):
''' Testing for formatting utilities used in this wrapper '''

def test_format_file_params(self):

I'm looking into it.

@jeteon
Copy link
Author

jeteon commented Jan 29, 2018

Sorry I haven't been more helpful with this up to now. I'll try to take a closer look too and possibly add a test that exposes the original issue in a reproducible way.

@alexmac05
Copy link
Contributor

Hi Neo,

Running the tests are sort of hard because I have updated them after this pull request and also I added decline to sign. If you send me an email at apisupport@hellosign.com, I can zip those files up and send them to you. Also, maybe we could do a quick call or work on it together with a screenshare.

The tests currently require that you have a production account with a test template and the tests do destructive things, so it might not make sense for you to run the tests on your production account. I'm looking into changing this about the tests but currently am trying to prioritize to existing pull requests and reported issues.

@jtreminio-dropbox jtreminio-dropbox added the legacy Related to legacy, non-OpenAPI SDK label Apr 5, 2022
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jtreminio-dropbox
Copy link
Contributor

Please use the new OpenAPI SDK: https://pypi.org/project/dropbox-sign/

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
legacy Related to legacy, non-OpenAPI SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants