Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

adds a couple of new constructors for Message, including New(*buffalo.App). this fixes #1296 #1338

Merged
merged 6 commits into from
Oct 1, 2018

Conversation

markbates
Copy link
Member

@markbates markbates commented Sep 30, 2018

@kteb I know you took a swing at this too. There were a things about your approach that didn't feel "right" to me.

My approach adds two new attributes to Message : Context Context and Data render.Data. When calls to functions like Message#AddBody the Message.Data and the passed in data will be merged together.

I've also add a New(c buffalo.Context) Message. We should recommend people use this function when sending emails from an action. This will add the context's data to the Message.Data. This brings along the route paths.

I'm up for discussion on this if people don't like it.

@markbates markbates requested a review from a team September 30, 2018 15:36
@codecov
Copy link

codecov bot commented Sep 30, 2018

Codecov Report

Merging #1338 into development will increase coverage by 0.37%.
The diff coverage is 92.3%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1338      +/-   ##
===============================================
+ Coverage        48.38%   48.76%   +0.37%     
===============================================
  Files               91       92       +1     
  Lines             4005     4040      +35     
===============================================
+ Hits              1938     1970      +32     
- Misses            1964     1966       +2     
- Partials           103      104       +1
Impacted Files Coverage Δ
route_mappings.go 96.89% <100%> (+0.14%) ⬆️
app.go 92.5% <100%> (ø) ⬆️
mail/mail.go 100% <100%> (ø)
mail/message.go 74% <75%> (-1.61%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39480b2...4b59538. Read the comment docs.

@kteb
Copy link
Member

kteb commented Oct 1, 2018

It looks nice. Quick question about the mutex, if I'm right, it here in case we have a process sending multiple emails, and we don't want to mix the data field between them, right ?

Shouldn't we move this package to a plugin ?

@markbates
Copy link
Member Author

markbates commented Oct 1, 2018 via email

Copy link
Member

@kteb kteb left a comment

Choose a reason for hiding this comment

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

Looks good thanks for help.

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.

2 participants