WIP: Move states into separate tables #1494

adelevie · 2017-01-11T21:45:21Z

Working with @baccigalupi to pay down some technical debt by moving many of the groupings of fields on Auction into their own state objects. The disambiguation will make the code easier to reason about, and easier to modify as state needs change.

adelevie · 2017-01-11T21:51:48Z

app/services/update_auction.rb

@@ -71,8 +86,12 @@ def auction_rejected?
    parsed_attributes[:status] == 'rejected'
  end

+  def parser


Just noticed I need to be smarter about the memoization. I could fix this by memoizing attributes when AuctionParser is instantiated.

adelevie · 2017-01-11T21:52:12Z

docker-compose.yml

@@ -1,6 +1,8 @@
 db:
   image: postgres:9
 web:
+    tty: true


Useful for prying in using docker-compose and docker attach.

baccigalupi

Good start. I wrote lots of comments on the testing, and a few other places.

baccigalupi · 2017-01-11T21:54:08Z

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, foreign_key: 'auction_id', class_name: 'AuctionState'


Do you need the foreign_key? I thought you only needed to specify that if it went against convention, like with the class_name in this case.

baccigalupi · 2017-01-11T21:55:39Z

app/models/auction_parser.rb

  private

+  def changing?(key, value)


This additional abstraction is confusing to me. What is it for?

baccigalupi · 2017-01-11T21:57:39Z

app/services/update_auction.rb

+    create_published_state
+    # add more state creation here, as needed
+  end
+


This I get!

baccigalupi · 2017-01-11T21:59:40Z

spec/models/auction_parser_spec.rb

+      publishing = parser.publishing?
+
+      expect(publishing).to be true
+    end


You are going to want to test the opposite too, when not publishing to make sure that works.

baccigalupi · 2017-01-11T22:04:10Z

spec/services/update_auction_spec.rb

+        # do we want to low-level test for the published state object, like this?
+        published_state = auction.states.find {|state| state.name == 'published'}
+        expect(published_state.state_value).to eq('published')
+


^^^ that seems like the important part to test. You should probably also test what happens when an auction moves from 'archived' to 'published' in that case it will already have a record, so changing the count of records should not happen.

I forgot to mention that you can put a constraint in the database on that not happening by adding a uniq index on ['auction_id', 'name']. Then if a unique index error is thrown you revert back to updating. Solves some obscure bugs that Rails generates via their expectation that there is always just one server running in one thread.

baccigalupi · 2017-01-11T22:08:02Z

spec/services/update_auction_spec.rb

+        expect(published_state.state_value).to eq('published')
+
+        # and/or this?:
+        # expect(auction.published?).to be true


This doesn't seem important to me. It seems like that is the interface expansion that I was complaining about. Special methods for states instead of just setting state and reading state. Since the states are distributed now, we will need a nice way to read states. auction.state_for('published') # "archived" ???

baccigalupi · 2017-01-11T22:08:56Z

spec/services/update_auction_spec.rb

+        # and/or this?:
+        # published = AuctionPublishedState.new(auction).perform
+        # expect(published).to be true
+


I would only test this as the return value if you already had plans to use it somewhere. Otherwise the maintenance is overhead.

baccigalupi · 2017-01-11T22:10:28Z

spec/services/update_auction_spec.rb

+        # and/or this?:
+        # published = FindAuctionState.new(auction, name: 'published').perform
+        # expect(published).to be true
+


Future class? I don't have a problem with a separate object for getting state ... probably good. But we are going to need an easy way to read state from an auction that is less wordy.

adelevie · 2017-01-13T14:17:41Z

An idea for a state builder:

ChangeState.new(auction, published: :unpublished).perform

Although maybe raw ActiveRecord is still fine. The abstractions don't seem to abstract too much other than the column names of AuctionState, which I'm not even sure need to be hidden.

UPDATE

Ended up writing something close:

ChangeState.new(auction, 'published', 'unpublished').perform

