[MIG][10.0] auth_session_timeout by lasley · Pull Request #580 · OCA/server-tools

lasley · 2016-10-21T01:51:08Z

WIP - I need to add test coverage for res.users

Bump versions
Installable to True
Add Usage section to ReadMe w/ Runbot link
_crypt_context now directly exposes the CryptContext
Change all instances of openerp to odoo
Add test coverage to IrConfigParameter

lasley · 2016-10-21T02:26:40Z

@@ -1,23 +0,0 @@
-# Translation of Odoo Server.


@pedrobaeza - I could have sworn I saw the removal of PO files from migrations somewhere, but I now can't remember where. Am I being stupid here, or are these actually supposed to be removed?

You need to remove POT files, not PO ones. Having PO files, although outdated, allow to reuse the sentences that haven't changed. Next Transifex synchronization will wipe out the non existing terms comparing with the newest POT file.

@atchuthan proposed in OCA/maintainer-tools#232 that we should avoid the commits that updates the languages in the migration process, but I'm evaluating if this is a good idea at all.

Thanks for the explanation @pedrobaeza - I think that the issue that you linked is the one that made me think this way. I'll add back 😄

lasley · 2016-10-21T21:39:15Z

Coverage added in all but res.users.check, which was going to be a real hassle. The logic is simple enough that I don't think it will be a problem.

Ready for review

elicoidal

👍

elicoidal · 2016-10-22T12:47:47Z

+Usage
+=====
+
+Setup the session parameters as described above.


You might want to move the above section in this one.

I thought about it, but I'm honestly torn. They are more of a configuration instruction vs usage. Maybe instead I add Use system like normal in place of the note to configure using instructions?

moylop260 · 2016-10-22T13:12:17Z

+1

lasley · 2016-12-20T08:45:07Z

Good to merge?

yajo

Just a little thing, not requirement.

yajo · 2016-12-20T09:58:57Z

    _inherit = 'ir.config_parameter'

+    @api.model
    @tools.ormcache(skiparg=0)


What should be the cache key? skiparg is deprecated. Better use @tools.ormcache("self.env.db") IMHO.

OK so I'm a bit confused here & I am pretty sure this is what is causing the current error.

What are we going for with this cache? Do you maybe have an example of some other code that does something similar, or would mind giving a quick breakdown for me? My google-fu is turning up zilch here.

nilshamerlinck · 2016-12-23T09:39:08Z

+        session = http.request.session
+        session_store = http.root.session_store
+        ConfigParam = http.request.env['ir.config_parameter']
+        delay, urls = ConfigParam.get_session_parameters(db)


No need for the db parameter anymore, right?

lasley · 2016-12-23T19:18:03Z

Thanks for the reviews @yajo & @nilshamerlinck - both points fixed.

yajo · 2016-12-27T09:00:23Z

Bots still ❌

yajo · 2016-12-29T08:31:54Z

-            cr.close()
-        return delay, urls
+    @api.model
+    @tools.ormcache('self.env.db')


@lasley, about #580 (comment) (I answer here so this does not get hidden)...

I learned the hard way to use ormcache when developing website_sale_wishlist. It turns out it's a pretty neat tool. It creates a key by the arguments you pass to it, and stores the method result in a cache attached to your current self.env, so further calls to the method return the cached result instead of executing the method again and again.

It's most useful when a method is CPU-expensive, is called quite often, and it returns a predictable result based on the incoming arguments/environment.

In old api, we had the skipargs, which was used to skip a number of arguments and use the rest to make a cache. For instance, if your method had cr, uid, ids, key, context=None and the result was the same no matter the cr, uid, you'd say skipargs=2.

Since v9, we have the new notation where you give the name of the arguments that form the cache key. Since self is always the first argument, you can use that one (and properties of it) to form a key.

When you change your env (like self.with_context(goal="I wanna be a pirate")), cache is empty. This is logical because a different context/cr/uid usually yields different results almost everywhere. However, when you apply cache to a method, it gets a clear_cache() method that you can use later to clear it without having to change your environment.

Thanks for the excellent explanation @yajo - this totally clarifies what we are going for here & I think points me in the right direction. I'll make the update soon 😄

lasley · 2017-01-28T00:21:11Z

Oops seems I forgot about this. With the excellent explanation @yajo gave regarding the cache, I abstracted that a decent replacement for the db arg we had previously is now self.cr.dbname.

It seems OCA/OCB just synced up with odoo/odoo though, so now we're red for other reasons. Oh the joy!

chienandalu

