Skip to content

[12.0][MIG] Module auth_u2f - #90

Closed
fkantelberg wants to merge 5 commits into
OCA:12.0from
initOS:12.0_mig_auth_u2f
Closed

fkantelberg wants to merge 5 commits into
OCA:12.0from
initOS:12.0_mig_auth_u2f

Conversation

@fkantelberg

Copy link
Copy Markdown
Member

Hello,
I would like to share the migration of the module aufh_u2f to version 12.0.

@pedrobaeza pedrobaeza added this to the 12.0 milestone Apr 26, 2019
@OCA-git-bot OCA-git-bot mentioned this pull request Apr 26, 2019
19 tasks
@fkantelberg
fkantelberg force-pushed the 12.0_mig_auth_u2f branch 2 times, most recently from a95fac6 to e7081d8 Compare April 26, 2019 13:49
@fkantelberg

Copy link
Copy Markdown
Member Author

@pedrobaeza I have no idea why the runbot is failing. I tested it locally without a problem and I can even access the runbot. Would you mind labelling it with needs_review?

@pedrobaeza

Copy link
Copy Markdown
Member

It's a runbot warning:

2019-06-22 06:39:04,018 172 WARNING openerp_test odoo.models: The model u2f.device has no _description

@fkantelberg
fkantelberg force-pushed the 12.0_mig_auth_u2f branch 4 times, most recently from 77321f5 to 9ce03c8 Compare July 7, 2019 09:06
@fkantelberg

Copy link
Copy Markdown
Member Author

@pedrobaeza Thanks for the hint 👍

@fkantelberg
fkantelberg force-pushed the 12.0_mig_auth_u2f branch 2 times, most recently from dbe40d9 to a73a54c Compare July 12, 2019 13:19

@tbaden tbaden left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks for the work 👍
I did a code review: LGTM

<?xml version="1.0" encoding="utf-8"?>
<odoo>
<data>
<record id="view_users_form_inherit_auth_u2f" model="ir.ui.view">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

optional change:
use original id (view_users_form)

<field name="domain">[('user_id', '=', uid)]</field>
</record>

<record id="view_users_form_simple_modif_inherit_auth_u2f" model="ir.ui.view">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

optional change:
use original id (view_users_form_simple_modif)

@yucer

yucer commented Jul 12, 2019

Copy link
Copy Markdown

It is great code.

Just one question about this call

Should one verify the previous result from super() before making the extra verification ?

If super fails, there is no need.

At least that the failed authentication trigger one exception.

@fkantelberg

Copy link
Copy Markdown
Member Author

If super fails it fails with an exception the result isn't really important in that case.

@yucer yucer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Clean code.

@fkantelberg

Copy link
Copy Markdown
Member Author

@pedrobaeza Can we merge this?

@pedrobaeza

Copy link
Copy Markdown
Member

/ocabot merge

@OCA-git-bot

Copy link
Copy Markdown
Contributor

@pedrobaeza The merge process could not start, because command git rebase --autosquash 12.0 failed with output:

First, rewinding head to replay your work on top of it...
Applying: Add auth_u2f module
Applying: [UPD] README.rst
Applying: [UPD] Update auth_u2f.pot
Applying: [ADD] icon.png
Applying: Adjust to odoo 12.0
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging requirements.txt
CONFLICT (add/add): Merge conflict in requirements.txt
error: Failed to merge in the changes.
Patch failed at 0005 Adjust to odoo 12.0
Use 'git am --show-current-patch' to see the failed patch

Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".


@pedrobaeza

Copy link
Copy Markdown
Member

Please solve the conflict.

@fkantelberg

Copy link
Copy Markdown
Member Author

@pedrobaeza
Should work now.

@pedrobaeza

Copy link
Copy Markdown
Member

/ocabot merge

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Rebased to 12.0-ocabot-merge-pr-90-by-pedrobaeza-bump-no, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Aug 30, 2019
Signed-off-by pedrobaeza
@OCA-git-bot

Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at 255e6ed. Thanks a lot for contributing to OCA. ❤️

PS: Don't worry if GitHub says there are unmerged commits: it is due to a rebase before merge. All commits of this PR have been merged into 12.0.

SiesslPhillip pushed a commit to grueneerde/OCA-server-auth that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-auth (10.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.

6 participants