-
Notifications
You must be signed in to change notification settings - Fork 9
Updated Mailinator to support Private Domain #13
Updated Mailinator to support Private Domain #13
Conversation
…d up Rubocop errors, added CircleCI support, updated Gems and version
Add Support for Private Domain
@ainformatico Can you take a look at this PR? |
Hey @mattrobbins I've been busy these days, I'm really sorry! I see the CI build is red, can you maybe please check why? I will take a look to this PR before the end of this week. |
Hi again @mattrobbins! thanks a lot for your PR, I've taken a look and I also think that it makes sense to release
I have created #14 and pointed your PR to that PR where I got some of your changes and also enabled some Code Analysis tools. I would suggest we keep this PR only with the changed needed for the |
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.
@mattrobbins I just left some minor comments, please let me know if you need some help on applying them, for now I will focus on making the other changes needed for v1
.rubocop.yml
Outdated
@@ -0,0 +1,40 @@ | |||
Lint/DuplicateMethods: |
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.
I would say we can go with the default community options, so no need for this file.
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.
Makes sense to me.
body: @data['data']['parts'].first['body'], | ||
body_html: retrieve_body_html, |
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.
Any special reason for removing f17d5b837f674eada25fe9e3e85cf262
?
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.
It appears the JSON has been updated to no longer have separate body and body_html elements. just the 'body'
spec/lib/mailinator/email_spec.rb
Outdated
expect(message.orig_from).to eq('Sender <sender@example.com>') | ||
expect(message.id).to eq('1419696967-44152505-recipient') | ||
expect(message.time).to eq(1_526_072_140_000) | ||
expect(message.seconds_ago).to eq(15_258) | ||
end | ||
|
||
it 'should return nil for #body_html when no html body' do |
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 body_html
needs to be removed then this test probably too.
spec/support/fake_web.rb
Outdated
file_path = File.join(__dir__, '../', 'fixtures/', filename) | ||
|
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.
Oops, my bad in the merge.
inbox = Mailinator::Inbox.get('inbox-abcd1234', private_domain: true) | ||
``` | ||
|
||
The 'download' and 'delete' work the same with private domain. |
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.
I think it's worth to also add a section where it's explained that it is possible to pass any custom option, not only private_domain
.
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.
Ok
@@ -32,3 +34,5 @@ def register_uri(url, file, method = :get) | |||
register_uri('http://api.mailinator.com:443/api/error?token=ABCD', 'error.response') | |||
register_uri('http://api.mailinator.com:443/api/delete?msgid=abcd1234&token=ABCD', 'delete.response') | |||
register_uri('http://api.mailinator.com:443/api/delete?msgid=1419696967-44152505-recipient&token=ABCD', 'delete.response') | |||
register_uri('http://api.mailinator.com:443/api/inbox?private_domain=true&to=abcd1234&token=ABCD', 'inbox_private.response') | |||
register_uri('http://api.mailinator.com:443/api/email?private_domain=true&msgid=1419696967-44152505-recipient&token=ABCD', 'email_private.response') |
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.
Metrics/LineLength: Line is too long. [148/80]
@@ -32,3 +34,5 @@ def register_uri(url, file, method = :get) | |||
register_uri('http://api.mailinator.com:443/api/error?token=ABCD', 'error.response') | |||
register_uri('http://api.mailinator.com:443/api/delete?msgid=abcd1234&token=ABCD', 'delete.response') | |||
register_uri('http://api.mailinator.com:443/api/delete?msgid=1419696967-44152505-recipient&token=ABCD', 'delete.response') | |||
register_uri('http://api.mailinator.com:443/api/inbox?private_domain=true&to=abcd1234&token=ABCD', 'inbox_private.response') |
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.
Metrics/LineLength: Line is too long. [124/80]
@@ -21,6 +21,8 @@ def register_uri(url, file, method = :get) | |||
register_uri('https://api.mailinator.com/api/error?token=ABCD', 'error.response') | |||
register_uri('https://api.mailinator.com/api/delete?msgid=abcd1234&token=ABCD', 'delete.response') | |||
register_uri('https://api.mailinator.com/api/delete?msgid=1419696967-44152505-recipient&token=ABCD', 'delete.response') | |||
register_uri('https://api.mailinator.com/api/inbox?private_domain=true&to=abcd1234&token=ABCD', 'inbox_private.response') | |||
register_uri('https://api.mailinator.com/api/email?private_domain=true&msgid=1419696967-44152505-recipient&token=ABCD', 'email_private.response') |
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.
Metrics/LineLength: Line is too long. [145/80]
@@ -21,6 +21,8 @@ def register_uri(url, file, method = :get) | |||
register_uri('https://api.mailinator.com/api/error?token=ABCD', 'error.response') | |||
register_uri('https://api.mailinator.com/api/delete?msgid=abcd1234&token=ABCD', 'delete.response') | |||
register_uri('https://api.mailinator.com/api/delete?msgid=1419696967-44152505-recipient&token=ABCD', 'delete.response') | |||
register_uri('https://api.mailinator.com/api/inbox?private_domain=true&to=abcd1234&token=ABCD', 'inbox_private.response') |
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.
Metrics/LineLength: Line is too long. [121/80]
spec/support/fake_web.rb
Outdated
file_path = File.join(__dir__, '../', 'fixtures/', filename) | ||
|
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.
Layout/TrailingWhitespace: Trailing whitespace detected.
@@ -0,0 +1,37 @@ | |||
require 'spec_helper' | |||
|
|||
describe Mailinator::Inbox do |
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.
Metrics/BlockLength: Block has too many lines. [29/25]
@@ -0,0 +1,37 @@ | |||
require 'spec_helper' |
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.
Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
expect(message.subject).to eq('This is a Subject') | ||
expect(message.request_id).to eq('638363') | ||
expect(message.body).to include('<p>This is a body</p>') | ||
expect(message.body_base_64).to include('This is a BASE64 body JVBERi0xLjUKJe') |
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.
Metrics/LineLength: Line is too long. [87/80]
expect(message.forwards_left).to eq(10) | ||
expect(message.original).to be_a(Hash) | ||
expect(message.received) | ||
.to include('from bmta1.example.com([9.9.9.9]) by mail.mailinator.com with SMTP (Postfix) for receipient@example.com; Fri, 11 May 2018 20:55:36 +0000 (UTC)') |
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.
Layout/MultilineMethodCallIndentation: Use 2 (not 0) spaces for indenting an expression spanning multiple lines.
Metrics/LineLength: Line is too long. [165/80]
@@ -7,31 +7,21 @@ module Models | |||
class Email < Base | |||
def transform_data | |||
{ | |||
id: @data['data']['id'], | |||
from_full: @data['data']['fromfull'], | |||
date: DateTime.parse(@data['data']['headers']['date']), |
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.
Style/DateTime: Prefer Date or Time over DateTime.
Pull Request Test Coverage Report for Build 96
💛 - Coveralls |
@ainformatico That sounds good to me. Sorry I was out on vacation all week last week and still getting caught up, which is why I haven't responded sooner. Do you want me to do another PR with just the private_domain updates or just update this one to have only the PR updates? |
@mattrobbins I started to copy your changes over to #14 and wanted to validate the new JSON provided by the API so I asked the Mailinator team for a trial token and still waiting for them to reply back. If you have a token already I would say let's use yours to get those fixtures updated (they already look updated in this very PR). |
Ok Makes sense. I can update it to only have the private_domain feature. I do have a token as we are a paid customer. I won't be able to send you the token but I can send you the JSON's? One without the private_domain and one with. Will that work? |
I just need the token so we can update all the fixtures, so if you can take care of that then perfect! Just remove the old fixtures and create the new ones. Don't forget to change the token from the fixtures as it is recorded in the url. |
This reverts commit a5d1704.
…N options for Email
@ainformatico I have updated the PR and merged the changes from you and resolved any conflicts. Looks like we are good to go. Let me know if I missed anything. Thanks! |
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.
Just left some cosmetic changes so we can ensure we don't have conflicts or repeated lines between branches.
I would say after these comments are addressed you can just go ahead and squash everything into a shorter list of commits that make sense or just a single commit. In case you prefer we can do it when merging by using Github's built-in button for merging with rebasing.
.gitignore
Outdated
@@ -1,5 +1,6 @@ | |||
/.bundle/ | |||
/.yardoc | |||
/Gemfile.lock |
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.
This change should not be needed as it's already on my branch: https://github.com/ainformatico/mailinator/blob/v1-release/.gitignore#L3
README.md
Outdated
@@ -1,6 +1,5 @@ | |||
[![Build Status](https://travis-ci.org/ainformatico/mailinator.svg)](https://travis-ci.org/ainformatico/mailinator) | |||
[![Code Climate](https://codeclimate.com/github/ainformatico/mailinator/badges/gpa.svg)](https://codeclimate.com/github/ainformatico/mailinator) | |||
[![Coverage Status](https://coveralls.io/repos/github/ainformatico/mailinator/badge.svg?branch=master)](https://coveralls.io/github/ainformatico/mailinator?branch=master) |
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.
This should not be removed as it has been added in the v1
branch.
Rakefile
Outdated
@@ -7,4 +7,4 @@ RSpec::Core::RakeTask.new(:spec) do |task| | |||
task.rspec_opts = ['--color', '--format', 'documentation'] | |||
end | |||
|
|||
task default: :spec |
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.
I think this should be the good one.
lib/mailinator/api.rb
Outdated
@@ -4,6 +4,7 @@ | |||
require 'json' | |||
|
|||
module Mailinator | |||
|
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.
Not really needed.
mailinator.gemspec
Outdated
@@ -9,7 +9,7 @@ Gem::Specification.new do |spec| | |||
spec.version = Mailinator::VERSION | |||
spec.authors = ['Alejandro Dev.'] | |||
spec.email = ['aeinformatico@gmail.com'] | |||
spec.summary = 'Mailinator REST API wrapper' | |||
spec.summary = %q{Mailinator REST API wrapper} |
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.
Should not be changed, I think it comes from the old Rubocop config you had.
spec/lib/mailinator/email_spec.rb
Outdated
@@ -52,6 +50,7 @@ | |||
let(:response) { subject.delete('abcd1234') } | |||
|
|||
it 'deletes an email' do | |||
response = Mailinator::Email.delete('abcd1234') |
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.
This should be covered by the let(:response) { subject.delete('abcd1234') }
on the lines above.
spec/lib/mailinator/model_spec.rb
Outdated
@@ -3,7 +3,8 @@ | |||
require 'spec_helper' | |||
|
|||
describe Mailinator::Models::Base do | |||
let(:data) { { custom: :custom } } | |||
|
|||
let(:data) { {custom: :custom} } |
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.
Not really necessary change IMHO.
spec/support/fake_web.rb
Outdated
file_path = File.join(__dir__, '../', 'fixtures/', filename) | ||
|
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.
These lines should not be in this PR.
I think you should also merge my branch to ensure you have the latest changes. |
@@ -3,6 +3,7 @@ | |||
require 'spec_helper' | |||
|
|||
describe Mailinator::Models::Base do | |||
|
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.
Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body beginning.
when 404 | ||
fail NotFound | ||
else | ||
fail RequestError, {status: response.message, code: response.code} |
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.
Style/SignalException: Always use raise to signal exceptions.
Layout/SpaceInsideHashLiteralBraces: Space inside { missing.
Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.
Layout/SpaceInsideHashLiteralBraces: Space inside } missing.
when 200 | ||
JSON.parse(response.body) | ||
when 404 | ||
fail NotFound |
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.
Style/SignalException: Always use raise to signal exceptions.
raise RequestError, status: response.message, code: response.code | ||
when 200 | ||
JSON.parse(response.body) | ||
when 404 |
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.
Layout/CaseIndentation: Indent when as deep as case.
raise NotFound | ||
else | ||
raise RequestError, status: response.message, code: response.code | ||
when 200 |
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.
Layout/CaseIndentation: Indent when as deep as case.
@@ -39,7 +39,7 @@ def generate_url(url) | |||
end | |||
|
|||
def generate_params(params) | |||
URI.encode_www_form(params.merge(token: token)) | |||
URI.encode_www_form(params.merge({token: token})) |
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.
Layout/SpaceInsideHashLiteralBraces: Space inside { missing.
Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.
Layout/SpaceInsideHashLiteralBraces: Space inside } missing.
@ainformatico I made the changes requested. I appreciate the help you have given me. I think it would be easiest to do the squash with the Github button. Let me know if any other changes are needed. Thanks I am looking forward to this being complete. |
Along with updating the gem to support the Private Domain I also updated the gem to handle the updated responses, added tests for the private domain upgrade, cleaned up a bunch of Rubocop errors and updated the documentation. This is a major change to I updated the version to 1.0.0.
Let me know if you approve or what you would like to see different.
Thanks!