This helps simplify UpdateAuction in that UpdateAuction#create_auction_states no longer needs to know about whether or not an AuctionState needs to be created or found-and-updated. This will be handy as we add more state changes to UpdateAuction.

adelevie · 2017-01-13T19:05:00Z

app/services/update_auction.rb

  end

  private

  attr_reader :auction, :params, :current_user

+  def create_auction_states
+    create_published_state


Let's consider changing the published state name to something like visibility.

So far: - created auction_states migration - added hook into UpdateAuction to create_auction_states - create auction state when publishing an auction - test that UpdateAuction creates an AuctionStatus when publishing

In doing so, also: - swept ArchiveAuction into UpdateAuction (so all updates pass through it and no forked behavior in the controller) - added AuctionParser#archiving? - added tests to UpdateAuction to test for archiving and to ensure published isn't inappropriately set

CreateAuction makes it easier to implement and test calls to ChangeState. In CreateAuction we will make explicit an previously implicit state: 'unpublished'. unpublished is the default published state for auctions thanks to a default call in a migration. But we will also ensure that any newly-created auctions also get a State object created reflecting this state.

This implementation isn't perfect. There's some trickness in figuring out the order of building and saving auctions and their child objects, states. ActiveRecord doesn't make this very clean to paper over. Rather than have overly-complex persistence-management logic, I'm naively sending potentially extra #save calls to the db.

adelevie added the needs review label Jan 11, 2017

adelevie changed the title ~~Move states into separate tables~~ WIP: Move states into separate tables Jan 11, 2017

adelevie commented Jan 11, 2017

View reviewed changes

baccigalupi reviewed Jan 11, 2017

View reviewed changes

adelevie force-pushed the move-states-into-separate-tables branch from af347c8 to 8870f1f Compare January 13, 2017 17:16

adelevie commented Jan 13, 2017

View reviewed changes

adelevie added 7 commits January 26, 2017 15:51

Corrected mis-named method in auction parser spec

827a6d6

First step towards moving Auction states into separate objects.

70bd6f0

So far: - created auction_states migration - added hook into UpdateAuction to create_auction_states - create auction state when publishing an auction - test that UpdateAuction creates an AuctionStatus when publishing

Removed unneeded foreign_key key in states relation

746ade0

Remove unneeded archive_params abstraction

87649ec

Created ChangeState class to manage state changes

4c13e04

Move auction archive action into create_published_state

601e0b3

adelevie force-pushed the move-states-into-separate-tables branch from 8870f1f to 601e0b3 Compare January 26, 2017 20:52

adelevie added 3 commits January 27, 2017 13:35

Use AuctionParser#published_param to reduce branching in UpdateAuction

1fab82b

Using pry-rails instead of pry

630842c

adelevie force-pushed the move-states-into-separate-tables branch from bab91c5 to 2d7ed6a Compare January 27, 2017 20:54

adelevie added 3 commits January 27, 2017 16:10

Outline breaking apart delivery_status in docs

7436f47

Create a default work state in CreateAuction

5b6f45e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Move states into separate tables #1494

WIP: Move states into separate tables #1494

adelevie commented Jan 11, 2017

adelevie Jan 11, 2017

adelevie Jan 11, 2017

baccigalupi left a comment

baccigalupi Jan 11, 2017

baccigalupi Jan 11, 2017

baccigalupi Jan 11, 2017

baccigalupi Jan 11, 2017

baccigalupi Jan 11, 2017

baccigalupi Jan 11, 2017

baccigalupi Jan 11, 2017

baccigalupi Jan 11, 2017

adelevie commented Jan 13, 2017 •

edited

Loading

adelevie Jan 13, 2017 •

edited

Loading

WIP: Move states into separate tables #1494

Are you sure you want to change the base?

WIP: Move states into separate tables #1494

Conversation

adelevie commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baccigalupi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adelevie commented Jan 13, 2017 • edited Loading

UPDATE

adelevie Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

adelevie commented Jan 13, 2017 •

edited

Loading

adelevie Jan 13, 2017 •

edited

Loading