diff --git a/app/controllers/api_tokens_controller.rb b/app/controllers/api_tokens_controller.rb index 9a85d7e..20cb09e 100644 --- a/app/controllers/api_tokens_controller.rb +++ b/app/controllers/api_tokens_controller.rb @@ -1,30 +1,14 @@ class ApiTokensController < ApplicationController + before_filter :ensure_authenticated! def create - claimed_num = api_token_creator.create(current_user, params[:api_token]) - - if claimed_num - redirect_to_profile(claimed_num) - else - render :error - end - end - - private - - def redirect_to_profile(claimed_num) - if claimed_num > 0 - notice = "Claimed #{claimed_num} asciicasts, yay!" - else - notice = "Authenticated successfully, yippie!" - end - - redirect_to profile_path(current_user), :notice => notice - end + current_user.assign_api_token(params[:api_token]) + redirect_to profile_path(current_user), + notice: "Successfully registered your API token. ^5" - def api_token_creator - @api_token_creator ||= ApiTokenCreator.new + rescue ActiveRecord::RecordInvalid, ApiToken::ApiTokenTakenError + render :error end end diff --git a/app/models/api_token.rb b/app/models/api_token.rb index b572134..a1308bf 100644 --- a/app/models/api_token.rb +++ b/app/models/api_token.rb @@ -1,5 +1,29 @@ class ApiToken < ActiveRecord::Base + + ApiTokenTakenError = Class.new(StandardError) + belongs_to :user - validates :user, :token, :presence => true + validates :user, :token, presence: true + validates :token, uniqueness: true + + attr_accessible :token + + def self.for_token(token) + ApiToken.where(token: token).first + end + + def reassign_to(target_user) + return if target_user == user + raise ApiTokenTakenError if taken? + + user.merge_to(target_user) + end + + private + + def taken? + !user.dummy? + end + end diff --git a/app/models/user.rb b/app/models/user.rb index f94f3d2..bf7343c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -39,7 +39,7 @@ class User < ActiveRecord::Base user.dummy = true user.nickname = username user.save! - user.add_api_token(token) + user.api_tokens.create!(token: token) user end end @@ -60,8 +60,24 @@ class User < ActiveRecord::Base nickname end - def add_api_token(token) - api_tokens.where(:token => token).first_or_create + def assign_api_token(token) + api_token = ApiToken.for_token(token) + + if api_token + api_token.reassign_to(self) + else + api_token = api_tokens.create!(token: token) + end + + api_token + end + + def merge_to(target_user) + self.class.transaction do |tx| + asciicasts.update_all(user_id: target_user.id, updated_at: DateTime.now) + api_tokens.update_all(user_id: target_user.id, updated_at: DateTime.now) + destroy + end end def asciicast_count diff --git a/app/services/api_token_creator.rb b/app/services/api_token_creator.rb deleted file mode 100644 index 76db775..0000000 --- a/app/services/api_token_creator.rb +++ /dev/null @@ -1,25 +0,0 @@ -class ApiTokenCreator - - def initialize(clock = DateTime) - @clock = clock - end - - def create(user, token) - api_token = user.add_api_token(token) - - if api_token.persisted? - update_asciicasts(api_token.token, user) - end - end - - private - - attr_reader :clock - - def update_asciicasts(token, user) - Asciicast.where(:user_id => nil, :api_token => token). - update_all(:user_id => user.id, :api_token => nil, - :updated_at => clock.now) - end - -end diff --git a/spec/controllers/api_tokens_controller_spec.rb b/spec/controllers/api_tokens_controller_spec.rb index 3612cfd..b3a9458 100644 --- a/spec/controllers/api_tokens_controller_spec.rb +++ b/spec/controllers/api_tokens_controller_spec.rb @@ -2,51 +2,58 @@ require 'spec_helper' describe ApiTokensController do - let(:api_token_creator) { double('api_token_creator') } - - before do - allow(controller).to receive(:api_token_creator) { api_token_creator } - end - describe '#create' do - let(:claimed_num) { 0 } - let(:api_token) { build(:api_token, :user => nil) } - let(:user) { create(:user) } + subject { get :create, api_token: 'a-toh-can' } + + let(:user) { double('user', assign_api_token: nil) } before do - login_as user - allow(api_token_creator).to receive(:create). - with(user, api_token.token) { claimed_num } - get :create, api_token: api_token.token + login_as(user) end context 'for guest user' do let(:user) { nil } + before do + subject + end + it { should redirect_to(login_path) } + specify { expect(flash[:notice]).to match(/sign in to proceed/) } end - context "when # of claimed asciicasts is nil" do - let(:claimed_num) { nil } - - it 'displays error page' do - expect(response).to render_template(:error) + context "when assigning succeeds" do + before do + allow(user).to receive(:assign_api_token).with('a-toh-can') + subject end + + it { should redirect_to(profile_path(user)) } + + specify { expect(flash[:notice]).to_not be_blank } end - context "when # of claimed asciicast is 0" do - let(:claimed_num) { 0 } + context "when token is invalid" do + before do + allow(user).to receive(:assign_api_token).with('a-toh-can'). + and_raise(ActiveRecord::RecordInvalid, ApiToken.new) + end - it { should redirect_to(profile_path(user)) } - specify { expect(flash[:notice]).to match(/Authenticated/) } + it 'displays error page' do + expect(subject).to render_template(:error) + end end - context "when # of claimed asciicast is > 0" do - let(:claimed_num) { 1 } + context "when token is taken" do + before do + allow(user).to receive(:assign_api_token).with('a-toh-can'). + and_raise(ApiToken::ApiTokenTakenError) + end - it { should redirect_to(profile_path(user)) } - specify { expect(flash[:notice]).to match(/Claimed #{claimed_num}/) } + it 'displays error page' do + expect(subject).to render_template(:error) + end end end diff --git a/spec/models/api_token_spec.rb b/spec/models/api_token_spec.rb index 9879c27..730c893 100644 --- a/spec/models/api_token_spec.rb +++ b/spec/models/api_token_spec.rb @@ -1,7 +1,76 @@ require 'spec_helper' describe ApiToken do - it "has valid factory" do - expect(build(:api_token)).to be_valid + + it { should validate_presence_of(:user) } + it { should validate_presence_of(:token) } + + describe "uniqueness validation" do + before do + create(:api_token) + end + + it { should validate_uniqueness_of(:token) } + end + + describe '.for_token' do + subject { described_class.for_token(token) } + + context "when ApiToken with given token exists" do + let(:token) { attributes_for(:api_token)[:token] } + let!(:api_token) { create(:api_token, token: token) } + + it { should eq(api_token) } + end + + context "when ApiToken with given token doesn't exist" do + let(:token) { 'no-no' } + + it { should be(nil) } + end + end + + describe '#reassign_to' do + subject { api_token.reassign_to(target_user) } + + let(:api_token) { described_class.new } + let(:user) { User.new } + let(:target_user) { User.new } + + before do + api_token.user = user + allow(user).to receive(:merge_to) + end + + context "when source user is a dummy user" do + before do + user.dummy = true + end + + it "merges user to target user" do + subject + expect(user).to have_received(:merge_to).with(target_user) + end + end + + context "when target user is the same user" do + let(:target_user) { user } + + it "doesn't do anything" do + subject + expect(user).to_not have_received(:merge_to) + end + end + + context "when source user is a real user" do + before do + user.dummy = false + end + + it "raises ApiTokenTakenError" do + expect { subject }.to raise_error(ApiToken::ApiTokenTakenError) + end + end end + end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0c35fa6..60a2b28 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -136,7 +136,7 @@ describe User do end it "assigns given api token to the user" do - expect(subject.api_tokens.first.token).to eq(token) + expect(subject.api_tokens.reload.first.token).to eq(token) end context "and username is nil" do @@ -201,29 +201,33 @@ describe User do end end - describe '#add_api_token' do - let(:user) { build(:user) } + describe '#assign_api_token' do + subject { user.assign_api_token(token) } - before { user.save } + let(:user) { create(:user) } + let(:token) { 'a33e6188-f53c-11e2-abf4-84a6c827e88b' } - context "when user doesn't have given token" do - let(:token) { attributes_for(:api_token)[:token] } + before do + allow(ApiToken).to receive(:for_token).with(token) { api_token } + end - it 'returns created ApiToken' do - ut = user.add_api_token(token) - expect(ut).to be_kind_of(ApiToken) - expect(ut.id).not_to be(nil) - end + context "when given token doesn't exist" do + let(:api_token) { nil } + + it { should be_kind_of(ApiToken) } + it { should be_persisted } + specify { expect(subject.token).to eq(token) } end - context "when user doesn't have given token" do - let(:existing_token) { create(:api_token, :user => user) } - let(:token) { existing_token.token } + context "when given token already exists" do + let(:api_token) { double('api_token', reassign_to: nil) } - it 'returns existing ApiToken' do - ut = user.add_api_token(token) - expect(ut).to eq(existing_token) + it "reassigns it to the user" do + subject + expect(api_token).to have_received(:reassign_to).with(user) end + + it { should be(api_token) } end end @@ -269,4 +273,38 @@ describe User do end end + describe '#merge_to' do + subject { user.merge_to(target_user) } + + let(:user) { create(:user) } + let(:target_user) { create(:user) } + let!(:api_token_1) { create(:api_token, user: user) } + let!(:api_token_2) { create(:api_token, user: user) } + let!(:asciicast_1) { create(:asciicast, user: user) } + let!(:asciicast_2) { create(:asciicast, user: user) } + + before do + subject + end + + it "reassigns all user api tokens to the target user" do + api_token_1.reload + api_token_2.reload + + expect(api_token_1.user).to eq(target_user) + expect(api_token_2.user).to eq(target_user) + end + + it "reassigns all user asciicasts to the target user" do + asciicast_1.reload + asciicast_2.reload + + expect(asciicast_1.user).to eq(target_user) + expect(asciicast_2.user).to eq(target_user) + end + + it "removes the source user" do + expect(user).to be_destroyed + end + end end diff --git a/spec/services/api_token_creator_spec.rb b/spec/services/api_token_creator_spec.rb deleted file mode 100644 index 554ef11..0000000 --- a/spec/services/api_token_creator_spec.rb +++ /dev/null @@ -1,75 +0,0 @@ -require 'spec_helper' - -describe ApiTokenCreator do - - let(:api_token_creator) { described_class.new(clock) } - let(:clock) { double('clock', now: now) } - let(:now) { Time.now } - - describe '#create' do - let(:user) { create(:user) } - let(:token) { 'a-toh-can' } - let(:api_token) { double('api_token', token: token, - persisted?: persisted) } - - subject { api_token_creator.create(user, token) } - - before do - allow(user).to receive(:add_api_token) { api_token } - end - - context "when token was persisted" do - let(:persisted) { true } - let(:old_updated_at) { 3.days.ago } - - let!(:asciicast_1) { create(:asciicast, :user => nil, - :api_token => token, - :updated_at => old_updated_at) } - let!(:asciicast_2) { create(:asciicast, :user => nil, - :api_token => 'please', - :updated_at => old_updated_at) } - let!(:asciicast_3) { create(:asciicast, :user => create(:user), - :api_token => 'nonono', - :updated_at => old_updated_at) } - - it { should be(1) } - - it 'assigns the user to all matching asciicasts' do - subject - - asciicast_1.reload; asciicast_2.reload; asciicast_3.reload - - expect(asciicast_1.user).to eq(user) - expect(asciicast_2.user).not_to eq(user) - expect(asciicast_3.user).not_to eq(user) - end - - it 'resets the token on all matching asciicasts' do - subject - - asciicast_1.reload; asciicast_2.reload; asciicast_3.reload - - expect(asciicast_1.api_token).to be(nil) - expect(asciicast_2.api_token).not_to be(nil) - expect(asciicast_3.api_token).not_to be(nil) - end - - it 'updates the updated_at field on all matching asciicasts' do - subject - - asciicast_1.reload; asciicast_2.reload; asciicast_3.reload - - expect(asciicast_1.updated_at.to_i).to eq(now.to_i) - expect(asciicast_2.updated_at.to_i).to eq(old_updated_at.to_i) - expect(asciicast_3.updated_at.to_i).to eq(old_updated_at.to_i) - end - end - - context "when token wasn't persisted " do - let(:persisted) { false } - - it { should be(nil) } - end - end - -end