Skip to content

Conversation

@meval1006
Copy link
Contributor

Created a new Margins page for Sales.

task link

@robodoo
Copy link
Collaborator

robodoo commented Oct 28, 2025

Pull request status dashboard

@C3POdoo C3POdoo requested a review from a team October 28, 2025 00:59
@meval1006 meval1006 requested a review from Felicious October 30, 2025 19:11
Copy link
Contributor

@Felicious Felicious left a comment

Choose a reason for hiding this comment

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

Congrats on pushing your first PR, @meval1006 !! 🎊

You’ve clearly understood the feature well. However, the doc currently reads more like a summary or description of the margin feature, rather than a step-by-step guide that helps users apply it on a quote or sales order.

There are also a few minor style notes (e.g., when to use :guilabel: for labels), but that’s nothing major as you’ll naturally pick up those conventions as you continue working with the team!

Your PR is basically there, we just need to reframe things differently and it'll be ready!

Comment on lines 8 to 10
In the Odoo **Sales** application, it is possible to show sales margins on quotations and sales
orders. Doing so allows for better management and monitoring of profitability and sustainable
growth.
Copy link
Contributor

Choose a reason for hiding this comment

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

Your introduction is clear, and it effectively explains the feature and its value without unnecessary fluff! Greaet work, it's what I'm looking for!

The only point that needs clarification is “sustainable growth.” What exactly does this refer to? The sentence “Doing so allows for better management and monitoring of profitability” already conveys a strong idea on its own.

You could make the message even stronger by addressing the reader directly. For instance:

Salespeople can use the feature to better monitor profitability per quotation line item.

Comment on lines 22 to 32
Calculating margins
-------------------

The sales margin for a product is calculated by subtracting the **Cost Price** from the **Sales
Price**: :math:`Sales~Margin = Sales~Price - Cost~Price`.

.. image:: margin/product-view.png
:alt: Cable Management box product page.

.. important::
Sales margin calculation requires the Cost and Sales Price fields of the product to be filled.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this section? It seems like you're instructing people to fill out the product form? Let's use a bit more commanding language if that's the case, like

Suggested change
Calculating margins
-------------------
The sales margin for a product is calculated by subtracting the **Cost Price** from the **Sales
Price**: :math:`Sales~Margin = Sales~Price - Cost~Price`.
.. image:: margin/product-view.png
:alt: Cable Management box product page.
.. important::
Sales margin calculation requires the Cost and Sales Price fields of the product to be filled.
Calculating margins
-------------------
To automatically calculate the sales margin for each quotation or sales order line item, go to :menuselection:`[insert directions to go to the product form]` and fill out the :guilabel:`Sales Price` and :guilabel:`Cost` fields.
The margin is calculated by subtracting the **Cost Price** from the **Sales
Price** :math:`Sales~Margin = Sales~Price - Cost~Price`.
.. image:: margin/product-view.png
:alt: Cable Management box product page.

(If you accept my suggestion of making it more instructional, i think we can move the contents of your important block into the main explanation itself)

.. important::
Sales margin calculation requires the Cost and Sales Price fields of the product to be filled.

View Margins
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicking here, but "view" sounds like we're looking at a graph. How about a title like "Compute margin on quote"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh but this might clash with the suggested title I gave above, "calculating margin" and "Compute margin" is too similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about having this structure/titles, so configuration sections look like theyre clearly set up, and the workflow titles are action-oriented? Feel free to change the wording around!


Configuration
=====

Configure price and cost
----------------

For product form setup

Compute margins on SOs
===============

For adding the margin on sales orders/quotes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the 'Compute margins on SOs' is 'Enable' an acceptable substitute?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's great! thank you (:

@meval1006 meval1006 force-pushed the 18.0-Sales-Margin-meval branch from af263e8 to f2965a8 Compare October 31, 2025 21:55
@meval1006 meval1006 self-assigned this Oct 31, 2025
@meval1006 meval1006 requested a review from Felicious October 31, 2025 21:59
Copy link
Contributor

@Felicious Felicious left a comment

Choose a reason for hiding this comment

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

omg @meval1006 this is 98% there!! I'm setting it as request changes because this is your very first PR, and i want to do one last double check before merging. I had no major issues to point out this iteration! Just some optional suggestions to perfect it to consider (:

Comment on lines 57 to 67
.. image:: margin/SO-with-margins-checkboxes.png
:alt: Sales order with Margin and Margin(%) columns displayed.
Copy link
Contributor

Choose a reason for hiding this comment

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

this picture is really similar with the picture above. let's keep just one of them.

I like the zoom of the second one, especially since you hid columns that weren't necessary. However, this second image hides the Margin% column, that I think the reader should see. Although I think it's helpful to show ppl where the settings adjust icon is, though. Alot of ppl miss it

@meval1006 meval1006 force-pushed the 18.0-Sales-Margin-meval branch from 4853a6a to a1272ce Compare November 1, 2025 00:12
Copy link
Contributor

@Felicious Felicious left a comment

Choose a reason for hiding this comment

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

Final quick review!

I ran make review on the RST file, and there are some trailing whitespaces and lines that ended too early:

Image

I also caught some very minor things in my review, and after that, this PR will definitely be ready to merge! tag me again after this for one final look!

Comment on lines 90 to 91
Imagine you want to apply a seasonal 5% discount on blue denim jeans. The discount requires a
minumim of two pairs of jeans in an order and applies only from October to the end of December.
Copy link
Contributor

Choose a reason for hiding this comment

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

We use "you" sparingly since it can come across as overly familiar with the reader. We're more lax with the use of "your" because sometimes, when we're talking about multiple roles, like the user, who owns a company and is setting up Odoo for their business, and how it might be displayed for the customer.When referring to the business, we might use "when setting up __ for your company..."

What do you think about:

Suggested change
Imagine you want to apply a seasonal 5% discount on blue denim jeans. The discount requires a
minumim of two pairs of jeans in an order and applies only from October to the end of December.
To apply a seasonal 5% discount on blue denim jeans that requires a
minimum of two pairs of jeans in an order and is valid only from October to the end of December,

Comment on lines 92 to 96
The pricelist rule should look like this:

.. image:: margin/pricelist-configuration.png
:align: center
:alt: Pricelist Rules pop-up window.
Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of an overkill, but we tend to not rely on images to tell stories, in case the images don't load for people. It can be quite wordy though, so maybe we can describe the image using a figure.

Suggested change
The pricelist rule should look like this:
.. image:: margin/pricelist-configuration.png
:align: center
:alt: Pricelist Rules pop-up window.
The pricelist rule should look like this:
.. figure:: margin/pricelist-configuration.png
:align: center
:alt: Pricelist Rules pop-up window.
[Insert description of the image here]

This is an optional suggestion! Don't have to follow it :D

Comment on lines 98 to 99
After saving the pricelist, go to your SO and select the newly created pricelist, and adjust the
quanity according to the pricelist rule.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. required: abbreviations should use the abbr markup
  2. opinion: don't think "your" is necessary here, but the suggested rewrite is longer
  3. required: typo for quantity

Note: Feel free to edit this suggestion to choose the rewrite that you want to accept, and commit that!

Suggested change
After saving the pricelist, go to your SO and select the newly created pricelist, and adjust the
quanity according to the pricelist rule.
After saving the pricelist, go to the desired :abbr:`SO (sales order)` and select the newly created pricelist, and adjust the
quantity according to the pricelist rule.

Comment on lines 105 to 107
After you made these changes, click :guilabel:`Update Prices` to update the product price and
margin. The margin is recalculated based on the price-list-adjusted sales price and the product's
cost price.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. optional: removing you
  2. optional/question: are you referring to specific Odoo fields when referring to "sales price* and "margin"? If so, we should match the capitalization of the UI, and use italics, but NOT guilabel, as guilabel only refers to a field that we're referring to directly in an instruction or on the page we're looking at (though, technically, we can see Margin on the SO, so maybe we should guilabel that, and change "Sales Price" to the equivalent column we see on the sales order, "Unit Price", and put a guilabel on that? This is kinda getting really nitty gritty and I don't want to argue with you about something so specific 🤡 , so let's go with whatever you decide!
  3. required: add the icon in front of Update Prices. Since the icon is associated with text, there's no need to do the thing where we describe the icon in guilabels and parentheses
  4. question?: price-list --> pricelist since that's how we've been spelling it the rest of the doc?
  5. required: drop "price" from "cost" since the field label in Odoo is "Cost"
Suggested change
After you made these changes, click :guilabel:`Update Prices` to update the product price and
margin. The margin is recalculated based on the price-list-adjusted sales price and the product's
cost price.
After making the changes, click [NEEDS ICON] :guilabel:`Update Prices` to update the product's *Sales Price* and
*Margin*. The margin is recalculated based on the pricelist-adjusted *Sales Price* and the product's
*Cost*.

margin. The margin is recalculated based on the price-list-adjusted sales price and the product's
cost price.

.. image:: margin/SO-with-applied-pricelist.png
Copy link
Contributor

Choose a reason for hiding this comment

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

super minor but we don't have any capitalizations with our images, so can you change all the image names from SO- to so-?


.. image:: margin/SO-with-applied-pricelist.png
:align: center
:alt: Sales order with margins recalculated based on the price-list adjustment.
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
:alt: Sales order with margins recalculated based on the price-list adjustment.
:alt: Sales order with margins recalculated based on the pricelist adjustment.

optional

@meval1006 meval1006 requested a review from Felicious November 3, 2025 21:28
@meval1006 meval1006 force-pushed the 18.0-Sales-Margin-meval branch 2 times, most recently from 5486256 to 79e31a8 Compare November 3, 2025 22:12
Copy link
Contributor

@Felicious Felicious left a comment

Choose a reason for hiding this comment

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

Approved with one typo caught!

Once you fix that, it's ready for merge!

@robodoo delegate=meval1006

@meval1006 meval1006 force-pushed the 18.0-Sales-Margin-meval branch from 7519bbb to cc01980 Compare November 3, 2025 22:50
@meval1006 meval1006 added the 5 label Nov 3, 2025
@meval1006 meval1006 force-pushed the 18.0-Sales-Margin-meval branch from cc01980 to 64d27ad Compare November 3, 2025 23:06
@meval1006
Copy link
Contributor Author

@robodoo r+

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants