-
Notifications
You must be signed in to change notification settings - Fork 31
Add first upsert methods for Deals and Contact #83
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: master
Are you sure you want to change the base?
Changes from all commits
0ea6dac
06356c5
f96d4a9
84195f5
0798f7b
8046592
29c8bd5
a2917a3
70927cc
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 |
|---|---|---|
|
|
@@ -105,6 +105,28 @@ def update(deal) | |
| Deal.new(root[:data]) | ||
| end | ||
|
|
||
| # Upsert a deal | ||
| # | ||
| # post '/deals/upsert?filter_param=filter_value | ||
| # | ||
| # Create a new deal or update an existing, based on a value of a filter or a set of filters. | ||
| # At least a single filter - query parameter - is required. If no parameters are present, the request will return an error. | ||
| # See full docs https://developers.getbase.com/docs/rest/reference/deals | ||
| # | ||
| # @param filters [Hash] - hash contain filters, one level deep e.g. { name: 'string', 'custom_fields[field]': 'value' } | ||
| # @param deal [Deal, Hash] - This object's attributes describe the object to be updated or created | ||
| # @return [Deal] The resulting object representing updated or created resource. | ||
| def upsert(filters, deal) | ||
| validate_upsert_filters!(filters) | ||
| validate_type!(deal) | ||
|
|
||
| attributes = sanitize(deal) | ||
| query_string = URI.encode_www_form(filters) | ||
|
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 was trying to find a way on encoding the hash without using Rails only methods (like Object.to_query) or a new gem like Addressable, this seems to do the trick, but other opinions welcome |
||
| _, _, root = @client.post("/deals/upsert?#{query_string}", attributes) | ||
redroot marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| Deal.new(root[:data]) | ||
| end | ||
|
|
||
|
|
||
| # Delete a deal | ||
| # | ||
|
|
@@ -127,6 +149,11 @@ def validate_type!(deal) | |
| raise TypeError unless deal.is_a?(Deal) || deal.is_a?(Hash) | ||
| end | ||
|
|
||
| def validate_upsert_filters!(filters) | ||
| raise TypeError unless filters.is_a?(Hash) | ||
| raise ArgumentError, "at least one filter is required" if filters.empty? | ||
| end | ||
|
|
||
| def extract_params!(deal, *args) | ||
| params = deal.to_h.select{ |k, _| args.include?(k) } | ||
| raise ArgumentError, "one of required attributes is missing. Expected: #{args.join(',')}" if params.count != args.length | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| it { should respond_to :destroy } | ||
| it { should respond_to :find } | ||
| it { should respond_to :update } | ||
| it { should respond_to :upsert } | ||
| it { should respond_to :where } | ||
| end | ||
|
|
||
|
|
@@ -48,6 +49,22 @@ | |
| end | ||
| end | ||
|
|
||
| describe :upsert do | ||
| it 'raises a TypeError if filters is nil' do | ||
| expect(client.contacts.upsert(nil, { name: 'unique_name' }).to raise_error(TypeError) | ||
| end | ||
|
|
||
| it 'raises an ArgumentError if filters is empty' do | ||
| expect(client.contacts.upsert({}, { name: 'unique_name' }).to raise_error(ArgumentError) | ||
| end | ||
|
|
||
| it 'calls the upsert route with encoded filters' do | ||
| filters = { last_name: 'unique_name', 'custom_fields[external_id]': 'unique-1' } | ||
| contact = create(:contact, last_name: 'unique_name', custom_fields: { external_id: 'unique-1'}) | ||
| expect(client.contacts.upsert(filters, contact)).to be_instance_of BaseCRM::Contact | ||
| end | ||
|
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 test seems to cover the "update" path, but we are missing the "insert" path.
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. Agreed, waiting on some guidance on the test suite in general here #83 (comment) before i improve this but i agree |
||
| end | ||
|
|
||
| describe :destroy do | ||
| it "returns true on success" do | ||
| @contact = create(:contact) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| it { should respond_to :destroy } | ||
| it { should respond_to :find } | ||
| it { should respond_to :update } | ||
| it { should respond_to :upsert } | ||
| it { should respond_to :where } | ||
|
|
||
| end | ||
|
|
@@ -59,6 +60,22 @@ | |
| end | ||
| end | ||
|
|
||
| describe :upsert do | ||
| it 'raises a TypeError if filters is nil' do | ||
| expect(client.deals.upsert(nil, { name: 'unique_name' }).to raise_error(TypeError) | ||
| end | ||
|
|
||
| it 'raises an ArgumentError if filters is empty' do | ||
| expect(client.deals.upsert({}, { name: 'unique_name' }).to raise_error(ArgumentError) | ||
| end | ||
|
|
||
| it 'calls the upsert route with encoded filters' do | ||
| filters = { name: 'unique_name', 'custom_fields[external_id]': 'unique-1' } | ||
| deal = create(:deal, name: 'unique_name', custom_fields: { external_id: 'unique-1'}) | ||
| expect(client.deals.upsert(filters, deal)).to be_instance_of BaseCRM::Deal | ||
| end | ||
|
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. Same comment as above |
||
| end | ||
|
|
||
| describe :destroy do | ||
| it "returns true on success" do | ||
| @deal = create(:deal) | ||
|
|
||
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.
💡 If the intention of
upsertis to be idempotent, then you should use aPUT.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.
That may be so, but according to the docs it Zendesk Sell's case its a POST https://developers.getbase.com/docs/rest/reference/deals