Merge pull request #441 from alphagov/promote-feedback-to-ar-model

Promote feedback model to a full AR model for persistence

app/controllers/feedback_controller.rb

@@ -1,26 +1,29 @@
 class FeedbackController < ApplicationController
   respond_to :html
 
+  before_action :build_feedback, only: [:new, :create]
+
   def index
-    respond_with @feedback = Feedback.new
+    respond_with @feedback
   end
 
-  # TODO: We should use deliver_later but serializing the feedback model is tricky.
   def create
-    @feedback = Feedback.new(feedback_params)
-    if @feedback.valid?
-      FeedbackMailer.send_feedback(@feedback).deliver_now
-      redirect_to thanks_feedback_url
-    else
-      render 'index'
+    if @feedback.save
+      FeedbackEmailJob.perform_later(@feedback)
     end
+
+    respond_with @feedback, location: thanks_feedback_url
   end
 
   def thanks
   end
 
   private
 
+  def build_feedback
+    @feedback = Feedback.new(params.key?(:feedback) ? feedback_params : {})
+  end
+
   def feedback_params
     params.require(:feedback).permit(*feedback_attributes).merge(user_agent)
   end

app/jobs/feedback_email_job.rb

@@ -0,0 +1,4 @@
+class FeedbackEmailJob < EmailJob
+  self.mailer = FeedbackMailer
+  self.email = :send_feedback
+end

app/models/feedback.rb

@@ -1,20 +1,5 @@
-class Feedback
-  include ActiveModel::Validations
-  include ActiveModel::Conversion
-
-  validates_presence_of :comment
-  validates_format_of :email, with: EMAIL_REGEX, allow_blank: true
-
-  attr_accessor :email, :petition_link_or_title, :comment, :user_agent
-
-  def initialize(options = {})
-    @email = options[:email]
-    @petition_link_or_title = options[:petition_link_or_title]
-    @comment = options[:comment]
-    @user_agent = options[:user_agent]
-  end
-
-  def persisted?
-    false
-  end
+class Feedback < ActiveRecord::Base
+  validates :comment, presence: true, length: { maximum: 32768 }
+  validates :petition_link_or_title, length: { maximum: 255 }, allow_blank: true
+  validates :email, format: { with: EMAIL_REGEX }, length: { maximum: 255 }, allow_blank: true
 end

app/views/feedback_mailer/send_feedback.text.erb

@@ -1,4 +1,4 @@
-Comment: <%= @feedback.comment %>
+Comments: <%= @feedback.comment %>
 Link: <%= @feedback.petition_link_or_title %>
 Email: <%= @feedback.email %>
 Browser: <%= @feedback.user_agent %>

config/initializers/inflections.rb

@@ -1,16 +1,5 @@
 # Be sure to restart your server when you modify this file.
 
-# Add new inflection rules using the following format. Inflections
-# are locale specific, and you may define rules for as many different
-# locales as you wish. All of these examples are active by default:
-# ActiveSupport::Inflector.inflections(:en) do |inflect|
-#   inflect.plural /^(ox)$/i, '\1en'
-#   inflect.singular /^(ox)en/i, '\1'
-#   inflect.irregular 'person', 'people'
-#   inflect.uncountable %w( fish sheep )
-# end
-
-# These inflection rules are supported but not enabled by default:
-# ActiveSupport::Inflector.inflections(:en) do |inflect|
-#   inflect.acronym 'RESTful'
-# end
+ActiveSupport::Inflector.inflections(:en) do |inflect|
+  inflect.uncountable %w( feedback )
+end

config/locales/activerecord.en-GB.yml

@@ -59,6 +59,8 @@ en-GB:
         too_long: "Background is too long"
       additional_details:
         too_long: "Additional details is too long"
+      comment:
+        blank: "Comments must be completed"
       name:
         blank: "Name must be completed"
       location_code:

config/routes.rb

@@ -6,9 +6,9 @@
     get 'browserconfig' => 'pages#browserconfig', format: 'xml'
     get 'manifest' => 'pages#manifest', format: 'json'
 
-    get 'feedback' => 'feedback#index', :as => 'feedback'
-    get 'feedback/thanks' => 'feedback#thanks', :as => 'thanks_feedback'
-    post 'feedback' => 'feedback#create', :as => nil
+    get  'feedback', to: 'feedback#new', as: 'feedback'
+    post 'feedback', to: 'feedback#create', as: nil
+    get  'feedback/thanks', to: 'feedback#thanks', as: 'thanks_feedback'
 
     scope 'petitions' do
       get 'local' => 'local_petitions#index', as: 'local_petitions'

db/migrate/20160214233414_create_feedback.rb

@@ -0,0 +1,10 @@
+class CreateFeedback < ActiveRecord::Migration
+  def change
+    create_table :feedback do |t|
+      t.string :comment, limit: 32768, null: false
+      t.string :petition_link_or_title
+      t.string :email
+      t.string :user_agent
+    end
+  end
+end

db/structure.sql

@@ -364,6 +364,38 @@ CREATE SEQUENCE email_sent_receipts_id_seq
 ALTER SEQUENCE email_sent_receipts_id_seq OWNED BY email_sent_receipts.id;
 
 
+--
+-- Name: feedback; Type: TABLE; Schema: public; Owner: -; Tablespace: 
+--
+
+CREATE TABLE feedback (
+    id integer NOT NULL,
+    comment character varying(32768) NOT NULL,
+    petition_link_or_title character varying,
+    email character varying,
+    user_agent character varying
+);
+
+
+--
+-- Name: feedback_id_seq; Type: SEQUENCE; Schema: public; Owner: -
+--
+
+CREATE SEQUENCE feedback_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+
+--
+-- Name: feedback_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: -
+--
+
+ALTER SEQUENCE feedback_id_seq OWNED BY feedback.id;
+
+
 --
 -- Name: government_responses; Type: TABLE; Schema: public; Owner: -; Tablespace: 
 --
@@ -774,6 +806,13 @@ ALTER TABLE ONLY email_requested_receipts ALTER COLUMN id SET DEFAULT nextval('e
 ALTER TABLE ONLY email_sent_receipts ALTER COLUMN id SET DEFAULT nextval('email_sent_receipts_id_seq'::regclass);
 
 
+--
+-- Name: id; Type: DEFAULT; Schema: public; Owner: -
+--
+
+ALTER TABLE ONLY feedback ALTER COLUMN id SET DEFAULT nextval('feedback_id_seq'::regclass);
+
+
 --
 -- Name: id; Type: DEFAULT; Schema: public; Owner: -
 --
@@ -909,6 +948,14 @@ ALTER TABLE ONLY email_sent_receipts
     ADD CONSTRAINT email_sent_receipts_pkey PRIMARY KEY (id);
 
 
+--
+-- Name: feedback_pkey; Type: CONSTRAINT; Schema: public; Owner: -; Tablespace: 
+--
+
+ALTER TABLE ONLY feedback
+    ADD CONSTRAINT feedback_pkey PRIMARY KEY (id);
+
+
 --
 -- Name: government_responses_pkey; Type: CONSTRAINT; Schema: public; Owner: -; Tablespace: 
 --
@@ -1504,5 +1551,7 @@ INSERT INTO schema_migrations (version) VALUES ('20160211003703');
 
 INSERT INTO schema_migrations (version) VALUES ('20160211020341');
 
+INSERT INTO schema_migrations (version) VALUES ('20160214233414');
+
 INSERT INTO schema_migrations (version) VALUES ('20160217192016');
 

features/step_definitions/feedback_steps.rb

@@ -1,12 +1,11 @@
 Then(/^I should be able to submit feedback$/) do
   page.driver.browser.header('User-Agent', 'Chrome')
 
-  @feedback = Feedback.new(:name => "Joe Public", :email => "[email protected]",
-    :comment => "I can't submit a petition for some reason", :petition_link_or_title => 'link')
+  @feedback = FactoryGirl.create(:feedback, comment: "I can't submit a petition for some reason")
 
-  fill_in "feedback[email]", :with => @feedback.email
-  fill_in "feedback[petition_link_or_title]", :with => @feedback.petition_link_or_title
-  fill_in "feedback[comment]", :with => @feedback.comment
+  fill_in "feedback[email]", with: @feedback.email
+  fill_in "feedback[petition_link_or_title]", with: @feedback.petition_link_or_title
+  fill_in "feedback[comment]", with: @feedback.comment
 
   click_button("Send feedback")
   expect(page).to have_content("Thank")

spec/controllers/feedback_controller_spec.rb

@@ -3,14 +3,16 @@
 RSpec.describe FeedbackController, type: :controller do
   describe "GET /feedback" do
     it "is successful" do
-      get :index
+      get :new
       expect(response).to be_success
     end
   end
 
   describe "POST /feedback" do
     def do_post(attributes = {})
-      post :create, feedback: attributes
+      perform_enqueued_jobs do
+        post :create, feedback: attributes
+      end
     end
 
     context "with valid input" do
@@ -35,7 +37,7 @@ def do_post(attributes = {})
 
       it "returns the user to the form" do
         do_post comment: ""
-        expect(response).to render_template("feedback/index")
+        expect(response).to render_template("feedback/new")
       end
     end
   end

spec/factories.rb

@@ -339,4 +339,11 @@
       end_date { 2.years.ago }
     end
   end
+
+  factory :feedback do
+    comment "This thing is wrong"
+    petition_link_or_title "Do stuff"
+    email "[email protected]"
+    user_agent "Mozilla/5.0"
+  end
 end

spec/jobs/email_job_spec.rb

@@ -202,3 +202,16 @@
     end
   end
 end
+
+RSpec.describe FeedbackEmailJob, type: :job do
+  let(:feedback) { FactoryGirl.create(:feedback) }
+
+  it "sends the FeedbackMailer#send_feedback email" do
+    expect(FeedbackMailer).to receive(:send_feedback).with(feedback).and_call_original
+
+    perform_enqueued_jobs do
+      described_class.perform_later(feedback)
+    end
+  end
+end
+

spec/models/feedback_spec.rb

@@ -1,40 +1,28 @@
 require 'rails_helper'
 
 RSpec.describe Feedback, type: :model do
-  it "can be constructed without params" do
-    expect(Feedback.new.comment).to be_nil
+  it "has a valid factory" do
+    expect(FactoryGirl.build(:feedback)).to be_valid
   end
 
-  it "has an email" do
-    expect(Feedback.new(:email => 'foo').email).to eq('foo')
+  describe "schema" do
+    it { is_expected.to have_db_column(:comment).of_type(:string).with_options(limit: 32768, null: false) }
+    it { is_expected.to have_db_column(:petition_link_or_title).of_type(:string) }
+    it { is_expected.to have_db_column(:email).of_type(:string) }
+    it { is_expected.to have_db_column(:user_agent).of_type(:string) }
   end
 
-  it "has a petition link or title" do
-    expect(Feedback.new(:petition_link_or_title => 'foo').petition_link_or_title).to eq('foo')
-  end
-
-  it "has a comment" do
-    expect(Feedback.new(:comment => 'foo').comment).to eq('foo')
-  end
-
-  def valid_attributes
-    { :email => "[email protected]",
-      :comment => "I can't submit a petition for some reason",
-      :petition_link_or_title => 'link' }
-  end
-
-  describe "valid?" do
-    it "is valid when all attributes are in place" do
-      expect(Feedback.new(valid_attributes)).to be_valid
-    end
-
-    it "is not valid when a required attribute is missing" do
-      expect(Feedback.new(valid_attributes.except(:comment))).not_to be_valid
+  describe "validations" do
+    it "is invalid by default" do
+      expect(subject).not_to be_valid
     end
 
-    it "is not valid when the email format is wrong" do
-      expect(Feedback.new(valid_attributes.merge(:email => 'foo'))).not_to be_valid
-    end
+    it { is_expected.to validate_presence_of(:comment) }
+    it { is_expected.to validate_length_of(:comment).is_at_most(32768) }
+    it { is_expected.to validate_length_of(:petition_link_or_title).is_at_most(255) }
+    it { is_expected.to validate_length_of(:email).is_at_most(255) }
+    it { is_expected.to allow_value("[email protected]").for(:email) }
+    it { is_expected.not_to allow_value("foo@example").for(:email) }
+    it { is_expected.not_to allow_value("foo").for(:email) }
   end
-
 end