-
Notifications
You must be signed in to change notification settings - Fork 30
Remove Her dependency #142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e5b2bf3
d916189
478505b
7c5086c
b6c2599
f2eebb7
42dba0b
6f42ea2
1b02044
5750615
f91a368
297e350
b2b4d4e
b4f9692
8a31499
06794e4
1cc8c9f
9a1250e
e872e45
9874de4
1a642d1
597073d
2cb115c
60c6d25
1224a75
effd79e
d7bca1d
55b0c65
847b139
3de6cee
36ec075
9539979
170a663
41aece8
57938f0
bf68235
bfe9cab
e415238
0bde9fe
3f3f1d9
f39f96c
86780f6
acc66ff
94e8388
7f4c8ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| module TestTrack::Persistence | ||
| extend ActiveSupport::Concern | ||
|
|
||
| module ClassMethods | ||
| def create!(attributes) | ||
| new(attributes).tap(&:save!) | ||
| end | ||
| end | ||
|
|
||
| def save | ||
| return false unless valid? | ||
|
|
||
| persist! | ||
| true | ||
| rescue Faraday::UnprocessableEntityError | ||
| errors.add(:base, 'The HTTP request failed with a 422 status code') | ||
| false | ||
| end | ||
|
|
||
| def save! | ||
| save or raise(ActiveModel::ValidationError, self) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rare
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ha, this is one of those rare cases where you're supposed to use |
||
| end | ||
|
|
||
| private | ||
|
|
||
| def persist! | ||
| raise NotImplementedError | ||
| end | ||
| end | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| module TestTrack::Resource | ||
| extend ActiveSupport::Concern | ||
|
|
||
| include ActiveModel::Model | ||
| include ActiveModel::Attributes | ||
|
|
||
| private | ||
|
|
||
| def _assign_attribute(name, value) | ||
| super | ||
| rescue ActiveModel::UnknownAttributeError | ||
| # Don't raise when we encounter an unknown attribute. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because there was an existing test for this behavior, but also because it makes sense with the way APIs are versioned. Also, if we don't allow unknown fields, any non-breaking change (e.g. new field is added) to the API will break the client.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do have an API versioning strategy, so the addition of a new field could (and probably should) fall under a new version release. But I can understand not introducing additional strictness as part of this PR. |
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,19 @@ | ||
| class TestTrack::Remote::Assignment | ||
| include TestTrack::RemoteModel | ||
| include TestTrack::Resource | ||
| include ActiveModel::Dirty | ||
|
|
||
| attributes :split_name, :variant, :context, :unsynced | ||
| attribute :split_name | ||
| attribute :variant | ||
| attribute :context | ||
| attribute :unsynced, :boolean | ||
|
|
||
| validates :split_name, :variant, :mixpanel_result, presence: true | ||
|
|
||
| def initialize(...) | ||
| super | ||
| clear_changes_information | ||
| end | ||
|
Comment on lines
+12
to
+15
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This resource uses |
||
|
|
||
| def unsynced? | ||
| unsynced || variant_changed? | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,11 @@ | ||
| class TestTrack::Remote::AssignmentDetail | ||
| include TestTrack::RemoteModel | ||
| include TestTrack::Resource | ||
|
|
||
| attributes :split_location, :split_name, :variant_name, :variant_description, :assigned_at | ||
|
|
||
| def assigned_at | ||
| original = super | ||
| if original.blank? || !original.respond_to?(:in_time_zone) | ||
| nil | ||
| else | ||
| original.in_time_zone rescue nil # rubocop:disable Style/RescueModifier | ||
| end | ||
| end | ||
|
Comment on lines
-6
to
-13
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume this is more or less what
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, exactly. There's a test that covers this, too. |
||
| attribute :split_location | ||
| attribute :split_name | ||
| attribute :variant_name | ||
| attribute :variant_description | ||
| attribute :assigned_at, :datetime | ||
|
|
||
| def self.fake_instance_attributes(_) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,25 @@ | ||
| class TestTrack::Remote::AssignmentEvent | ||
| include TestTrack::RemoteModel | ||
| include TestTrack::Resource | ||
| include TestTrack::Persistence | ||
|
|
||
| collection_path 'api/v1/assignment_event' | ||
|
|
||
| attributes :visitor_id, :split_name, :unsynced | ||
| attribute :visitor_id | ||
| attribute :split_name | ||
| attribute :mixpanel_result | ||
| attribute :context | ||
| attribute :unsynced, :boolean | ||
|
|
||
| validates :visitor_id, :split_name, :mixpanel_result, presence: true | ||
|
|
||
| alias unsynced? unsynced | ||
|
|
||
| def fake_save_response_attributes | ||
| nil # :no_content is the expected response type | ||
| private | ||
|
|
||
| def persist! | ||
| TestTrack::Client.request( | ||
| method: :post, | ||
| path: 'api/v1/assignment_event', | ||
| body: { context:, visitor_id:, split_name:, mixpanel_result: }, | ||
| fake: nil | ||
| ) | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,7 @@ | ||
| class TestTrack::Remote::FakeServer | ||
| include TestTrack::RemoteModel | ||
|
|
||
| def self.reset!(seed) | ||
| raise('Cannot reset FakeServer if TestTrack is enabled.') if TestTrack.enabled? | ||
|
|
||
| put('api/v1/reset', seed:) | ||
| TestTrack::Client.connection.put('api/v1/reset', seed:) | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,27 +1,31 @@ | ||
| class TestTrack::Remote::Identifier | ||
| include TestTrack::RemoteModel | ||
| include TestTrack::Resource | ||
| include TestTrack::Persistence | ||
|
|
||
| collection_path 'api/v1/identifier' | ||
|
|
||
| has_one :remote_visitor, data_key: :visitor, class_name: "TestTrack::Remote::Visitor" | ||
|
|
||
| attributes :identifier_type, :visitor_id, :value | ||
| attribute :identifier_type | ||
| attribute :visitor_id | ||
| attribute :value | ||
|
|
||
| validates :identifier_type, :visitor_id, :value, presence: true | ||
|
|
||
| def fake_save_response_attributes | ||
| { visitor: { id: visitor_id, assignments: [] } } | ||
| def visitor | ||
| @visitor or raise('Visitor data unavailable until you save this identifier.') | ||
| end | ||
|
|
||
| def visitor | ||
| @visitor ||= TestTrack::Visitor.new(visitor_opts!) | ||
| def visitor=(visitor_attributes) | ||
| @visitor = TestTrack::Remote::Visitor.new(visitor_attributes).to_visitor | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def visitor_opts! | ||
| raise("Visitor data unavailable until you save this identifier.") unless attributes[:remote_visitor] | ||
| def persist! | ||
| result = TestTrack::Client.request( | ||
| method: :post, | ||
| path: 'api/v1/identifier', | ||
| body: { identifier_type:, visitor_id:, value: }, | ||
| fake: { visitor: { id: visitor_id, assignments: [] } } | ||
| ) | ||
|
|
||
| { id: remote_visitor.id, assignments: remote_visitor.assignments } | ||
| assign_attributes(result) | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,19 @@ | ||
| class TestTrack::Remote::IdentifierType | ||
| include TestTrack::RemoteModel | ||
| include TestTrack::Resource | ||
| include TestTrack::Persistence | ||
|
|
||
| collection_path 'api/v1/identifier_type' | ||
|
|
||
| attributes :name | ||
| attribute :name | ||
|
|
||
| validates :name, presence: true | ||
|
|
||
| def fake_save_response_attributes | ||
| nil # :no_content is the expected response type | ||
| private | ||
|
|
||
| def persist! | ||
| TestTrack::Client.request( | ||
| method: :post, | ||
| path: 'api/v1/identifier_type', | ||
| body: { name: }, | ||
| fake: nil | ||
| ) | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,13 +1,30 @@ | ||||||||||||||||
| class TestTrack::Remote::SplitConfig | ||||||||||||||||
| include TestTrack::RemoteModel | ||||||||||||||||
| include TestTrack::Resource | ||||||||||||||||
| include TestTrack::Persistence | ||||||||||||||||
|
|
||||||||||||||||
| collection_path 'api/v1/split_configs' | ||||||||||||||||
|
|
||||||||||||||||
| attributes :name, :weighting_registry | ||||||||||||||||
| attribute :name | ||||||||||||||||
| attribute :weighting_registry | ||||||||||||||||
|
|
||||||||||||||||
| validates :name, :weighting_registry, presence: true | ||||||||||||||||
|
|
||||||||||||||||
| def fake_save_response_attributes | ||||||||||||||||
| nil # :no_content is the expected response type | ||||||||||||||||
| def self.destroy_existing(id) | ||||||||||||||||
| TestTrack::Client.request( | ||||||||||||||||
| method: :delete, | ||||||||||||||||
| path: "api/v1/split_configs/#{id}", | ||||||||||||||||
| fake: nil | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| nil | ||||||||||||||||
| end | ||||||||||||||||
|
Comment on lines
+10
to
+18
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What calls this method?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test_track_rails_client/app/models/test_track/config_updater.rb Lines 16 to 22 in 109bcb0
|
||||||||||||||||
|
|
||||||||||||||||
| private | ||||||||||||||||
|
|
||||||||||||||||
| def persist! | ||||||||||||||||
| TestTrack::Client.request( | ||||||||||||||||
| method: :post, | ||||||||||||||||
| path: 'api/v1/split_configs', | ||||||||||||||||
| body: { name:, weighting_registry: }, | ||||||||||||||||
| fake: nil | ||||||||||||||||
| ) | ||||||||||||||||
| end | ||||||||||||||||
| end | ||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,23 @@ | ||
| class TestTrack::Remote::SplitDetail | ||
| include TestTrack::RemoteModel | ||
| include TestTrack::Resource | ||
|
|
||
| collection_path 'api/v1/split_details' | ||
|
|
||
| attributes :name, :hypothesis, :assignment_criteria, :description, :owner, :location, :platform, :variant_details | ||
| attribute :name | ||
| attribute :hypothesis | ||
| attribute :assignment_criteria | ||
| attribute :description | ||
| attribute :owner | ||
| attribute :location | ||
| attribute :platform | ||
| attribute :variant_details | ||
|
|
||
| def self.from_name(name) | ||
| # TODO: FakeableHer needs to make this faking a feature of `get` | ||
| if faked? | ||
| new(fake_instance_attributes(name)) | ||
| else | ||
| get("api/v1/split_details/#{name}") | ||
| end | ||
| result = TestTrack::Client.request( | ||
| method: :get, | ||
| path: "api/v1/split_details/#{name}", | ||
| fake: fake_instance_attributes(name) | ||
| ) | ||
|
|
||
| new(result) | ||
| end | ||
|
|
||
| def self.fake_instance_attributes(name) | ||
|
|
@@ -39,4 +45,8 @@ def self.fake_variant_details | |
| } | ||
| ] | ||
| end | ||
|
|
||
| def variant_details=(values) | ||
| super(values.map(&:symbolize_keys)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked into that. The This project is been pretty inconsistent with usage of symbol versus string keys. Fakes mostly have symbol keys. Some of our stubs mix string/symbol keys. Splits have string keys (deep). Variant details use symbol keys (shallow). Using |
||
| end | ||
| end | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: The 422 response contains the errors that occurred, and Her would normally assign that to the record. I don't think we're relying on that functionality, so I did the bare minimum here.