Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1 into lp:openobject-server

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1 into lp:openobject-server

Stefan Rijnhart (Therp)
Review: Resubmit

Hi Olivier,

thanks again for the valuable review.

On your suggestion, I have replaced the exception with a warning. I used a logging.warn instead of a warnings.DeprecationWarning as these cannot easily be redirected to the application log unless one requires Python 2.7 (with logging.captureWarnings()).

Also, it does indeed make sense to mention all unknown fields at once.

However, I cannot think of a situation that warrants the overhead of performing a similar check in every read operation. Usually, the absense of fields will become immediately clear when trying to read their values from the resulting data structure. For completeness, note that a check on non-existing fields in domain expressions does already exist in expression.py.

Regards,
Stefan.




--
https://code.launchpad.net/~openerp-community/openobject-server/stefan-therp_lp727727-6.1/+merge/80904
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

[Openerp-community] [Merge] lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1 into lp:openobject-server

Stefan Rijnhart (Therp)
Stefan Rijnhart (Therp) has proposed merging lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1 into lp:openobject-server.

Requested reviews:
  Stefan Rijnhart (Therp) (stefan-therp)
  Olivier Dony (OpenERP) (odo-openerp)
Related bugs:
  Bug #727727 in OpenERP Server: "Orm.create() should not drop nonexisting fields in passed values"
  https://bugs.launchpad.net/openobject-server/+bug/727727

For more details, see:
https://code.launchpad.net/~openerp-community/openobject-server/stefan-therp_lp727727-6.1/+merge/80904

This branch raises an exception when a value is passed to the ORM for a non existing field during create() or write(). Previously these values were simply dropped, which could lead to data loss.

--
https://code.launchpad.net/~openerp-community/openobject-server/stefan-therp_lp727727-6.1/+merge/80904
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1.

=== modified file 'openerp/addons/base/base_data.xml'
--- openerp/addons/base/base_data.xml 2011-10-24 12:02:19 +0000
+++ openerp/addons/base/base_data.xml 2011-11-02 20:27:24 +0000
@@ -1580,7 +1580,6 @@
 
         <record id="res_bank_1" model="res.bank">
             <field name="name">Reserve</field>
-            <field name="code">RSV</field>
         </record>
 
         <record id="CRC" model="res.currency">
@@ -1600,7 +1599,7 @@
             <field name="name">localhost</field>
             <field name="smtp_host">localhost</field>
             <field eval="25" name="smtp_port"/>
-            <field eval="10" name="priority"/>
+            <field eval="10" name="sequence"/>
     </record>
 
         <record id="MUR" model="res.currency">

=== modified file 'openerp/addons/base/module/module_view.xml'
--- openerp/addons/base/module/module_view.xml 2011-07-27 12:34:44 +0000
+++ openerp/addons/base/module/module_view.xml 2011-11-02 20:27:24 +0000
@@ -75,7 +75,6 @@
             <field eval="'ir.module.category'" name="model"/>
             <field name="name">Categorized Modules</field>
             <field eval="'ir.actions.act_window,%d'%action_module_open_categ" name="value"/>
-            <field eval="True" name="object"/>
         </record>
 
 

=== modified file 'openerp/addons/base/res/res_partner_view.xml'
--- openerp/addons/base/res/res_partner_view.xml 2011-10-25 20:48:47 +0000
+++ openerp/addons/base/res/res_partner_view.xml 2011-11-02 20:27:24 +0000
@@ -651,7 +651,6 @@
             <field eval="'res.partner.category'" name="model"/>
             <field name="name">Open partners</field>
             <field eval="'ir.actions.act_window,%d'%action_partner_by_category" name="value"/>
-            <field eval="True" name="object"/>
         </record>
 
         <record id="action_partner_category_form" model="ir.actions.act_window">

=== modified file 'openerp/osv/orm.py'
--- openerp/osv/orm.py 2011-10-19 11:46:18 +0000
+++ openerp/osv/orm.py 2011-11-02 20:27:24 +0000
@@ -3808,6 +3808,7 @@
             for id in ids:
                 result += self._columns[field].set(cr, self, id, field, vals[field], user, context=rel_context) or []
 
+        unknown_fields = updend[:]
         for table in self._inherits:
             col = self._inherits[table]
             nids = []
@@ -3820,9 +3821,14 @@
             for val in updend:
                 if self._inherit_fields[val][0] == table:
                     v[val] = vals[val]
+                    unknown_fields.remove(val)
             if v:
                 self.pool.get(table).write(cr, user, nids, v, context)
 
+        if unknown_fields:
+            self.__logger.warn(
+                'No such field(s) in model %s: %s.',
+                self._name, ', '.join(unknown_fields))
         self._validate(cr, user, ids, context)
 
         # TODO: use _order to set dest at the right position and not first node of parent
@@ -3945,6 +3951,7 @@
                 tocreate[v] = {'id': vals[self._inherits[v]]}
         (upd0, upd1, upd2) = ('', '', [])
         upd_todo = []
+        unknown_fields = []
         for v in vals.keys():
             if v in self._inherit_fields:
                 (table, col, col_detail, original_parent) = self._inherit_fields[v]
@@ -3953,6 +3960,11 @@
             else:
                 if (v not in self._inherit_fields) and (v not in self._columns):
                     del vals[v]
+                    unknown_fields.append(v)
+        if unknown_fields:
+            self.__logger.warn(
+                'No such field(s) in model %s: %s.',
+                self._name, ', '.join(unknown_fields))
 
         # Try-except added to filter the creation of those records whose filds are readonly.
         # Example : any dashboard which has all the fields readonly.(due to Views(database views))


_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

[Openerp-community] [Merge] lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1 into lp:openobject-server

Stefan Rijnhart (Therp)
In reply to this post by Stefan Rijnhart (Therp)
The proposal to merge lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1 into lp:openobject-server has been updated.

Description changed to:

This branch issues a warning when a value is passed to the ORM for a non existing field during create() or write(). These values are simply dropped, which can lead to data loss. Additionaly, this branch corrects non existing fields in the base module's XML files.

For more details, see:
https://code.launchpad.net/~openerp-community/openobject-server/stefan-therp_lp727727-6.1/+merge/80904
--
https://code.launchpad.net/~openerp-community/openobject-server/stefan-therp_lp727727-6.1/+merge/80904
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1 into lp:openobject-server

Olivier Dony (OpenERP)
In reply to this post by Stefan Rijnhart (Therp)
Review: Approve

I can live with logging.warn and not having an explicit check in read() :-)
It's less consistent, but it's true that data loss is not as much a concern as with create/write.

Thanks for the update, great job!
--
https://code.launchpad.net/~openerp-community/openobject-server/stefan-therp_lp727727-6.1/+merge/80904
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

[Openerp-community] [Merge] lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1 into lp:openobject-server

Olivier Dony (OpenERP)
In reply to this post by Stefan Rijnhart (Therp)
The proposal to merge lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1 into lp:openobject-server has been updated.

    Status: Needs review => Approved

For more details, see:
https://code.launchpad.net/~openerp-community/openobject-server/stefan-therp_lp727727-6.1/+merge/80904
--
https://code.launchpad.net/~openerp-community/openobject-server/stefan-therp_lp727727-6.1/+merge/80904
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

Re: [Openerp-community] [Merge] lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1 into lp:openobject-server

Stefan Rijnhart (Therp)
In reply to this post by Stefan Rijnhart (Therp)
Hi Olivier,

Thanks for all your work on this review and other community branches!

Cheers,
Stefan.

--
https://code.launchpad.net/~openerp-community/openobject-server/stefan-therp_lp727727-6.1/+merge/80904
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|

[Openerp-community] [Merge] lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1 into lp:openobject-server

noreply
In reply to this post by Stefan Rijnhart (Therp)
The proposal to merge lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1 into lp:openobject-server has been updated.

    Status: Approved => Merged

For more details, see:
https://code.launchpad.net/~openerp-community/openobject-server/stefan-therp_lp727727-6.1/+merge/80904
--
https://code.launchpad.net/~openerp-community/openobject-server/stefan-therp_lp727727-6.1/+merge/80904
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-community
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-community
More help   : https://help.launchpad.net/ListHelp