[trunk] - ir.actions.act_window - missing required flag on several fields (res_model, context, view_type, ...)

[Ferdinand]

Currently the following fields are required for ir.actions.act_window records but are not marked so in the model, which can cause hard-to-diagnose issues (usually the client will silently fail to launch the action):
 - res_model
 - context
 - view_type
 - view_mode
 - type (but is redundant and readonly)

Marking them explicitly as required will provide a safeguard for OpenERP admins.

[xrg (xrg)]

On Friday 14 May 2010, you wrote:
> Public bug reported:
>
> context field must not be empty - and should issue an user friendly
> error message
>
> the problem is that sometimes the context has to be deleted manually -
> as this is not done automatically if context tag is removed from xml
> file

Do you mean it's the issue where xml updates change the action but forget to
update/reset the context field at it?

So, you want the xml files to remove the context (or put a new one), when they
are loaded, right?
If that's the case, the problem may lie in the way xml files are loaded. That's
a more tricky thing. We might want a mechanism to say "if you reload field1,
field2 from xml, then at least reset field3, field4 to default"..

[Ferdinand (office-chricar)]

yes, but I am not sure, which version is more desirable
(1) reset the complete record on reload
(2) maintain user (admin) changes

but it's strange that after reloading an xml file the db-content is different from the file.

it's even worse if an act_window is deleted in the xml , it never gets deleted in the db.

IMHO this needs a more detailed investigation.

[xrg (xrg)]

On Friday 14 May 2010, you wrote:
> yes, but I am not sure, which version is more desirable
> (1) reset the complete record on reload
> (2) maintain user (admin) changes
>
> but it's strange that after reloading an xml file the db-content is
> different from the file.
>
> it's even worse if an act_window is deleted in the xml , it never gets
> deleted in the db.
>
> IMHO this needs a more detailed investigation.
>
This would call for several new features in the ORM :
 - <record id="the_old_id" action="delete" /> element (say..)
 - Field-level xml loading and complex rules.
 - Much more attention in writting those xml's. It is not enough if they only
work for a new database.

[JMA(Open ERP) (jma-openerp)]

Hello guys,
I would like to interrupt here.

Here,there are 2 aspects coming into picture:
1. Updation of XML file
2. Database values.

 While updating an xml file, it will only consider the tags available and it will update them.
Example:
<record id="act_values_form_action" model="ir.actions.act_window">
            <field name="name">Connect Actions To Client Events</field>
            <field name="type">ir.actions.act_window</field>
            <field name="res_model">ir.values</field>
            <field name="view_type">form</field>
            <field name="view_mode">form,tree</field>
            <field name="context">{''democracy" : False}</field>
        </record>

Is the main record in xml. Now if I change the value of field context to anything like {'Barack':'Obama''}, context will be updated in DB for this record as context is available in the xml record.
If I delete the record, on updatin of xml file, server won't find the context key and simply,won't update the field in database.

If you want to remove the XML record, you may use delete tag :
<delete model="ir.actions.act_window" search="[('name','=','act_values_form_action')]"/>

If I only want to remove a value of any field, I should set default value like
<field name="context">{}</field>.

If I am using a module which uses the record and want to edit the record, I would use:
<record id="MODULENAME.act_values_form_action" model="ir.actions.act_window">
            <field name="name">MODIFIED HEADER</field>
            <field name="context">{'Are you a new context': 'Yes'}</field>
        </record>

Hope this helps.
Thanks.

[Jay Vora (Serpent Consulting Services) (jayvora)]

This issue has been discussed many times.
We are open to work on some suitable solution , but this behavior cannot be called as a bug.
Solving it, would be called an added feather!
Thanks.

[Ferdinand (office-chricar)]

IMHO allowing to delete field content which causes failure of the program is a bug.

And please do not put bugs to "invalid" - these will never be handled.

[Olivier Dony (Odoo) (odo-openerp)]

Ferdinand, JMA marked the bug as Invalid because he thinks it is invalid, that's the whole point.

The system is working as expected: XML data that is not set to "noupdate" will be updated when the module it belongs to is updated, but only the fields that are explicitly mentioned in the XML. This is by design, and a good thing.
How and with what value would you update the other fields that are unspecified?? Think about all the possible cases: default values that have changed, custom fields, admin customizations, etc.

If you have an idea for a clean, generic and fast solution there, please describe, but really this matter has been discussed extensively and your patch is definitely not correct.

The module developers need to learn the framework and work correctly with it: when you update your module it's your job to foresee an appropriate migration. So if you explicitly want to drop an attribute or field value that used to be specified in the XML, replace it with an empty or False value. And if the admin has added a context manually and this introduces a bug, then the admin needs to fix his mess himself.

And BTW the ORM does drop records of module data that have been removed from XML files, as soon as it notices that an ir.model.data object has been removed in the newer version of the module, so there's no need for <record id="the_old_id" action="delete" /> or other nonsense ;-)

I'm setting this bug back to invalid again to clean up our list, please don't put it back as new unless you have a valid solution to offer, there are real bugs out there that need the attention of the developers, I'm sure you agree :-)

[Ferdinand (office-chricar)]

@Olivier!
I do not under your argumentation

I proposed a solution which avoids that a user / admin can set the context field to empty.
Empty context fields are not working correctly.

Example
"List of accounts"
set the context field from '{}' to '' in "Windows Action"
and try to open the list of accounts - nothing happens.

I do not think it's the desired behavior and difficult to track down as no error is thrown.

So I leave it up to you to reset this bug to "New"

[Olivier Dony (Odoo) (odo-openerp)]

Ferdinand,

It was a bit confusing to me when I read the original bug report and discussion because it seemed to be about a mechanism for updating attributes and record *from the XML* side of things.
I have updated the bug description to reflect this more clearly (I think), and make it more generic as well (other fields have this same issue)

Now concerning your proposed patch, it seems to me that you are re-implementing the "required" flag using constraints (and not even a SQL one). I suggest to simply mark these fields as required, with the added value that this will provide a visual hint in the client directly (it will not provide your custom error message but the admins should know what to do)

What do you think?

[Ferdinand (office-chricar)]

You are right - there are 2 issues
* xml definitions do not allow to "delete" context (and probably other) fields
* this leads to the need to do manual updates

your proposed solution "mandatory" does not give any positive feedback what to do if the field is set to None.

the admin has to "know" that at least '{}' has to be entered. I doubt that all admins will know or guess.

probably the logic could/should be changed to handle empty fields gracefully.

I also doubt slightly that the help text (currently missing) will be consulted in case of mandatory error.

[Olivier Dony (Odoo) (odo-openerp)]

Le 05/07/10 18:23, Ferdinand @ ChriCar a écrit :
> You are right - there are 2 issues
> * xml definitions do not allow to "delete" context (and probably other) fields

Sorry but, why don't you want to use the following in your module, as
explained previously:

    <record id="my_action" model="ir.actions.act_window">
       ...
       <field name="context">{}</field> <!-- was wrong, reset it -->
    </record>

> * this leads to the need to do manual updates

I think you should never ask the administrators to do manual updates,
this is too error-prone and too time-consuming for support afterwards.

> your proposed solution "mandatory" does not give any positive feedback
> what to do if the field is set to None.
> the admin has to "know" that at least '{}' has to be entered. I doubt
> that all admins will know or guess.

This is why:
1) don't ask them to do it
2) do it yourself properly in your module ;-)

> probably the logic could/should be changed to handle empty fields
> gracefully.

You're right, we can do both: handle it gracefully and help prevent it.
I'll check this with the client team as well.

> I also doubt slightly that the help text (currently missing) will be
> consulted in case of mandatory error.

Good point, it doesn't hurt to put a tooltip.

The commit with everything has landed on trunk with revision 2471
(<email address hidden>) so I mark this bug as
fixed.

Thanks a lot for reporting and providing a patch as well!

[Ferdinand (office-chricar)]

Thanks for pointing out

    <record id="my_action" model="ir.actions.act_window">
        ...
        <field name="context">{}</field> <!-- was wrong, reset it -->
     </record>

IMHO this is not very intuitive (but probably the way to go and should be documented)

intuitive (for me) is to delete the name="context">{wrong definition}</field>

[Borja López Soilán (NeoPolus) (borjals)]

I think the ORM should be smart enough to use {} (that works) instead of None (that causes exceptions) for the for any field that requires a dictionary; and [] instead of None for any field that requires a list.

So, when it finds something like this:
    <record id="my_action" model="ir.actions.act_window">
        <field name="context"></field> <!-- (currently this does not work) -->
     </record>

It should work just as good as:
    <record id="my_action" model="ir.actions.act_window">
        <field name="context">{}</field>
     </record>

Ferdinand: That is the intuitive way for me: Explicitly set the value to none/null/nothing if I want to disable/remove it.

[Ferdinand (office-chricar)]

Yes this is ok and intuitive for python programmers.

I just assume that not all administrators and coder will be firm enough in python to realize that deleting / omitting
<field name="context">{}</field>
will not set the db-entry correctly.

why this: because obviously nowhere in the xml files a line
<field name="context">{}</field>
is found as example as it is not necessary.

So I strongly suggest to add this trick as hint in the documentation.