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.

Sign up for GitHub

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

Jump to bottom

[NEW] enable advanced SMTP config #110

Merged
merged 2 commits into from
Jan 24, 2018
Merged

[NEW] enable advanced SMTP config #110

merged 2 commits into from
Jan 24, 2018

Conversation

njeudy
Copy link
Contributor

@njeudy njeudy commented Jan 5, 2018

  • enable advanced SMTP config in odoo.conf

Needed by #107

@njeudy njeudy mentioned this pull request Jan 5, 2018
Merged
1 task
@njeudy
Copy link
Contributor Author

njeudy commented Jan 5, 2018

@yajo some did did'nt pass but It's not due to my commit .. what should I do ?

yajo
yajo reviewed Jan 9, 2018
View reviewed changes
conf.d/40-smtp.conf
smtp_user = $SMTP_USER
smtp_password = $SMTP_PASSWORD
smtp_ssl = $SMTP_SSL
email_from = $EMAIL_FROM
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Does this actually work? Isn't a record in ir.mail.server required?

cc @pedrobaeza

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check in my test database .. but think I have no ir.mail.server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to check this 4 params I just take odoo.conf from https://github.com/odoo/docker/blob/16d5988408434e4678f3bf6318aabad542dec4ee/11.0/odoo.conf.

server and port is working but never really try this 4 params.

@yajo
Copy link
Contributor

yajo commented Jan 9, 2018

Yeah, Travis is sick since recently with #109, dunno why, it started happening suddenly. I'll have to do manual tests instead.

Please add docs under development (opening port for mailhog) and testing (using extra routing path from traefik).

@yajo
Copy link
Contributor

yajo commented Jan 10, 2018

Please rebase and add tests, now that #109 is fixed

@njeudy njeudy force-pushed the smtp-custom-config branch from 8b58881 to 8a91369 Compare January 11, 2018 19:27
@njeudy
Copy link
Contributor Author

njeudy commented Jan 12, 2018

@Yago: I have to correct travis or this is in your work list ?

@Yago
Copy link

Yago commented Jan 14, 2018

@njeudy Not sure, ask @yajo just in case 😅

@njeudy
Copy link
Contributor Author

njeudy commented Jan 14, 2018

@Yago oups :) sorry @yajo I have to correct travis or this is in your work list ?

@njeudy
Copy link
Contributor Author

njeudy commented Jan 14, 2018

@yajo All green now :) is it ok for you ?

yajo
yajo suggested changes Jan 16, 2018
View reviewed changes
Copy link
Contributor

@yajo yajo left a comment

Choose a reason for hiding this comment

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

2 minor details please

8.0.Dockerfile Outdated
@@ -20,7 +20,13 @@ ONBUILD ARG COMPILE=true
ONBUILD ARG CONFIG_BUILD=true
ONBUILD ARG PIP_INSTALL_ODOO=true
ONBUILD ARG ADMIN_PASSWORD=admin

Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

11.0.Dockerfile Outdated
ONBUILD ARG SMTP_PASSWORD=false
ONBUILD ARG SMTP_SSL=false
ONBUILD ARG EMAIL_FROM=""

Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

@njeudy njeudy force-pushed the smtp-custom-config branch from ee5fbe5 to 219b72e Compare January 16, 2018 21:39
@njeudy
Copy link
Contributor Author

njeudy commented Jan 16, 2018

@yajo ok done :) sorry miss this !

@njeudy njeudy force-pushed the smtp-custom-config branch from 219b72e to afa9d02 Compare January 17, 2018 10:40
yajo
yajo reviewed Jan 18, 2018
View reviewed changes
11.0.Dockerfile
@@ -36,6 +41,11 @@ ONBUILD ENV ADMIN_PASSWORD="$ADMIN_PASSWORD" \
PGDATABASE="$PGDATABASE" \
PROXY_MODE="$PROXY_MODE" \
SMTP_SERVER="$SMTP_SERVER" \
SMTP_PORT=$SMTP_PORT \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why this has no surrounding quotes? I'd put them, just for consistency.

8.0.Dockerfile
@@ -38,6 +43,11 @@ ONBUILD ENV ADMIN_PASSWORD="$ADMIN_PASSWORD" \
PGDATABASE="$PGDATABASE" \
PROXY_MODE="$PROXY_MODE" \
SMTP_SERVER="$SMTP_SERVER" \
SMTP_PORT=$SMTP_PORT \
Copy link
Contributor

Choose a reason for hiding this comment

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

Same about quotes.

@yajo
Copy link
Contributor

yajo commented Jan 18, 2018

Apart from those 2 minor details, there's still one thing remaining here: tests.

You're adding here the default settings to make work the bundled smtp relay, which is really cool! I guess we should now create an odoo shell script that sends an email with those default settings and checks the relay returns an OK.

Wanna try yourself? 😉

@njeudy
Copy link
Contributor Author

njeudy commented Jan 18, 2018

@yajo yes I will give it a try, but can you merge this one (will open a new for test) because today I need to rebuild my personnal docker all the time there is a change just to add this params.

It's up to you, i will work on test next week I think I have some module to end before :)

@yajo
Copy link
Contributor

yajo commented Jan 19, 2018

Fine, we can wait. I really like the feature, but Doodba is really a very critical piece of software in many companies' production toolchain, I usually don't merge anything without tests, not even my own PRs... Surely you understand 😇

@yajo yajo force-pushed the smtp-custom-config branch from afa9d02 to ae27c11 Compare January 23, 2018 13:56
@yajo
Copy link
Contributor

yajo commented Jan 23, 2018

I added the test, it was quite simple.

@njeudy njeudy force-pushed the smtp-custom-config branch 2 times, most recently from 88af2a1 to bd4476c Compare January 23, 2018 21:10
@njeudy
Copy link
Contributor Author

njeudy commented Jan 23, 2018

@yajo try to make ci green but did not understand actual error .. can you help me ?

@yajo yajo force-pushed the smtp-custom-config branch from bd4476c to 61e8e14 Compare January 24, 2018 09:51
@yajo
Copy link
Contributor

yajo commented Jan 24, 2018

It seems the DB was missing, I did a new try.

@yajo yajo force-pushed the smtp-custom-config branch from 61e8e14 to b75b35c Compare January 24, 2018 10:17
@yajo
Copy link
Contributor

yajo commented Jan 24, 2018

I added a new test scaffolding. The dotd one is a little bit crowded and did not require a DB until now. Adding a DB into it clutters it even more, so I guess it's time to have a new one for the settings things. It will help testing #62 too.

@yajo yajo force-pushed the smtp-custom-config branch from b75b35c to d3c0a16 Compare January 24, 2018 10:36
@yajo
Test SMTP settings
2be84ff 
Use a new scaffolding that makes sure there's a preexisting database.
@yajo yajo force-pushed the smtp-custom-config branch from dd7a4a6 to 2be84ff Compare January 24, 2018 12:11
@yajo yajo merged commit 52ad126 into Tecnativa:master Jan 24, 2018
@yajo
Copy link
Contributor

yajo commented Jan 24, 2018

Finally I decided to skip the mail sending part. It's too low level to investigate. Just testing settings are OK should be OK for this project's support surface. Thanks pal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yajo yajo yajo approved these changes

Assignees
No one assigned
Labels
enhancement work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@njeudy @yajo @Yago