Welcome! <%= @user.name %>.
+To confirm your registration, please go to the signup confirmation page..
+ + \ No newline at end of file diff --git a/lib/jam_ruby/app/views/jam_ruby/user_mailer/welcome_message.text.erb b/lib/jam_ruby/app/views/jam_ruby/user_mailer/welcome_message.text.erb new file mode 100644 index 000000000..3667d0935 --- /dev/null +++ b/lib/jam_ruby/app/views/jam_ruby/user_mailer/welcome_message.text.erb @@ -0,0 +1,2 @@ +Welcome! <%= @user.name %> +To confirm your registration, please go to the signup confirmation page at : <%= @signup_confirm_url %>. \ No newline at end of file diff --git a/lib/jam_ruby/base_manager.rb b/lib/jam_ruby/base_manager.rb new file mode 100644 index 000000000..d2970c24b --- /dev/null +++ b/lib/jam_ruby/base_manager.rb @@ -0,0 +1,31 @@ +module JamRuby + class BaseManager + + attr_accessor :pg_conn + + def initialize(options={}) + @log = Logging.logger[self] + @pg_conn = options[:conn] + + unless PG.threadsafe? + raise Exception, "a non-threadsafe build of libpq is present." + end + end + + # Creates a connection manager, and associates the connection created by active_record with ourselves + def self.active_record_transaction(&block) + + manager = self.new + ActiveRecord::Base.connection_pool.with_connection do |connection| + # create a transaction, and pass the current connection to ConnectionManager. + # this lets the entire operation work with the same transaction, + # across Rails ActiveRecord and the pg-gem based code in ConnectionManager. + manager.pg_conn = connection.instance_variable_get("@connection") + + connection.transaction do + block.call(manager) + end + end + end + end +end \ No newline at end of file diff --git a/lib/jam_ruby/init.rb b/lib/jam_ruby/init.rb new file mode 100644 index 000000000..adf32cf04 --- /dev/null +++ b/lib/jam_ruby/init.rb @@ -0,0 +1,3 @@ +# initialize actionmailer +ActionMailer::Base.raise_delivery_errors = true +ActionMailer::Base.view_paths = File.expand_path('../../jam_ruby/app/views/', __FILE__) \ No newline at end of file diff --git a/lib/jam_ruby/models/user.rb b/lib/jam_ruby/models/user.rb index fe4a60485..460c36bce 100644 --- a/lib/jam_ruby/models/user.rb +++ b/lib/jam_ruby/models/user.rb @@ -1,9 +1,8 @@ module JamRuby class User < ActiveRecord::Base include Tire::Model::Search - include Tire::Model::Callbacks - attr_accessible :name, :email, :password, :password_confirmation + attr_accessible :name, :email, :password, :password_confirmation, :city, :state, :country attr_accessor :updating_password self.primary_key = 'id' @@ -59,7 +58,7 @@ module JamRuby after_save :limit_to_five_instruments - validates :name, presence: true, length: {maximum: 50} + validates :name, uniqueness: {case_sensitive: false}, presence: true, length: {maximum: 50} VALID_EMAIL_REGEX = /\A[\w+\-.]+@[a-z\d\-.]+\.[a-z]+\z/i validates :email, presence: true, format: {with: VALID_EMAIL_REGEX}, uniqueness: {case_sensitive: false} @@ -111,7 +110,7 @@ module JamRuby end # helper method for creating / updating a User - def self.save(params) + def self.save(params) if params[:id].nil? user = User.new() else @@ -207,6 +206,15 @@ module JamRuby }.to_json end + # only put users into the search index if their eail is confirmed + after_save do + update_index if email_confirmed + end + + after_destroy do + update_index + end + class << self def create_search_index Tire.index(User.index_name) do diff --git a/lib/jam_ruby/user_manager.rb b/lib/jam_ruby/user_manager.rb new file mode 100644 index 000000000..446f59de2 --- /dev/null +++ b/lib/jam_ruby/user_manager.rb @@ -0,0 +1,65 @@ +module JamRuby + class UserManager < BaseManager + + def initialize(options={}) + super(options) + @log = Logging.logger[self] + end + + # throws ActiveRecord::RecordNotFound if instrument is invalid + # throws an email delivery error if unable to connect out to SMTP + def signup(name, email, password, password_confirmation, + city, state, country, instruments, signup_confirm_url) + user = User.new + + UserManager.active_record_transaction do |user_manager| + user.name = name + user.email = email + user.password = password + user.password_confirmation = password_confirmation + user.admin = false + user.email_confirmed = false + user.city = city + user.state = state + user.country = country + unless instruments.nil? + instruments.each_with_index do |musician_instrument_param, index| + instrument = Instrument.find(musician_instrument_param["id"]) + musician_instrument = MusicianInstrument.new + musician_instrument.user = current_user + musician_instrument.instrument = instrument + musician_instrument.proficiency_level = musician_instrument_param["proficiency_level"] + musician_instrument.priority = index + musician_instrument.save + user.instruments < musician_instrument + end + end + user.signup_token = SecureRandom.urlsafe_base64 + + user.save + + if user.errors.any? + raise ActiveRecord::Rollback + else + # any errors here should also rollback the transaction; that's OK. If emails aren't going to be delivered, + # it's already a really bad situation; make user signup again + UserMailer.welcome_message(user, signup_confirm_url + "/" + user.signup_token).deliver + end + end + + return user + end + + # throws RecordNotFound if signup token is invalid + def signup_confirm(signup_token) + UserManager.active_record_transaction do |user_manager| + # throws ActiveRecord::RecordNotFound if invalid + user = User.find_by_signup_token!(signup_token) + user.signup_token = nil + user.email_confirmed = true + user.save + return user + end + end + end +end \ No newline at end of file diff --git a/spec/factories.rb b/spec/factories.rb index 6ccd2c674..bc82f9e43 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -4,6 +4,7 @@ FactoryGirl.define do sequence(:email) { |n| "person_#{n}@example.com"} password "foobar" password_confirmation "foobar" + email_confirmed true factory :admin do admin true @@ -26,4 +27,8 @@ FactoryGirl.define do factory :friendship, :class => JamRuby::Friendship do end + + factory :band, :class => JamRuby::Band do + + end end \ No newline at end of file diff --git a/spec/jam_ruby/models/search_spec.rb b/spec/jam_ruby/models/search_spec.rb index 3733fc5f1..9ab057f4d 100644 --- a/spec/jam_ruby/models/search_spec.rb +++ b/spec/jam_ruby/models/search_spec.rb @@ -11,11 +11,9 @@ describe Search do def create_peachy_data - @user = User.save(name: "Peach", email: "user@example.com", - password: "foobar", password_confirmation: "foobar", musician: true) - @fan = User.save(name: "Peach Peach", email: "fan@example.com", - password: "foobar", password_confirmation: "foobar", musician: false) - @band = Band.save(name: "Peach pit", website: "www.bands.com", biography: "zomg we rock") + @user = FactoryGirl.create(:user, name: "Peach", email: "user@example.com", musician: true) + @fan = FactoryGirl.create(:user, name: "Peach Peach", email: "fan@example.com", musician: false) + @band = FactoryGirl.create(:band, name: "Peach pit", website: "www.bands.com", biography: "zomg we rock") end def assert_peachy_data diff --git a/spec/jam_ruby/models/tire_search_spec.rb b/spec/jam_ruby/models/tire_search_spec.rb index 237c87204..2ab89207d 100644 --- a/spec/jam_ruby/models/tire_search_spec.rb +++ b/spec/jam_ruby/models/tire_search_spec.rb @@ -20,8 +20,8 @@ describe "tire search" do end it "full search for single user" do - @user = User.save(name: "User One", email: "user@example.com", - password: "foobar", password_confirmation: "foobar", musician: true) + @user = FactoryGirl.create(:user, name: "User One", email: "user@example.com", musician: true) + User.search_index.refresh s = Tire.search ['test-jamruby-users', 'test-jamruby-bands'], :load => true do @@ -35,7 +35,7 @@ describe "tire search" do end it "full search for single band" do - @band = Band.save(name: "Example Band", website: "www.bands.com", biography: "zomg we rock") + @band = FactoryGirl.create(:band, name: "Example Band", website: "www.bands.com", biography: "zomg we rock") Band.search_index.refresh s = Tire.search ['test-jamruby-users', 'test-jamruby-bands'], :load => true do @@ -49,9 +49,8 @@ describe "tire search" do end it "full search for a band & user" do - @user = User.save(name: "Peach", email: "user@example.com", - password: "foobar", password_confirmation: "foobar", musician: true) - @band = Band.save(name: "Peach pit", website: "www.bands.com", biography: "zomg we rock") + @user = FactoryGirl.create(:user, name: "Peach", email: "user@example.com", musician: true) + @band = FactoryGirl.create(:band, name: "Peach pit", website: "www.bands.com", biography: "zomg we rock") User.search_index.refresh Band.search_index.refresh @@ -71,9 +70,8 @@ describe "tire search" do it "pagination" do - @user = User.save(name: "Peach", email: "user@example.com", - password: "foobar", password_confirmation: "foobar", musician: true) - @band = Band.save(name: "Peach pit", website: "www.bands.com", biography: "zomg we rock") + @user = FactoryGirl.create(:user, name: "Peach", email: "user@example.com", musician: true) + @band = FactoryGirl.create(:band, name: "Peach pit", website: "www.bands.com", biography: "zomg we rock") User.search_index.refresh Band.search_index.refresh @@ -113,8 +111,7 @@ describe "tire search" do s.results.total.should == 0 - @user = User.save(name: "Peach", email: "user@example.com", - password: "foobar", password_confirmation: "foobar", musician: true) + @user = FactoryGirl.create(:user, name: "Peach", email: "user@example.com", musician: true) User.search_index.refresh sleep 1 # https://jamkazam.atlassian.net/browse/VRFS-69 diff --git a/spec/jam_ruby/models/user_search_spec.rb b/spec/jam_ruby/models/user_search_spec.rb index 4fa012121..31d63ed8c 100644 --- a/spec/jam_ruby/models/user_search_spec.rb +++ b/spec/jam_ruby/models/user_search_spec.rb @@ -6,8 +6,8 @@ describe User do User.delete_search_index User.create_search_index - @user = User.save(name: "Example User", email: "user@example.com", - password: "foobar", password_confirmation: "foobar", musician: true) + @user = FactoryGirl.create(:user, name: "Example User", email: "user@example.com", + password: "foobar", password_confirmation: "foobar", musician: true, email_confirmed: true) # you have to poke elasticsearch because it will batch requests internally for a second User.search_index.refresh @@ -59,12 +59,31 @@ describe User do end it "should tokenize correctly" do - @user2 = User.save(name: "peaches", email: "peach@example.com", - password: "foobar", password_confirmation: "foobar", musician: true) + @user2 = FactoryGirl.create(:user, name: "peaches", email: "peach@example.com", + password: "foobar", password_confirmation: "foobar", musician: true, email_confirmed: true) User.search_index.refresh ws = User.search("pea") ws.results.length.should == 1 user_result = ws.results[0] user_result.id.should == @user2.id + end + + + it "users who have signed up, but not confirmed should not show up in search index" do + @user3 = FactoryGirl.create(:user, name: "unconfirmed", email: "unconfirmed@example.com", + password: "foobar", password_confirmation: "foobar", musician: true, email_confirmed: false) + User.search_index.refresh + ws = User.search("unconfirmed") + ws.results.length.should == 0 + + # Ok, confirm the user, and see them show up + @user3.email_confirmed = true + @user3.save + User.search_index.refresh + + ws = User.search("unconfirmed") + ws.results.length.should == 1 + user_result = ws.results[0] + user_result.id.should == @user3.id end end \ No newline at end of file diff --git a/spec/jam_ruby/user_manager_spec.rb b/spec/jam_ruby/user_manager_spec.rb new file mode 100644 index 000000000..f47c89c9a --- /dev/null +++ b/spec/jam_ruby/user_manager_spec.rb @@ -0,0 +1,81 @@ +require 'spec_helper' + +# these tests avoid the use of ActiveRecord and FactoryGirl to do blackbox, non test-instrumented tests +describe UserManager do + + + before(:each) do + @user_manager = UserManager.new(:conn => @conn) + UserMailer.deliveries.clear + end + + describe "signup" do + it "signup successfully" do + @user = @user_manager.signup("bob", "bob@jamkazam.com", "foobar", "foobar", "Austin", "TX", "USA", nil, "http://localhost:3000/confirm" ) + + @user.errors.any?.should be_false + @user.name.should == "bob" + @user.email.should == "bob@jamkazam.com" + @user.email_confirmed.should be_false + @user.city.should == "Austin" + @user.state.should == "TX" + @user.country.should == "USA" + @user.instruments.length.should == 0 + @user.signup_token.should_not be_nil + + UserMailer.deliveries.length.should == 1 + end + + it "duplicate signup failure" do + @user = @user_manager.signup("bob", "bob@jamkazam.com", "foobar", "foobar", "Austin", "TX", "USA", nil, "http://localhost:3000/confirm" ) + UserMailer.deliveries.length.should == 1 + @user.errors.any?.should be_false + + # exactly the same parameters; should dup on email, and send no email + @user = @user_manager.signup("bob", "bob@jamkazam.com", "foobar", "foobar", "Austin", "TX", "USA", nil, "http://localhost:3000/confirm" ) + UserMailer.deliveries.length.should == 1 + @user.errors.any?.should be_true + @user.errors[:email][0].should == "has already been taken" + + # change email so that name appears dupped + @user = @user_manager.signup("bob", "bobbie@jamkazam.com", "foobar", "foobar", "Austin", "TX", "USA", nil, "http://localhost:3000/confirm" ) + UserMailer.deliveries.length.should == 1 + @user.errors.any?.should be_true + @user.errors[:name][0].should == "has already been taken" + end + + it "fail on no username" do + @user = @user_manager.signup("", "bob@jamkazam.com", "foobar", "foobar", "Austin", "TX", "USA", nil, "http://localhost:3000/confirm" ) + UserMailer.deliveries.length.should == 0 + @user.errors.any?.should be_true + @user.errors[:name][0].should == "can't be blank" + end + + it "fail on no username" do + @user = @user_manager.signup("murp", "", "foobar", "foobar", "Austin", "TX", "USA", nil, "http://localhost:3000/confirm" ) + UserMailer.deliveries.length.should == 0 + @user.errors.any?.should be_true + @user.errors[:email][0].should == "can't be blank" + end + end + + describe "signup_confirm" do + it "fail on no username" do + @user = @user_manager.signup("bob", "bob@jamkazam.com", "foobar", "foobar", "Austin", "TX", "USA", nil, "http://localhost:3000/confirm" ) + @user = @user_manager.signup_confirm(@user.signup_token) + @user.email_confirmed.should be_true + end + + it "fail to confirm bogus signup token" do + expect { @user_manager.signup_confirm("murp") }.to raise_error ActiveRecord::RecordNotFound + end + + it "fail to confirm empty signup token" do + expect { @user_manager.signup_confirm("") }.to raise_error ActiveRecord::RecordNotFound + end + + it "fail to confirm nil signup token" do + expect { @user_manager.signup_confirm(nil) }.to raise_error ActiveRecord::RecordNotFound + end + end +end diff --git a/spec/mailers/user_mailer_spec.rb b/spec/mailers/user_mailer_spec.rb new file mode 100644 index 000000000..5364de7e5 --- /dev/null +++ b/spec/mailers/user_mailer_spec.rb @@ -0,0 +1,44 @@ +require "spec_helper" + +describe UserMailer do + + let(:user) { FactoryGirl.create(:user) } + + before(:each) do + UserMailer.deliveries.clear + end + + it "should send welcome email" do + signup_confirmation_url = "http://example.com/confirm" + signup_confirmation_url_with_token = "#{signup_confirmation_url}/#{user.signup_token}" + UserMailer.welcome_message(user, signup_confirmation_url_with_token).deliver + + UserMailer.deliveries.length.should == 1 + mail = UserMailer.deliveries[0] + + mail['from'].to_s.should == UserMailer::DEFAULT_SENDER + mail['to'].to_s.should == user.email + mail.multipart?.should == true # because we send plain + html + + # verify that the messages are correctly configured + mail.html_part.body.include?("Welcome").should be_true + mail.html_part.body.include?(signup_confirmation_url_with_token).should be_true + mail.text_part.body.include?("Welcome").should be_true + mail.text_part.body.include?(signup_confirmation_url_with_token).should be_true + end + + it "should send reset password" do + UserMailer.reset_password(user).deliver + + UserMailer.deliveries.length.should == 1 + mail = UserMailer.deliveries[0] + + mail['from'].to_s.should == UserMailer::DEFAULT_SENDER + mail['to'].to_s.should == user.email + mail.multipart?.should == true # because we send plain + html + + # verify that the messages are correctly configured + mail.html_part.body.include?("Reset").should be_true + mail.text_part.body.include?("Reset").should be_true + end +end