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

feat: Improve e-sign request flow and email logic #285

Conversation

Raju-kadel-27
Copy link
Contributor

PR for issue #273

Copy link

github-actions bot commented May 1, 2024

Thank you for following the naming conventions for pull request titles! 🙏

@Raju-kadel-27 Raju-kadel-27 marked this pull request as ready for review May 2, 2024 08:24
@Raju-kadel-27
Copy link
Contributor Author

@dahal I think it's ready for review.

Copy link
Collaborator

@G3root G3root left a comment

Choose a reason for hiding this comment

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

Great work as always ❤️ 🚀 🚀 .

Here are some of my feedback and suggestions:

  1. Save as a draft is not working.
  2. optional message should be saved in the template model because ordered signing needs those values.
  3. Maybe we can have an input for a custom email subject as well. We can save those optional values in the template model like "optional message" => emailBody and emailSubject. cc @dahal.
  4. @dahal, should we allow optional message to be added to the draft as well? If so, rather than rendering a dropdown for Save & Continue, we can render a Popover that has all the inputs above and provide the user the option to complete or save as a draft.

emailPayload: z.object({
optionalMessage: z.string().optional(),
expiryDate: z.date().or(z.null()).or(z.string()).optional(),
documentName: z.string(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

documentName,companyName, completedOn and companyLogo should be queried from the backend no need to pass it on from the frontend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@G3root Sure ! Thanks for the feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

documentName,companyName, completedOn and companyLogo should be queried from the backend no need to pass it on from the frontend

I second that.

const closeModal = watch("closeModal");

// close modal is triggered from template-field-provider
useEffect(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This useEffect should be avoided, and we don't need closeModal. This would lead to some unexpected bugs and boilerplate code because we are syncing two states through a useEffect. a form submit would close the modal.

recipient: z.string(),
recipientColors: z.record(z.string()),
emailPayload: z.object({
optionalMessage: z.string().optional(),
expiryDate: z.date().or(z.null()).or(z.string()).optional(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

how are we handling expiryDate ? is it being used in the backend ?

Copy link
Contributor Author

@Raju-kadel-27 Raju-kadel-27 May 3, 2024

Choose a reason for hiding this comment

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

Currently we are just displaying it in email body only.
Do not have a clear idea. (Might be just for the information purpose)

Copy link
Contributor

Choose a reason for hiding this comment

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

We will have the expiry on second iteration, lets remove for now

Copy link
Contributor

@dahal dahal left a comment

Choose a reason for hiding this comment

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

Looking good, please go through the reviews and make necessary changes.

package.json Outdated
@@ -130,6 +130,6 @@
"initVersion": "7.25.1"
},
"trigger.dev": {
"endpointId": "captable-A0-I"
"endpointId": "opencap-WhPZ"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to hardcode this? Each contributors and env will have different ids

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, i will fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have /public/placeholders/user.svg, please remove this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this, and if we need re-usable icon, create one on icons.tsx

name: payload.company.name,
logo:
payload.company.logo ??
"https://avatars.githubusercontent.com/u/163377635?s=48&v=4",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this hardcoded?

<Column align="left">
<Img
className="rounded-full"
src={"https://www.svgrepo.com/show/437226/signature.svg"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please dont hardcode these

href={signingLink}
>
Sign document
Esign the template
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Esign the template
Sign the document

export default EsignEmail;
<Hr className="mx-0 my-[26px] w-full border border-solid border-neutral-200" />
<Text className="mx-auto text-center text-[12px] leading-[24px] text-[#666666]">
If you were not expecting this email, you can ignore this email.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If you were not expecting this email, you can ignore this email.
Please ignore, if you were not expecting this email.

return hour > 12;
}

export function getSanitizedDateTime(datetime: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use @/common/dayjs to sanitize these dates and timestamps

recipient: z.string(),
recipientColors: z.record(z.string()),
emailPayload: z.object({
optionalMessage: z.string().optional(),
expiryDate: z.date().or(z.null()).or(z.string()).optional(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We will have the expiry on second iteration, lets remove for now

return {
success: false,
message: "Email requires sender name , email and company logo.",
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not make company logo a requirement to eSign. We have a placeholder we can use if company logo is not available.

/public/placeholders/company.svg

@dahal
Copy link
Contributor

dahal commented May 3, 2024

Here are my thoughts

  1. optional message should be saved in the template model because ordered signing needs those values.

Yes, optional message should be on template model

  1. Maybe we can have an input for a custom email subject as well. We can save those optional values in the template model like "optional message" => emailBody and emailSubject. cc @dahal.

For now, we can just limit to message and we can add subject, expiry and other optional fields on second iteration.

  1. @dahal, should we allow optional message to be added to the draft as well? If so, rather than rendering a dropdown for Save & Continue, we can render a Popover that has all the inputs above and provide the user the option to complete or save as a draft.

We dont need to allow optional message on draft. Regarding popover, i have been experimenting it on Data room, i kind of like it. But for this use case, since modal is already implemented, lets limit message to Send for signature button.

@Raju-kadel-27 Raju-kadel-27 force-pushed the feat/improve-esign-request-flow-and-email branch from aa0a2a3 to 23f0156 Compare May 4, 2024 05:54
@Raju-kadel-27
Copy link
Contributor Author

@dahal @G3root Fixed it.

@dahal dahal force-pushed the feat/improve-esign-request-flow-and-email branch from 24a3b2e to 971e083 Compare May 4, 2024 08:38
@Raju-kadel-27 Raju-kadel-27 deleted the feat/improve-esign-request-flow-and-email branch May 4, 2024 15:58
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