@lasley Well, sorry for my part of contribution to this confusion...

Anyway, are you going to implement any of the changes made on #887 or #875?

lasley · 2017-09-06T15:38:57Z

Anyway, are you going to implement any of the changes made on #887 or #875?

I'm going to have to go through the diffs to understand the changes. I'm hoping maybe a cherry pick of some stuff will do it 😆

pedrobaeza · 2017-11-03T18:15:55Z

@lasley can you finish this integrating #887 ?

lasley · 2017-11-03T19:05:36Z

Cherry picked. My bad, I forgot this

pedrobaeza · 2017-11-04T12:50:20Z

I'm getting this error installing this module:

Traceback (most recent call last):
  File "/opt/odoo/custom/src/odoo/odoo/addons/base/ir/ir_http.py", line 103, in _authenticate
    request.session.check_security()
  File "/opt/odoo/custom/src/odoo/odoo/http.py", line 1058, in check_security
    security.check(self.db, self.uid, self.password)
  File "/opt/odoo/custom/src/odoo/odoo/service/security.py", line 13, in check
    return res_users.check(db, uid, passwd)
TypeError: unbound method check() must be called with res.users instance as first argument (got unicode instance instead)

lasley · 2017-11-06T16:23:36Z

Oops this also reminded me that some of these methods needed to be switched around now that we're not worrying about compat. I'll submit a fix in about 30m

lasley · 2017-11-06T19:20:34Z

Alright we're ready for a final review here, then I'll squash up

yajo · 2017-11-07T13:00:36Z

Still no luck with tests 😕

lasley · 2017-11-07T15:53:17Z

Still no luck with tests 😕

Yeah got me dude. It seems we're fighting an uphill battle with Travis and I am damned close to just removing tests from multiple PRs until 🍏

lasley · 2017-11-07T16:20:27Z

Yup I've got nothing - it's failing on a bunch of modules unrelated to me. The option I see is to remove all tests from this module, but we obviously still have a timebomb on our hands.

From a global perspective, this is an issue I am encountering in numerous places. My immediate thought on a solution is that all modules should be tested independently from each other, instead of inline with each other. That covers up incompatibility problems though, so I'm at a loss.

hbrunn · 2017-11-07T16:24:59Z

exclude/include is no solution?

lasley · 2017-11-07T16:30:00Z

It is technically, but we've already done this in server-tools, so we're just N+1ing IMO. It's also damned near impossible to find the failing/conflicting module without a bunch of hunt/pecking with includes and excludes.

Biggest issue though is that only the first test case is available on Runbot, so we miss out on functional review of anything excluded.

lasley · 2017-11-07T16:57:47Z

Welp exclude is what I went with - better than removing tests. Thanks for reminding me that exists Holger.

Hopefully our server-tools split helps with this. Or our MQT redesign. Or something.

Furthermore, we are finally 🍏 (+1y1m later 😆 )

yajo · 2017-11-08T11:43:24Z

Yikes, It's not fun having tests that get broken when an addon is installed... Well, in any case could you squash the migration commits for merge?

* Bump versions * Installable to True * Add Usage section to ReadMe w/ Runbot link * `_crypt_context` now directly exposes the `CryptContext` * Change all instances of openerp to odoo * Add test coverage to IrConfigParameter * Add test coverage for res.users * Remove db from `get_session_parameters` method call * Remove deprecated skiparg for ormcache * Fix tests & lint * Switch cache to use self.cr.dbname * Fix ormcache

* Module auth_session_timeout: --------------------------- * Refactor to allow other modules to inherit and augment or override the following: ** Session expiry time (deadline) calculation ** Ignored URLs ** Final session expiry (with possibility to late-abort) * Re-ordered functionality to remove unnecessary work, as this code is called very often. * Do not expire a session if delay gets set to zero (or unset / false) * WIP * Fixed flake8 lint errors * Fixed flake8 lint errors * WIP * WIP * WIP * WIP * WIP * WIP * Module: auth-session-timeout: Refactor ResUser tests to use `unittest.mock` patching * Module: auth_session_timeout: Fixed flake8 lint errors * Module: auth_session_timeout: Fixed flake8 lint errors

* Deprecate backwards compatibility methods that were retained during v9 rework * Upgrade API and rename a few things for PEP-8 * Switch to HttpCase for tests * Switch to isolated build

lasley · 2017-11-08T16:04:57Z

Squashed!

Syncing from upstream OCA/server-tools (13.0)

lasley added the work in progress label Oct 21, 2016

lasley added this to the 10.0 milestone Oct 21, 2016

