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

[8.0][ADD] Sendgrid modules for transactional e-mails #102

Closed
wants to merge 3 commits into from

Conversation

ecino
Copy link
Contributor

@ecino ecino commented Oct 12, 2016

Two modules for connecting e-mails and mass_mailing with Sendgrid

This enables the use of Sendgrid transactional e-mails with templates and Sendgrid Event Notifications hook for tracking all e-mails sent.

Note: we don't have any tests because it requires an API Key (cannot be used without).

@ecino
Copy link
Contributor Author

ecino commented Oct 12, 2016

I cannot find where I should add the sendgrid library dependency so that it is installed in TRAVIS with PIP. Can someone help?

@ecino ecino changed the title [ADD] Sendgrid modules for transactional e-mails [8.0][ADD] Sendgrid modules for transactional e-mails Oct 12, 2016
@hbrunn hbrunn added this to the 8.0 milestone Oct 12, 2016
@hbrunn
Copy link
Member

hbrunn commented Oct 12, 2016

in https://github.com/OCA/social/blob/10.0/.travis.yml#L29, just add another line there.
For the tests: This amount of untested code is not acceptable. If there's no way to obtain a demo/testing api key there, you'll have to use mock or the like to simulate this service's responses.

@ecino
Copy link
Contributor Author

ecino commented Oct 12, 2016

I won't have time to write tests now.

There are no demo API keys. Do you think I could create a free account and use it for that ? But this may expose it for people to use it for sending e-mails. How then could I set the api key in the configuration of odoo (if possible encrypted).

Otherwise I don't see how we can test the code if we cannot make any real call of the sendgrid api (a lot of methods use it). I'm not familiar with mock.

@hbrunn
Copy link
Member

hbrunn commented Oct 12, 2016

no tests = no merge
Travis allows to encrypt variables: https://docs.travis-ci.com/user/environment-variables I'm not so sure if a free account is a good idea, because it's probably subject to rate limiting and the like, tests can be run very often. You're better off with simulating the services responses, which is where mock comes into play.
About mock: It's a good thing to be familiar with anyways: https://mock.readthedocs.io/en/latest - the idea would be to replace the relevant function(s) in the library you use by an object that returns whatever you need it to return in order to run successful tests.

@ecino
Copy link
Contributor Author

ecino commented Oct 12, 2016

Thanks for the tips, I'll have a look at it when I have time ! I'll be back with the pull request when I can manage to implement the tests.

In the meantime if someone is interested in those modules and would like to help with the tests, please contact me.

@hbrunn
Copy link
Member

hbrunn commented Oct 12, 2016

I reopened your PR and set it to work in progress. This way it's still visible, but nobody is going to bug you about this until you request to remove the wip label

@hbrunn hbrunn reopened this Oct 12, 2016
@pedrobaeza
Copy link
Member

You can mock your tests the same we make for mail_tracking_mailgun module. You can also use that "framework" to build on top of it your module instead of repeating everything.

@ecino
Copy link
Contributor Author

ecino commented Oct 12, 2016

Ok I will have a look at this.

@rafaelbn
Copy link
Member

Hi @ecino , thanks!

No one commented by please make to diferents pull requiest for:

1 - mail_sendgrid
2 - mail_sendgrid_mass_mailing

We will review (1) and after mergin it, we review (2).

Thanks

Maybe @antespi you could make a review here, please

@pedrobaeza
Copy link
Member

@rafaelbn, my comment is that they should use mail_tracking framework and with this, they only need mail_tracking_sendgrid. Mass mailing part is automatically done.

@ecino
Copy link
Contributor Author

ecino commented Oct 17, 2016

Like I said, I won't have time now to do all those requests. I'm not sure if mass_mailing can be removed. When I wrote the module the mass tracking was poorly done and didn't work with sendgrid. I guess it changed in between but this will take time to adapt.

If someone is interested, I'd be happy to have some help (PR are welcome).

@pedrobaeza
Copy link
Member

mass_mailing_tracking provides the needed framework also for having full capacities on mass mailing without the need of making anything more specific for Sendgrid. That's why I say it's worth to take a look, @ecino, you will have to make little effort to adapt it to this system as you know already Sendgrid, but obviously it's your choice.

As is, the module can't enter OCA right now.

@ecino
Copy link
Contributor Author

ecino commented Apr 20, 2017

New PR for version 9.0

@ecino ecino closed this Apr 20, 2017
@ecino ecino deleted the 8.0 branch April 12, 2018 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants