-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[10.0][MIG] auth_admin_passkey #776
Conversation
@Fenkiou thanks as well, i was trying to get this module working on V.10 and discovered your pull. You forgot to change openerp to odoo in auth_admin_passkey/view/res_config_view.xml |
6b6092e
to
62d4a00
Compare
@eugen-don Good spot, thanks |
62d4a00
to
1cac349
Compare
470f921
to
659a354
Compare
<group> | ||
<label for="id" string="Passkey"/> | ||
<div> | ||
<div> | ||
<field name="auth_admin_passkey_send_to_admin" class="oe_inline"/> | ||
<field name="" class="oe_inline"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace this line with: <field name="auth_admin_passkey_send_to_admin" class="oe_inline"/>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, I don't understand why this change is in the PR as not in my own branch..
EDIT: Nevermind, this is present in my branch but I still don't understand why I removed this..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to call get_default_auth_admin_passkey_send_to_admin method with one more argument fields
edit: applies to the other methods as well
@eugen-don You're right about the missing argument, I'll fix that. |
659a354
to
586f0d2
Compare
@Fenkiou I also was trying to get it to work with the adittional fields argument. But if i add fields as argument, like some other modules inheriting res.config do i get this error: Traceback (most recent call last): Any idea? |
Thanks for the info @JonathanNEMRY @Fenkiou what do you think? Any way to join efforts? |
@eugen-don, @JonathanNEMRY I saw this PR but as not merged didn't start from it. But it's still overriding authenticate method as in 8.0 version and IMHO is not the right way to do it as odoo provide check_credentials method to use custom auth mechanism. I should have told that in PR. @eugen-don, about your error, I couldn't reproduce it. Are you using my code or your own ? If it's your own, you should post it here :) |
@Fenkiou |
@Fenkiou looks like theres still some work to do. Any idea? Traceback (most recent call last): code res_config: class BaseConfigSettings(models.TransientModel):
` |
@eugen-don Hum.. I still cannot reproduce your issue.. When the error is raised ? When you install the module ? When you go to |
@Fenkiou But i just did a fresh pull from odoo repo and still get the error... |
@Fenkiou |
5d5b13d
to
ff10208
Compare
@legalsylvain, @pedrobaeza Hi guys, sorry to ping you but we have an issue with this addon with @eugen-don and don't understand why. We get 3 different behaviours.
Any idea ? I'll recheck the runbot instance when build is done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on runbot.
Works on V. 10.0
@eugen-don So you had this module installed too ? Otherwise I don't understand why this module is causing your error without being installed. |
@Fenkiou Hi, yes it was installed on my local system and on the runbot instance of this pr, i uninstalled it on runbot and see there the general settings menu was accessible. On my local system the module version was a little bit older so it was causing a severe error. Notice base_exception was migrated recently. |
ff10208
to
237bff3
Compare
@Fenkiou |
@eugen-don I don't understand. Could you assure me that on a new odoo database with no other module installed than auth_admin_passkey, this is working correctly ? Otherwise please describe all your steps to help me reproduce. |
Well, thanks to Travis, I learned how to use coverage on my machine.
Nothing to do with the PR Writing UI test by writing steps in JS to call them from python test is completely absurd (IMO). I took inspiration in auth_signup_verify_email to write tests using python only but still, this is ugly and I couldn't write my tests as I wanted to. EDIT: For the multiple database problem, a --db-filter should fix the issue. |
b2745d6
to
4431b82
Compare
@pedrobaeza, @legalsylvain do you think a --db-filter could solve my problem with Travis ? |
@Fenkiou Maybe you can start by calling the |
Thanks @sylvain-garancher, this is an interesting one ! Is it intentional to not use HttpCase ? |
I don't remember exactly, but I didn't achieve to do some tests with it, so I used werkzeug's test client instead of Odoo's HttpCase. |
this fixes an issue reported on Transifex
ba196d4
to
72254ae
Compare
Remove authenticate as check_credentials is dedicated for this purpose. Removed mail translations maybe possible in some way ? Give some space to the code Make the addon compliant to OCA guidelines Adapt readme to new template and compress header in tests Make the addon a python package NOTE: authenticate() method cannot be used in tests because a new cr is created in _login method that does not contains our user. Signed-off-by: Eugen Don <[email protected]>
72254ae
to
e16b37a
Compare
It's working, finally ! You can merge when you'll have the time, thanks for the help. |
How is this going? Will it be merged? |
Syncing from upstream OCA/server-tools (11.0)
This is a port of auth_admin_passkey from 8.0.
I changed the way the verification is done, but it works same as before.