lasley changed the title ~~[MIG] password_security: Migrate to v10~~ [MIG][10.0] password_security Oct 21, 2016

lasley changed the title ~~[MIG][10.0] password_security~~ [MIG][10.0] auth_session_timeout Oct 21, 2016

lasley force-pushed the release/10.0/auth_session_timeout branch from 002d954 to e50a728 Compare October 21, 2016 01:52

pedrobaeza mentioned this pull request Oct 21, 2016

Migration to version 10.0 #565

Closed

63 tasks

lasley commented Oct 21, 2016

View reviewed changes

lasley force-pushed the release/10.0/auth_session_timeout branch 2 times, most recently from 41fd23c to 85182ac Compare October 21, 2016 18:47

lasley added needs review and removed work in progress labels Oct 21, 2016

lasley force-pushed the release/10.0/auth_session_timeout branch 7 times, most recently from bed5b78 to 9f80777 Compare October 21, 2016 21:32

elicoidal approved these changes Oct 22, 2016

View reviewed changes

lasley force-pushed the release/10.0/auth_session_timeout branch from 9f80777 to 3807cce Compare December 20, 2016 08:44

yajo approved these changes Dec 20, 2016

View reviewed changes

nilshamerlinck reviewed Dec 23, 2016

View reviewed changes

lasley force-pushed the release/10.0/auth_session_timeout branch from e914399 to 21e6177 Compare December 28, 2016 20:41

yajo reviewed Dec 29, 2016

View reviewed changes

chienandalu reviewed Sep 6, 2017

View reviewed changes

lasley force-pushed the release/10.0/auth_session_timeout branch from a331714 to 779c01a Compare November 3, 2017 19:06

lasley force-pushed the release/10.0/auth_session_timeout branch 2 times, most recently from f9aeace to 6c01b29 Compare November 6, 2017 16:42

lasley mentioned this pull request Nov 6, 2017

Migrate module auth_session_timeout from 9.0 to 10.0 #897

Closed

lasley force-pushed the release/10.0/auth_session_timeout branch 2 times, most recently from 518bf14 to b1dfeb2 Compare November 6, 2017 18:08

lasley force-pushed the release/10.0/auth_session_timeout branch from 8446a1a to ea711ff Compare November 7, 2017 15:38

lasley and others added 3 commits November 8, 2017 08:03

[IMP] auth_session_timeout: Deprecate backwards compat + improve

00e7930

* Deprecate backwards compatibility methods that were retained during v9 rework * Upgrade API and rename a few things for PEP-8 * Switch to HttpCase for tests * Switch to isolated build

lasley force-pushed the release/10.0/auth_session_timeout branch from daad3fa to 00e7930 Compare November 8, 2017 16:04

yajo approved these changes Nov 9, 2017

View reviewed changes

pedrobaeza merged commit cbf0747 into OCA:10.0 Nov 9, 2017

lasley deleted the release/10.0/auth_session_timeout branch November 9, 2017 13:21

SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024

Merge pull request OCA#580 from OCA/13.0

ae8f112

Syncing from upstream OCA/server-tools (13.0)

Uh oh!

Uh oh!

Conversation

lasley commented Oct 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lasley commented Oct 21, 2016

Uh oh!

elicoidal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moylop260 commented Oct 22, 2016

Uh oh!

lasley commented Dec 20, 2016

Uh oh!

yajo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lasley commented Dec 23, 2016

Uh oh!

yajo commented Dec 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lasley commented Jan 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chienandalu left a comment

Choose a reason for hiding this comment

Uh oh!

lasley commented Sep 6, 2017

Uh oh!

pedrobaeza commented Nov 3, 2017

Uh oh!

lasley commented Nov 3, 2017

Uh oh!

pedrobaeza commented Nov 4, 2017

Uh oh!

lasley commented Nov 6, 2017

Uh oh!

lasley commented Nov 6, 2017

Uh oh!

yajo commented Nov 7, 2017

Uh oh!

lasley commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lasley commented Nov 7, 2017

Uh oh!

hbrunn commented Nov 7, 2017

Uh oh!

lasley commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lasley commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yajo commented Nov 8, 2017

Uh oh!

lasley commented Nov 8, 2017

Uh oh!

Reviewers

elicoidal left a comment •

edited

Loading

lasley commented Jan 28, 2017 •

edited

Loading

lasley commented Nov 7, 2017 •

edited

Loading

lasley commented Nov 7, 2017 •

edited

Loading

lasley commented Nov 7, 2017 •

edited

Loading