Skip to content
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

Backend Code for #870 Add ability to send emails to Monica and associate it with a contact [WIP] #1311

Closed
wants to merge 49 commits into from

Conversation

turtles2
Copy link

@turtles2 turtles2 commented May 11, 2018

This PR will add the backend code for #870. This PR only is for getting the emails and parsing them into the DB. This code assumes the inbound processor is Postmark as that is what the hosted version uses per #1224 and I could not find a good library to add support for others. It also assumes that users are forwarding the email into Monica from there account. This could be changed but we need a way to link an email to a user.

  • Add Middleware to Whitlist Postmark IPs for security
  • Phrase the email body to get data which includes contact and message data
  • Add table in DB to store email information
  • Add tests where needed

@turtles2
Copy link
Author

@djaiss Can you take a look at this PR right now it is a very rough proof of concept. What I would like focused on is the workflow lets not worry too much about the specifics of the code at this juncture. Here is the current workflow.

  1. User Forwards Email to Inbound Address one address for all users
  2. Receive Webhook from Postmark
  3. Find user based on Source of Forward Email
  4. Extract Date, To, From form the most recent email in the chain
  5. Find all of the users contacts that have the to and from emails
  6. Save the email to the DB it will save the plain text of the whole chain similar to printing an email
  7. Link the contacts to the Email. It is a Many to Many

@djaiss
Copy link
Member

djaiss commented May 14, 2018

Great approach.

We need to make sure that the emails do not appear in the wrong account, as users could potentially have the same contact email address for a similar contact... This is the top priority I think.

  1. I think this feature should be a premium feature (many businesses will use it).
  2. Unfortunately we need to create a new env variable for this feature, as we need to provide an email address for the instance (which will come from Postmark), something like INBOUND_EMAIL_ADDRESS. That means if the env variable is not there, this feature should not work and should also degrade gracefully - in the sense that it should not crash the instance. We should simply refuse the webhook.
  3. I don't think many users will be happy that we support a paid external service for this feature. That being said, we haven't found a great parsing library yet. Postmark offers 100 emails per month for free. Mailgun seems to offer 10 000 emails for free. Could we do a system where we could accept mutiple services like that easily? I know it's not a trivial task. Just asking in case you would think of something.
  4. I think emails should be put in the same section as the notes in a contact sheet (even though the objects are different by nature, but I have an idea of how we could represent it). I would like to provide a design for this. Could you simply make a list of those emails above the Notes section and I will do a design in a separate branch for this?
  5. Related to the point above, could you create the API methods for this feature first, before doing the front end? I'd like to start doing the API first from now on, then work on the front end and using the API for it.
  6. Even though I won't review the code right now (just a bit), I've seen that you are calling the object Email. I think it's a bit confusing for new comers who won't get immediately what an Email is. What do you think of calling the class InboundEmail instead?

routes/api.php Outdated
@@ -2,6 +2,8 @@

use Illuminate\Support\Facades\Route;

Route::post('/emailhook', 'InBoundEmailController@new');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/inboundemail might be a better name.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree

@turtles2
Copy link
Author

@djaiss Some of the points you made have left me questioning how you implemented the hosted version. Do you have a running instance for each user? If not comfortable answering publicly let me know and we can take that discussion privately because for this feature I think it is key for me to understand how you are running things. Below I will respond to the various points you raised.

  1. I agree
  2. No as Postmark dose not send any emails to the server. It simply posts a webhook to an address you set in Postmark. We keep things secure by only allowing requests from Postmark IPs
  3. We could we would just need a separate method on the controller for each service. I only chose Postmark as that it what the hosted version uses from my understanding. Again this is why I need a better understanding of how you are running things. Myself I prefer Mailgun I have used them in other projects. We could IMAP or POP which there are libraries for but I think that it would lead to a sub par user experience as set up would be difficult on there end. Not to mention the load on the servers needing to be requesting and processing every email on the regular instead of waiting for a webhook.
  4. On this PR I would prefer not to do anything with the front end to keep things simple on my end. I would like to leave it all to you or somebody else.
  5. I have not done anything on the API side yet for this project but I will give it a shot. Can you clarify on using the APIs for the front end?
  6. I agree

I will be awaiting a response before preceding with any work on this PR.

On a side note I have some experience with DNS and infrastructure work not too long ago I was very close to starting my own ISP I was just short on funding and had some bad breaks. So I have a solid grasp on what is at play here.

@djaiss
Copy link
Member

djaiss commented May 15, 2018

Thanks for your answers. They've also helped me understand that I've been wrong in my answers above 😀

Do you have a running instance for each user?

Of course not 😀 I have a single instance that hosts all users.

To answer the other points:
3. Indeed, the hosted version uses Postmark as the transactional email service provider. I just use one account for this. I don't think we should start dealing with IMAP and POP ourselves, this will be a nightmare to implement and to maintain and this is not our core "business".
4. Perfect with the no front end aspect of this PR. That's even better.
5. The API would be used to retrieve, edit and delete emails that have been stored by this new feature. Why would I like to do this? Because right now we have a logic for the front-end (with controllers etc...), and another logic for the API, leading to duplication of logics, which I don't like. That being said, I've started lately to put all actions on the models directly as much as possible - so both the frontend and the API can use it through their respective controllers without duplicating the logic. So... I don't know, let's think about this.

@turtles2
Copy link
Author

turtles2 commented May 16, 2018

@djaiss Lets go ahead and get a review on this to see where we are at. I want to get the logic done before adding tests

@djaiss
Copy link
Member

djaiss commented May 18, 2018

@turtles2 Sorry for the delay - I'll check this over the weekend 😀

@turtles2
Copy link
Author

@djaiss No worries, I feel like this one might take a bit to get right.

@turtles2
Copy link
Author

@djaiss I have fixed the naming issues as well as implemented HashIDs for the mailbox hash. It uses the raw ID without the prefix. If you could go ahead and take another look that would be awesome. All the added code will have tests once I am confident in the behavior I need to be testing.

@djaiss
Copy link
Member

djaiss commented May 30, 2018

@turtles2 great, thanks a lot. I'll review all this very soon!

@turtles2
Copy link
Author

@djaiss or @asbiin Git is not my strong suite. Can you help get this branch even with master? Because of the file structure change it is very out of date.

@djaiss
Copy link
Member

djaiss commented Jun 20, 2018

@turtles2 I'll take care of it. Yeah master has changed drastically.

@turtles2
Copy link
Author

@djaiss When you are helping me out with that, can you go ahead and review this, as I have been waiting on that.

@djaiss
Copy link
Member

djaiss commented Aug 18, 2018

https://laravel-news.com/laravel-inbound-email

Also going to review this PR in the next days. Really sorry for the extremely long wait.

@djaiss
Copy link
Member

djaiss commented Mar 21, 2019

We are going to replace this by https://github.com/beyondcode/laravel-mailbox instead.

I'm really sorry @turtles2 that we didn't follow through with this awesome PR.

@github-actions
Copy link

This pull request has been automatically locked since there
has not been any recent activity after it was closed.
Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants