Issue 2349

Title
Remove Property fields.
Priority
feature
Status
resolved
Superseder
Change depreciation_duration from Property to Integer (issue 6395)
Nosy list
albertca, angel, ced, eric, guillemNaN, jesteve, ohuisman, pokoli, resteve, reviewbot, roundup-bot, smarro, unicode2013, yangoon
Assigned to
ced
Keywords
review

Created on 2011-12-15.14:29:43 by ced, last changed 55 months ago by roundup-bot.

Messages

New changeset b4e130264ee5 by C?dric Krier in branch 'default':
Remove unused configuration tax rounding views
http://hg.tryton.org/modules/account/rev/b4e130264ee5
New changeset 98c88c70fcd2 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/stock_supply_production/rev/98c88c70fcd2
New changeset 20b9447f41bd by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/sale_supply_drop_shipment/rev/20b9447f41bd
New changeset a7fa702f151e by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/stock_supply/rev/a7fa702f151e
New changeset c72176517614 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/purchase_requisition/rev/c72176517614
New changeset 051891407b59 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/account_payment_sepa/rev/051891407b59
New changeset 966c9cac2029 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/sale_price_list/rev/966c9cac2029
New changeset 45d7ba8f8a17 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/sale_shipment_grouping/rev/45d7ba8f8a17
New changeset 1c0807a352c3 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/sale_complaint/rev/1c0807a352c3
New changeset f6bd46dfcd25 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/account_asset/rev/f6bd46dfcd25
New changeset af356d1e6b64 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/sale_invoice_grouping/rev/af356d1e6b64
New changeset 25f17ac2ad06 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/sale_opportunity/rev/25f17ac2ad06
New changeset 00714c4f09ae by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/sale_shipment_cost/rev/00714c4f09ae
New changeset e46ac21dc18f by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/sale_credit_limit/rev/e46ac21dc18f
New changeset 42cfbd7b53f6 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/sale/rev/42cfbd7b53f6
New changeset bdff713fdce7 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/purchase/rev/bdff713fdce7
New changeset a9421b4bfddf by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/account_stock_anglo_saxon/rev/a9421b4bfddf
New changeset c2d04dab428b by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/stock_lot_sled/rev/c2d04dab428b
New changeset df1ea8c50af7 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/account_payment/rev/df1ea8c50af7
New changeset 9650f0839dd8 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/account_stock_landed_cost/rev/9650f0839dd8
New changeset 0d6e55352e0c by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/account_stock_continental/rev/0d6e55352e0c
New changeset 72b41ef1e96f by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/stock_package/rev/72b41ef1e96f
New changeset b52f84d30237 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/account_credit_limit/rev/b52f84d30237
New changeset c6a613650958 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/production/rev/c6a613650958
New changeset 83ae4ad41bab by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/account_invoice/rev/83ae4ad41bab
New changeset 6cda7e11ee63 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/stock/rev/6cda7e11ee63
New changeset 10b25f1b8806 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/account_product/rev/10b25f1b8806
New changeset 96f371b4b3c7 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/account_dunning/rev/96f371b4b3c7
New changeset 813825234b34 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/product_cost_history/rev/813825234b34
New changeset 59ec6183cd0f by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/product/rev/59ec6183cd0f
New changeset a81c06e383e1 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/account/rev/a81c06e383e1
New changeset 8140f6cd9d3d by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/company/rev/8140f6cd9d3d
New changeset f9d6b409dee8 by C?dric Krier in branch 'default':
Remove Property fields
http://hg.tryton.org/modules/party/rev/f9d6b409dee8
New changeset 65e38221f0ca by C?dric Krier in branch 'default':
Add MultiValueMixin and ValueMixin and remove Propery
http://hg.tryton.org/trytond/rev/65e38221f0ca
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2017-03-28.23:59:19
The 1417 tests pass now. Ready for testing.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2017-03-28.22:03:13
I think I will remove the warnings in __get__/__set__ MultiValue because there are cases that we could not remove like the test of require in ModelStorage.
Indeed the important is that we can now call get_multivalue with a specific pattern when ever we need it like for issue4080.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2017-03-28.20:54:19
I just found another issue when use ValueMixin with required fields on it.
Because MultiValueMixin will create the Value record and set only one field at a time, the required constraint will raise an error.
I think the right design is to have the default value stored on the Value model and have the MultiValue model calling them. At least for Value model with many required fields (like ConfigurationSaleMethod).
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2017-03-28.17:01:00
I do not plan to implement new feature (pattern) here.
Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2017-03-28.15:01:43
Indeed, I have a doubt about how the sequences are managed. In party module _new_code function has been added in order to customize the pattern used to get the sequence, but I see this pattern not applied to all the other modules (sale, purchase, stock). Is this done in purpose?
Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2017-03-28.12:32:17
I've done a first testing and it looks good despite some minor fixes that I have commented on codereview. I'm missing msg32817, so I will retest when this is uploaded.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2017-03-27.15:57:55
I just found running tests that a behaviour is removed with the multivalue per company. A record with no company does not become a default value when there is no record for this company.
This happens mainly for sequence value for document that are created by default from the XML. I'm fixing them by defining a default method on the model which return the id from the XML. Another solution could be to have the default method retrieving the multivalue for no company but in the case of those standard module, I find it is too much cumbersome.
Finally, I think this lost of feature is indeed a good thing because it enforce explicit declaration of the default behaviour.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2017-03-23.17:20:58
I have decided to not migrate default value on configuration models.
The SQL query required for such migration is quite complex and at the end we have only few default value to migrate. I think it is fare enough to ask the user to check the configuration after the migration.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2017-03-11.14:21:23
I think we have now a final design that could be tested.
I will start migrate other modules. We need to migrate all properties of a model over all modules at once. For that, I think it will be good to migrate all modules to multivalue inside one release and remove the property fields because it will force external module to also do the same.
Author: [hidden] (albertca)
Date: 2015-08-10.10:47:25
2015-08-10 10:35 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
>
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
> On 2015-08-10 10:00, Albert Cervera i Areny wrote:
>> >> Regarding the API I must say that like Raimon I also see it a little bit complex to have a class per field.
>> >
>> > I never said it is a rule.
>> >
>>
>> That's why I said that maybe it's me that doesn't fully understand how
>> you intend to use the proposed API...
>>
>> Do you intend to have a:
>>
>> class ConfigurationAccounting(_ConfigurationValue, ModelSQL, ValueMixin):
>>     'Party Configuration Accounting'
>>     __name__ = 'party.configuration.accounting'
>>
>> Which would store all party accounting parameters or something like that?
>
> Yes probably.
>

Alright, now I see the database structure is similar in your proposal
and my idea. It also easily supports the creation of a
"Company-dependen fields tab" if that is necessary for some
installations, while keeping standard behaviour as it currently is.

>> >> After all, in the Party model we'll have several fields that depend on company but probably no fields that will depend on warehouse or other parameters.
>> >
>> > So what?
>> >
>> >> Also, probably working on the party module is not a very good example because IMHO both properties should be replaced by simple Many2One fields.
>> >
>> > They can not.
>>
>> What I mean is that I think that default language should not be
>> company-dependent so a simple Many2One on the configuration would be
>> enough.
>
> But this will break the migration path.
>

You're right but still it makes sense to me to remove that behaviour,
and warn users of the change.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2015-08-10.10:35:06
On 2015-08-10 10:00, Albert Cervera i Areny wrote:
> >> Regarding the API I must say that like Raimon I also see it a little bit complex to have a class per field.
> >
> > I never said it is a rule.
> >
> 
> That's why I said that maybe it's me that doesn't fully understand how
> you intend to use the proposed API...
> 
> Do you intend to have a:
> 
> class ConfigurationAccounting(_ConfigurationValue, ModelSQL, ValueMixin):
>     'Party Configuration Accounting'
>     __name__ = 'party.configuration.accounting'
> 
> Which would store all party accounting parameters or something like that?

Yes probably.

> >> After all, in the Party model we'll have several fields that depend on company but probably no fields that will depend on warehouse or other parameters.
> >
> > So what?
> >
> >> Also, probably working on the party module is not a very good example because IMHO both properties should be replaced by simple Many2One fields.
> >
> > They can not.
> 
> What I mean is that I think that default language should not be
> company-dependent so a simple Many2One on the configuration would be
> enough.

But this will break the migration path.
Author: [hidden] (albertca)
Date: 2015-08-10.10:00:34
2015-08-10 0:44 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
>
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
> On 2015-08-05 15:38, Albert Cervera i Areny wrote:
>> Regarding the fact that multi-company should be a rare case: I see it is very frequent to have several companies. I've just checked with real cases and from the latest 14 customers we've got, 7 have more than one company. Of those 7, 2 of them have 2 companies just for fiscal reasons. In the other 4 cases there are:
>>
>> - In two cases there are 2 companies doing the same activity
>> - In the other three cases there are +4 companies doing the same actiity
>
> Maybe it is your business target but it makes no doubt that there are
> more single companies in the world than groups that need to be managed
> by a single application.
> Or maybe it is just a local fiscal specificity that creates such weird
> company setups.
> Most of the time when such multi-company setup exist for fiscal reason,
> they don't require to have multi-company in the system because it is
> just a fiscal thing and not a business configuration.
>
>> I can understand that we should try not to make things more complex if only one company is envolved, but I'd like to remark that I don't see it as such a strange case. Also, personally I don't think the o2m on party with a tab on "Company dependent fields" make things much more complex for users as the first company in form-view can be shown by default (just like addresses in party).
>
> I still don't agree. Your UI suggest that everything is linked to
> company but as I explained that not a correct goal otherwise the current
> design will be OK. More over, it will require to group all "company"
> stuff in one tab even if those properties has no link together instead
> of grouping by logical usage.
> And again, your proposal makes the UI much more complex for the simple
> and most common cases.
>
>> Regarding the API I must say that like Raimon I also see it a little bit complex to have a class per field.
>
> I never said it is a rule.
>

That's why I said that maybe it's me that doesn't fully understand how
you intend to use the proposed API...

Do you intend to have a:

class ConfigurationAccounting(_ConfigurationValue, ModelSQL, ValueMixin):
    'Party Configuration Accounting'
    __name__ = 'party.configuration.accounting'

Which would store all party accounting parameters or something like that?

>> After all, in the Party model we'll have several fields that depend on company but probably no fields that will depend on warehouse or other parameters.
>
> So what?
>
>> Also, probably working on the party module is not a very good example because IMHO both properties should be replaced by simple Many2One fields.
>
> They can not.

What I mean is that I think that default language should not be
company-dependent so a simple Many2One on the configuration would be
enough.

>> I find myself thinking that the patch tries to plan that another module will extend the lang field and allows overriding the default value by adding a pattern but I think we cannot pretend that core modules are ready for those customizations out of the box.
>
> I don't understand why you say that. I tried to setup a new design
> pattern for Tryton as we already have some because exactly such pattern
> allows us to say the system is ready for customizations out of the box.
>
>> I mean, another module could make the default country company-dependent, for example, and we don't want to have to modify the core party module for that.
>
> I don't understand this sentence.
>
>> Could you update the patch including the account module (that includes company too)? Maybe that would make some things clearer for me...
>
> I don't know when I will have the time to continue this work.
>
> _______________________________________________
> Tryton issue tracker <issue_tracker@tryton.org>
> <https://bugs.tryton.org/issue2349>
> _______________________________________________
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2015-08-10.00:44:26
On 2015-08-05 15:38, Albert Cervera i Areny wrote:
> Regarding the fact that multi-company should be a rare case: I see it is very frequent to have several companies. I've just checked with real cases and from the latest 14 customers we've got, 7 have more than one company. Of those 7, 2 of them have 2 companies just for fiscal reasons. In the other 4 cases there are:
> 
> - In two cases there are 2 companies doing the same activity
> - In the other three cases there are +4 companies doing the same actiity

Maybe it is your business target but it makes no doubt that there are
more single companies in the world than groups that need to be managed
by a single application.
Or maybe it is just a local fiscal specificity that creates such weird
company setups.
Most of the time when such multi-company setup exist for fiscal reason,
they don't require to have multi-company in the system because it is
just a fiscal thing and not a business configuration.

> I can understand that we should try not to make things more complex if only one company is envolved, but I'd like to remark that I don't see it as such a strange case. Also, personally I don't think the o2m on party with a tab on "Company dependent fields" make things much more complex for users as the first company in form-view can be shown by default (just like addresses in party).

I still don't agree. Your UI suggest that everything is linked to
company but as I explained that not a correct goal otherwise the current
design will be OK. More over, it will require to group all "company"
stuff in one tab even if those properties has no link together instead
of grouping by logical usage.
And again, your proposal makes the UI much more complex for the simple
and most common cases.

> Regarding the API I must say that like Raimon I also see it a little bit complex to have a class per field.

I never said it is a rule.

> After all, in the Party model we'll have several fields that depend on company but probably no fields that will depend on warehouse or other parameters.

So what?

> Also, probably working on the party module is not a very good example because IMHO both properties should be replaced by simple Many2One fields.

They can not.

> I find myself thinking that the patch tries to plan that another module will extend the lang field and allows overriding the default value by adding a pattern but I think we cannot pretend that core modules are ready for those customizations out of the box.

I don't understand why you say that. I tried to setup a new design
pattern for Tryton as we already have some because exactly such pattern
allows us to say the system is ready for customizations out of the box.

> I mean, another module could make the default country company-dependent, for example, and we don't want to have to modify the core party module for that.

I don't understand this sentence.

> Could you update the patch including the account module (that includes company too)? Maybe that would make some things clearer for me...

I don't know when I will have the time to continue this work.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2015-08-10.00:26:32
On 2015-08-04 15:39, Raimon Esteve wrote:
> I review23361002 (party) how to do new API about "property fields".
> 
> IMHO new API NOT help you to create "Property fields".

It is not the goal.

> Here there are step by step to do it:
> 
> 1- Create field outsite class (m2o)
> 2- Create SAME field, but in "PartyConfiguration" class (in this case, function fields)
> 3- Create a new class: _ConfigurationValue.
> 4- Each Property fied from step 1 and step 2, you need to create a NEW Class.

Absolutly WRONG.

> Also, those classes need to register on __init_.
> 
> My conclusion:
> 
> 1- Not easy to develop/write Property fields (framework don't help you because need to write a lot of py lines)

You write the lines you want to.

> 2- If you have a lot of Property fieds in an object, for example, sale.configuration + 24 property fields => in step 4, need to create 24 classes and register in __init__ those 24 classes. I think this step NOT help you to use/develop property fields.

Again completly WRONG.

You have to THINK about the design and not blindly follow some kind of
pattern.
Author: [hidden] (albertca)
Date: 2015-08-05.15:38:33
Regarding the fact that multi-company should be a rare case: I see it is very frequent to have several companies. I've just checked with real cases and from the latest 14 customers we've got, 7 have more than one company. Of those 7, 2 of them have 2 companies just for fiscal reasons. In the other 4 cases there are:

- In two cases there are 2 companies doing the same activity
- In the other three cases there are +4 companies doing the same actiity

I can understand that we should try not to make things more complex if only one company is envolved, but I'd like to remark that I don't see it as such a strange case. Also, personally I don't think the o2m on party with a tab on "Company dependent fields" make things much more complex for users as the first company in form-view can be shown by default (just like addresses in party).

Regarding the API I must say that like Raimon I also see it a little bit complex to have a class per field. After all, in the Party model we'll have several fields that depend on company but probably no fields that will depend on warehouse or other parameters.

Also, probably working on the party module is not a very good example because IMHO both properties should be replaced by simple Many2One fields. I find myself thinking that the patch tries to plan that another module will extend the lang field and allows overriding the default value by adding a pattern but I think we cannot pretend that core modules are ready for those customizations out of the box. I mean, another module could make the default country company-dependent, for example, and we don't want to have to modify the core party module for that.

Could you update the patch including the account module (that includes company too)? Maybe that would make some things clearer for me...
Author: [hidden] (resteve)
Date: 2015-08-04.15:39:10
I review23361002 (party) how to do new API about "property fields".

IMHO new API NOT help you to create "Property fields". Here there are step by step to do it:

1- Create field outsite class (m2o)
2- Create SAME field, but in "PartyConfiguration" class (in this case, function fields)
3- Create a new class: _ConfigurationValue.
4- Each Property fied from step 1 and step 2, you need to create a NEW Class. Also, those classes need to register on __init_.

My conclusion:

1- Not easy to develop/write Property fields (framework don't help you because need to write a lot of py lines)
2- If you have a lot of Property fieds in an object, for example, sale.configuration + 24 property fields => in step 4, need to create 24 classes and register in __init__ those 24 classes. I think this step NOT help you to use/develop property fields.

My 2 cent.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2015-07-30.17:14:51
About the switching to edit such Function fields, there are few options:

- working with many clients
- customize the views to show a one2many or relate to the multi-values

About the switching to create invoice/sale/etc.:

This is something that will be fixed by issue4080, but it can not without having a better API for Property, and your proposal will not help.

About your proposal, it just make the UI more complex for the majority of the cases single-company. Multi-company is and must be extremely rare case.
Your design will make writing code much more complicate because we will have to find the right line of the one2many to use.
Also there is not only the multi-company to solve but any other possible distinction.
For example, the cost price computation of a product could be distinct per warehouse.
So my proposal is an attempt for a generic design for any field of a Model that has many value depending of the context/pattern.

After what, in practice the database schema of both solution will be the same. It is just about API and UX.
Author: [hidden] (albertca)
Date: 2015-07-30.16:56:35
I reviewed the patches but there's something I don't quite like and it is that we just replace the "Property" by a "Function". That is also how we did some of the modules in bitbucket but I think in fact the current UX is not good and we should remove the idea of getting the values out of the user's context.

The problems we see are:

- There is no way for the user to know if a value in the party (or product or whatever) depends on the company or not.
- The user must switch company in order to create an invoice/sale/whatever for another company. Some organizations can easily have +5 companies with very few users which makes current UX less than convenient.

Our idea was to replace Property fields by a One2Many field in party which is explicitly shown to the user (and he has no "current company" anymore). (Where I say "party" read "any model with properties"). Something like:

class Party:
    __name__ = 'party.party'
    company_properties = fields.One2Many('party.company', 'party', 'Per Comapany Properties')


class PartyCompany:
    __name__ = 'party.company'
    party = fields.Many2One('party.party', 'Party', required=True)
    company = fields.Many2One('company.company', 'Company', required=True)
    customer_payment_term = fields.Many2One('account.invoice.payment_term', 'Customer Payment Term', domain=[('company', '=', Eval('company'))])
    supplier_payment_term = fields.Many2One('account.invoice.payment_term', 'Supplier Payment Term', domain=[('company', '=', Eval('company'))])

In party_form.xml:

<page string="Company Properties">
    <field name="company_properties" colspan="4"/>
</page>


What do you think?
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2015-06-09.12:57:03
From [1]:

Property fields must be removed and replace by an explicit design where
values are stored per company (like a product.company that will contain
prices, account etc.). But Function field will still be available to
show and edit value from the main Model by the User (but never used by
code).  The setter of those Function field will accept a company
argument to override the company from context (to use by code).

[1] https://groups.google.com/d/msg/tryton/UQB657-KcGI/Lwi5vlVW9Q4J
History
Date User Action Args
2017-04-01 00:50:15roundup-botsetmessages: + msg32997
2017-04-01 00:47:08roundup-botsetmessages: + msg32996
2017-04-01 00:47:02roundup-botsetmessages: + msg32995
2017-04-01 00:46:36roundup-botsetmessages: + msg32994
2017-04-01 00:46:30roundup-botsetmessages: + msg32993
2017-04-01 00:46:27roundup-botsetmessages: + msg32992
2017-04-01 00:46:25roundup-botsetmessages: + msg32991
2017-04-01 00:46:20roundup-botsetmessages: + msg32990
2017-04-01 00:46:17roundup-botsetmessages: + msg32989
2017-04-01 00:46:14roundup-botsetmessages: + msg32988

Showing 10 items. Show all history (warning: this could be VERY long)