-
Notifications
You must be signed in to change notification settings - Fork 0
mailer integration, remaining flow control #2
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
base: master
Are you sure you want to change the base?
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,11 @@ | ||
| class ApplicationController < ActionController::Base | ||
| before_filter :set_mailer_host | ||
|
|
||
| # Prevent CSRF attacks by raising an exception. | ||
| # For APIs, you may want to use :null_session instead. | ||
| protect_from_forgery with: :exception | ||
|
|
||
| def set_mailer_host | ||
| ActionMailer::Base.default_url_options[:host] = request.host_with_port | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,22 +21,20 @@ def show | |
| def create | ||
| @message = Message.new(message_params) | ||
| @message.message_id = SecureRandom.uuid; | ||
| @message.authenticated = [email protected]? | ||
|
|
||
| respond_to do |format| | ||
| if @message.save | ||
| format.html { redirect_to action: 'index', notice: 'Message was successfully created.' } | ||
| else | ||
| format.html { render :new } | ||
| end | ||
| if @message.save | ||
| MessageMailer.sd_message(@message).deliver_now | ||
| redirect_to action: 'index', notice: 'Message was successfully created.' | ||
| else | ||
| render :new | ||
| end | ||
| end | ||
|
|
||
| # DELETE /messages/1 | ||
| def destroy | ||
| @message.destroy | ||
| respond_to do |format| | ||
| format.html { redirect_to action: 'index', notice: 'Message was successfully destroyed.' } | ||
| end | ||
| redirect_to action: 'index', notice: 'Message was successfully destroyed.' | ||
| end | ||
|
|
||
| private | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| class RecipientsController < ApplicationController | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since most of the logic below is operating upon a
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, I split these apart mainly due to routing. I took a look at how tmwsd's message edit navigation progressed vs how the messages were actually read. It used different routes for message admin vs message consumption. I went with a similar approach, which lead to some ugliness with endpoint config. I liked this separation due to the distinctiveness of the recipient view and authentication flow against that of the admin flow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, yeah that makes sense in context. I think the best thing to do here is to use semantic resources still, but namespace them for separation: http://guides.rubyonrails.org/routing.html#controller-namespaces-and-routing. This is trivial right now, and probably not necessary to perfect, as this is just a small learning app. The custom routes will work fine for this. |
||
|
|
||
| before_action :set_message | ||
|
|
||
| # GET /recipients/1 | ||
| def show | ||
| if @message.authenticated? | ||
| @message.delete() | ||
| else | ||
| redirect_to action: 'authenticate' | ||
| end | ||
| end | ||
|
|
||
| # GET /recipients/1/authenticate | ||
| def authenticate | ||
| end | ||
|
|
||
| # POST /recipients/1 | ||
| def authenticatedShow | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can probably do away with this unconventional route, and move everything to the show method. If you retrieve the message before destroying it, it will still be viewable at the time of request, but after refresh will be gone :)
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I don't understand how I would handle show and authentication with the same route. Can you point me in a direction with this? I'm thinking that at a minimum I'll need a get and post endpoint (maybe a third for index which would display the auth form). Get to view the message and post to service an authentication form submission. One of the things I'm uncertain about is what rails expects in terms of convention between endpoints and views. I would think that authentication support would require a distinct view. If the authentication form lives in its own view then does convention demand a corresponding endpoint / controller method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see what you're doing here. You're flagging the record if they provide a valid password first... which is fine.
In terms of controller methods and view conventions, here is a good reference: http://guides.rubyonrails.org/layouts_and_rendering.html Basically, your Your routes could be simplified to something like: resources :messages, only :show do
post 'authenticate', on: :member
end |
||
| paramHash = message_params | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to store this off in a new variable. we can just reference
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||
| if @message.password == paramHash[:password] && @message.update(authenticated: true) | ||
| redirect_to action: 'show' | ||
| else | ||
| redirect_to action: 'authenticate' | ||
| end | ||
| end | ||
|
|
||
| private | ||
| # Use callbacks to share common setup or constraints between actions. | ||
| def set_message | ||
| @message = Message.find_by message_id: params[:id] | ||
| if !@message | ||
| redirect_to root_path | ||
| end | ||
| end | ||
|
|
||
| # Never trust parameters from the scary internet, only allow the white list through. | ||
| def message_params | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. part of 'the rails way' is sticking to rails convention whenever we can. they have conventionalized a standard filtering of params as
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, are you speaking towards naming as in message_params should be renamed to 'strong_params''? |
||
| params.require(:message).permit(:password) | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| class ApplicationMailer < ActionMailer::Base | ||
| default from: "[email protected]" | ||
| layout 'mailer' | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| class MessageMailer < ApplicationMailer | ||
|
|
||
| default from: '[email protected]' | ||
|
|
||
| def sd_message(message) | ||
| @message = message | ||
| @url = 'http://example.com/login' | ||
| mail(to: @message.recipient, subject: 'A Message') | ||
| end | ||
|
|
||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| <html> | ||
| <body> | ||
| <%= yield %> | ||
| </body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| <%= yield %> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| <!DOCTYPE html> | ||
| <html> | ||
| <head> | ||
| <meta content='text/html; charset=UTF-8' http-equiv='Content-Type' /> | ||
| </head> | ||
| <body> | ||
| <h1>A message has been left for you</h1> | ||
| <p> | ||
| View it here: | ||
| <%= url_for(only_path: false, action: 'show', controller: 'recipients', id: @message.message_id) %>. | ||
| <br> | ||
| </p> | ||
| </body> | ||
| </html> |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| <h1>Please Authenticate</h1> | ||
|
|
||
| <%= form_for(@message, method: 'post', url: '/recipients/' + @message.message_id) do |m| %> | ||
|
|
||
| <div class="field"> | ||
| <%= m.label :password %><br> | ||
| <%= m.password_field :password %> | ||
| </div> | ||
|
|
||
| <div class="actions"> | ||
| <%= m.submit %> | ||
| </div> | ||
| <% end %> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| <h1>This message has been deleted. Once you leave this page it will no longer be available.</h1> | ||
|
|
||
| <p> | ||
| <strong>Content:</strong> | ||
| <%= @message.content %> | ||
| </p> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,9 @@ | ||
| Rails.application.routes.draw do | ||
| resources 'messages', :only => [:index, :new, :create, :show, :destroy] | ||
|
|
||
| get 'recipients/:id' => 'recipients#show' | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rails can generate RESTful routes automagically for you. Here you would use: resources :recipients, only: [:show]and if you wanted to use an unconventional route, you would do: resources :recipients, only: [:show] do
get "authenticated" on: :member # gets with an id e.g. recipients/:id/authenticated
get "authenticated" on: :collection # gets a collection e.g. recipients/authenticated
endWe always want to use RESTful routes when we can. The default rails requests mapped to methods are:
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, will give this: Suppose I could use default routes, though the authentication post would be a bit of a misnomer. GET: index <-- auth form or redirect to show if auth is not required |
||
| get 'recipients/:id/authenticate' => 'recipients#authenticate' | ||
| post 'recipients/:id' => 'recipients#authenticatedShow' | ||
|
|
||
| # The priority is based upon order of creation: first created -> highest priority. | ||
| # See how all your routes lay out with "rake routes". | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,9 +18,9 @@ | |
| t.text "recipient" | ||
| t.text "password" | ||
| t.text "message_id" | ||
| t.boolean "viewed" | ||
| t.datetime "created_at", null: false | ||
| t.datetime "updated_at", null: false | ||
| t.boolean "authenticated" | ||
| t.datetime "created_at", null: false | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to set |
||
| t.datetime "updated_at", null: false | ||
| end | ||
|
|
||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| require 'test_helper' | ||
|
|
||
| class ReadmeControllerTest < ActionController::TestCase | ||
| # test "the truth" do | ||
| # assert true | ||
| # end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| require 'test_helper' | ||
|
|
||
| class MessageMailerTest < ActionMailer::TestCase | ||
| # test "the truth" do | ||
| # assert true | ||
| # end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| # Preview all emails at http://localhost:3000/rails/mailers/message_mailer | ||
| class MessageMailerPreview < ActionMailer::Preview | ||
|
|
||
| end |
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.
configuration stuff like like can go in either
config/application.rborconfig/environments/env-specific-config. that way it's set when the app loads up.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.
Ok, moved it to development.rb. If I set it in either of those locations then it will be a literal assignment, right? Suppose that's good enough if made per env. I liked the idea of using the request to provide the host info. Seemed more flexible.