-
Notifications
You must be signed in to change notification settings - Fork 0
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
Email transcript use case #34
Conversation
✅ Deploy Preview for callhub ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked out the branch and the email transcript sending works on my end! Overall, coding is clear and follows style conventions. Just one question about the api.
src/components/EndSession.jsx
Outdated
const emailRef = useRef(); | ||
const sessionCode = sessionStorage.getItem("sessionCode"); | ||
|
||
useEffect(() => emailjs.init("OScF2lHq5ESl_o9lU"), []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it’s a public API key. I followed their website, which says it’s ok to expose it? https://www.emailjs.com/docs/faq/is-it-okay-to-expose-my-public-key/
I can see why we might want to move it though. Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use environment variable for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried putting the key into an .env file, but for some reason, it shows up as undefined when I try to run the code. There might be something wrong with what I'm doing, though I searched google and it seems like someone had the same issue 2 years ago, that went unresolved: https://stackoverflow.com/questions/66491790/why-can-t-i-send-messages-with-emailjs-when-hiding-my-keys-in-env
src/components/EndSession.jsx
Outdated
const emailRef = useRef(); | ||
const sessionCode = sessionStorage.getItem("sessionCode"); | ||
|
||
useEffect(() => emailjs.init("OScF2lHq5ESl_o9lU"), []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it’s a public API key. I followed their website, which says it’s ok to expose it? https://www.emailjs.com/docs/faq/is-it-okay-to-expose-my-public-key/
I can see why we might want to move it though. Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minor comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
- Tested locally and it works as expected
- Just a minor comment on use of alert.
src/components/EndSession.jsx
Outdated
alert("Email successfully sent. Check inbox."); | ||
} catch (error) { | ||
alert("Oops! Something went wrong while trying to send the email. Please make sure there are messages in the conversation before sending."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the alerts to mui alerts to stay consistent with the ones in my pull request?
- This is the documentation: https://mui.com/material-ui/react-alert/
- You can reference my pr: Session authentication #33 to see how to make them disappear automatically after a certain amount of time so users don't need to close it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
Fixes # (issue)
Type of change
How Has This Been Tested?
Manually tested. Make sure to get the associated backend branch for code review.
Checklist: