Add/Remove from do not mail list #7
Conversation
@serene removed the attributes for inputting names from this check.
|
📖 |
lib/maropost/do_not_mail_list.rb
Outdated
|
|
||
| response['id'].present? ? true : false | ||
| rescue RestClient::ResourceNotFound, RestClient::BadRequest, RestClient::InternalServerError => e | ||
| contact.errors << 'Unexpected error occured' |
There was a problem hiding this comment.
Instead of unepected error occured, what if we use e.message for the error message? It may help debug a little quicker.
There was a problem hiding this comment.
Also, should we be rescuing from RestClient::InternalServerErrors for all of these API requests?
We use Rollbar to notify us of unhandled exceptions. If we're swallowing maropost 500 exceptions, we'll never get notifications when their API breaks (assuming 400/422 responses are common for invalid input).
lib/maropost/do_not_mail_list.rb
Outdated
| class DoNotMailList | ||
| def self.exists?(contact) | ||
| response = request(:get, | ||
| "#{Maropost.configuration.api_url}/global_unsubscribes/email.json?contact[email]=#{contact.email}") |
There was a problem hiding this comment.
What if we wrote a maropost_url(path, query) private helper method to help join the URL paths for us?
Also, could we use URI.join() here to generate the base url?
i.e.
private
def maropost_url(path, query = nil)
URI.join(Maropost.configuration.api_url, path).tap { |u| query && u.query = query }.to_s
endWe could call the method above like this:
maropost_url('/global_unsubscribes/email.json', "contact[email]=#{contact.email}")
lib/maropost/do_not_mail_list.rb
Outdated
| "#{Maropost.configuration.api_url}/global_unsubscribes/email.json?contact[email]=#{contact.email}") | ||
| JSON.parse response.body | ||
|
|
||
| response['id'].present? ? true : false |
There was a problem hiding this comment.
The ternary is not necessary here. response['id'].present? should return a boolean
lib/maropost/do_not_mail_list.rb
Outdated
| RestClient::Request.logged_request( | ||
| method: method, | ||
| timeout: 10, | ||
| open_timeout: 10, |
There was a problem hiding this comment.
Can we make our open time 5 seconds. This would match our standards elsewhere (10 second read timeout, 5 second open timeout).
lib/maropost/do_not_mail_list.rb
Outdated
| def self.request(method, url, payload = {}) | ||
| RestClient::Request.logged_request( | ||
| method: method, | ||
| timeout: 10, |
There was a problem hiding this comment.
Can we change this to read_timeout? RestClient overrides all timeouts otherwise. See: https://github.com/rest-client/rest-client#timeouts
spec/api_spec.rb
Outdated
| @@ -52,17 +52,39 @@ | |||
| it 'calls update' do | |||
| expect(Maropost::Api).to receive(:find).with(contact.email).and_return(existing_contact) | |||
| expect(Maropost::Api).to receive(:update).with(contact) | |||
| allow(Maropost::DoNotMailList).to receive(:create).with(contact) | |||
There was a problem hiding this comment.
Can we not stub methods in this file? Instead we should stub the request with WebMock.
spec/do_not_mail_list_spec.rb
Outdated
| subject { Maropost::DoNotMailList.exists?(contact) } | ||
|
|
||
| context 'email address is found on list' do | ||
| let(:found_response) { File.read File.join('spec', 'fixtures', 'do_not_mail_list', 'do_not_mail_found.json') } |
There was a problem hiding this comment.
As we're reading fixtures often, can we create a read_fixture helper? We use this helper essentially in all of rspec suites.
We should be able to (mostly) copy the code here: https://github.com/amaabca/crm/blob/master/spec/support/helpers/fixtures.rb#L11
Then include the helper in our Rspec config: https://github.com/amaabca/crm/blob/master/spec/spec_helper.rb#L24
lib/maropost/version.rb
Outdated
| @@ -1,3 +1,3 @@ | |||
| module Maropost | |||
| VERSION = '0.2.2' | |||
| VERSION = '0.2.3' | |||
There was a problem hiding this comment.
We should probably bump this to 0.3.0 as it adds new functionality.
|
💩 |
|
👋 |
|
The changes look good. Thanks! 🐘🐘🐘 |
lib/maropost/do_not_mail_list.rb
Outdated
| contact.errors << 'Unexpected error occured' | ||
| response['id'] ? true : false | ||
| rescue RestClient::ResourceNotFound, RestClient::BadRequest => e | ||
| contact.errors << "Unexpected error occurred. Error: #{e.message}" |
There was a problem hiding this comment.
Yup - works for me thanks!
| @@ -39,6 +40,10 @@ def self.update(contact) | |||
|
|
|||
| private | |||
|
|
|||
| def self.maropost_url(path, query = nil) | |||
No description provided.