-
Notifications
You must be signed in to change notification settings - Fork 10.7k
[ADD] discuss: specific email server from #2627
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
Conversation
|
@cth-odoo @tiku-odoo @std-odoo @AntoineVDV @samueljlieber took all of the change requests from the prior PR (#1449) and moved them over to this new feature branch. Now we have a clean commit history and no more technical hiccups. Please take a look at what we have here, and let Sam know if any changes are needed in the content and he will take care of making those updates. Afterwards we can ship to final reviews. Thank you 🙏 |
|
@Abridbus @std-odoo When you have a moment can you review this change? We will be documenting the from_filter in as well: #2055 We are open to suggestions. Thanks, Tim @StraubCreative Although this PR is for V15 only, this document is being updated in PR #2075 (V13-V15) |
content/applications/productivity/discuss/advanced/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/productivity/discuss/advanced/email_servers.rst
Outdated
Show resolved
Hide resolved
tiku-odoo
left a comment
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.
@samueljlieber Here is a draft of the changes to be made per @std-odoo
Thanks
content/applications/productivity/discuss/advanced/email_servers.rst
Outdated
Show resolved
Hide resolved
b855871 to
9930359
Compare
|
Hi @tiku-odoo and @std-odoo! Thanks for the suggestions! I made the content change and I updated the commit message tag to use all uppercase. |
tiku-odoo
left a comment
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.
@samueljlieber Looks great! Tagging @mivu-odoo for final review.
mivu-odoo
left a comment
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.
Hi @samueljlieber and @tiku-odoo! Thanks for your patience on this!
As I was going through this file, I noticed things were sounding very familiar. Is it the same content as PR #2075? If it is the same content, with no changes, it might be easier to finish PR #2075 first and get that approved, merged, and forward-ported. Then, we can simply copy the content onto this PR.
If there are slight changes, please let me know and I can finish my review, while trying to keep the content of the email_servers.rst file from this PR as close to the email_servers.rst file from PR #2075.
If both PRs are editing the same content in the same file, then we will likely run into a merge conflict during rebase. Normally, the merge conflict is solved by picking the the changes from w/e commit has the correct content, as in, there shouldn't be a need to manually make sure the versions match. However, given that there are considerable differences between the two versions, either @samueljlieber or I would need guidance as to where the last PR ends and where the new one begins. It sounds like @mivu-odoo is asking for the same thing as to avoid reviewing content that would become obsolete with the forward-port from #2075 . I agree to let #2075 get merged first-- afterwards, let's see if a merge conflict shows up. If it does, then let's jump on a a screenshare @samueljlieber @tiku-odoo and go through the merge conflict together so we can set this up right for @mivu-odoo 's review. |
|
Update: We're just waiting for the forward-port PRs to finish up on #2075. Had to solve a merge conflict on one of them which affected a few others, but they should be on track again. After those are done we can rebase those changes into this PR and @tiku-odoo / @std-odoo we'll need your help picking what changes we should keep for the final version of the file. |
|
@StraubCreative / @samueljlieber -- It looks like all the forward ports for PR #2075 have been closed out. The material in this PR should be placed before the section on 'Set up different dedicated servers for transactional and mass mails'. For this change we will be adding Lines 217 --> 247 in the files changes doc on this PR: Setting up different outgoing email servers for a multi-company environmentThe "From Filter" allows for the use of a specific outgoing email server depending on the "From" #. Odoo searches for an email server having the same "From Filter" as the "From" email address #. If no email servers are found, then Odoo searches for an email server having the same domain in If no email servers are found after checking for the domain, then Odoo will return all email Should this previous query return no results, then Odoo performs a search for an email server If no email server is found then Odoo will return the first outgoing email server (sorted by .. note:: |
9930359 to
e8a7283
Compare
9a26c86 to
af46e00
Compare
mivu-odoo
left a comment
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.
Hi @samueljlieber and @tiku-odoo!
I made a couple of comments on formatting and small word changes. Once you integrate those, please tag me again and I'll approve this PR so it can move onto ZST for final technical review. Thank you 😸
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
af46e00 to
d2b951f
Compare
|
@mivu-odoo This document has been updated with your changes. Can you review the doc once more? Thanks in advance for your help on this doc :) |
mivu-odoo
left a comment
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.
Hi @samueljlieber and @tiku-odoo!
Thank you for making the edits! I found one more quick edit on Line 211 (changing the single quotation marks to backticks). Once you push that through, feel free to go straight to ZST for final technical review, no need to tag me again.
Thank you 😸
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
StraubCreative
left a comment
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.
Hi @tiku-odoo 👋
I have a few suggestions below, which mainly involve GUI elements, context, and word choices. They are as follows:
- add
:guilabel:'s to GUI elements, since we are directly referring to them AND entertaining interactions with them. - add navigational steps to provide location-based context to the user; the documentation is not helpful if the user doesn't know where they're supposed to be 😉
- some word choices here and there to improve flow/clarity.
- one instance of an early line break
For future PRs, here are a few improvements I'd recommend to bring up the quality of the whole doc 🚀
- clean up and streamline the RST formatting to the latest markup standards.
- Use less passive/past tense in favor of more active/present tense, per Content Guidelines.
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
0422c98 to
65aef56
Compare
|
@mivu-odoo When you have a moment can you review this doc again? Thanks in advance for your review. 👍 |
mivu-odoo
left a comment
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.
Hi @tiku-odoo!
I made some comments/suggestions, mainly targeted towards clarity. I also think some parts can benefit from a sample use case to help the reader understand why they might need to use the feature.
When you're ready, tag me again for another look 😸 Thank you!
| ======================================================================= | ||
|
|
||
| The :guilabel:`FROM Filtering` field allows for the use of a specific outgoing email server | ||
| depending on the :guilabel:`From` email address that Odoo is sending on behalf of. Access this |
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.
After the first sentence (The...on behalf of), I would add a real-life example (1-3 sentences) of why a company might want to use this feature. The section header says it's for multi-company environments. Try to simplify for the reader and help them visualize how they would apply this section's knowledge to their business.
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.
@mivu-odoo I've added two separate sentences: 1. to improve deliverability 2. In a multi-company environment
The title seemed a bit too specific and I've made it more general as multi-company sending is not the only application. I've changed it to: Utilizing the FROM Filter on an outgoing email server
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
| If no email servers are found after checking for the domain, then Odoo will return all email | ||
| servers that do not have any :guilabel:`FROM Filtering` value(s) set. | ||
|
|
||
| Should this previous query return no results, then Odoo performs a search for an email server using |
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.
Clarify this previous query - which one are we talking about?
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 sentence refers to the sentence in the previous line. I've removed the word previously.
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
content/applications/general/email_communication/email_servers.rst
Outdated
Show resolved
Hide resolved
65aef56 to
975643a
Compare
|
@mivu-odoo This PR is ready for another review from you when you have a chance. Thanks in advance for your help on this From Filter PR. |
mivu-odoo
left a comment
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.
Hi @tiku-odoo!
Thank you for addressing my comments! The first paragraph of the Utilizing the From Filter on on outgoing email server section is easier to understand now.
Please move forward to final technical review and tag ZST. Thank you 😸
|
Thanks @mivu-odoo @StraubCreative This doc is ready for your technical review whenever you have a moment. Thanks in advance for your help on this PR. 👍 |
975643a to
ed0ed06
Compare
StraubCreative
left a comment
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.
Edited a line break + fresh rebase in ed0ed06 but otherwise LGTM 👍
Note/reminder to create a card to do entire doc update since a lot of the RST formatting seems out of date @tiku-odoo @samueljlieber . Content could always use another look too just in case ;)
Tagging in @odoo/doc-review
AntoineVDV
left a comment
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.
@robodoo r+
closes #2627 Signed-off-by: Antoine Vandevenne (anv) <anv@odoo.com>
|
@samueljlieber @AntoineVDV this pull request has forward-port PRs awaiting action (not merged or closed): |
5 similar comments
|
@samueljlieber @AntoineVDV this pull request has forward-port PRs awaiting action (not merged or closed): |
|
@samueljlieber @AntoineVDV this pull request has forward-port PRs awaiting action (not merged or closed): |
|
@samueljlieber @AntoineVDV this pull request has forward-port PRs awaiting action (not merged or closed): |
|
@samueljlieber @AntoineVDV this pull request has forward-port PRs awaiting action (not merged or closed): |
|
@samueljlieber @AntoineVDV this pull request has forward-port PRs awaiting action (not merged or closed): |
Manual cherry pick of #2626 which is a follow up from #1449.