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
[ADD] email marketing: lost leads email doc #7623
Conversation
c8dafc5
to
16600a8
Compare
Hi @StraubCreative this is ready for first review, please let me know if I should assign anyone else as a reviewer. Thanks! |
Hi KC, I'm reviewing the PRs Max opened, and this one seems to be in your scope. It's a new doc, looks like it's ready for a quick look and then off to peer review. Thanks 👍 |
16600a8
to
b6237e8
Compare
Rebased to latest on b6237e8. |
b6237e8
to
3f403f6
Compare
ZST revisions/rewrite added on 3f403f6. You can view the diff here to see the specific changes I made vs. NIMK's original. This is ready for peer review now 👍 @tiku-odoo would you mind giving this a look-over before I pass to @odoo/us-doc-review for final reviews? |
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've completed my review and made a few suggestions for you. I'm approving to move over to final review. This doc is clear and concise, with easy steps to follow, and looks great.
I noticed under the 'Create a lost leads reactivation email' section there is a list format. Are we okay with using this documentation style?
Looking forward to seeing this doc in publication, nice work 👍
Thanks,
Tim
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
3f403f6
to
a5b9e40
Compare
Hi @tiku-odoo thank you for the very helpful review! I've addressed all of your change requests, as well as the list format concern you had, which can be seen on a5b9e40. When I pushed up the rewrite I kept the list style, begrudgingly, since the flow does require that all steps are completed and we were also trying to educate users about how filter order matters... however I went with a JERO-style approach instead, and replaced the list style format with subheadings. Hopefully you and the other reviewers find that amenable 👍 @odoo/us-doc-review this is ready for a look from y'all. Thanks in advance! |
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.
Hey there, @StraubCreative -- just finished my Final Review. Great work on this re-write! Very in-depth, detailed, and informative. The majority (if not all) of my suggestions are "style-based" and very minor in nature. I don't anticipate you having any issues implementing the ones you agree with, so I'll be 'approving' this one now. If you have any questions or need clarification on anything, just let me know. If not, feel free to tag this for Tech Review/Merge with Sam whenever it's ready to go. Thanks! 👍
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
a5b9e40
to
7fa86b5
Compare
Hi @ksc-odoo thank you very much for the detailed and helpful review. I incorporated most of your suggestions into the latest version (via 7fa86b5), and think it's in a really good spot now 🙂 Moving to tech review— @samueljlieber, can you review this for me, please? |
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.
Hello @StraubCreative, this doc is looking pretty great– I enjoyed reading about Lost Leads! I have a small handful of corrections and suggestions, please see below.
Lastly, please update the commit message and PR title to have the correct app/module name:
[ADD] email marketing: lost leads email doc
Tag me for another quick look once these are addressed, thank you! :)
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
---------- | ||
|
||
Begin by clicking :guilabel:`New Rule` beneath the default :guilabel:`Blacklist` rule. Then, click | ||
the first field of the new rule that appears, and select the :guilabel:`Created on` parameter from |
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.
Suggestion to use the term 'input' for the 3 inputs in a rule row rather than 'field' since there are fields in the drop-down menu. Feel free to ignore this, I just find it easier to understand personally :)
the first field of the new rule that appears, and select the :guilabel:`Created on` parameter from | |
the first input of the new rule that appears, and select the :guilabel:`Created on` parameter from |
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.
Yea I see what you mean.
I think it's arguably more "truthy" to say field since it's an input field, where input is the qualifying adjective to the noun field. Where it gets confusing, to me at least, is that the back-end terminology Odoo uses also matches the front-end in this case, since they're both "fields." 😅
With that said, I'd like to keep it as is for now and see how it sits in the wild, with the hope that someone in the community would give us corrective feedback, if it's needed.
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email.rst
Outdated
Show resolved
Hide resolved
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 is an unused image and can be removed
content/applications/marketing/email_marketing/lost_leads_email/filters.png
Outdated
Show resolved
Hide resolved
content/applications/marketing/email_marketing/lost_leads_email/match-all.png
Outdated
Show resolved
Hide resolved
7fa86b5
to
c710d6b
Compare
Hi @samueljlieber. Thanks for the review! |
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 @StraubCreative, nice job! Looks good to me, delegating to you as there are small changes:
Please update the commit message to be the app scope:
[ADD] email marketing: lost leads email doc
.....
@robodoo delegate=StraubCreative
Co-authored-by: StraubCreative <zst@odoo.com> Co-authored-by: tiku-odoo <tiku@odoo.com> Co-authored-by: ksc-odoo <ksc@odoo.com> Co-authored-by: samueljlieber <sali@odoo.com>
c710d6b
to
f09fe7f
Compare
@robodoo r+ |
need confirmation on filters, definitions, and follow up steps for analyzing results and email nurturingAll set here: added more context and opens the door to create more docs on certain topics (ZST)