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

Added new element type: ExternalSignature #69

Merged
merged 9 commits into from
Jul 26, 2019
Merged

Conversation

jtrein
Copy link
Contributor

@jtrein jtrein commented Jul 25, 2019

From @fforbeck:

It adds a new element type called ExternalSignature, which is an Identity type with a service name used to indicate which service must sign the contract.

🚨 Dependencies:

  1. External Signature integration in core openlaw-core#165
  2. External signature integration openlaw-client#86

@jtrein jtrein requested a review from jdville03 July 25, 2019 10:31
@jtrein
Copy link
Contributor Author

jtrein commented Jul 25, 2019

I've approved this in the old PR: #68

Reposting feedback:

Looks good! I added only a few small changes to align with another branch I am working in. Just getting ahead.

Question

Is it possible to obtain the serviceName separate from the savedValue (e.g. Openlaw.getExternalServiceName(variable))? It may be nice to show the signing serviceName in the UI, especially if the user opts for a generic placeholder, like "Email". The reason I ask is that it suddenly appears when there is a savedValue, but is not present if not.

@jtrein
Copy link
Contributor Author

jtrein commented Jul 25, 2019

@jdville03 I can't approve this b/c I opened a new PR. Could you, if you have time, review? Or re-approve the changes?

Though, we don't want to merge this, until the above dependencies have been satisfied.

@jdville03
Copy link
Member

Is it possible to obtain the serviceName separate from the savedValue (e.g. Openlaw.getExternalServiceName(variable))? It may be nice to show the signing serviceName in the UI, especially if the user opts for a generic placeholder, like "Email". The reason I ask is that it suddenly appears when there is a savedValue, but is not present if not.

Yeah I agree that could be convenient to have.

Copy link
Member

@jdville03 jdville03 left a comment

Choose a reason for hiding this comment

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

This looks good to me too. We'll just need to update package.json when this is ready to merge.

Signed-off-by: Felipe Forbeck <[email protected]>
@fforbeck
Copy link

@jtrein
the serviceName is a required field for that variable type, the user has to provide a valid name. Right now we don't have this validation in place, but the idea is to check if the user added a name that matches the services that are registered into Integrators API. If the service name does not match, we would raise an error in the template editor, so we make sure it will always have a valid value to display in the UI.

@jdville03 jdville03 merged commit f457ed8 into master Jul 26, 2019
@delete-merged-branch delete-merged-branch bot deleted the ext-signature-type branch July 26, 2019 22:45
@jtrein
Copy link
Contributor Author

jtrein commented Jul 27, 2019

Thank you, @fforbeck. Makes sense. I’ll create an issue for Elements repo, so I don’t forget about that label.

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.

3 participants