Skip to content

Conversation

@lasley
Copy link
Contributor

@lasley lasley commented Aug 29, 2017

  • Remove new cursor creation/commit in favor of using current cursor

REF #889 (comment)

@lasley lasley added this to the 10.0 milestone Aug 29, 2017
* Remove new cursor creation/commit in favor of using current cursor
@lasley lasley force-pushed the bugfix/10.0/user_threshold-remove_commit branch from 9f58d64 to 8c72f30 Compare August 29, 2017 16:15
@Garamotte
Copy link

Garamotte commented Aug 30, 2017

Odoo has many hooks everywhere in the modules loading process, so that we can integrate where we want. The _register_hook method is the last called in the modules loading process, on a point where all modules are loaded, so the ORM is fully useable.

I think we should use the _register_hook method, instead of __init__. This method is too low-level, and should often not be overriden.
Another argument is that __init__ is called once per module that inherits the model, and _register_hook only once per model. In both cases, this is done for each process.
We also don't need to check if the module is installed.

IMO, the right fix is:

diff --git a/user_threshold/models/res_users.py b/user_threshold/models/res_users.py
index b26151215b2..3f5a5bc7679 100644
--- a/user_threshold/models/res_users.py
+++ b/user_threshold/models/res_users.py
@@ -6,7 +6,7 @@ import os
 from csv import reader
 from lxml import etree
 
-from odoo import SUPERUSER_ID, _, api, fields, models, registry
+from odoo import _, api, fields, models
 from odoo.exceptions import AccessError, ValidationError
 
 from .ir_config_parameter import THRESHOLD_HIDE, MAX_DB_USER_PARAM
@@ -20,7 +20,7 @@ class ResUsers(models.Model):
         'Exempt User From User Count Thresholds',
     )
 
-    def __init__(self, pool, cr):
+    def _register_hook(self):
         """
         Override to check if env var to hide threshold configuration and
         reset the database state is set. If it is, run those actions
@@ -28,24 +28,15 @@ class ResUsers(models.Model):
         if THRESHOLD_HIDE:
             exempt_users_var = os.environ.get('USER_THRESHOLD_USER', '')
             exempt_users = reader([exempt_users_var])
-            with api.Environment.manage():
-                with registry(cr.dbname).cursor() as new_cr:
-                    new_env = api.Environment(new_cr, SUPERUSER_ID, {})
-                    installed = new_env['ir.module.module'].search_count([
-                        ('name', '=', 'user_threshold'),
-                        ('state', '=', 'installed'),
-                    ])
-                    if installed:
-                        users = new_env['res.users'].search([
-                            ('share', '=', False),
-                            ('threshold_exempt', '=', True),
-                        ])
-                        non_ex = users.filtered(
-                            lambda r: r.login not in exempt_users
-                        )
-                        for user in non_ex:
-                            user.threshold_exempt = False
-                        new_cr.commit()
+            users = self.env['res.users'].search([
+                ('share', '=', False),
+                ('threshold_exempt', '=', True),
+            ])
+            non_ex = users.filtered(lambda r: r.login not in exempt_users)
+            for user in non_ex:
+                user.threshold_exempt = False
 
     def _check_thresholds(self):
         """

@lasley
Copy link
Contributor Author

lasley commented Sep 2, 2017

Thanks @sylvain-garancher - I had no idea about _register_hook - good to know & updated!

@lasley lasley force-pushed the bugfix/10.0/user_threshold-remove_commit branch from 91d11c1 to 60d1d3e Compare September 2, 2017 02:09
@Garamotte
Copy link

Oops, sorry, I forgot the decorator...
The _register_hook method must has an @api.model_cr decorator.


def __init__(self, pool, cr):
@api.model_cr
def _register_hook(self, pool, cr):
Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow Sep 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply def _register_hook(self):? We are in v10 api!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annnnnnd now I have actually RTFM. This is why blindly copying code is probably a bad idea lmao

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should have copy-pasted my patch :D

@Garamotte
Copy link

Garamotte commented Sep 4, 2017

Travis still fails on NameError: global name 'env' is not defined.
Definitely, you should copy/paste my patch :)

@Garamotte
Copy link

Garamotte commented Sep 4, 2017

A PEP8 error you added in the last commit: user_threshold/models/res_users.py:40:1: W293 blank line contains whitespace.

And other lint errors not related to your changes:

user_threshold/README.rst:50: [E7901(rst-syntax-error), ]  Line block ends without a blank line.
user_threshold/README.rst:50: [E7901(rst-syntax-error), ]  Undefined substitution referenced: "------".
user_threshold/README.rst:62: [E7901(rst-syntax-error), ]  Unknown target name: "github issues `<https://github.com/oca/server-tools/issues>".
user_threshold/tests/test_res_company.py:18: [W7950(odoo-addons-relative-import), TestResCompany.test_fields_view_get] Same Odoo module absolute import. You should use relative import with "." instead of "openerp.addons.user_threshold"
user_threshold/tests/test_res_users.py:72: [W7950(odoo-addons-relative-import), TestResUsers.test_fields_view_get] Same Odoo module absolute import. You should use relative import with "." instead of "openerp.addons.user_threshold"

@lasley
Copy link
Contributor Author

lasley commented Sep 19, 2017

Oops forgot about this. We should be 🍏 now!

@lasley lasley force-pushed the bugfix/10.0/user_threshold-remove_commit branch from 8adf04e to a3117d4 Compare September 19, 2017 20:00
@pedrobaeza pedrobaeza merged commit 37968c3 into OCA:10.0 Sep 19, 2017
@lasley lasley deleted the bugfix/10.0/user_threshold-remove_commit branch September 20, 2017 15:12
dsolanki-initos pushed a commit to initOS/server-tools that referenced this pull request Jan 31, 2020
* [FIX] user_threshold: Don't create or commit new cursor
* Remove new cursor creation/commit in favor of using current cursor
dsolanki-initos pushed a commit to initOS/server-tools that referenced this pull request Dec 10, 2020
* [FIX] user_threshold: Don't create or commit new cursor
* Remove new cursor creation/commit in favor of using current cursor
SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (15.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants