openerp-community team mailing list archive

["Stefan Rijnhart \(Therp\)" <stefan@xxxxxxxx>]

Review: Resubmit
> On 06/20/2011 10:21 AM, Stefan Rijnhart (Therp) wrote:
> > I will therefore reuse this branch to suggest such a breakdown in
> > separate functions, and leave the added functionality for a separate
> > module of our own make.
> 
> Sounds great, thanks! :-)

Hi Olivier,

this branch now features the refactured version of users_ldap. Here is an account of the changes I made:

[REM] reverted the populate functionality, moving to extra addon

As discussed, my team will publish a separate module with the populate functionality.

[REF] split up LDAP authentication in atomic methods

As discussed, res.company.ldap now has simple methods for retrieving the ldap configurations, connecting to the ldap server, query the subtree with the user accounts, authenticate dn/password combinations and compose a set of field values for creating res.users.

[IMP] leave it to res.users' defaults to select menu item for new users

Selecting the correct ir_action_action was broken in the module. After having installed several modules, a dashboard from one of these modules would be picked. I noticed that res.users assigned the correct default anyway and removed the corresponding code from users_ldap. As for the user's action_id, users_ldap used to set the menu action here as well which caused a continuous refreshing in the web client when such a user logs on. As res.users leaves this value empty by default, I removed this code as well.

[REF] remove unnecessary lambdas from defaults

[REF] improve pep8 compatibility

Respect 79 columns almost everywhere. Load system libraries first.

[REF] remove unnecessary conditionals when an exception is raised anyway

The old code checks the result of the user authentication against the ldap server. However, an unsuccesful authentication raises an exception which was handled already, rendering the conditional meaningless.

[IMP] do not log redundant exception ldap.INVALID_CREDENTIALS

An invalid login is already logged by WEB-SERVICE.

[IMP] replaced generic exceptions by ldap subclass

[FIX] update timestamp when LDAP user logs in
[FIX] prevent inactive LDAP users from logging in

Looking for guidance in res.users, I encounted these aspects of logins and I remembered coming across lp:784501. It seemed artificial to make an extra effort to preserve the original behaviour, but admittedly this is the only area in which the behaviour of the module has actually changed.

[FIX] use ldap.search_st() which actually does take the timeout argument

The old code uses ldap.search() with a timeout argument, but this function takes no such argument. In fact, ldap.search() is an asynchronous function which the old code does not anticipate so it seems to me that ldap.search_st() was the intended function. Also, the new code replaces ldap.open(), which is deprecated, by ldap.initialize().

Cheers,
Stefan.

-- 
https://code.launchpad.net/~openerp-community/openobject-addons/stefan-therp_lp794584/+merge/63872
Your team OpenERP Community is subscribed to branch lp:~openerp-community/openobject-addons/stefan-therp_lp794584.