-
Notifications
You must be signed in to change notification settings - Fork 196
[WIP][GH-4359] fix wrong email notification after leaving review #4426
base: master
Are you sure you want to change the base?
Conversation
When buyer gives a review, they is receiving an email notification that is supposed to be for the seller.
@@ -127,6 +133,15 @@ const messageTemplates = { | |||
text: template( | |||
fs.readFileSync(`${templateDir}/seller-OfferFinalized.txt`).toString() | |||
) | |||
}, | |||
OfferData: { |
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.
We have OfferFinalized, OfferCreated, ...Offer[Action]. It would be nice to name this accordingly OfferReviewed
or something to that nature.
I would like an Origin team member to weigh in on this before you make any changes.
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.
OfferData
is event name from our Solidity contracts. Don't think we plan on upgrading the contracts anytime soon. So, I'd suggest to leave it as it is for now.
OfferData: { | ||
subject: template('New Review for <%= listing.title %>'), | ||
mjml: template( | ||
fs.readFileSync(`${templateDir}/buyer-OfferReview.mjml`).toString() |
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.
These files should get renamed to the seller side
fs.readFileSync(`${templateDir}/buyer-OfferReview.mjml`).toString() | |
fs.readFileSync(`${templateDir}/seller-OfferReview.mjml`).toString() |
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.
Yeah, that's reasonable. I'd suggest to change it too.
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.
Unless I'm mistaken, I don't think the template files seller-OfferReview.mjml/txt already exist?
They should be added as part of this PR.
fs.readFileSync(`${templateDir}/buyer-OfferReview.mjml`).toString() | ||
), | ||
text: template( | ||
fs.readFileSync(`${templateDir}/buyer-OfferReview.txt`).toString() |
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.
Same here
fs.readFileSync(`${templateDir}/buyer-OfferReview.txt`).toString() | |
fs.readFileSync(`${templateDir}/seller-OfferReview.txt`).toString() |
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.
Thanks for looking into this @fedealconada and @rembrandtreyes. Looks good to me. But @franckc might want to have a look too.
@@ -127,6 +133,15 @@ const messageTemplates = { | |||
text: template( | |||
fs.readFileSync(`${templateDir}/seller-OfferFinalized.txt`).toString() | |||
) | |||
}, | |||
OfferData: { |
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.
OfferData
is event name from our Solidity contracts. Don't think we plan on upgrading the contracts anytime soon. So, I'd suggest to leave it as it is for now.
OfferData: { | ||
subject: template('New Review for <%= listing.title %>'), | ||
mjml: template( | ||
fs.readFileSync(`${templateDir}/buyer-OfferReview.mjml`).toString() |
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.
Yeah, that's reasonable. I'd suggest to change it too.
Description:
Fixes 4359
NOTE I've put it in WIP as I'm not sure how I can run
marketplace.test.js
to see the test passes... Any feedback is welcome!