Skip to content

Conversation

@ksc-odoo
Copy link
Contributor

@ksc-odoo ksc-odoo commented Aug 2, 2022

Task ID: 2942125

@ksc-odoo ksc-odoo requested a review from mivu-odoo August 2, 2022 18:37
@robodoo
Copy link
Collaborator

robodoo commented Aug 2, 2022

@ksc-odoo
Copy link
Contributor Author

ksc-odoo commented Aug 2, 2022

Hi @mivu-odoo

I added some basic, "essential," documentation to the SMS Marketing doc.

Once I get your approval/review, I'll pass it along to Zac.

Thanks!

@C3POdoo C3POdoo requested a review from a team August 2, 2022 18:38
@mivu-odoo mivu-odoo force-pushed the 15.0-sms-marketing-adding-essentials-ksc branch from 432e64e to d542657 Compare September 2, 2022 00:59
@mivu-odoo
Copy link
Contributor

Hi @ksc-odoo!

This PR wasn't passing the ci/documentation code check b/c of some build errors.

As of commit d542657

  • Renamed the RST files to replace the hyphens with underscores in contacts-blacklist.rst and sms-integrations.rst
  • Renamed first-campaign.rst to first_sms_campaign.rst to be more specific b/c I know PR [ADD] marketing automation: added 'getting started' section and docs … #2552 is going to add a file named first_campaign.rst for Marketing Automation user docs, and I thought it would be confusing to have two files in different places with the same name
  • Fixed the RST file names referenced in essentials.rst to match the new RST file names (the ones with the underscores)
  • Renamed the image folders to replace the hyphens with underscores (so they match the new RST file names)
  • Fixed the image folder name references in first_sms_campaign.rst, sms_integrations.rst, and contacts_blacklist.rst
  • Fixed the indentation of sms_marketing/essentials and sms_marketing/pricing in sms_marketing.rst to align with the first colon of :titlesonly:

Despite all these changes, the PR still won't pass the code check b/c there's one error I haven't quite figured out how to crack.

When I run make html, I get an error for first_sms_campaign.rst:140: WARNING: unknown document: pricing_and_faq. I tried several different iterations like including more of the file path, but I would still get the same build error. Will probably have to loop in ZST to fix.

@samueljlieber samueljlieber force-pushed the 15.0-sms-marketing-adding-essentials-ksc branch from d542657 to 6e978b7 Compare September 2, 2022 14:44
@samueljlieber
Copy link
Contributor

Hi @mivu-odoo and @ksc-odoo 👋

I fixed what was causing the build error here! It was due to an incorrect referenced doc path on line 140 of first_sms_campaign.rst 🙂

The path was:
pricing_and_faq

The referenced doc path needs to be relative to the document you are linking from, so the correct path looks like this:
../pricing/pricing_and_faq

@ksc-odoo
Copy link
Contributor Author

@StraubCreative Ready for your review!

@StraubCreative StraubCreative removed the request for review from mivu-odoo February 5, 2023 23:48
Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

Hi @ksc-odoo

These docs need another round of proofreading and revisions.

Lots of 2nd-person you/your language, some screenshots are too big, unnecessary, or not named properly, etc...the usual suspects 😉

Please submit updated content to @samueljlieber he'll handle the git flow for you.

@samueljlieber samueljlieber self-assigned this Feb 14, 2023
@samueljlieber samueljlieber force-pushed the 15.0-sms-marketing-adding-essentials-ksc branch from 6e978b7 to b4acc53 Compare February 14, 2023 20:48
@samueljlieber
Copy link
Contributor

Hi @ksc-odoo, in b4acc53 I implemented your changes per @StraubCreative suggestions:

  • FYI renamed the following files:
    • first_sms_campaign.rst to sms_essentials.rst
    • sms_integrations.rst to sms_campaigns.rst
    • contacts_blacklists.rst to mailing_lists_blacklists.rst
  • sms_essentials.rst lines 29-35 & lines 136-142 I formatted as a list rather than sentences because they are referring to options that can be selected from a dropdown, it made sense to me for the options to be displayed in a list.

I think your changes to these docs look really good 👍 I made a few small RST adjustments, @ksc-odoo can you please confirm these changes with a review before I pass to @StraubCreative? 🙏

@ksc-odoo
Copy link
Contributor Author

Hi @ksc-odoo, in b4acc53 I implemented your changes per @StraubCreative suggestions:

  • FYI renamed the following files:

    • first_sms_campaign.rst to sms_essentials.rst
    • sms_integrations.rst to sms_campaigns.rst
    • contacts_blacklists.rst to mailing_lists_blacklists.rst
  • sms_essentials.rst lines 29-35 & lines 136-142 I formatted as a list rather than sentences because they are referring to options that can be selected from a dropdown, it made sense to me for the options to be displayed in a list.

I think your changes to these docs look really good 👍 I made a few small RST adjustments, @ksc-odoo can you please confirm these changes with a review before I pass to @StraubCreative? 🙏

Hey @samueljlieber - Feel free to pass it along to @StraubCreative 👍

Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

Hi @ksc-odoo

Nice work. I like the depth of explanation and walk-through on these 👍

My CRs below are mostly around small things and fine tuning since the content already hits the mark. See below where I made comments just on the sms_essentials.rst doc, which apply to all docs in this PR, as well.

Overview:

  • formatting of sentences, mainly parenthesis use. I see what you're doing in most cases, using parenthesis as tiny aside comments in sentences, however, I don't think they're needed in most cases. Most of what I see comes off as redundant or could easily be included into the sentence with a comma.
  • line breaks. Just a few spots to fix.
  • headings. Two headings are duplicates and we don't need How to... prefixes.
  • images. Most images are too large, not in HD, and could use red markup to illustrate cause > effect. As well, some screenshots would benefit from showcasing more information or a more complete use case to illustrate the depth of the product.

Can you take a look and apply to every doc on the PR? Can work with @samueljlieber to push up the next batch of changes.

Thank you both!

@samueljlieber samueljlieber force-pushed the 15.0-sms-marketing-adding-essentials-ksc branch from b4acc53 to ac8b150 Compare March 22, 2023 15:12
@samueljlieber
Copy link
Contributor

Hi @ksc-odoo I implemented your changes in ac8b150 🙂

@ksc-odoo ksc-odoo requested a review from StraubCreative March 22, 2023 16:17
@StraubCreative StraubCreative force-pushed the 15.0-sms-marketing-adding-essentials-ksc branch 2 times, most recently from 36bb56f to 01d323d Compare March 30, 2023 21:51
Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

Hi @ksc-odoo 👋

A few more change requests here. See below and on 01d323d.

If you agree with the changes, let me know so I can squash/merge, otherwise indicate if and where you might want to go over something in more detail.

Ty/lmk 🤙

.. toctree::
:titlesonly:

sms_marketing/essentials
Copy link
Contributor

Choose a reason for hiding this comment

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

@ksc-odoo @samueljlieber

Think we wanted to remove the pricing FAQ here and just make essentials the top-level directory for SMS, right?

If so, we'll need to follow up in another PR to clean that up, which could be a good opportunity to wipe out all mid-level directories that aren't needed for all US scopes.

Comment on lines 9 to 11
Odoo's :guilabel:`SMS Marketing` application can also help boost conversion rates around valuable
actions, such as event registrations, free trials, purchases, etc., since :abbr:`SMS (Short Message
Service)` text and mobile-based channels typically have high open-rates.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove guilabel here + slight wording changes.

Suggested change
Odoo's :guilabel:`SMS Marketing` application can also help boost conversion rates around valuable
actions, such as event registrations, free trials, purchases, etc., since :abbr:`SMS (Short Message
Service)` text and mobile-based channels typically have high open-rates.
Odoo's *SMS Marketing* application can also help boost conversion rates around valuable actions,
such as event registrations, free trials, purchases, etc., since text and mobile-based marketing
channels typically yield higher :abbr:`CTOR (click-to-open rate)` and :abbr:`CTR (click-through
rate)` outcomes.

Comment on lines 25 to 26
An :abbr:`SMS (Short Message Service)` can have one of the following statuses: :guilabel:`Draft`,
:guilabel:`In Queue`, :guilabel:`Sending`, or :guilabel:`Sent`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Specificity

Suggested change
An :abbr:`SMS (Short Message Service)` can have one of the following statuses: :guilabel:`Draft`,
:guilabel:`In Queue`, :guilabel:`Sending`, or :guilabel:`Sent`.
An :abbr:`SMS (Short Message Service)` mailing can have one of the following statuses:
:guilabel:`Draft`, :guilabel:`In Queue`, :guilabel:`Sending`, or :guilabel:`Sent`.

===================

To start, click :guilabel:`Create` on the main :guilabel:`SMS Marketing` dashboard, and Odoo reveals
a blank SMS template, which can be configured in a number of different ways.
Copy link
Contributor

Choose a reason for hiding this comment

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

Specificity

Suggested change
a blank SMS template, which can be configured in a number of different ways.
a blank SMS template form, which can be configured in a number of different ways.

the choices Odoo makes avaialble.

When another field (other than :guilabel:`Mailing List`) is selected, the option to specify that
chosen field even further becomes available - either with a default recipient filter equation that
Copy link
Contributor

Choose a reason for hiding this comment

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

Would use an em (long) dash here.

Suggested change
chosen field even further becomes available - either with a default recipient filter equation that
chosen field even further becomes available either with a default recipient filter equation that

Comment on lines 113 to 115
To see a complete collection of blacklisted numbers, navigate to the :guilabel:`SMS Marketing` app,
and go to :menuselection:`Configuration --> Blacklisted Phone Numbers`. Doing so, reveals a separate
page containing every blacklisted phone number in the database.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wording/formatting.

Suggested change
To see a complete collection of blacklisted numbers, navigate to the :guilabel:`SMS Marketing` app,
and go to :menuselection:`Configuration --> Blacklisted Phone Numbers`. Doing so, reveals a separate
page containing every blacklisted phone number in the database.
To see a complete collection of blacklisted numbers, navigate to the :menuselection:`SMS Marketing
app --> Configuration --> Blacklisted Phone Numbers` to reveal a dashboard containing every
blacklisted phone number in the database.

Comment on lines 121 to 122
To manually add a number, click the :guilabel:`Create` button in the upper-left. When clicked, Odoo
reveals a separate page, in which the phone number to be blacklisted is entered. There's also a
checkbox to indicate whether that particular phone numnber is :guilabel:`Active` (or not).
Copy link
Contributor

Choose a reason for hiding this comment

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

"Odoo reveals a separate page" phrase is too wordy. Just say what's happening in clear terms 😉

Suggested change
To manually add a number, click the :guilabel:`Create` button in the upper-left. When clicked, Odoo
reveals a separate page, in which the phone number to be blacklisted is entered. There's also a
checkbox to indicate whether that particular phone numnber is :guilabel:`Active` (or not).
To manually add a number to a blacklist, click the :guilabel:`Create` button in the upper-left
corner of the dashboard and enter the phone number on the next page's form. There's also a

Comment on lines 129 to 131
Once the form is completed, click :guilabel:`Save` to add it to the :guilabel:`Blacklist`. To
remove any number from the :guilabel:`Blacklist`, select the desired number, and click
:guilabel:`Unblacklist`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Blacklisted Phone Numbers gets a guilabel because it's a menu item and the name of a dashboard, while blacklist does not. Added some more specificity here too.

Suggested change
Once the form is completed, click :guilabel:`Save` to add it to the :guilabel:`Blacklist`. To
remove any number from the :guilabel:`Blacklist`, select the desired number, and click
:guilabel:`Unblacklist`.
Once the form is completed, click :guilabel:`Save` to add it to the :guilabel:`Blacklisted Phone
Numbers` list. To remove any number from the blacklist, select the desired number on the dashboard,
and then, on the phone number's form, click :guilabel:`Unblacklist`.

--------------------

During a software/platform migration, it is possible to import an already existing blacklist of
contacts. This would include customers, who have already asked to be :guilabel:`Blacklisted` on SMS
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
contacts. This would include customers, who have already asked to be :guilabel:`Blacklisted` on SMS
contacts. This would include customers, who have already asked to be blacklisted` on :abbr:`SMS
(Short Message Service)` mailings.

Comment on lines 140 to 142
To do that, navigate to the :guilabel:`SMS Marketing` app, and go to :menuselection:`Configuration
--> Blacklisted Phone Numbers`. Then, select the :guilabel:`Favorites` drop-down (beneath
the search bar), and click :guilabel:`Import records`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To do that, navigate to the :guilabel:`SMS Marketing` app, and go to :menuselection:`Configuration
--> Blacklisted Phone Numbers`. Then, select the :guilabel:`Favorites` drop-down (beneath
the search bar), and click :guilabel:`Import records`.
To do that, navigate to :menuselection:`SMS Marketing app --> Configuration --> Blacklisted Phone
Numbers`, and then select the :guilabel:`Favorites` drop-down menu (beneath the search bar), and
click :guilabel:`Import records`.

Copy link
Contributor

@StraubCreative StraubCreative left a comment

Choose a reason for hiding this comment

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

EDIT: see review notes above. GH bugged out and submitted the review twice 😓

@ksc-odoo
Copy link
Contributor Author

ksc-odoo commented Apr 5, 2023

EDIT: see review notes above. GH bugged out and submitted the review twice 😓

If you agree with the changes, let me know so I can squash/merge, otherwise indicate if and where you might want to go over something in more detail.

Hey @StraubCreative I agree with ALL these suggested changes - feel free to squash/merge at your earliest convenience. Thanks!

@ksc-odoo ksc-odoo requested a review from StraubCreative April 5, 2023 18:00
@StraubCreative StraubCreative force-pushed the 15.0-sms-marketing-adding-essentials-ksc branch from 01d323d to 10ecd29 Compare April 5, 2023 20:22
@StraubCreative
Copy link
Contributor

@robodoo r+

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

Labels

repeat designation for PRs worth a second look

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants