diff --git a/Gemfile b/Gemfile index bc317177..04919823 100644 --- a/Gemfile +++ b/Gemfile @@ -57,7 +57,7 @@ group :development, :test do gem 'capybara' gem 'poltergeist' gem 'byebug' - gem 'pry' + gem 'pry-rails', group: :development gem 'database_cleaner' gem 'brakeman', require: false gem 'hakiri', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 04ba9ff4..597a007f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -271,6 +271,8 @@ GEM coderay (~> 1.1.0) method_source (~> 0.8.1) slop (~> 3.4) + pry-rails (0.3.4) + pry (>= 0.9.10) puma (3.6.0) rack (1.6.4) rack-cors (0.4.0) @@ -467,7 +469,7 @@ DEPENDENCIES omniauth-saml pg poltergeist - pry + pry-rails puma rack-cors railroady @@ -497,4 +499,4 @@ RUBY VERSION ruby 2.3.3p222 BUNDLED WITH - 1.13.6 + 1.13.7 diff --git a/app/controllers/admin/auctions_controller.rb b/app/controllers/admin/auctions_controller.rb index d9e39f31..0174b49b 100644 --- a/app/controllers/admin/auctions_controller.rb +++ b/app/controllers/admin/auctions_controller.rb @@ -13,9 +13,9 @@ def new end def create - auction = BuildAuction.new(params, current_user).perform + auction = CreateAuction.new(params, current_user).perform - if SaveAuction.new(auction).perform + if auction.persisted? flash[:success] = I18n.t('controllers.admin.auctions.create.success') redirect_to admin_auction_path(auction) else @@ -33,11 +33,9 @@ def edit def update auction = Auction.find(params[:id]) - update_auction = if ArchiveAuction.archive_submit?(params) - ArchiveAuction.new(auction: auction) - else - UpdateAuction.new(auction: auction, params: params, current_user: current_user) - end + update_auction = UpdateAuction.new(auction: auction, + params: params, + current_user: current_user) if update_auction.perform flash[:success] = I18n.t('controllers.admin.auctions.update.success') diff --git a/app/models/auction.rb b/app/models/auction.rb index f353b1ec..3401c979 100644 --- a/app/models/auction.rb +++ b/app/models/auction.rb @@ -8,6 +8,7 @@ class Auction < ActiveRecord::Base belongs_to :customer has_many :bids has_many :bidders, through: :bids + has_many :states, class_name: 'AuctionState' has_and_belongs_to_many :skills has_secure_token diff --git a/app/models/auction_parser.rb b/app/models/auction_parser.rb index d4147eb2..5ad98e12 100644 --- a/app/models/auction_parser.rb +++ b/app/models/auction_parser.rb @@ -7,7 +7,7 @@ def initialize(params, user) end def attributes - auction_params.merge( + @_attributes ||= auction_params.merge( delivery_due_at: delivery_due_at, ended_at: ended_at, started_at: started_at, @@ -16,8 +16,24 @@ def attributes ).delete_if { |_key, value| value.nil? } end + def published_param + if publishing? + 'published' + elsif archiving? + 'archived' + end + end + private + def publishing? + attributes['published'] == 'published' + end + + def archiving? + params.key? :archive_auction + end + def auction_params strong_params.require(:auction).permit( :billable_to, diff --git a/app/models/auction_state.rb b/app/models/auction_state.rb new file mode 100644 index 00000000..880ed905 --- /dev/null +++ b/app/models/auction_state.rb @@ -0,0 +1,3 @@ +class AuctionState < ActiveRecord::Base + belongs_to :auction +end diff --git a/app/services/change_state.rb b/app/services/change_state.rb new file mode 100644 index 00000000..8ac83bbe --- /dev/null +++ b/app/services/change_state.rb @@ -0,0 +1,36 @@ +class ChangeState + def initialize(auction, state_name, state_value) + @auction = auction + @state_name = state_name + @state_value = state_value + end + + def perform + state.state_value = state_value + state.save + end + + def state + @state ||= find_or_build_state_record + end + + def self.perform(auction, name, value) + new(auction, name, value).perform + end + + private + + attr_reader :auction, :state_name, :state_value + + def find_or_build_state_record + find_state_record || build_state_record + end + + def find_state_record + auction.states.find {|state| state.name == state_name} + end + + def build_state_record + auction.states.build(name: state_name, state_value: state_value) + end +end diff --git a/app/services/create_auction.rb b/app/services/create_auction.rb new file mode 100644 index 00000000..296b5ce7 --- /dev/null +++ b/app/services/create_auction.rb @@ -0,0 +1,42 @@ +class CreateAuction + def initialize(params, current_user) + @params = params + @current_user = current_user + end + + def perform + build_auction + save_auction + create_auction_states + + auction + end + + private + + attr_reader :params, :current_user, :auction + + def create_auction_states + create_published_state + create_work_state + # add more states as needed + end + + def create_published_state + change_state = ChangeState.new(auction, 'published', 'unpublished') + change_state.perform + end + + def create_work_state + change_state = ChangeState.new(auction, 'work', 'not_started') + change_state.perform + end + + def build_auction + @auction ||= BuildAuction.new(params, current_user).perform + end + + def save_auction + SaveAuction.new(auction).perform + end +end diff --git a/app/services/update_auction.rb b/app/services/update_auction.rb index 82517f00..b3b195e7 100644 --- a/app/services/update_auction.rb +++ b/app/services/update_auction.rb @@ -6,17 +6,33 @@ def initialize(auction:, params:, current_user:) end def perform - assign_attributes - update_auction_ended_job - perform_accepted_auction_tasks - perform_rejected_auction_tasks - auction.save + create_auction_states + + if ArchiveAuction.archive_submit?(params) + ArchiveAuction.new(auction: auction).perform + else + assign_attributes + update_auction_ended_job + perform_accepted_auction_tasks + perform_rejected_auction_tasks + auction.save + end end private attr_reader :auction, :params, :current_user + def create_auction_states + create_published_state + # add more state creation here, as needed + end + + def create_published_state + change_state = ChangeState.new(auction, 'published', parser.published_param) + change_state.perform + end + def assign_attributes auction.assign_attributes(parsed_attributes) end @@ -71,8 +87,12 @@ def auction_rejected? parsed_attributes[:status] == 'rejected' end + def parser + @_parser ||= AuctionParser.new(params, user) + end + def parsed_attributes - @_parsed_attributes ||= AuctionParser.new(params, user).attributes + parser.attributes end def user diff --git a/db/migrate/20170111193433_create_auction_states.rb b/db/migrate/20170111193433_create_auction_states.rb new file mode 100644 index 00000000..aa916f84 --- /dev/null +++ b/db/migrate/20170111193433_create_auction_states.rb @@ -0,0 +1,9 @@ +class CreateAuctionStates < ActiveRecord::Migration + def change + create_table :auction_states do |t| + t.integer :auction_id + t.string :state_value + t.string :name + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 0a3cbc9d..c78ddd2b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,11 +11,16 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20161221232955) do - +ActiveRecord::Schema.define(version: 20170111193433) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" + create_table "auction_states", force: :cascade do |t| + t.integer "auction_id" + t.string "state_value" + t.string "name" + end + create_table "auctions", force: :cascade do |t| t.string "issue_url", default: "" t.integer "start_price", default: 3500, null: false diff --git a/docker-compose.yml b/docker-compose.yml index 2e015bf7..a1ec4080 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,6 +1,8 @@ db: image: postgres:9 web: + tty: true + stdin_open: true build: . env_file: .env environment: diff --git a/docs/moving-states-to-tables.md b/docs/moving-states-to-tables.md new file mode 100644 index 00000000..db08943c --- /dev/null +++ b/docs/moving-states-to-tables.md @@ -0,0 +1,126 @@ +Refactoring data pattern: +* Build the new data abstraction +* Find all writes +* Write to old and new location (ongoing, evented) +* Backfill task to populate new location (batch) +* Read from the new location +* Remove old writes (in code) +* Remove old fields (in database) + +## splitting apart delivery_status + +enum delivery_status: { + pending_delivery: 0, - (DEFAULT in db) + work_in_progress: 5, - + missed_delivery: 6, - + pending_acceptance: 3, - + accepted_pending_payment_url: 4, + accepted: 1, - + rejected: 2 - +} + +work + +- not_started (pending_delivery) +- in_progress (work_in_progress) +- delivered (pending_acceptance) +- not_delivered (missed_delivery) + +acceptance + +- not_evaluated +- accepted (accepted) +- rejected (rejected) + +issue: how should information about payment_url be pulled out? + +I think we can just stop using payment url info as part of these state buckets. + +For example, when we need to find `payment_needed` auctions, we currently use: + +``` +relation + .accepted_or_accepted_and_pending_payment_url + .not_paid +``` + +If we pull out `delivery_status` into `work` and `acceptance`, then we can find `payment_needed_auctions` with something like: + +``` +relation + .accepted + .missing_payment_url + .not_paid +``` + +(note: missing_payment_url scope is not currently implemented) + +--------- +# try this one first: +Published: unpublished => published | archived + +# next up +Auction work state: not started -> in progress -> delivered | not delivered +Work acceptance: not evaluated -> accepted | rejected + +Auction bidding state: not yet bidding -> bidding -> closed +Auction won state: no bids -> won + +Budget: not requested => requested => approved | rejected +Payment: not paid -> payment sent -> payment confirmed ?? + + +create migration + +create_table :auction_states do |t| + t.string 'name' + t.string '_value' # reserved :( + t.int 'auction_id' +end + +class SetAuctionState + def initialize(name, auction) + end +end + +SetAuctionState.new('published', auction).set('published').save + +class PublishedAuctionStates + StateValues = { + published: ['not published', 'published', 'archived'] + }.freeze + + def initialize(name, auction) + end + + #def name + # 'published' + #end + + def state + # @state = AuctionStates.where(auction_id: auciton.id, name: name) + end + + def new_state + # AuctionState.new(auction_id: auction.id) + auction.states.build + end + + def save + auction.save + state.save + end + + private + + def set(value) + # validator = "#{name.capitalize.constantize}Validator + # polymporphism here, for validation + # do we want to validate the value + valid_states = StateValues[name] + raise if !(valid_states.include?(value)) + state._value = value; + end +end + +set_state('visibility', 'published') diff --git a/spec/models/auction_parser_spec.rb b/spec/models/auction_parser_spec.rb index 1797df7e..be76b030 100644 --- a/spec/models/auction_parser_spec.rb +++ b/spec/models/auction_parser_spec.rb @@ -1,7 +1,70 @@ require 'rails_helper' describe AuctionParser do - describe '#perform' do + describe '#published_params' do + context 'when the params contains archive_auction' do + it 'returns archived' do + user = create(:user) + params = { + auction: {title: 'the title'}, + archive_auction: :Auction + } + parser = AuctionParser.new(params, user) + + expect(parser.published_param).to eq('archived') + end + end + + context 'when the params does not contain archive_auction' do + it 'returns nil' do + user = create(:user) + params = { + auction: {title: 'the title'} + } + parser = AuctionParser.new(params, user) + + expect(parser.published_param).to be nil + end + end + + context 'when the params contains {published => published}' do + it 'returns published' do + user = create(:user) + params = { + auction: { + title: 'title', + description: 'description', + github_repo: 'github url', + issue_url: 'issue url', + published: 'published' + } + } + parser = AuctionParser.new(params, user) + + expect(parser.published_param).to eq('published') + end + end + + context 'when the params contains {published => unpublished}' do + it 'returns nil' do + user = create(:user) + params = { + auction: { + title: 'title', + description: 'description', + github_repo: 'github url', + issue_url: 'issue url', + published: 'unpublished' + } + } + parser = AuctionParser.new(params, user) + + expect(parser.published_param).to be nil + end + end + end + + describe '#attributes' do context 'attributes in params' do it 'assigns attributes correctly' do user = create(:user) diff --git a/spec/services/change_state_spec.rb b/spec/services/change_state_spec.rb new file mode 100644 index 00000000..90eee08d --- /dev/null +++ b/spec/services/change_state_spec.rb @@ -0,0 +1,29 @@ +require 'rails_helper' + +describe ChangeState, '#perform' do + it 'when there is no associated state record, it creates a new record' do + auction = create(:auction) + service = ChangeState.new(auction, 'visibility', 'published') + + expect { + service.perform + }.to change { auction.states.to_a.length }.by(1) + + service.state.reload + expect(service.state.name).to eq('visibility') + expect(service.state.state_value).to eq('published') + end + + it 'will update of the state record when the state with that name already exsits' do + auction = create(:auction) + auction.states.create(name: 'visibility', state_value: 'draft') + service = ChangeState.new(auction, 'visibility', 'published') + + expect { + service.perform + }.to change { auction.states.to_a.length }.by(0) + + service.state.reload + expect(service.state.state_value).to eq('published') + end +end diff --git a/spec/services/create_auction_spec.rb b/spec/services/create_auction_spec.rb new file mode 100644 index 00000000..9546dd73 --- /dev/null +++ b/spec/services/create_auction_spec.rb @@ -0,0 +1,118 @@ +require 'rails_helper' + +describe CreateAuction do + describe '#perform' do + it 'returns an auction' do + params = { + auction: { + title: 'hello' + } + } + current_user = create(:user) + create_auction = CreateAuction.new(params, current_user) + auction = create_auction.perform + + expect(auction).to be_a Auction + end + + context 'when the auction is valid' do + let(:valid_auction_params) do + { + "title"=>"This is the form-edited title", + "started_at"=>"2017-01-30", + "started_at(1i)"=>"11", + "started_at(2i)"=>"30", + "started_at(3i)"=>"AM", + "ended_at"=>"2017-01-30", + "ended_at(1i)"=>"4", + "ended_at(2i)"=>"45", + "ended_at(3i)"=>"PM", + "due_in_days"=>"6", + "delivery_due_at"=>"2017-02-7", + "start_price"=>"3500", + "type"=>"sealed_bid", + "summary"=>"The Summary!", + "description"=>"and the admin related stuff", + "skill_ids"=>[""], + "github_repo"=>"https://github.com/18F/calc", + "issue_url"=>"https://github.com/18F/calc/issues/255", + "purchase_card"=>"default", + "c2_status"=>"not_requested", + "customer_id"=>"", + "billable_to"=>"Client Account 1 (Billable)", + "notes"=>"" + } + end + + it 'saves the auction' do + params = HashWithIndifferentAccess.new({ + auction: valid_auction_params, + commit: "Create", + controller: "admin/auctions", + action: "create" + }) + + current_user = create(:user) + + create_auction = CreateAuction.new(params, current_user) + + expect { + create_auction.perform + }.to change { Auction.count }.by(1) + end + + it "creates a published state of 'unpublished'" do + params = HashWithIndifferentAccess.new({ + auction: valid_auction_params, + commit: "Create", + controller: "admin/auctions", + action: "create" + }) + + current_user = create(:user) + + auction = CreateAuction.new(params, current_user).perform + + published_state = auction + .states + .find {|state| state.name == 'published'} + + expect(published_state.state_value).to eq('unpublished') + end + + it "creates a work state of 'not_started'" do + params = HashWithIndifferentAccess.new({ + auction: valid_auction_params, + commit: "Create", + controller: "admin/auctions", + action: "create" + }) + + current_user = create(:user) + + auction = CreateAuction.new(params, current_user).perform + + published_state = auction + .states + .find {|state| state.name == 'work'} + + expect(published_state.state_value).to eq('not_started') + end + end + + context 'when the auction is not valid' do + it 'does not save the auction' do + params = { + auction: {title: nil} + } + current_user = create(:user) + + create_auction = CreateAuction.new(params, current_user) + + expect { + create_auction.perform + }.to change { Auction.count }.by(0) + end + end + end +end diff --git a/spec/services/update_auction_spec.rb b/spec/services/update_auction_spec.rb index 50192375..4141eb4e 100644 --- a/spec/services/update_auction_spec.rb +++ b/spec/services/update_auction_spec.rb @@ -2,6 +2,83 @@ describe UpdateAuction do describe '#perform' do + context 'when archiving an auction' do + context 'when there is no associated auction_state' do + it 'creates a new AuctionState and sets value to archived' do + auction = create(:auction, :unpublished) + + params = { + auction: {"foo" => "bar"}, + archive_auction: :Archive + } + + expect do + UpdateAuction.new( + auction: auction, + params: params, + current_user: auction.user + ).perform + end.to change { AuctionState.count }.by(1) + + auction.reload + + published_state = auction.states.find {|state| state.name == 'published'} + expect(published_state.state_value).to eq('archived') + end + end + + context 'when there is an associated auction_state' do + it 'updates the AuctionState and sets value to archived' do + auction = create(:auction, :unpublished) + + # adding the existing auction_state + # not using a factory since we want to use less of them + # this could eventually be pulled into a builder class/method + auction.states.build(name: 'published', state_value: 'unpublished') + auction.save + + params = { + auction: {"foo" => "bar"}, + archive_auction: :Archive + } + + expect do + UpdateAuction.new( + auction: auction, + params: params, + current_user: auction.user + ).perform + end.to change { AuctionState.count }.by(0) + + auction.reload + + published_state = auction.states.find {|state| state.name == 'published'} + expect(published_state.state_value).to eq('archived') + end + end + end + + context 'when publishing an auction' do + it 'creates an AuctionState record' do + auction = create(:auction, :unpublished) + + params = { + auction: {"published" => "published"} + } + + UpdateAuction.new( + auction: auction, + params: params, + current_user: auction.user + ).perform + + auction.reload + + published_state = auction.states.find {|state| state.name == 'published'} + expect(published_state.state_value).to eq('published') + end + end + context 'when changing ended_at' do context 'job not enqueued' do it 'creates the Auction Ended Job' do