VSTS-3397 - Maropost Subscriptions #10
Conversation
* Add `simplecov` and set the minimum coverage to 100%. * Add a spec for `Maropost.configure`. * [**breaking change**] - Refactor `Maropost::Contact` to store subscriptions inside a `lists` hash (as opposed to unique instance accessors). This allows us to iterate subscriptions easily to simplify logic as well as allow for future updates. * Fix the `DoNotMailList` - previously, the user was not opted into the do not mail list when they had no subscriptions. * Make the `.rspec` file include `spec_helper`. Remove `spec_helper` inclusions in individual tests. * Fix awkward method call syntax in `Maropost::Api`. * Use `class << self` where possible - this allows for private class methods. SEE: https://amaabca.visualstudio.com/membership_backlog/_queries?id=3397
|
📖 |
lib/maropost/api.rb
Outdated
| rescue RestClient::ResourceNotFound | ||
| nil | ||
| end | ||
| class << self |
There was a problem hiding this comment.
do you prefer this to keep class methods contained in a block?
There was a problem hiding this comment.
I personally prefer it because it allows private class-level methods:
class Foo
private
def self.bar
'called!'
end
end
Foo.bar # => "called!"vs.
class Foo
class << self
private
def bar
'called!'
end
end
end
Foo.bar # => NoMethodError: private method `bar' called for Foo:ClassThough maybe there's an argument against the use of private class methods.
lib/maropost/api.rb
Outdated
| else | ||
| create(contact) | ||
| def update_subscriptions(contact) | ||
| if existing_contact = find(contact.email) |
There was a problem hiding this comment.
is it worth doing the same kind of presence check on these methods as you have on the find method?
lib/maropost/do_not_mail_list.rb
Outdated
| maropost_url('global_unsubscribes/email.json', "contact[email]=#{CGI.escape(contact.email)}") | ||
| ) | ||
| JSON.parse(response.body) | ||
| response['id'].present? |
lib/maropost/do_not_mail_list.rb
Outdated
| def request(method, url, payload = {}) | ||
| RestClient::Request.logged_request( | ||
| method: method, | ||
| read_timeout: 10, |
There was a problem hiding this comment.
Was going to say we could make this configurable, but we shouldn't have to bump the read_timeout.
There was a problem hiding this comment.
Yea it should be configuration. With a default.
| @@ -0,0 +1,4 @@ | |||
| SimpleCov.start do | |||
| SimpleCov.minimum_coverage 100.00 | |||
There was a problem hiding this comment.
would it be worth adding rubocop? how many violations would the current version of the gem have?
There was a problem hiding this comment.
I'd like to. Let's give it a shot and see!
lib/maropost/contact.rb
Outdated
| lists | ||
| errors | ||
| ).freeze | ||
| LISTS = %i( |
There was a problem hiding this comment.
I like this. It makes it clearer what's a subscription vs a subscriber attribute. Too bad cell_phone_number is an outlier.
|
👋 |
__PR Review__ * Refactor common API handling code into a `Service` class. We no longer have to store state inside method call parameters. * Add a request_id to every external request. * Make the open_timeout and read_timeout configurable for the gem. * Refactor query handling - use methods provided in RestClient to do the work for us. * We must specify the `auth_token` parameter on `get` requests as per Maropost's documentation - this parameter is needed in the url. * Implement rubocop and fix all warnings. SEE: https://amaabca.visualstudio.com/membership_backlog/_queries?id=3397
|
📖 |
|
|
||
| private | ||
|
|
||
| def initialize_lists(opts = {}) |
|
🐘 🐘 🐘 |
🐘
simplecovand set the minimum coverage to 100%.Maropost.configure.Maropost::Contactto storesubscriptions inside a
listshash (as opposed to uniqueinstance accessors). This allows us to iterate subscriptions
easily to simplify logic as well as allow for future updates.
DoNotMailList- previously, the user was not optedinto the do not mail list when they had no subscriptions.
.rspecfile includespec_helper. Removespec_helperinclusions in individual tests.
Maropost::Api.class << selfwhere possible - this allows for privateclass methods.
SEE: https://amaabca.visualstudio.com/membership_backlog/_queries?id=3397