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

post recipient view lookup logic #103

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

smcoll
Copy link
Contributor

@smcoll smcoll commented Feb 9, 2017

This will use clientUserId+userId to lookup a recipient if userId is available, else clientUserId+email+userName. It also removes envelopeId from the lookup kwargs because i believe that value is ignored, and it is already used in the url endpoint path anyway.

fixes #102

@zebuline
Copy link
Contributor

Nice !
Is it possible to add some tests @smcoll ?
For example, you could mock the post method and check the params.

for key in ('email', 'userName', 'userId'):
# ignore empty values; they won't help with a lookup
if locals()[key]:
data[key] = locals()[key]
Copy link

@ewjoachim ewjoachim Feb 13, 2017

Choose a reason for hiding this comment

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

Using locals() here might seem a bit overkill, but...

Other ways to achive the same result:

  • Manually write a dict - but it means one has to write both keys and values and it's a bit ugly (beautiful is better than ugly)
user_info = [("userName", userName), ("userId", userId), ("email", email)]
data.update({key: value for key, value in user_info if value})
  • Change these 3 values to **kwargs in the function call (as they're not used elsewhere) - but that's breaking the API (is it a public facing API ?).

Using locals is not problematic at all, but it's less "straightfoward code". Thing is, we're using local to help us change a kinda ugly 1 line simple code into a purer 4 line more complex code. Maybe it could be a case of "practicality beats purity"...

EDIT: After reading both your code and the snippet above, I'm not even convinced that mine would be any better actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ewjoachim yeah, i don't love using locals but also wasn't satisfied with any other option i could think of which would avoid changing the function signature...

@smcoll
Copy link
Contributor Author

smcoll commented Feb 13, 2017

@zebuline i can put tests on my list, but i probably won't get to them soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DocuSignException in post_recipient_view when recipient's name change is not reflected in database
3 participants