Tryton - Issues

 

Issue4341

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

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

Messages
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-01-09 19:08:17cedsetmessages: + msg37542
2017-12-21 10:48:43reviewbotsetmessages: + msg37416
2017-12-21 10:48:23reviewbotsetmessages: + msg37415
2017-12-20 18:45:06cedsetmessages: + msg37401
2017-12-20 17:51:39pokolisetmessages: + msg37399
2017-12-20 17:44:41reviewbotsetmessages: + msg37398
2017-12-20 17:38:28cedsetmessages: + msg37397
2017-12-20 16:48:36pokolisetmessages: + msg37394
2017-09-12 16:19:53cedsetmessages: + msg35635
2017-06-08 12:35:05cedsetmessages: + msg33962

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