Financial controlling and costing

Merge lp:~camptocamp/margin-analysis/7.0-port-product_historical_margin-xml-retab-yvr into lp:~margin-analysis-core-editors/margin-analysis/7.0

  1. 7.0-port-product_historical_margin-xml-retab-yvr
  2. Merge into 7.0
Proposed by Yannick Vaucher @ Camptocamp
Status: Merged
Merged at revision: 50
Proposed branch: lp:~camptocamp/margin-analysis/7.0-port-product_historical_margin-xml-retab-yvr
Merge into: lp:~margin-analysis-core-editors/margin-analysis/7.0
Diff against target: 285 lines (+118/-119)
4 files modified
product_historical_margin/account_invoice_view.xml (+74/-75)
product_historical_margin/invoice.py (+4/-3)
product_historical_margin/product_view.xml (+2/-2)
product_historical_margin/wizard/historical_margin_view.xml (+38/-39)
To merge this branch: bzr merge lp:~camptocamp/margin-analysis/7.0-port-product_historical_margin-xml-retab-yvr
Reviewer Review Type Date Requested Status
Pedro Manuel Baeza code review, no test Needs Information
Joël Grand-Guillaume @ camptocamp code review, no tests Approve
Review via email: mp+190350@code.launchpad.net

Description of the change

Pre-portage only retabing XML for readability

To post a comment you must log in.
Revision history for this message
Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c) wrote :

LGTM, Thanks !

review: Approve (code review, no tests)
Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

Hi, Yannick, thank you very much for the cleaning.

I wonder, because we have never discussed this before, what is the correct space indentation for XML. In Python it is used 4 spaces. I have seen that you have used 2 spaces. I see this a little short, but I would like to know the opinion of others.

Regards.

review: Needs Information (code review, no test)
Revision history for this message
Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c) wrote :

Hi,

We do not really define anything regarding the XML file... I will be in favor of having the same space identation : 4 spaces.

Regards,

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

I prefer 2 spaces indentations as in xml you can have a lot of indentation levels
In python with max width of 80 or 100 and for readability you better have a maximum of 5 indentation level and can have only 3 levels.

class:
4s def:
8s code
12s nested code

In XML if you want to do inheritance you will need at least 6 levels

<openerp>
4s <data>
8s <record id=
12s <field name="arch"
16s <xpath
20s <field name="my_new_field"

Furthermore, with html and v7 style you get even more indentation levels

Let's take the kanban view of res.partner as a bad example:
line 237 in res_partner_view.xml
it begins at columns 77
(19 levels of indentation and if should be 20 levels as sheet tag isn't properly indented)

In my opinion, 2 spaces in XML doesn't make it less readable.

But I don't intend to indent by 2 if we choose 4 for our code.

3 spaces would be a compromise and tabs would let people decide how to display it and reduce file size.
But let's keep the discution on 2 or 4 as it would start being more philosophical :)

Revision history for this message
Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c) wrote :

I vote then for 2 spaces

Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

Then maybe we have to tell to whole community and get an agreement to use this convention across all the contributions. Don't you think?

Regards.

Revision history for this message
Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c) wrote :

+1 to add this in the community docs Maxime wrote a little while ago.

On Thu, Oct 17, 2013 at 1:35 PM, Pedro Manuel Baeza
<email address hidden>wrote:

> Then maybe we have to tell to whole community and get an agreement to use
> this convention across all the contributions. Don't you think?
>
> Regards.
> --
>
> https://code.launchpad.net/~camptocamp/margin-analysis/7.0-port-product_historical_margin-xml-retab-yvr/+merge/190350
> You are reviewing the proposed merge of
> lp:~camptocamp/margin-analysis/7.0-port-product_historical_margin-xml-retab-yvr
> into lp:margin-analysis.
>

--

*camptocamp*
INNOVATIVE SOLUTIONS
BY OPEN SOURCE EXPERTS

*Joël Grand-Guillaume*
Division Manager
Business Solutions

+41 21 619 10 28
www.camptocamp.com

Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

With regards to XML tab width: the defacto standard in official addons is 4 throughout even if there are some inconsistencies. I vote for sticking to 4 for that reason, even if 2 would have been more practical.

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

About OpenERP "standard" of former code we also have import with bad namespace, use of class alias and useless class instanciations.

In OpenERP coding guidelines nothing is clearly said about XML indentation:
https://doc.openerp.com/7.0/contribute/15_guidelines/
As you said, this is defacto

Nevertheless, my opinion is that we shouldn't say "this is a standard" if there are no obvious reasons beside it.

So what will be the community standard we will decide and why?

By the way, readability counts
(Does it also counts on code review in lp?)

Revision history for this message
David BEAL (ak) (davidbeal) wrote :

I vote then for 2 spaces

more readeable for new use case (kanban, futures cases)

Revision history for this message
Bogdan Stanciu (bstanciu) wrote :

2 spaces for reasons given above. thanks

Revision history for this message
Ronald Portier (ronald-portier) wrote :

I also vote for 2 spaces.

Also please: if there is more then one attribute, have each attribute on a new line.

(Unfortunately OpenERP will break if you spread attributes over multiple lines, even where this would be perfectly logical, as with long domain expressions).

Having each attribute on a new line will make changes much more "diff friendly". if you change one attribute, instead of having to scan a very long line where exactly anything changed, this will be immediately obvious.

lp:~camptocamp/margin-analysis/7.0-port-product_historical_margin-xml-retab-yvr updated
48. By Joël Grand-Guillaume @ camptocamp

[FIX] Function signature addiing context

Revision history for this message
Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c) wrote :

We go for 2 spaces then it seems. Can we go further on this merge please ?

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'product_historical_margin/account_invoice_view.xml'
--- product_historical_margin/account_invoice_view.xml 2012-07-12 13:56:17 +0000
+++ product_historical_margin/account_invoice_view.xml 2013-11-14 15:32:34 +0000
@@ -1,81 +1,80 @@
1<?xml version="1.0" encoding="utf-8"?>1<?xml version="1.0" encoding="utf-8"?>
2<openerp>2<openerp>
3 <data>3 <data>
4 4
5 <record id="view_invoice_line_tree_from_product" model="ir.ui.view">5 <record id="view_invoice_line_tree_from_product" model="ir.ui.view">
6 <field name="name">account.invoice.line.from.product.tree</field>6 <field name="name">account.invoice.line.from.product.tree</field>
7 <field name="model">account.invoice.line</field>7 <field name="model">account.invoice.line</field>
8 <field name="type">tree</field>8 <field name="type">tree</field>
9 <field name="priority">20</field>9 <field name="priority">20</field>
10 <field name="arch" type="xml">10 <field name="arch" type="xml">
11 <tree string="Invoice Line">11 <tree string="Invoice Line">
12 <field name="invoice_type"/>12 <field name="invoice_type"/>
13 <field name="partner_id"/>13 <field name="partner_id"/>
14 <field name="invoice_user_id"/>14 <field name="invoice_user_id"/>
15 <field name="invoice_id"/>15 <field name="invoice_id"/>
16 <field name="invoice_date"/>16 <field name="invoice_date"/>
17 <field name="name" invisible="1"/>17 <field name="name" invisible="1"/>
18 <field name="product_id"/>18 <field name="product_id"/>
19 <field name="account_id" invisible="1"/>19 <field name="account_id" invisible="1"/>
20 <field name="quantity"/>20 <field name="quantity"/>
21 <field name="uos_id"/>21 <field name="uos_id"/>
22 <field name="price_unit" invisible="1"/>22 <field name="price_unit" invisible="1"/>
23 <field name="discount" invisible="1"/>23 <field name="discount" invisible="1"/>
24 <field name="price_subtotal" invisible="1"/>24 <field name="price_subtotal" invisible="1"/>
25 <field name="subtotal_cost_price" invisible="1"/>25 <field name="subtotal_cost_price" invisible="1"/>
26 <field name="company_id" invisible="1"/>26 <field name="company_id" invisible="1"/>
27 <field name="subtotal_company"/>27 <field name="subtotal_company"/>
28 <field name="subtotal_cost_price_company"/>28 <field name="subtotal_cost_price_company"/>
29 <field name="margin_absolute"/>29 <field name="margin_absolute"/>
30 <field name="margin_relative" invisible="context.get('group_by',False)"/>30 <field name="margin_relative" invisible="context.get('group_by',False)"/>
31 </tree>31 </tree>
32 </field>32 </field>
33 </record>33 </record>
34 34
35 35
36 <record id="view_account_invoice_line_filter" model="ir.ui.view">36 <record id="view_account_invoice_line_filter" model="ir.ui.view">
37 <field name="name">account.invoice.line.select</field>37 <field name="name">account.invoice.line.select</field>
38 <field name="model">account.invoice.line</field>38 <field name="model">account.invoice.line</field>
39 <field name="type">search</field>39 <field name="type">search</field>
40 <field name="arch" type="xml">40 <field name="arch" type="xml">
41 <search string="Search Invoice Line">41 <search string="Search Invoice Line">
42 <group>42 <group>
43 <filter icon="terp-go-year" string="Year" 43 <filter icon="terp-go-year" string="Year"
44 name="year"44 name="year"
45 domain="[('invoice_date','&lt;=', time.strftime('%%Y-%%m-%%d')),('invoice_date','&gt;=',time.strftime('%%Y-01-01'))]"45 domain="[('invoice_date','&lt;=', time.strftime('%%Y-%%m-%%d')),('invoice_date','&gt;=',time.strftime('%%Y-01-01'))]"
46 help="year"/>46 help="year"/>
47 <separator orientation="vertical"/>47 <separator orientation="vertical"/>
48 <filter icon="terp-go-month" string="Month"48 <filter icon="terp-go-month" string="Month"
49 name="month"49 name="month"
50 domain="[('invoice_date','&lt;=',(datetime.date.today()+relativedelta(day=31)).strftime('%%Y-%%m-%%d')),('invoice_date','&gt;=',(datetime.date.today()-relativedelta(day=1)).strftime('%%Y-%%m-%%d'))]"50 domain="[('invoice_date','&lt;=',(datetime.date.today()+relativedelta(day=31)).strftime('%%Y-%%m-%%d')),('invoice_date','&gt;=',(datetime.date.today()-relativedelta(day=1)).strftime('%%Y-%%m-%%d'))]"
51 help="current month"/>51 help="current month"/>
52 <filter icon="terp-go-month" string="Month-1"52 <filter icon="terp-go-month" string="Month-1"
53 domain="[('invoice_date','&lt;=', (datetime.date.today() - relativedelta(day=31, months=1)).strftime('%%Y-%%m-%%d')),('invoice_date','&gt;=',(datetime.date.today() - relativedelta(day=1,months=1)).strftime('%%Y-%%m-%%d'))]"53 domain="[('invoice_date','&lt;=', (datetime.date.today() - relativedelta(day=31, months=1)).strftime('%%Y-%%m-%%d')),('invoice_date','&gt;=',(datetime.date.today() - relativedelta(day=1,months=1)).strftime('%%Y-%%m-%%d'))]"
54 help="last month"/>54 help="last month"/>
55 <separator orientation="vertical"/>55 <separator orientation="vertical"/>
56 <field name="product_id"/>56 <field name="product_id"/>
57 <field name="invoice_type"/>57 <field name="invoice_type"/>
58 <field name="partner_id"/>58 <field name="partner_id"/>
59 <field name="invoice_user_id"/>59 <field name="invoice_user_id"/>
60 <field name="invoice_date"/>60 <field name="invoice_date"/>
61 <field name="account_id"/>61 <field name="account_id"/>
62 <field name="company_id"/>62 <field name="company_id"/>
63 </group>63 </group>
64 <newline/>64 <newline/>
65 <group expand="1" string="Group By...">65 <group expand="1" string="Group By...">
66 <filter string="Partner" icon="terp-partner" domain="[]" context="{'group_by':'partner_id'}"/>66 <filter string="Partner" icon="terp-partner" domain="[]" context="{'group_by':'partner_id'}"/>
67 <filter string="Salesman" name='user' icon="terp-personal" context="{'group_by':'invoice_user_id'}"/>67 <filter string="Salesman" name='user' icon="terp-personal" context="{'group_by':'invoice_user_id'}"/>
68 <separator orientation="vertical"/>68 <separator orientation="vertical"/>
69 <filter string="Account" icon="terp-folder-orange" domain="[]" context="{'group_by':'account_id'}"/>69 <filter string="Account" icon="terp-folder-orange" domain="[]" context="{'group_by':'account_id'}"/>
70 <filter string="Invoice" icon="terp-folder-orange" domain="[]" context="{'group_by':'invoice_id'}"/>70 <filter string="Invoice" icon="terp-folder-orange" domain="[]" context="{'group_by':'invoice_id'}"/>
71 <separator orientation="vertical"/>71 <separator orientation="vertical"/>
72 <filter string="Invoice Type" icon="terp-stock_effects-object-colorize" domain="[]" context="{'group_by':'invoice_type'}"/>72 <filter string="Invoice Type" icon="terp-stock_effects-object-colorize" domain="[]" context="{'group_by':'invoice_type'}"/>
73 <filter name="product" string="Product" icon="terp-accessories-archiver" context="{'group_by':'product_id'}"/>73 <filter name="product" string="Product" icon="terp-accessories-archiver" context="{'group_by':'product_id'}"/>
74 </group>74 </group>
75 </search>75 </search>
76 </field>76 </field>
77 </record>77 </record>
7878
79 </data>79 </data>
80</openerp>80</openerp>
81
8281
=== modified file 'product_historical_margin/invoice.py'
--- product_historical_margin/invoice.py 2012-11-14 15:53:00 +0000
+++ product_historical_margin/invoice.py 2013-11-14 15:32:34 +0000
@@ -26,10 +26,11 @@
26class account_invoice(Model):26class account_invoice(Model):
27 _inherit = 'account.invoice'27 _inherit = 'account.invoice'
2828
29 def _refund_cleanup_lines(self, cr, uid, lines):29 def _refund_cleanup_lines(self, cr, uid, lines, context=None):
30 for line in lines:30 for line in lines:
31 del line['invoice_user_id']31 line.invoice_user_id = False
32 return super(account_invoice, self)._refund_cleanup_lines(cr, uid, lines)32 return super(account_invoice, self)._refund_cleanup_lines(cr, uid,
33 lines, context=context)
3334
3435
35class account_invoice_line(Model):36class account_invoice_line(Model):
3637
=== modified file 'product_historical_margin/product_view.xml'
--- product_historical_margin/product_view.xml 2012-11-14 15:53:00 +0000
+++ product_historical_margin/product_view.xml 2013-11-14 15:32:34 +0000
@@ -9,8 +9,8 @@
9 <field name="type">form</field>9 <field name="type">form</field>
10 <field name="arch" type="xml">10 <field name="arch" type="xml">
11 <field name="standard_margin_rate" position="after">11 <field name="standard_margin_rate" position="after">
12 <field name="margin_absolute" />12 <field name="margin_absolute" />
13 <field name="margin_relative" />13 <field name="margin_relative" />
14 </field>14 </field>
15 </field>15 </field>
16 </record>16 </record>
1717
=== modified file 'product_historical_margin/wizard/historical_margin_view.xml'
--- product_historical_margin/wizard/historical_margin_view.xml 2012-07-12 13:56:17 +0000
+++ product_historical_margin/wizard/historical_margin_view.xml 2013-11-14 15:32:34 +0000
@@ -1,43 +1,42 @@
1<?xml version="1.0" encoding="utf-8"?>1<?xml version="1.0" encoding="utf-8"?>
2
3<openerp>2<openerp>
4 <data>3 <data>
5 <record id="view_historical_margin" model="ir.ui.view">4 <record id="view_historical_margin" model="ir.ui.view">
6 <field name="name">historical.margin.form</field>5 <field name="name">historical.margin.form</field>
7 <field name="model">historical.margin</field>6 <field name="model">historical.margin</field>
8 <field name="type">form</field>7 <field name="type">form</field>
9 <field name="arch" type="xml">8 <field name="arch" type="xml">
10 <form string="Historical Margin Properties">9 <form string="Historical Margin Properties">
11 <group colspan="4">10 <group colspan="4">
12 <field name="from_date" />11 <field name="from_date" />
13 <field name="to_date"/>12 <field name="to_date"/>
14 <field name="product_ids" invisible="1"/>13 <field name="product_ids" invisible="1"/>
15 </group>14 </group>
16 <separator string="" colspan="4"/>15 <separator string="" colspan="4"/>
17 <group colspan="4" col="6">16 <group colspan="4" col="6">
18 <button icon="gtk-cancel" special="cancel" string="Cancel"/>17 <button icon="gtk-cancel" special="cancel" string="Cancel"/>
19 <button icon="terp-gtk-go-back-rtl" string="Compute margins" name="action_open_window" type="object"/>18 <button icon="terp-gtk-go-back-rtl" string="Compute margins" name="action_open_window" type="object"/>
20 </group>19 </group>
21 </form>20 </form>
22 </field>21 </field>
23 </record>22 </record>
2423
25 <record id="action_product_margin_view" model="ir.actions.act_window">24 <record id="action_product_margin_view" model="ir.actions.act_window">
26 <field name="name">Product Historical Margin</field>25 <field name="name">Product Historical Margin</field>
27 <field name="res_model">historical.margin</field>26 <field name="res_model">historical.margin</field>
28 <field name="view_type">form</field>27 <field name="view_type">form</field>
29 <field name="view_mode">tree,form</field>28 <field name="view_mode">tree,form</field>
30 <field name="view_id" ref="view_historical_margin"/>29 <field name="view_id" ref="view_historical_margin"/>
31 <field name="target">new</field>30 <field name="target">new</field>
32 </record>31 </record>
33 32
34 33
35 <menuitem icon="STOCK_INDENT" action="action_product_margin_view"34 <menuitem icon="STOCK_INDENT" action="action_product_margin_view"
36 id="menu_action_product_margin_tree"35 id="menu_action_product_margin_tree"
37 parent="account.menu_finance_statistic_report_statement" />36 parent="account.menu_finance_statistic_report_statement" />
3837
39 38
40 <!-- Remove it for right management <act_window name="Product Historical Margins"39 <!-- Remove it for right management <act_window name="Product Historical Margins"
41 res_model="historical.margin"40 res_model="historical.margin"
42 src_model="product.product"41 src_model="product.product"
43 view_mode="form"42 view_mode="form"
@@ -46,5 +45,5 @@
46 id="historical_margin_act_window"/> -->45 id="historical_margin_act_window"/> -->
4746
4847
49 </data>48 </data>
50</openerp>49</openerp>

Subscribers

People subscribed via source and target branches