Skip to content
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

Get Action Mailer guide ready for prime time #10103

Conversation

senny
Copy link
Member

@senny senny commented Apr 5, 2013

I noticed that the Action Mailer guide still has a WIP tag. I find the information provided in the guide very useful and also complete. As it is hidden in the navigation it is very easily overlooked. As I find the guide very useful I think we should no longer hold it back.

I did some minor edits:

  • removed testing from the index as it is covered in the testing guide. The section still exists but merely to link to the testing guide.
  • linked the mentioned RFC
  • replaced usages of user with @user if @user is available
  • linked to the Action Mailer section in the configuring guide

I also removed the WIP flag in documents.yml

@senny
Copy link
Member Author

senny commented Apr 5, 2013

@fxn @steveklabnik thoughts?

@steveklabnik
Copy link
Member

As it is hidden in the navigation it is very easily overlooked.

Yeah, that's the WIP tag. :)

@senny
Copy link
Member Author

senny commented Apr 5, 2013

@steveklabnik hehe :trollface:

Do you remember a reason to keep the WIP tag on the AM guide? It feels quite complete to me.

@steveklabnik
Copy link
Member

A few more thoughts:

Since this is the debut of this file, it'd be nice to wrap it all to 80 chars. Since it's not public yet, the history isn't as important.

A blank line here would be awesome:

* How to test your Action Mailer classes.

--------------------------------------------------------------------------------

Right now, markdown is interpreting the last one as a section heading because of this. I can't comment on your diff since this line isn't part of it. ;)

So we got the mailer, the views, and the tests.

This is... sorta tiny. I'm not sure how to expand it, but to start the guide off with what's basically "Here's some terminal output. Yep." isn't super great.

Edit the Mailer

This section doesn't explain why methods in mailers are important. It basically just describes the process of making one, without any of the background info on why you're doing any of this.

It is also a good idea to make a text part for this email.

I'd prefer to make this statement stronger, as some people only accept text/plain emails. Also, it'd be nice to explain how this process works, ie, multipart.

Wire It Up So That the System Sends the Email When a User Signs Up

This heading is terrible.

some people create Rails Observers to fire off emails,

Observers were removed.

Due to this, it makes sense to just have your controller tell the mailer to send an email when a user is successfully created.

I'd really like to see this whole part get a bit more... forceful. Rather than saying "well, some people do this, others do this...." let's cut the hippie bullshit and say "Send emails in the controller, but you should really be using Resque." ;)

This provides a much simpler implementation that does not require the registering of observers and the like.

Same here, observers suck, and if they suck, why do we mention them?

Action Mailer now handles

Guides should always be written as though they're in the present tense. Throw the past down the memory-hole.

If you are using UTF-8 as your character set,

Ruby 2 does this by default, I'd like to add a caveat for 1.9 rather than present 'if' as the norm. "Strongly recommend 2.0" and all that.

  • Defining a header field as a parameter to the mail method:

If we're going to suggest custom headers, X-* has been deprecated, so we shouldn't encourage that pattern.

Adding attachments has been simplified in Action Mailer 3.0.

Same deal. 3.0? Who cares?

pre-encode

http://www.youtube.com/watch?v=wBo3-XnhXNM

Action Mailer 3.0

Same deal here.

Firstly, to tell Mail

Wait, what's Mail?

attachments[] as a hash

should drop the []s.

Thus, when using named routes only the "_url" variant makes sense.

This sentence is really awkward.

It is also possible to set a default host that will be used in all mailers by setting the :host option as a configuration option in config/application.rb:

This shouldn't be at the end, it should be at the beginning. Why tell people to add :host to every _url call when they should set it globally?

prepopulate

See George Carlin above

Abstract Controller

Should be AbstractController.

GMail

Gmail.

domain: 'baci.lindsaar.net',

Best to use example.com for all sample domains.


That's all I got for now. Let's whip this guide into shape. :)

@senny
Copy link
Member Author

senny commented Apr 5, 2013

@steveklabnik thanks for all that good stuff! I'll update the PR later today.

@senny
Copy link
Member Author

senny commented Apr 8, 2013

@steveklabnik @fxn can you take another look?

I adjusted most things pointed out by @steveklabnik, there were a few I left out because I didn't have a good Idea how to fix them:

So we got the mailer, the views, and the tests.
This is... sorta tiny. I'm not sure how to expand it, but to start the guide off with what's basically "Here's some terminal output. Yep." isn't super great.

Edit the Mailer
It is also a good idea to make a text part for this email.
I'd prefer to make this statement stronger, as some people only accept text/plain emails. Also, it'd be nice to explain how this process works, ie, multipart.

@steveklabnik
Copy link
Member

For the second, basically just "Let's also make a text part for this email. Not all clients prefer HTML emails, and so sending both is best practice." rather than "maaaaaybe you should do that too"

@steveklabnik
Copy link
Member

How about this for the first one:

As you can see, you can generate mailers just like you use other generators with Rails. Mailers are conceptually similar to controllers, and so we get a mailer, a directory for views, and a test.

If you didn't want to use a generator, you could create your own file inside of app/mailers, just make sure that it
inherits from ActionMailer::Base:

class MyMailer < ActionMailer::Base
end

@senny
Copy link
Member Author

senny commented Apr 9, 2013

@steveklabnik thanks for your inputs, it's much appreciated ❤️ ! I pushed an updated version.

@steveklabnik
Copy link
Member

Looks great. Just squash it up and I'm good to merge. @fxn any objections?

@senny
Copy link
Member Author

senny commented Apr 9, 2013

@steveklabnik squashed up.

@fxn
Copy link
Member

fxn commented Apr 9, 2013

Hey guys! I'll have a look at it after dinner.

@fxn
Copy link
Member

fxn commented Apr 9, 2013

Good job! It needs some copy-editing here and there, "Mailer" goes in lowercase like "controller", "HTTP" and "Email protocol" do not match, and a bunch of other similar details. I'll make a pass but let's merge!

@steveklabnik
Copy link
Member

let's merge!

:shipit:

steveklabnik added a commit that referenced this pull request Apr 9, 2013
…r_prime_time

Get Action Mailer guide ready for prime time [ci skip]
@steveklabnik steveklabnik merged commit f4e4ba8 into rails:master Apr 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants