-
Notifications
You must be signed in to change notification settings - Fork 9k
[IMP] sales: update simplified pricelist settings #11709
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
Hi @larm-odoo this PR is ready for peer review, when you have a chance. Thank you! |
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 @dikd-odoo - super simple, easy update! Just a couple of minor suggestions, and one formatting correction. Approving because the formatting one is quick, and the others are totally unnecessary.
content/applications/sales/sales/products_prices/prices/pricing.rst
Outdated
Show resolved
Hide resolved
content/applications/sales/sales/products_prices/prices/pricing.rst
Outdated
Show resolved
Hide resolved
3f07a35
to
f39b49a
Compare
Thank you for the suggestions, @larm-odoo! Hi @Felicious this PR is ready for final review, when you have a chance. Thank you! |
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.
Your changes look great to me, @dikd-odoo ! Grateful for the effort you’ve put into improving clarity and conciseness in this document. Approving now! 😊
That said, I need to adjust the point value of this PR from 3 points to 1 point. I’m sorry for the confusion, and I take partial responsibility—our current guidelines for task scoring could be clearer, and we're working on a document to better explain these specifications.
Here’s a quick summary of how tasks are scored:
- 3 points: A significant rework where ~50-80% of the document is revised.
- 2 points: Version-specific updates or improvements to a single section.
- 1 point: Tiny edits such rewriting a few sentences.
Your rewrite of lines 16–28 into the streamlined version at lines 16–18 is fantastic—clearer and more concise! However, since this PR focuses on rewording a few sentences rather than updating the entire section or addressing outdated UI comprehensively, it qualifies as a small cosmetic change worth 1 point.
To bring this PR to 2 points, you’d need to update the rest of the section (from the Pricelist configuration heading to the Price rules tab section), ensure all instructions are fully relevant for saas 17.4, refine the rest for clarity and conciseness, and replace outdated images.
If you'd like to make the changes to be 2 points, tag me again after you've made the updates, and I'll review it again. Let me know if you have any questions!
82a2072
to
48a74b3
Compare
Hi @Felicious thank you for the feedback! I further edited the section "Pricelists configuration," as well as updated the following sections: -Removed Multiple & Advanced Pricelists sections as they are deprecated in 17.4+ This is ready for a second look, when you have time. Thank you! |
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 @dikd-odoo ! Good job looking through the rest of the document and deleting sections about depreciated features. Since you've changed more than 50% of the doc, I grant this PR 3 points (:
I've added a few more comments to consider, and after working through those, feel free to send this along to final review!
.. example:: | ||
To formulate a 100% markup (or 2 times the cost of the product), with a $5 minimum margin, set | ||
the :guilabel:`Based on` field to :guilabel:`Cost`, the :guilabel:`Discount` to `-100`, and the | ||
:guilabel:`Margins` to `5`. This is often seen in retail situations. |
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.
good example!
content/applications/sales/sales/products_prices/prices/pricing.rst
Outdated
Show resolved
Hide resolved
content/applications/sales/sales/products_prices/prices/pricing.rst
Outdated
Show resolved
Hide resolved
content/applications/sales/sales/products_prices/prices/pricing.rst
Outdated
Show resolved
Hide resolved
content/applications/sales/sales/products_prices/prices/pricing.rst
Outdated
Show resolved
Hide resolved
content/applications/sales/sales/products_prices/prices/pricing.rst
Outdated
Show resolved
Hide resolved
content/applications/sales/sales/products_prices/prices/pricing.rst
Outdated
Show resolved
Hide resolved
2ecdc6b
to
c89bfd9
Compare
Thank you for the helpful final review, @Felicious! Hi @samueljlieber this PR is ready for tech review when you have a chance. Thank you! |
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 @dikd-odoo! Really great work on this doc to remove the deprecated sections and expand with some great examples! I have a small handful of technical changes for you, please see my suggestions and following comments :)
Please run make review
on both the file (content/applications/sales/sales/products_prices/prices/pricing.rst
) and the image folder (content/applications/sales/sales/products_prices/prices/pricing
), there are some early line breaks and uncompressed images ;)
There are also a number of unused images, the following can be removed:
pricing/create-pricelist-rules-popup.png
pricing/extra-prices-smartbutton.png
pricing/formula-computation-options.png
pricing/price-rules-product-page.png
pricing/sales-pricelist-formula-computation.png
I'm requesting changes just so I can double check these images once you have had a chance to address them, thank you for your work! Let me know if you have any Qs!
content/applications/sales/sales/products_prices/prices/pricing.rst
Outdated
Show resolved
Hide resolved
content/applications/sales/sales/products_prices/prices/pricing.rst
Outdated
Show resolved
Hide resolved
content/applications/sales/sales/products_prices/prices/pricing.rst
Outdated
Show resolved
Hide resolved
content/applications/sales/sales/products_prices/prices/pricing.rst
Outdated
Show resolved
Hide resolved
content/applications/sales/sales/products_prices/prices/pricing.rst
Outdated
Show resolved
Hide resolved
145efe3
to
b1b12cd
Compare
Hi @samueljlieber thank you for the helpful review! I've implemented your changes, removed the unused images, and compressed the images in the folder. This is ready for another look, please let me know if you need anything else. Thank you! |
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 @dikd-odoo, thank you for addressing my changes! I am going to push up a commit that reverts your accidental edit to the Makefile as well as the images that were re-compressed. Thank you for your work, I will merge after I commit and the checks pass.
Makefile
Outdated
@@ -96,4 +96,4 @@ review: | |||
if echo $$path | grep -q 'content/'; then path=`echo $$path | sed 's|content/||'`; fi; \ | |||
if [ -z "$$line_length" ]; then line_length=100; fi; \ | |||
export REVIEW=1; \ | |||
python tests/main.py --max-line-length=$$line_length $(SOURCE_DIR)/$$path | |||
python3 tests/main.py --max-line-length=$$line_length $(SOURCE_DIR)/$$path |
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.
Make sure not to push up any unintended edits ;)
python3 tests/main.py --max-line-length=$$line_length $(SOURCE_DIR)/$$path | |
python tests/main.py --max-line-length=$$line_length $(SOURCE_DIR)/$$path |
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.
No need to re-compress these images, we want to avoid unnecessarily modifying images :)
b1b12cd
to
1532272
Compare
@robodoo r+ |
closes #11709 Signed-off-by: Samuel Lieber (sali) <[email protected]>
Updating Pricelists picture and documentation text for 17.4+, simplified Pricelists description in settings, removed 'Pricing Strategy Options' section. Also cleaning up language in documentation.
+Added updates:
-Revised "Pricelist configuration" section to match current UI flow
-Removed Multiple & Advanced Pricelists sections as they are deprecated in 17.4+
-Updated images in "Pricelists configuration" and "Price Rules tab" sections to match current UI and style guide
-Updated language for clarity and concision
-Began staging content changes in intro to re-format this into a parent doc for Sales pricelists
Task: https://www.odoo.com/odoo/my-tasks/4343498