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

Fix for issue #44 - #62

Merged
martinytodorov merged 13 commits into
hellosign:v3from
oconnor-sn:v3
Feb 6, 2020
Merged

martinytodorov merged 13 commits into
hellosign:v3from
oconnor-sn:v3

Conversation

@oconnor-sn

Copy link
Copy Markdown
Contributor

Commit based on PR #50 that was never merged.
Added code to incorporate functionality for attachments with signer
requests

Commit based on PR hellosign#50 that was never merged.
Added code to incorporate functionality for attachments with signer
requests

@martinytodorov martinytodorov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the attachments parameter. I left a few comments, mostly typos.

Comment thread hellosign_sdk/hsclient.py Outdated
Comment thread hellosign_sdk/hsclient.py Outdated
Comment thread hellosign_sdk/hsclient.py Outdated
Comment thread hellosign_sdk/hsclient.py Outdated
Comment thread hellosign_sdk/hsclient.py Outdated
Comment thread hellosign_sdk/hsclient.py Outdated
oconnor-sn and others added 4 commits January 6, 2020 12:28
Co-Authored-By: Martin Todorov <martin.todorov@hellosign.com>
Co-Authored-By: Martin Todorov <martin.todorov@hellosign.com>
Co-Authored-By: Martin Todorov <martin.todorov@hellosign.com>
Co-Authored-By: Martin Todorov <martin.todorov@hellosign.com>
@oconnor-sn

Copy link
Copy Markdown
Contributor Author

@martinytodorov - please verify updates.

Commit based on PR hellosign#50 that was never merged.
Added code to incorporate functionality for attachments with signer
requests
# Conflicts:
#	hellosign_sdk/hsclient.py
@oconnor-sn

Copy link
Copy Markdown
Contributor Author

Amended the commit and re-pushed @martinytodorov

Comment thread hsclient.py Outdated
@@ -0,0 +1,2441 @@
from hellosign_sdk.utils import HSRequest, HSException, NoAuthMethod, HSAccessTokenAuth, HSFormat, api_resource, api_resource_list

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@oconnor-sn I think you accidentally added this file. I don't think it should be part of this PR. Can you please remove it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@martinytodorov - Thanks Martin - changes were made to hsclient.py which is why I added this file - please review and advise further.

Add in error
Comment thread hellosign_sdk/hsclient.py Outdated
Comment thread hellosign_sdk/hsclient.py Outdated
oconnor-sn and others added 2 commits January 14, 2020 17:37
Co-Authored-By: Martin Todorov <martin.todorov@hellosign.com>
Co-Authored-By: Martin Todorov <martin.todorov@hellosign.com>
Comment thread hellosign_sdk/hsclient.py Outdated

is_for_embedded_signing (bool): send_signature_request and send_signature_request_embedded share the same sending logic. To differenciate the two calls embedded requests are now flagged.

attachments (list of dict): A list of attachments, each with the following attributes::

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
attachments (list of dict): A list of attachments, each with the following attributes::
attachments (list of dict): A list of attachments, each with the following attributes:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry I must have added an additional column.

Comment thread hellosign_sdk/hsclient.py Outdated

signing_options (dict, optional): Allows the requester to specify the types allowed for creating a signature. Defaults to account settings.

attachments (list of dict): A list of attachments, each with the following attributes::

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
attachments (list of dict): A list of attachments, each with the following attributes::
attachments (list of dict): A list of attachments, each with the following attributes:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry I must have added an additional column.

@martinytodorov

Copy link
Copy Markdown
Contributor

Hey @oconnor-sn I checked out the branch on my machine and made a few tests. I realized that there are couple of more things that need to be fixed.

  1. You added the attachments to the function _send_signature_request_with_template. You should remove it from there.
  2. Although you added attachments to _send_signature_request, you are not using them in the payload for the request.

Here's some tips you should follow when opening the PRs:

  1. Always test you changes. If you are adding attachments to signature_request/send and signature_request/create_embedded, then you should use the SDK and make test requests to make sure that your changes are working. Here's my test request if you need one.
  2. Use an IDE that can highlight problems for you. That first problem mentioned above is something that my IDE caught. I am using VSCode.
    image

Let me know if you have any questions.

Commit based on PR hellosign#50 that was never merged.
Added code to incorporate functionality for attachments with signer
requests
Commit based on PR hellosign#50 that was never merged.
Added code to incorporate functionality for attachments with signer
requests
@oconnor-sn

Copy link
Copy Markdown
Contributor Author

@martinytodorov - anything outstanding on this PR?

@martinytodorov martinytodorov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good.

Comment thread hellosign_sdk/hsclient.py
if (merge_fields is not None):
data.update(merge_fields_payload)
data = HSFormat.strip_none_values(data)
data.update(attachments_payload)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we move this line with the rest of the updates above.

@martinytodorov
martinytodorov merged commit c4103a7 into hellosign:v3 Feb 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants