Skip to content
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

[12.0][REF] purchase_landed_cost: Rename variables #901

Conversation

clementmbr
Copy link
Member

@clementmbr clementmbr commented Apr 29, 2020

This PR is linked with #897 where I opened suggestions to improve purchase_landed_cost.

Here, the objective is to :

  1. Avoid confusion between product's standard_price (which will be modified by clicking on 'Update Cost') and the sought-after "product's Landed Cost"
  2. Avoid confusion between the words 'price', 'expenses' and 'cost'. Following the idea that :
    Cost = Price + Expenses

Thus, the following changes were made :

  • delete product_price_unit (a related field to stock move's
    price_unit)
  • rename standard_price_old into product_price_unit (a calculated
    field which give stock move's price_unit)
  • rename standard_price_new into landed_cost_unit (and reset it to
    zero when the Cost Distribution calculation is canceled)
  • rename total_amount into product_price_amount
  • rename cost_ratio into expense_unit

The question I ask myself right now is if it will be useful to add two more fields :

  • standard_price_old which would be the real "old product's standard_price before the stock_move"
  • and standard_price_new which would be the real "new updated product's standard_price after clicking on 'Update Cost' "

These two fields may be interesting, but it might be difficult to display them on the same o2m tree view which already have 9 columns :

image

@pedrobaeza
Copy link
Member

If you do this, you need to include migration scripts using openupgradelib and calling rename_fields method.

@clementmbr
Copy link
Member Author

@pedrobaeza Yes it will be done if these changes are approved.
Do you think they make sense ?
Should I also add these 2 informative fields referring to the "old standard_price before the stock move" and the "new standard_price after the stock_move and after the added landed_cost expenses" ?

@clementmbr
Copy link
Member Author

@pedrobaeza I've just added the migration scripts.
Travis is still red here because of small flake8 problems that were corrected on #897 .

However I hope the rest of the PR is good to be reviewed now.

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

make sense. Code review / no test.

@pedrobaeza
Copy link
Member

I see 2 renamings incorrect, putting expenses in plural, which is very misleading and tend to typos coding. Also, Travis should be green. If there's the need of cleaning the branch, please do it in other PR which will be merged quicker.

@clementmbr
Copy link
Member Author

clementmbr commented May 5, 2020

@pedrobaeza ok i've done a very small PR for the flake8 problems : #907

About this name for the Cost Distribution Expenses i thought expenses_unit and expenses_amount would be interesting to help differentiate them from fields related to a single expense... But I agree it can lead to typos errors coding.

I've changed them to expense_unit and expense_amount then. I'll do the rebase to make Travis green as soon as #907 is merged.

Thank you very much!

@clementmbr clementmbr force-pushed the 12.0-purchase-landed-cost-price-variables branch 2 times, most recently from cd599a1 to ea19066 Compare May 7, 2020 17:17
@clementmbr clementmbr force-pushed the 12.0-purchase-landed-cost-price-variables branch from ea19066 to 544269f Compare May 8, 2020 15:12
This commit aim to :
1. Avoid confusion between product's `standard_price` (which will be mod
ified by clicking on 'Update Cost') and the sought-after "product's
Landed Cost"
2. Avoid confusion between the words 'price', 'expenses' and 'cost'.
Following the idea that :
    Cost = Price + Expenses

Thus, the following changes were made :

- delete `product_price_unit` (a **related** field to stock move's
`price_unit`)
- rename `standard_price_old` into `product_price_unit` (a 
**calculated**
 field which give stock move's `price_unit`)
- rename `standar_price_new` into `landed_cost_unit` (and reset it to
zero when the Cost Distribution calculation is canceled)
- rename `total_amount` into `product_price_amount`
- rename `cost_ratio` into `expense_unit`
@clementmbr clementmbr force-pushed the 12.0-purchase-landed-cost-price-variables branch from 544269f to f030d35 Compare May 8, 2020 15:17
@clementmbr
Copy link
Member Author

@pedrobaeza now Travis is green on this PR too and I corrected the plural in expenses.

Hope it looks good for you now ! Let me know if there is anything more I can do to be able to merge this PR.
cc @rvalyi @legalsylvain

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Dec 11, 2022
@github-actions github-actions bot closed this Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants