diff --git a/app/controllers/bids_controller.rb b/app/controllers/bids_controller.rb index 812ca5a8..e4d4703c 100644 --- a/app/controllers/bids_controller.rb +++ b/app/controllers/bids_controller.rb @@ -23,31 +23,40 @@ def new end def confirm - bid = PlaceBid.new(params: params, user: current_user, via: via).dry_run - @confirm_bid = ConfirmBidViewModel.new(auction: Auction.find(params[:auction_id]), bid: bid) - rescue UnauthorizedError => error - flash[:error] = error.message - redirect_to new_auction_bid_path(params[:auction_id]) - end + bid = PlaceBid.new(params: params, bidder: current_user, via: via) + auction = Auction.find(params[:auction_id]) - def create - unless current_user.sam_accepted? - fail UnauthorizedError, "You must have a valid SAM.gov account to place a bid" + if bid.valid? + readonly_bid = bid.dry_run + @confirm_bid = ConfirmBidViewModel.new(auction: auction, bid: readonly_bid) + else + flash[:error] = bid.errors.full_messages.to_sentence + redirect_to new_auction_bid_path(auction) end + end - @bid = PlaceBid.new(params: params, user: current_user, via: via).perform + def create + @bid = PlaceBid.new(params: params, bidder: current_user, via: via) - respond_to do |format| - format.html do - flash[:bid] = "success" - redirect_to auction_path(@bid.auction) + if @bid.perform + respond_to do |format| + format.html do + flash[:bid] = "success" + redirect_to auction_path(@bid.auction) + end + format.json { render json: @bid.bid, serializer: BidSerializer } end - format.json do - render json: @bid, serializer: BidSerializer + else + respond_to do |format| + format.html do + flash[:error] = @bid.errors.full_messages.to_sentence + redirect_to new_auction_bid_path(params[:auction_id]) + end + format.json do + render json: { error: @bid.errors.full_messages.to_sentence }, status: 403 + end end end - rescue UnauthorizedError => e - respond_error(e, redirect_path: new_auction_bid_path(params[:auction_id])) end rescue_from 'ActiveRecord::RecordNotFound' do @@ -57,18 +66,4 @@ def create end end end - - private - - def respond_error(error, redirect_path: '/') - respond_to do |format| - format.html do - flash[:error] = error.message - redirect_to redirect_path - end - format.json do - render json: { error: error.message }, status: 403 - end - end - end end diff --git a/app/services/place_bid.rb b/app/services/place_bid.rb index 11657bdc..80e4769a 100644 --- a/app/services/place_bid.rb +++ b/app/services/place_bid.rb @@ -1,94 +1,37 @@ class PlaceBid - BID_LIMIT = 3500 + include ActiveModel::Validations + BID_INCREMENT = 1 - attr_reader :bid, :params, :user, :via + attr_reader :params, :bidder, :via + + validates_with PlaceBidValidator - def initialize(params:, user:, via:nil) + def initialize(params:, bidder:, via: nil) @params = params - @user = user + @bidder = bidder @via = via end def perform - validate_bid_data - create_bid + if valid? + bid.save + end end def dry_run - validate_bid_data - unsaveable_bid + bid.readonly! + bid end - def create_bid - @bid ||= Bid.create( - amount: amount, - bidder_id: user.id, - auction_id: auction.id, - via: via - ) + def bid + @_bid ||= Bid.new(amount: amount, auction: auction, bidder: bidder, via: via) end - def unsaveable_bid - @bid ||= Bid.new( - amount: amount, - bidder_id: user.id, - auction_id: auction.id, - via: via - ) - - @bid.readonly! - @bid - end - - private - def auction @auction ||= Auction.find(params[:auction_id]) end - def max_allowed_bid - rules.max_allowed_bid - end - - def validate_bid_data - unless auction_available? - fail UnauthorizedError, 'Auction not available' - end - - unless user_can_bid? - fail UnauthorizedError, 'You are not allowed to bid on this auction' - end - - if amount.to_i != amount - fail UnauthorizedError, 'Bids must be in increments of one dollar' - end - - if amount > BID_LIMIT - fail UnauthorizedError, 'Bid too high' - end - - if amount <= 0 - fail UnauthorizedError, 'Bid amount out of range' - end - - if amount > max_allowed_bid - fail UnauthorizedError, "Bids cannot be greater than the current max bid" - end - end - - def auction_available? - AuctionStatus.new(auction).available? - end - - def user_can_bid? - rules.user_can_bid?(user) - end - - def rules - @_rules ||= RulesFactory.new(auction).create - end - def amount params_amount = params[:bid][:amount] params_amount = params_amount.delete(',') if params_amount.is_a?(String) diff --git a/app/validators/place_bid_validator.rb b/app/validators/place_bid_validator.rb new file mode 100644 index 00000000..3de7f710 --- /dev/null +++ b/app/validators/place_bid_validator.rb @@ -0,0 +1,33 @@ +class PlaceBidValidator < ActiveModel::Validator + def validate(bid) + unless user_can_bid?(bid) + add_error(bid, 'permissions') + end + + if bid.amount <= 0 + add_error(bid, 'amount.below_zero') + end + + if bid.amount > max_allowed_bid(bid) + add_error(bid, 'amount.greater_than_max') + end + end + + private + + def add_error(bid, message) + bid.errors.add :base, I18n.t("activerecord.errors.models.bid.#{message}") + end + + def user_can_bid?(bid) + rules(bid).user_can_bid?(bid.bidder) + end + + def max_allowed_bid(bid) + rules(bid).max_allowed_bid + end + + def rules(bid) + RulesFactory.new(bid.auction).create + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 53756bde..c2b13199 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -8,6 +8,11 @@ en: errors: models: + bid: + permissions: 'You are not allowed to bid on this auction' + amount: + greater_than_max: 'Bids cannot be greater than the current max bid' + below_zero: 'Bid amount out of range' user: attributes: duns_number: diff --git a/spec/controllers/bids_controller_spec.rb b/spec/controllers/bids_controller_spec.rb index 894ecf5e..641660e9 100644 --- a/spec/controllers/bids_controller_spec.rb +++ b/spec/controllers/bids_controller_spec.rb @@ -155,7 +155,7 @@ expect(flash[:bid]).to eq("success") expect(response).to redirect_to("/auctions/#{auction.id}") - bid = auction.bids.order('created_at DESC').first + bid = auction.bids.last expect(bid.bidder).to eq(current_bidder) expect(bid.amount).to eq(1000) expect(bid.via).to eq('web') diff --git a/spec/factories/users.rb b/spec/factories/users.rb index dade7855..990d2d15 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -29,6 +29,5 @@ contracting_officer true end end - end end diff --git a/spec/requests/bids_spec.rb b/spec/requests/bids_spec.rb index 419609bf..6743bd95 100644 --- a/spec/requests/bids_spec.rb +++ b/spec/requests/bids_spec.rb @@ -95,7 +95,9 @@ it 'returns a json error' do post auction_bids_path(auction), params, headers - expect(json_response['error']).to eq('Auction not available') + expect(json_response['error']).to eq( + 'You are not allowed to bid on this auction' + ) end it 'returns a 403 status code' do @@ -120,7 +122,9 @@ it 'returns a json error' do post auction_bids_path(auction), params, headers - expect(json_response['error']).to eq('Auction not available') + expect(json_response['error']).to eq( + 'You are not allowed to bid on this auction' + ) end it 'returns a 403 status code' do @@ -286,7 +290,9 @@ it 'returns a json error' do post auction_bids_path(auction), params, headers - expect(json_response['error']).to eq('You must have a valid SAM.gov account to place a bid') + expect(json_response['error']).to eq( + 'You are not allowed to bid on this auction' + ) end it 'returns a 403 status code' do @@ -307,7 +313,9 @@ it 'returns a json error' do post auction_bids_path(auction), params, headers - expect(json_response['error']).to eq('You must have a valid SAM.gov account to place a bid') + expect(json_response['error']).to eq( + 'You are not allowed to bid on this auction' + ) end it 'returns a 403 status code' do @@ -364,23 +372,6 @@ end.to_not change { auction.bids.count } end end - - context 'and the bid amount contains cents' do - let(:bid_amount) { 1.99 } - - it 'returns a json error' do - post auction_bids_path(auction), params, headers - expect(json_response['error']).to eq('Bids must be in increments of one dollar') - end - - it 'returns a 403 status code' do - post auction_bids_path(auction), params, headers - expect(status).to eq(403) - end - - it 'should not create a bid' do - end - end end end end diff --git a/spec/services/place_bid_spec.rb b/spec/services/place_bid_spec.rb index e90e31b4..2c494741 100644 --- a/spec/services/place_bid_spec.rb +++ b/spec/services/place_bid_spec.rb @@ -1,218 +1,65 @@ require 'rails_helper' -RSpec.describe PlaceBid do - let(:place_bid) { PlaceBid.new(params: params, user: current_user) } - let(:place_second_bid) { PlaceBid.new(params: second_params, user: current_user) } - let(:current_user) { FactoryGirl.create(:user, sam_status: :sam_accepted) } - let(:second_user) { FactoryGirl.create(:user, sam_status: :sam_accepted) } - let(:amount) { 1005 } - let(:params) do - { - auction_id: auction_id, - bid: { - amount: amount - } - } - end - let(:second_amount) { 1000 } - let(:second_params) do - { - auction_id: auction_id, - bid: { - amount: second_amount - } - } - end - let(:auction_id) { auction.id } - let(:auction) do - FactoryGirl.create(:auction, - :multi_bid, - started_at: Time.now - 3.days, - ended_at: Time.now + 7.days) - end - - context 'when the auction start price is above the micro-purchase threshold' do - let(:auction) { create(:auction, :between_micropurchase_and_sat_threshold) } +describe PlaceBid do + describe '#valid?' do + it 'converts numbers with commas to integers' do + user = create(:user, sam_status: :sam_accepted) + auction = create(:auction, :multi_bid, :available) + create(:bid, amount: 1000, auction: auction) + params = { auction_id: auction.id, bid: { amount: '2,000' } } - context 'and the vendor is not a small business' do - let(:current_user) { create(:user, :not_small_business) } + place_bid = PlaceBid.new(params: params, bidder: user) - it 'should reject the bid' do - expect { place_bid.perform }.to raise_error(UnauthorizedError) - end - - it 'should not create a new bid' do - # need to rescue otherwise the raising of UnauthorizedError - # causes the test to error before the change can be evaluated - expect do - begin - place_bid.perform - rescue - end - end.not_to change { Bid.count } - end + expect(place_bid).not_to be_valid end - context 'and the vendor is a small business' do - let(:current_user) { create(:user, :small_business) } - - it 'should allow the bid' do - expect { place_bid.perform }.to change { Bid.count }.by(1) - end - end - end + it 'is true when bid is valid' do + user = create(:user, sam_status: :sam_accepted) + auction = create(:auction, :single_bid, :available) + params = { auction_id: auction.id, bid: { amount: 10 } } - context 'when the auction is single-bid' do - let(:auction) do - FactoryGirl.create(:auction, - :single_bid, - :with_bidders, - started_at: Time.now - 3.days, - ended_at: Time.now + 7.days) - end + place_bid = PlaceBid.new(params: params, bidder: user) - it 'should reject bids when the user has already bid on the given auction' do - expect do - place_bid.perform - place_second_bid.perform - end.to raise_error(UnauthorizedError) + expect(place_bid).to be_valid end - it 'should allow tie bids' do - params = { - auction_id: auction_id, - bid: { - amount: 10 - } - } - expect do - PlaceBid.new(params: params, user: current_user).perform - PlaceBid.new(params: params, user: second_user).perform - end.to_not raise_error - end - end + it 'is false when bid is invalid' do + user = create(:user, sam_status: :sam_accepted) + auction = create(:auction, :single_bid, :available) + params = { auction_id: auction.id, bid: { amount: 100000 } } - context 'when auction cannot be found' do - let(:auction) { double('auction', id: 1000) } + place_bid = PlaceBid.new(params: params, bidder: user) - it 'should raise a not found' do - expect { place_bid.perform }.to raise_error(ActiveRecord::RecordNotFound) + expect(place_bid).not_to be_valid end end - context 'when the auction has expired' do - let(:auction) do - FactoryGirl.create(:auction, - started_at: Time.now - 5.days, - ended_at: Time.now - 3.day) - end + describe '#perform' do + context 'valid bid' do + it 'creates a bid' do + user = create(:user, sam_status: :sam_accepted) + auction = create(:auction, :single_bid, :available) + params = { auction_id: auction.id, bid: { amount: 10 } } - it 'should raise an authorization error (because we have that baked in)' do - expect { place_bid.perform }.to raise_error(UnauthorizedError) - end - end - - context 'when the auction has not started yet' do - let(:auction) do - FactoryGirl.create(:auction, - started_at: Time.now + 5.days, - ended_at: Time.now + 7.days) - end - - it 'should raise an authorization error (same as above)' do - expect { place_bid.perform }.to raise_error(UnauthorizedError) - end - end - - context 'when the bid amount is in increments small than one dollar' do - let(:amount) { 1_000.99 } - - it 'should raise an authorization error' do - expect { place_bid.perform }.to raise_error(UnauthorizedError) - end - end - - context 'when the bid amount is above the starting price' do - let(:amount) { 3600 } - - it 'should raise an authorization error' do - expect { place_bid.perform }.to raise_error(UnauthorizedError) - end - end + place_bid = PlaceBid.new(params: params, bidder: user) + bid = place_bid.bid - context 'when the bid amount is above the current bid price' do - let(:amount) { 405 } - - it 'should raise an authorization error' do - FactoryGirl.create(:bid, auction: auction, amount: amount - 5) - expect { place_bid.perform }.to raise_error(UnauthorizedError) - end - end - - context 'when the bid amount is above the auction start price and there are no bids' do - let(:amount) { 3600 } - - it 'should raise an authorization error' do - expect { place_bid.perform }.to raise_error(UnauthorizedError) - end - end - - context 'when the bid amount is equal to the current bid price' do - let(:amount) { 400 } - - it 'should raise an authorization error' do - FactoryGirl.create(:bid, auction: auction, amount: amount) - expect { place_bid.perform }.to raise_error(UnauthorizedError) - end - end - - context 'when the bid amount is equal to the auction start price and there are no bids' do - let(:amount) { 3500 } - - it 'should raise an authorization error' do - expect { place_bid.perform }.to raise_error(UnauthorizedError) - end - end - - context 'when the bid amount is negative' do - let(:amount) { '-10' } - - it 'should raise an authorization error' do - expect { place_bid.perform }.to raise_error(UnauthorizedError) - end - end - - context 'when the bid amount is 0' do - let(:amount) { 0 } - - it 'should raise an authorization error' do - expect { place_bid.perform }.to raise_error(UnauthorizedError) - end - end - - context 'when the amount has a comma' do - let(:bid) { place_bid.bid } - let(:amount) { '1,000' } - - it 'should disregard the comma' do - place_bid.perform - expect(bid.amount).to eq(1000) + expect { place_bid.perform }.to change { Bid.count }.by(1) + expect(bid.auction).to eq(auction) + expect(bid.bidder).to eq(user) + end end - end - context 'when all the data is great' do - let(:bid) { place_bid.bid } + context 'invalid bid' do + it 'does not create a bid' do + user = create(:user) + auction = create(:auction, :single_bid, :available) + params = { auction_id: auction.id, bid: { amount: -10 } } - it 'creates a bid' do - expect { place_bid.perform }.to change { Bid.count }.by(1) - expect(bid.auction_id).to eq(auction.id) - expect(bid.bidder_id).to eq(current_user.id) - end + place_bid = PlaceBid.new(params: params, bidder: user) - it 'rounds the amount to two decimal places' do - place_bid.perform - bid.reload - expect(bid.amount).to eq(1005) + expect { place_bid.perform }.to change { Bid.count }.by(0) + end end end end diff --git a/spec/validators/place_bid_validator_spec.rb b/spec/validators/place_bid_validator_spec.rb new file mode 100644 index 00000000..335227b8 --- /dev/null +++ b/spec/validators/place_bid_validator_spec.rb @@ -0,0 +1,61 @@ +require 'rails_helper' + +describe PlaceBidValidator do + describe '#validate' do + it 'validates that the bidder is sam accepted' do + user = create(:user, sam_status: :sam_rejected) + bid = Bid.new(bidder: user, amount: valid_amount, auction: auction) + + PlaceBidValidator.new.validate(bid) + + expect(bid.errors.full_messages).to include( + I18n.t('activerecord.errors.models.bid.permissions') + ) + end + + it 'validates that the auction is availble' do + auction = build(:auction, :future) + bid = Bid.new(bidder: user, amount: valid_amount, auction: auction) + + PlaceBidValidator.new.validate(bid) + + expect(bid.errors.full_messages).to include( + I18n.t('activerecord.errors.models.bid.permissions') + ) + end + + it 'validates that the bid amount is less than the max allowed bid' do + auction = build(:auction, :multi_bid) + bid = create(:bid, auction: auction, amount: 150) + bid = Bid.new(bidder: user, amount: 155, auction: auction) + + PlaceBidValidator.new.validate(bid) + + expect(bid.errors.full_messages).to include( + I18n.t('activerecord.errors.models.bid.amount.greater_than_max') + ) + end + + it 'validates that the bid amount is greater than or equal to zero' do + bid = Bid.new(bidder: user, amount: -2, auction: auction) + + PlaceBidValidator.new.validate(bid) + + expect(bid.errors.full_messages).to include( + I18n.t('activerecord.errors.models.bid.amount.below_zero') + ) + end + end + + def auction + @_auction ||= build(:auction) + end + + def user + @_user ||= build(:user, sam_status: :sam_accepted) + end + + def valid_amount + 100 + end +end