Flexible active

added type::behavior + 1 deleted label

New review38401002 at https://codereview.tryton.org/38401002/#ps1

added 1 deleted label and removed 1 deleted label

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.

assigned to @pokoli

added sao tryton trytond + 1 deleted label and removed 1 deleted label

New review40411002 at https://codereview.tryton.org/40411002/#ps1

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.

> 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.

review38401002 updated at https://codereview.tryton.org/38401002/#ps20001

review40411002 updated at https://codereview.tryton.org/40411002/#ps20001

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.

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...

> 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.

>> 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?

review38401002 updated at https://codereview.tryton.org/38401002/#ps60001

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.

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.

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.

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.

review38401002 updated at https://codereview.tryton.org/38401002/#ps70001

> 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.

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.

review38401002 updated at https://codereview.tryton.org/38401002/#ps90001

review40411002 updated at https://codereview.tryton.org/40411002/#ps40001

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.

review38401002 updated at https://codereview.tryton.org/38401002/#ps110001

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?

review40411002 updated at https://codereview.tryton.org/40411002/#ps60001

I do not understand the question.

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).

review40411002 updated at https://codereview.tryton.org/40411002/#ps80001

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.