-
Notifications
You must be signed in to change notification settings - Fork 195
Conversation
Temporary for prod until we get styled messages done.
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.
One question inline otherwise LGTM...
infra/notifications/src/emailSend.js
Outdated
@@ -85,6 +85,16 @@ async function emailSend( | |||
logger.info(`No message found`) | |||
} | |||
} else { | |||
const vars = { | |||
dappUrl: | |||
listing.id[0] === '4' |
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.
This works only for staging and mainnet. Do we care about local and dev environments ? Might be nice for testing purposes to support those as well ?
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.
Ah yes, just added support for those. However, long term I feel that logic needs to get moved into the dapp itself: the network number is baked into the listing ID, so the dapp should do the right logic of getting the user to the right subdomain to display it.
Likewise, the only reason we need custom logic on the ipfs gateway is because federating is currently blocked on staging. Once that is fixed we can always have the emails just point to our main gateway.
logger.log(config) | ||
|
||
// ------------------------------------------------------------------ | ||
|
||
// should be tightened up for security | ||
// TODO: Should be tightened up for security |
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.
When deployed this is all handled by nginx, so this should only be applied in development, i.e. NODE_ENV === 'development'
Note: I feel this logic should actually live dapp-side, as the network number is right there in the listing id. We should work towards that.
listing.id[0] === '4' | ||
? 'https://dapp.staging.originprotocol.com' | ||
: 'https://dapp.originprotocol.com', | ||
`https://dapp.${networkSubDomains[listingNetwork]}originprotocol.com`, |
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.
for network 999, I don't think https://dapp.localhost.originprotocol.com will resolve to local dapp ? I think we use http://localhost on local...
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.
Ah crap you are right! Bit the bullet and have everything laid out explicitly.
Closing in favor of #2103 which has real styled templates per aure. |
This adds links to the outgoing notification emails, taking user to the offer/purchase the email is about.
Turns off mobile and browser push for now.
This is temporary minimal messaging template until I'm done with the aure-styled templates.