Split Detail Remote Model #26

agirlnamedsophia · 2017-04-05T18:17:50Z

/domain @Betterment/test_track_core
/platform @dschaub @samandmoore

This adds a SplitDetail remote model that hits the split_details endpoint in test_track
relies on this PR to be merged first

nanda-prbot · 2017-04-05T18:17:53Z

Needs somebody from @Betterment/test_track_core to claim domain review.
Needs somebody from @dschaub and @samandmoore to claim platform review.

Use the shovel operator to claim, e.g.:

dschaub · 2017-04-06T18:21:52Z

app/models/test_track/remote/split_detail.rb

+  attributes :name
+
+  def self.from_name(name)
+    raise "must provide a name" unless name.present?


is this necessary? if you pass a blank value you'll get a 404, and if you leave the argument out you'll get a ruby error

dschaub · 2017-04-06T18:22:19Z

app/models/test_track/remote/split_detail.rb

+
+  def self.fake_instance_attributes(_)
+    {
+      split_name: "fake_visitor_id",


fake_split_name?

dschaub · 2017-04-06T18:22:39Z

app/models/test_track/remote/split_detail.rb

+      description: "fake_description",
+      owner: "fake_retail",
+      location: "fake_activity",
+      platform: "fake_mobile"


that should be mobile cause it can only be one of two values, right?

dschaub · 2017-04-06T18:23:59Z

spec/models/test_track/remote/split_detail_spec.rb

+      expect(subject.description).to eq("fake_description")
+      expect(subject.owner).to eq("fake_retail")
+      expect(subject.location).to eq("fake_activity")
+      expect(subject.platform).to eq("fake_mobile")


s/fake_mobile/mobile/

dschaub · 2017-04-06T18:24:44Z

<< domain tafn

nanda-prbot · 2017-04-06T18:24:46Z

Needs somebody from @dschaub and @samandmoore to claim platform review.

Use the shovel operator to claim, e.g.:

agirlnamedsophia · 2017-04-06T18:47:31Z

nanda?

nanda-prbot · 2017-04-06T18:47:33Z

Needs somebody from @dschaub and @samandmoore to claim platform review.

Use the shovel operator to claim, e.g.:

nanda-prbot · 2017-04-06T18:47:38Z

Needs somebody from @dschaub and @samandmoore to claim platform review.

Use the shovel operator to claim, e.g.:

agirlnamedsophia · 2017-04-06T18:47:49Z

accidental click :(

dschaub · 2017-04-06T18:55:47Z

app/models/test_track/remote/split_detail.rb

+
+  collection_path '/api/v1/split_details'
+
+  attributes :name


missed this, i think we want all of the other attributes here too

dschaub

some initial thoughts on the new stuff

dschaub · 2017-04-07T19:10:58Z

app/models/test_track/fake/schema_loading.rb

@@ -0,0 +1,34 @@
+class TestTrack::Fake::SchemaLoading


we could probably just name this Schema

also, worth it to make this a singleton? otherwise if we have multiple uses it'll load the same schema file twice, right?

dschaub · 2017-04-07T19:11:32Z

app/models/test_track/fake/schema_loading.rb

+
+  def initialize
+    @split_hash = _split_hash
+  end


can we make this a public method called split_hash that memoizes the result of _split_hash? (or uses the begin...end style)

dschaub · 2017-04-07T19:18:17Z

app/models/test_track/fake/split.rb

+class TestTrack::Fake::Split
+  def self.instance
+    @instance ||= new
+  end


can we just use Singleton for this?

dschaub · 2017-04-07T19:19:43Z

spec/controllers/tt/api/v1/split_details_controller_spec.rb

+  describe '#show' do
+    it 'returns fake split details' do
+      get :show, format: :json
+      expect(response_json).to eq TestTrack::FakeServer.split_details


do we want to list out the expected values? this seems like it's just asserting that a variable is equal to itself

agirlnamedsophia · 2017-04-07T22:03:26Z

nanda?

nanda-prbot · 2017-04-07T22:03:27Z

Needs somebody from @dschaub and @samandmoore to claim platform review.

Use the shovel operator to claim, e.g.:

samandmoore

A few comments about the specifics on the implementation, but in general this looks great. Let's just catch up re: fake data offline. We should be able to get on the same page and then ship this!

samandmoore · 2017-04-09T21:51:59Z

spec/dummy/db/test_track_schema.yml

+    platform: "desktop"
+    owner: "hat makers"
+    assignment_criteria: "user has bought one hat"
+    description: "promotion test to see if users will buy more hats"


is this actually getting dumped into the schema.yml now? i don't think that it is and i don't think that it should be.

the schema.yml is only meant to track the critical details of splits, their variants, and the associated weighting. we don't want it to include any of the optional split detail data that is inputted through the admin ui.

samandmoore · 2017-04-09T21:58:50Z

app/models/test_track/fake/split.rb

+
+  def _split_details
+    split_hash = TestTrack::Fake::Schema.instance.split_hash
+    { "name" => "banner_color" }.merge(split_hash["banner_color"].except("weights"))


going along with the comment i left elsewhere about not putting the split details in the schema.yml file, i think that we should just have this return fake data just like we do in the fake_instance_attributes method.

i am also noticing that this fake doesn't include any of the details about the variants in this split. is that an intentional omission?

samandmoore · 2017-04-09T21:59:42Z

app/models/test_track/fake/split.rb

@@ -0,0 +1,14 @@
+class TestTrack::Fake::Split


i think this is actually SplitDetail, right?

agirlnamedsophia · 2017-04-10T15:40:50Z

nanda?

nanda-prbot · 2017-04-10T15:40:52Z

Needs somebody from @dschaub and @samandmoore to claim platform review.

Use the shovel operator to claim, e.g.:

… the endpoint works

agirlnamedsophia · 2017-04-10T18:40:30Z

alright, I think I've got it!

nanda?

nanda-prbot · 2017-04-10T18:40:32Z

Needs somebody from @dschaub and @samandmoore to claim platform review.

Use the shovel operator to claim, e.g.:

dschaub · 2017-04-10T19:51:09Z

domain lgtm

nanda-prbot · 2017-04-10T19:51:11Z

Needs somebody from @dschaub and @samandmoore to claim platform review.

Use the shovel operator to claim, e.g.:

samandmoore · 2017-04-10T20:50:59Z

<<domainlgtm

samandmoore · 2017-04-10T20:51:12Z

nanda?

nanda-prbot · 2017-04-10T20:51:12Z

Needs somebody from @dschaub and @samandmoore to claim platform review.

Use the shovel operator to claim, e.g.:

samandmoore · 2017-04-10T20:51:28Z

<<domainlgtm again

nanda-prbot · 2017-04-10T20:51:30Z

Needs somebody from @dschaub and @samandmoore to claim platform review.

Use the shovel operator to claim, e.g.:

samandmoore · 2017-04-10T20:51:42Z

oh. i can't read. sigh.

<<platformlgtm

nanda-prbot · 2017-04-10T20:51:44Z

Ready to merge! 🔩 🙆‍♀️ ⚡

agirlnamedsophia added 2 commits April 4, 2017 15:21

split detail spec

4380210

identifier > name

6a5c160

agirlnamedsophia added 2 commits April 5, 2017 15:19

name!

4f17d02

name not identifier model

be1eacb

dschaub reviewed Apr 6, 2017

View reviewed changes

agirlnamedsophia changed the title ~~Spit Detail Remote Model~~ Split Detail Remote Model Apr 6, 2017

feedback

1ce26a9

agirlnamedsophia closed this Apr 6, 2017

agirlnamedsophia reopened this Apr 6, 2017

dschaub reviewed Apr 6, 2017

View reviewed changes

agirlnamedsophia added 2 commits April 6, 2017 15:01

fixes specs

93c9dac

modifies dummy data to include split details, abstracts schema loading

e481bc3

dschaub reviewed Apr 7, 2017

View reviewed changes

SchemaLoading > Schema, singleton pattern, split_details controller spec

ae0c549

singleton pattern tweaks

5db1905

samandmoore requested changes Apr 9, 2017

View reviewed changes

agirlnamedsophia added 2 commits April 10, 2017 10:59

no need to modify schema

ab9ceb1

split details

8fa1c31

fake data displayed correctly

60932c0

add param to split_details_controller to more realistically match how…

289090b

… the endpoint works

samandmoore approved these changes Apr 10, 2017

View reviewed changes

agirlnamedsophia merged commit 510bde9 into master Apr 10, 2017

agirlnamedsophia deleted the sr/master/split-detail-remote-model branch April 10, 2017 20:53

Split Detail Remote Model #26

Split Detail Remote Model #26

Conversation

agirlnamedsophia commented Apr 5, 2017

nanda-prbot commented Apr 5, 2017

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

dschaub commented Apr 6, 2017

nanda-prbot commented Apr 6, 2017

agirlnamedsophia commented Apr 6, 2017

nanda-prbot commented Apr 6, 2017

nanda-prbot commented Apr 6, 2017

agirlnamedsophia commented Apr 6, 2017 • edited

Choose a reason for hiding this comment

dschaub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschaub Apr 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agirlnamedsophia commented Apr 7, 2017

nanda-prbot commented Apr 7, 2017

samandmoore 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

agirlnamedsophia commented Apr 10, 2017

nanda-prbot commented Apr 10, 2017

agirlnamedsophia commented Apr 10, 2017

nanda-prbot commented Apr 10, 2017

dschaub commented Apr 10, 2017

nanda-prbot commented Apr 10, 2017

samandmoore commented Apr 10, 2017 • edited

samandmoore commented Apr 10, 2017

nanda-prbot commented Apr 10, 2017

samandmoore commented Apr 10, 2017

nanda-prbot commented Apr 10, 2017

samandmoore commented Apr 10, 2017

nanda-prbot commented Apr 10, 2017

agirlnamedsophia commented Apr 6, 2017 •

edited

dschaub Apr 7, 2017 •

edited

samandmoore commented Apr 10, 2017 •

edited