Skip to content
This repository has been archived by the owner on Sep 20, 2023. It is now read-only.

Stan/simple notifications #2101

Closed
wants to merge 5 commits into from
Closed

Conversation

wanderingstan
Copy link
Contributor

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.

@wanderingstan wanderingstan requested review from nick and franckc April 22, 2019 20:11
Copy link
Contributor

@franckc franckc left a 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...

@@ -85,6 +85,16 @@ async function emailSend(
logger.info(`No message found`)
}
} else {
const vars = {
dappUrl:
listing.id[0] === '4'
Copy link
Contributor

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 ?

Copy link
Contributor Author

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
Copy link
Contributor

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`,
Copy link
Contributor

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...

Copy link
Contributor Author

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.

@wanderingstan
Copy link
Contributor Author

wanderingstan commented Apr 23, 2019

Closing in favor of #2103 which has real styled templates per aure.

@nick nick deleted the stan/simple-notifications branch June 6, 2019 15:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants