Tryton - Issues

 

Issue4341

Title Flexible active
Priority feature Status resolved
Superseder Nosy List ced, pokoli, reviewbot, roundup-bot
Type behavior Components sao, tryton, trytond
Assigned To pokoli Keywords review
Reviews 38401002, 40411002,38081002
View: 38401002, 40411002, 38081002

Created on 2014-11-11.19:03:28 by ced, last changed by roundup-bot.

Messages
New changeset e7059be59f9e by Sergi Almacellas Abellana in branch 'default':
Manage active field on search widget
http://hg.tryton.org/tryton/rev/e7059be59f9e
New changeset c3cba41333ad by Sergi Almacellas Abellana in branch 'default':
Manage active field on search widget
http://hg.tryton.org/sao/rev/c3cba41333ad
New changeset 5cc56add30b2 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/web_user/rev/5cc56add30b2
New changeset 3660c02284b6 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/timesheet/rev/3660c02284b6
New changeset eac477c46939 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/stock_forecast/rev/eac477c46939
New changeset 9708f306100d by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/stock/rev/9708f306100d
New changeset 9560a30b1aba by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/sale_promotion_coupon/rev/9560a30b1aba
New changeset f3b3e9ecc1ec by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/sale_promotion/rev/f3b3e9ecc1ec
New changeset de9b58e614c7 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/sale_extra/rev/de9b58e614c7
New changeset e6cee18a0df7 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/sale_advance_payment/rev/e6cee18a0df7
New changeset 57d3bca6c991 by Sergi Almacellas Abellana in branch 'default':
Remove active field from product tree view
http://hg.tryton.org/modules/sale/rev/57d3bca6c991
New changeset d5c2c5230fd1 by Sergi Almacellas Abellana in branch 'default':
Remove active field from product tree view
http://hg.tryton.org/modules/purchase/rev/d5c2c5230fd1
New changeset 991dbe20abd1 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/production_work/rev/991dbe20abd1
New changeset c8c63d45508a by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/production_routing/rev/c8c63d45508a
New changeset 26e4c7dee264 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/production/rev/26e4c7dee264
New changeset d2fe609ca60e by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/product_classification_taxonomic/rev/d2fe609ca60e
New changeset 2f165f61daa3 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/product_classification/rev/2f165f61daa3
New changeset df42d4b86156 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/product/rev/df42d4b86156
New changeset c3e05b8fe651 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/party/rev/c3e05b8fe651
New changeset 215446688015 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/customs/rev/215446688015
New changeset 3d5ec853eefb by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/currency/rev/3d5ec853eefb
New changeset cce06852a46c by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/carrier/rev/cce06852a46c
New changeset fe8346ebd8d3 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/bank/rev/fe8346ebd8d3
New changeset bb4c7ab4988e by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/analytic_account/rev/bb4c7ab4988e
New changeset 12a5298fa147 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/account_payment_stripe/rev/12a5298fa147
New changeset 9c73215af617 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/account_invoice/rev/9c73215af617
New changeset 8217f45cc798 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/account_dunning_fee/rev/8217f45cc798
New changeset c71f8ca2ed52 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/account_dunning/rev/c71f8ca2ed52
New changeset eb77cdd3c196 by Sergi Almacellas Abellana in branch 'default':
Use deactivable mixin
http://hg.tryton.org/modules/account/rev/eb77cdd3c196
New changeset 0856eb077b1c by Sergi Almacellas Abellana in branch 'default':
Add deactivable mixin and ensure active field is present
http://hg.tryton.org/trytond/rev/0856eb077b1c
review38081002 updated at https://codereview.tryton.org/38081002/#ps130001
review38081002 updated at https://codereview.tryton.org/38081002/#ps110001
review38081002 updated at https://codereview.tryton.org/38081002/#ps90001
review38081002 updated at https://codereview.tryton.org/38081002/#ps70001
review38081002 updated at https://codereview.tryton.org/38081002/#ps60001
review40411002 updated at https://codereview.tryton.org/40411002/#ps200001
review38081002 updated at https://codereview.tryton.org/38081002/#ps40001
review40411002 updated at https://codereview.tryton.org/40411002/#ps180001
review38081002 updated at https://codereview.tryton.org/38081002/#ps20001
review40411002 updated at https://codereview.tryton.org/40411002/#ps160001
review38081002 updated at https://codereview.tryton.org/38081002/#ps1
msg39220 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2018-03-22.16:03:13
I've added the sao review, so we are ready to test it.
review40411002 updated at https://codereview.tryton.org/40411002/#ps140001
review40411002 updated at https://codereview.tryton.org/40411002/#ps120001
review38401002 updated at https://codereview.tryton.org/38401002/#ps190001
review40411002 updated at https://codereview.tryton.org/40411002/#ps100001
review38401002 updated at https://codereview.tryton.org/38401002/#ps170001
review38401002 updated at https://codereview.tryton.org/38401002/#ps150001
review38401002 updated at https://codereview.tryton.org/38401002/#ps130001
msg38229 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-02-06.13:12:31
I think we need a special widget for the search box because there is always a value for active search. So probably a checkbox name "inactive" (or "archived" like Odoo) would be good.
I think we need a visual effect always visible on the list view to indicate that there is an active field. I think it could be the icon of the search box.
review40411002 updated at https://codereview.tryton.org/40411002/#ps80001
msg37896 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2018-01-23.15:23:47
I'm not sure to understand what are you requesting. 

I understand we need some way to make visible to the user that there is an active test, but not sure how to do it. 

I'm wondering if adding the active field on the domain parser is enough, or we should do anything else (don't know what).
msg37895 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-01-23.14:45:53
I do not understand the question.
review40411002 updated at https://codereview.tryton.org/40411002/#ps60001
msg37892 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2018-01-23.14:25:05
Indeed the special input for active field in the search dialog is the same as adding the active field to the domain parser. Isn't it?
review38401002 updated at https://codereview.tryton.org/38401002/#ps110001
msg37542 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-01-09.19:08:17
I find the checkbox next to the search box not very nice especially because there are no labels.
Maybe we should only have a special input for active field in the search dialog box.
review40411002 updated at https://codereview.tryton.org/40411002/#ps40001
review38401002 updated at https://codereview.tryton.org/38401002/#ps90001
msg37401 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2017-12-20.18:45:06
On 2017-12-20 17:51, Sergi Almacellas Abellana wrote:
> > I find it will be more user-friendly to have a check box near the search bar to "include inactive". For now, most of the time the search is for active only or inactive only. I do not think it is what user expect. Also it should be visible by default that the option 'active' is present on this model.
> 
> Ok, let's go this way. But then there is no need to automatically add the active field on server side as it will be managed by the client.

Still the client must know about the active field so it must be added by
the server.
msg37399 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2017-12-20.17:51:39
> I find it will be more user-friendly to have a check box near the search bar to "include inactive". For now, most of the time the search is for active only or inactive only. I do not think it is what user expect. Also it should be visible by default that the option 'active' is present on this model.

Ok, let's go this way. But then there is no need to automatically add the active field on server side as it will be managed by the client.
review38401002 updated at https://codereview.tryton.org/38401002/#ps70001
msg37397 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2017-12-20.17:38:27
I find it will be more user-friendly to have a check box near the search bar to "include inactive". For now, most of the time the search is for active only or inactive only. I do not think it is what user expect. Also it should be visible by default that the option 'active' is present on this model.
msg37394 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2017-12-20.16:48:36
I see the simplest solution is to automatically add the active field on tree views on server side.

I do not see any benefit about showing active field in a diferent way, only adding more complexity to the code.
msg35635 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2017-09-12.16:19:53
After thoughts, I think we could keep the name 'active'. We have already other thing for which the name is hard-coded. I think it will simplify the code on both side.
Also I think the active field should be just added by default to the tree view if the model has it. And maybe the client can show it as special checkbox.
msg33962 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2017-06-08.12:35:05
On 2017-06-08 11:19, Sergi Almacellas Abellana wrote:
> > But maybe it should be simplified to have only one way to activate/deactivate it.
> 
> Don't understand. AFAIU, currently there is only one way to activate/deactivate and this way is using the active_test flag on the context. Do we have to change this?

I think that parsing the domain to see if there is a clause using
'active' field is not very good.
This is mainly useful because the client does not know about 'active'
field and the test should be disable if the user search for inactive
record.
But if we improve the client to know about such search, then this code
can be removed and just keep the active_test context.
But we should ensure that any module use a clause on 'active' instead of
using the context.
review38401002 updated at https://codereview.tryton.org/38401002/#ps60001
msg33959 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2017-06-08.11:19:17
>> I think also the automatic appending to domain should be removed and only managed on client side. This will simplify the work for the developer and will prevent weird issue.
> After some thoughts, I think this is wrong. The active field must behave like a deletion. I see many cases where on the server side we rely on this. So I think it should stay managed on the server side. 

I was going to put a comment on the same direction right now. We agree that it should stay on server side as there is a lot of code that relaying on it. 

> But maybe it should be simplified to have only one way to activate/deactivate it.

Don't understand. AFAIU, currently there is only one way to activate/deactivate and this way is using the active_test flag on the context. Do we have to change this?
msg33957 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2017-06-08.11:10:49
> I think also the automatic appending to domain should be removed and only managed on client side. This will simplify the work for the developer and will prevent weird issue.

After some thoughts, I think this is wrong. The active field must behave like a deletion. I see many cases where on the server side we rely on this. So I think it should stay managed on the server side. But maybe it should be simplified to have only one way to activate/deactivate it.
msg33955 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2017-06-08.11:02:02
In order to implement msg16082, will be enougth to make all the form fields readonly if the active_field is not checked on client side? Of course the only editable field should be the active_field...
msg33946 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2017-06-07.14:45:06
On 2017-06-07 13:18, Sergi Almacellas Abellana wrote:
> > There is still another possible issue. The xxx2Many are not supposed to show the inactive records. I guess we could add a filter for that. But all models should be reviewed to ensure a correct behavior.
> 
> Maybe we can set the filter by default on xxx2many fields if the target model has an active_field defined. What do you think? Of course, this should be documented.

I think explicit is better than implicit. So it will be much more work
but I think all xxx2many should be reviewed.
Also I do not think we could add a test because there should be
legitimate cases where you want also the inactive records.
review40411002 updated at https://codereview.tryton.org/40411002/#ps20001
review38401002 updated at https://codereview.tryton.org/38401002/#ps20001
msg33943 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2017-06-07.13:18:27
> There is still another possible issue. The xxx2Many are not supposed to show the inactive records. I guess we could add a filter for that. But all models should be reviewed to ensure a correct behavior.

Maybe we can set the filter by default on xxx2many fields if the target model has an active_field defined. What do you think? Of course, this should be documented.
msg33935 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2017-06-06.23:02:04
I do not like the '_search_domain_active' in the client. You just moved this horrible method from server side to client side.
The client should not need to perform such stuffs. I think we should expose a simple checkbox "Show inactive" that will append or not an active clause to the user domain. By default it should not show inactive.

There is still another possible issue. The xxx2Many are not supposed to show the inactive records. I guess we could add a filter for that. But all models should be reviewed to ensure a correct behavior.
New review40411002 at https://codereview.tryton.org/40411002/#ps1
msg33931 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2017-06-06.16:43:39
Here is review38401002 for trytond and review40411002 for tryton.

Still pending: 

- Remove all the active fields from the tree views
- Remove active_test usage from modules
- Sao patch (will be provided once we agree on the tryton desing). 

But a first review can be done.
New review38401002 at https://codereview.tryton.org/38401002/#ps1
msg18862 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2014-11-11.19:03:27
The active behavior is now hard-coded, it should be configurable.
The field name should be sent to the client with the model definition.
And the client should handle it to for example add it to search box etc.
I think also the automatic appending to domain should be removed and only managed on client side. This will simplify the work for the developer and will prevent weird issue.
History
Date User Action Args
2018-03-31 00:05:12cedlinkissue7254 superseder
2018-03-30 23:48:17cedlinkissue4340 superseder
2018-03-26 14:26:58roundup-botsetmessages: + msg39409
2018-03-26 14:26:48roundup-botsetmessages: + msg39408
2018-03-26 14:25:39roundup-botsetmessages: + msg39407
2018-03-26 14:25:34roundup-botsetmessages: + msg39406
2018-03-26 14:25:24roundup-botsetmessages: + msg39405
2018-03-26 14:25:21roundup-botsetmessages: + msg39404
2018-03-26 14:25:14roundup-botsetmessages: + msg39403
2018-03-26 14:25:10roundup-botsetmessages: + msg39402

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