Skip to content

Conversation

@samueljlieber
Copy link
Contributor

Task: #3010897

@robodoo
Copy link
Collaborator

robodoo commented Oct 25, 2022

@tiku-odoo tiku-odoo requested a review from mivu-odoo October 26, 2022 12:10
@tiku-odoo
Copy link
Contributor

@mivu-odoo This doc for Sendcloud configuration Odoo 16 is ready for your final review when you have a moment. Thanks, Tim :)

@samueljlieber samueljlieber marked this pull request as ready for review October 26, 2022 12:55
@C3POdoo C3POdoo requested a review from a team October 26, 2022 12:57
Copy link
Contributor

@mivu-odoo mivu-odoo left a comment

Choose a reason for hiding this comment

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

Hello @tiku-odoo and @samueljlieber!

I had a few questions on capitalizations of field labels that @tiku-odoo will need to double-check, if possible. Once confirmed and the changes are applied, please tag me again for another look. Thank you 😸

@tiku-odoo tiku-odoo force-pushed the 16.0-inventory-sendcloud-shipping-connector-tiku branch from c5c2ffc to fe7bf5f Compare October 31, 2022 20:27
Copy link
Contributor

@tiku-odoo tiku-odoo left a comment

Choose a reason for hiding this comment

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

@mivu-odoo

This document is ready for your review. I'd like to remove lines 40-41. I look forward to any changes you'd like to make. Thanks in advance for reviewing this doc!

Copy link
Contributor

@mivu-odoo mivu-odoo left a comment

Choose a reason for hiding this comment

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

Hello @samueljlieber and @tiku-odoo!

Thank you for revising the PR based on my comments! I left a couple of small comments on spacing/line breaks and :guilabel:'s. I agree with @tiku-odoo that the .. note:: on Lines 40-41 can be deleted.

Feel free to make those revisions and go straight to ZST for final technical review.

Thank you 😸

@samueljlieber samueljlieber force-pushed the 16.0-inventory-sendcloud-shipping-connector-tiku branch from fe7bf5f to 83a4e56 Compare November 2, 2022 14:47
@samueljlieber
Copy link
Contributor Author

Hi @StraubCreative! This doc is ready for a final technical review! 🙂

@tiku-odoo
Copy link
Contributor

@StraubCreative Hi Zac, Thomas has a quick edit for this doc that I'm going to add on regarding the return labels. Please hold off on this review for now until I make the update. Thanks for your flexibility.

@tiku-odoo tiku-odoo force-pushed the 16.0-inventory-sendcloud-shipping-connector-tiku branch from 83a4e56 to e4ed44d Compare November 9, 2022 14:40
@tiku-odoo
Copy link
Contributor

@mivu-odoo There is another update from Thomas on this PR. I've added in lines 160-162. Can you proof-read and push to final review? Thanks for reviewing again :)

Copy link
Contributor

@mivu-odoo mivu-odoo left a 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 a small edit for the new content on Lines 160-162. Once you push the edit, feel free to tag ZST again for final technical review!

@tiku-odoo tiku-odoo force-pushed the 16.0-inventory-sendcloud-shipping-connector-tiku branch from e4ed44d to 6931f9f Compare November 10, 2022 18:47
@tiku-odoo
Copy link
Contributor

@StraubCreative This document is ready for your technical review. Whenever you have a moment can you take a look? Thanks in advance for your help on this! 👍

@StraubCreative
Copy link
Contributor

Kicking back to @mivu-odoo for second content review.
If you run into merge conflict on your revisions bc of #2983 let me or @samueljlieber know.

@StraubCreative StraubCreative removed their request for review December 2, 2022 22:05
Copy link
Contributor

@mivu-odoo mivu-odoo left a comment

Choose a reason for hiding this comment

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

Hi @tiku-odoo!

A couple of edits needed. Please tag me for a second look when you're ready. Thank you 😸

@tiku-odoo tiku-odoo force-pushed the 16.0-inventory-sendcloud-shipping-connector-tiku branch from 6931f9f to 1a550e2 Compare December 6, 2022 22:46
@tiku-odoo
Copy link
Contributor

@mivu-odoo I've made the requested changes; it's ready for your review again. Thanks for your help on this doc. Have a good evening! 👍

Copy link
Contributor

@mivu-odoo mivu-odoo left a comment

Choose a reason for hiding this comment

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

Hello @tiku-odoo!

Thank you for the quick edits! I found a couple more lines that needed edits. Please tag me again when you're ready. Thank you 😸

@tiku-odoo tiku-odoo force-pushed the 16.0-inventory-sendcloud-shipping-connector-tiku branch from 1a550e2 to 46a3062 Compare December 7, 2022 21:16
@tiku-odoo
Copy link
Contributor

@mivu-odoo

Thanks for your changes! 💯

I've pushed the changes you requested and made a minor edit to line 91, where the module was said to be "activated" instead of "installed" first. I've changed both "activated" references in the last sentence on line 91 to state "install." After the installation, the connector needs to be activated, which we state in the following section.

👍

@tiku-odoo tiku-odoo force-pushed the 16.0-inventory-sendcloud-shipping-connector-tiku branch 3 times, most recently from 725f3de to 2bea601 Compare December 22, 2022 17:27
@tiku-odoo
Copy link
Contributor

@mivu-odoo I've incorporated the edits from Thomas (thd) into this Sendcloud PR.

This document is ready for another review! 👍

Edited lines:
Lines:
41-44 - edited

78-79 - edited to say Odoo Native

Removed lines 83-84 (no longer in PR)

Added Lines 221-227

When you have a moment, can you review these changes?

Thank you!

Tim

Copy link
Contributor

@mivu-odoo mivu-odoo left a comment

Choose a reason for hiding this comment

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

Hello @tiku-odoo!

I'm approving with just one comment. Once you push the edit, please tag ZST directly for a final technical review. Thank you 😸

@StraubCreative StraubCreative self-requested a review December 26, 2022 02:19
@samueljlieber samueljlieber force-pushed the 16.0-inventory-sendcloud-shipping-connector-tiku branch from 2bea601 to 37f9e71 Compare December 28, 2022 18:35
@samueljlieber
Copy link
Contributor Author

Updates in 37f9e71:

  • @mivu-odoo's suggestion
  • Corrected a couple instances of where 'Sendcloud' was pascal cased (i.e. SendCloud)

@StraubCreative ready for your technical review!

cc. @tiku-odoo

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 @tiku-odoo

I'm pausing technical review for the moment as there are some content issues that need to be addressed first.

@samueljlieber let's connect on the example tags and see if there's a better way we can structure those with or without images.

@mivu-odoo can you give this another pass-through please? We can trim a bunch of unnecessary words/sentences I think and I could use a second opinion about the example tag usage 🙏 It's a new tag for us as a team so maybe we can try to define a convention of when/how to use it?

@samueljlieber samueljlieber force-pushed the 16.0-inventory-sendcloud-shipping-connector-tiku branch from 37f9e71 to bb2e256 Compare January 5, 2023 22:39
@samueljlieber
Copy link
Contributor Author

bb2e256 for example block changes, still need to make the remaining changes from @StraubCreative.

@tiku-odoo tiku-odoo force-pushed the 16.0-inventory-sendcloud-shipping-connector-tiku branch from bb2e256 to 755c6ce Compare January 6, 2023 18:11
@tiku-odoo
Copy link
Contributor

tiku-odoo commented Jan 6, 2023

@mivu-odoo This doc is ready for your review again when you have a moment.

See above comments from Zac (zst): #2864 (review)

Thanks for your review 👍

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 @tiku-odoo
Change requests and suggestions below for content.
Let's knock these out and I can give it another look 👍

@tiku-odoo tiku-odoo force-pushed the 16.0-inventory-sendcloud-shipping-connector-tiku branch from 755c6ce to 5eab1d0 Compare January 17, 2023 22:37
@tiku-odoo
Copy link
Contributor

Thanks, @StraubCreative, for the review and recommending changes. I've made the requested changes and understand where you're coming from. I've also added GUI-labels to the examples.

This document is ready for another final review when you have some time. Thanks in advance for your help on this Sendcloud doc 💯 👍

@StraubCreative StraubCreative force-pushed the 16.0-inventory-sendcloud-shipping-connector-tiku branch from 5eab1d0 to 635cad7 Compare January 22, 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 @tiku-odoo

We're at the finish line 🚀

I've added some final considerations below, mostly around the formatting of the example tags which included a few formatting items we'd have to address anyway.

Please see the second commit 635cad7 that I attached to the PR to view the updates using Make.

Instructions:

  • git checkout 16.0-inventory-sendcloud-shipping-connector-tiku
  • git pull --rebase
  • git log --> the HEAD should be at 635cad7. If not, do git checkout 635cad7.
  • make html and view the changes in a browser.

If you agree with the changes I made, then I can squash the commits, push back up and ship to DR.

Thanks and let me know!

cc: @samueljlieber

Comment on lines 46 to 60
.. example::

| **SendClould configuration:**
| :guilabel:`Miscellaneous`
| :guilabel:`Address Name (optional)` - **Warehouse #1**
| :guilabel:`Brand` - Default

| **Odoo warehouse configuration:**
| :guilabel:`Warehouse` - **Warehouse #1**
| :guilabel:`Short Name` - WH
| :guilabel:`Company` - My company (San Francisco)
| :guilabel:`Address` - My Company (San Francisco)

Notice how the name for both the Odoo configuration and the Sendcloud configuration are the exact
same.
Copy link
Contributor

Choose a reason for hiding this comment

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

This section still isn't quite right.

  • removed colons : from sub-section headings (not needed imo)
  • labeled field data with backticks
  • touched up the psuedo-note at the end for clarity.

Here's are before and after screenshots to visually show the difference.

Suggested change
.. example::
| **SendClould configuration:**
| :guilabel:`Miscellaneous`
| :guilabel:`Address Name (optional)` - **Warehouse #1**
| :guilabel:`Brand` - Default
| **Odoo warehouse configuration:**
| :guilabel:`Warehouse` - **Warehouse #1**
| :guilabel:`Short Name` - WH
| :guilabel:`Company` - My company (San Francisco)
| :guilabel:`Address` - My Company (San Francisco)
Notice how the name for both the Odoo configuration and the Sendcloud configuration are the exact
same.
.. example::
| **SendClould configuration**
| :guilabel:`Miscellaneous`
| :guilabel:`Address Name (optional)`: `Warehouse #1`
| :guilabel:`Brand`: `Default`
| **Odoo warehouse configuration**
| :guilabel:`Warehouse`: `Warehouse #1`
| :guilabel:`Short Name`: `WH`
| :guilabel:`Company`: `My company (San Francisco)`
| :guilabel:`Address`: `My Company (San Francisco)`
Notice how the inputs for the :guilabel:`Warehouse` field, for both the Odoo configuration and
the Sendcloud configuration, are the exact same.

Comment on lines 26 to 27
completing the account setup, activate (or deactivate) the shipping carriers that will be used in
the Odoo database.
Copy link
Contributor

Choose a reason for hiding this comment

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

100th char break

Suggested change
completing the account setup, activate (or deactivate) the shipping carriers that will be used in
the Odoo database.
completing the account setup, activate (or deactivate) the shipping carriers that will be used
in the Odoo database.

Comment on lines 127 to 146
.. example::
Sample Sendcloud shipping products configured in Odoo:

| **Delivery:**
| :guilabel:`Shipping Product` - DPD Home 0-31.5kg
| :guilabel:`Carrier` - DPD
| :guilabel:`Minimum Weight` - 0.00
| :guilabel:`Maximum Weight` - 31.50

:guilabel:`Countries` - Austria, Belgium, Bosnia and Herzegovina, Bulgaria, Croatia, Czech
Republic, Denmark, Estonia, Finland, France, Germany, Greece, Hungary, Iceland, Ireland, Italy,
Latvia, Liechtenstein, Lithuania, Luxembourg, Monaco, Netherlands, Norway, Poland, Portugal,
Romania, Serbia, Slovakia, Slovenia, Spain, Sweden, Switzerland

| **Return:**
| :guilabel:`Return Shipping Product`- DPD Return 0-20kg
| :guilabel:`Return Carrier` - DPD
| :guilabel:`Return Minimum Weight` - 0.00
| :guilabel:`Return Minimum Weight` - 20.00
| :guilabel:`Return Countries` - Belgium, Netherlands
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here— would format everything like previous example tag.
Notice the guilabel formatting for DELIVERY and RETURN as this is how it shows up on the GUI.

Suggested change
.. example::
Sample Sendcloud shipping products configured in Odoo:
| **Delivery:**
| :guilabel:`Shipping Product` - DPD Home 0-31.5kg
| :guilabel:`Carrier` - DPD
| :guilabel:`Minimum Weight` - 0.00
| :guilabel:`Maximum Weight` - 31.50
:guilabel:`Countries` - Austria, Belgium, Bosnia and Herzegovina, Bulgaria, Croatia, Czech
Republic, Denmark, Estonia, Finland, France, Germany, Greece, Hungary, Iceland, Ireland, Italy,
Latvia, Liechtenstein, Lithuania, Luxembourg, Monaco, Netherlands, Norway, Poland, Portugal,
Romania, Serbia, Slovakia, Slovenia, Spain, Sweden, Switzerland
| **Return:**
| :guilabel:`Return Shipping Product`- DPD Return 0-20kg
| :guilabel:`Return Carrier` - DPD
| :guilabel:`Return Minimum Weight` - 0.00
| :guilabel:`Return Minimum Weight` - 20.00
| :guilabel:`Return Countries` - Belgium, Netherlands
.. example::
Sample Sendcloud shipping products configured in Odoo:
| :guilabel:`DELIVERY`
| :guilabel:`Shipping Product`: `DPD Home 0-31.5kg`
| :guilabel:`Carrier`: `DPD`
| :guilabel:`Minimum Weight`: `0.00`
| :guilabel:`Maximum Weight`: `31.50`
:guilabel:`Countries`: `Austria` `Belgium` `Bosnia` `Herzegovina` `Bulgaria` `Croatia` `Czech`
`Republic` `Denmark` `Estonia` `Finland` `France` `Germany` `Greece` `Hungary` `Iceland`
`Ireland` `Italy` `Latvia` `Liechtenstein` `Lithuania` `Luxembourg` `Monaco` `Netherlands`
`Norway` `Poland` `Portugal` `Romania` `Serbia` `Slovakia` `Slovenia` `Spain` `Sweden`
`Switzerland`
| :guilabel:`RETURN`
| :guilabel:`Return Shipping Product`: `DPD Return 0-20kg`
| :guilabel:`Return Carrier`: `DPD`
| :guilabel:`Return Minimum Weight`: `0.00`
| :guilabel:`Return Minimum Weight`: `20.00`
| :guilabel:`Return Countries`: `Belgium` `Netherlands`

Copy link
Contributor

Choose a reason for hiding this comment

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

@StraubCreative
CC: @samueljlieber

The changes in this commit look great! The examples are really clear now. Thanks for making the changes 👍

635cad7

This commit is good to push up and send to DR.

Thanks for all your work on this. Looking forward to seeing it published.

👍

@StraubCreative StraubCreative force-pushed the 16.0-inventory-sendcloud-shipping-connector-tiku branch from 635cad7 to a031a6c Compare January 25, 2023 19:11
@StraubCreative StraubCreative requested a review from a team January 27, 2023 21:59
@StraubCreative
Copy link
Contributor

This doc on sendcloud shipping is good for your review @odoo/doc-review 🚀
Thanks in advance!

Copy link
Collaborator

@AntoineVDV AntoineVDV left a comment

Choose a reason for hiding this comment

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

💯

@robodoo r+

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants