[19.0][MIG] auth_oauth_multi_token: Migration to 19.0

[AEstLo]

• Odoo has refactored users view, so we updated the location of the new fields.

- Added new test

[CRogos]

Code LGTM, but not tested.

Some commits could be squashed.

[AEstLo]

@CRogos Thanks for the review!
I've squashed the commits!

[thomashaunschmid86]

LGTM

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[AEstLo]

@pedrobaeza do you mind to check if we can merge this? Thanks in advance!

[pedrobaeza]

AFAIK, you are changing the setting from the user to a general system parameter, so it's a huge change. Why?

[AEstLo]

AFAIK, you are changing the setting from the user to a general system parameter, so it's a huge change. Why?

Because in 19.0 users do not have such field anymore

[pedrobaeza]

This is not something of Odoo 19, but of this module:

server-auth/auth_oauth_multi_token/models/res_users.py

Line 23 in 05bf187

oauth_access_max_token = fields.Integer(

And you are removing it in the migration.

[AEstLo]

But odoo removed the page with all the configuration for users about this topic, and moved it.
Odoo 18:

Odoo 19:

[pedrobaeza]

It doesn't matter. You just need to add the field in that new page.

[AEstLo]

I disagree, if Odoo moved all the configuration related to OAuth there, I think it makes sense to follow the same path, to keep it consistent. However, I am open to change it and appreciate your feedback. Do you mind if we ask @simahawk (as the creator of the PR which added this module) for his thoughts, and I will happily update the code if he agrees with you :)

[pedrobaeza]

That's not really true. What you are showing is the general provider configuration, not each of the OAuth specific user data. Odoo has simply hide that information on the user in 19 (according to what you are showing in the screenshots), but they are 2 different granularity levels.

[AEstLo]

Odoo completely removed the page: odoo/odoo@edc6562

[pedrobaeza]

But I insist, they are 2 different granular levels. Previously, the max logins configuration was per user. Now you are putting it once for the whole system. It doesn't matter if the page is visible or not on the user. You are changing the configuration granularity.

[AEstLo]

that's correct, I know, actually it would have been easier to include the field in the "Security" tab of users. But I decided to refactor it because I didn't think that having it per user is really that useful in this case.
Anyway, if even after this long discussion you continue thinking that it must be in the "security" tab, I will modify this migration to restore previous behaviour :)

[pedrobaeza]

The question is that this is something outside the migration scope, as it's something more that can even be done in any other version. And not only that, this doesn't contain a migration script to handle possible inconsistencies: what happen if a system has all the users to have 100 max, and you are resetting that to 10? Or if there are some with 100, and another with 10?

For that reason, I think it's better to limit the migration to copy this as is, and place the user field on the possible best place (agreed that the page where it was located has been removed, but that page only contained info, and this is a configuration field), and leave the discussion for a separate PR.

And in general, on migration PRs, it's not convenient to do heavy refactorings/improvements that are outside of the scope of the upstream changes.

[AEstLo]

You convinced me :)
Thanks for the discussion, I will ping you again when this is as agreed

[AEstLo]

@CRogos @thomashaunschmid86 do you mind to check it again?

[thomashaunschmid86]

LGTM

[pedrobaeza]

/ocabot merge nobump

[OCA-git-bot]

What a great day to merge this nice PR. Let's do it!
Prepared branch 19.0-ocabot-merge-pr-890-by-pedrobaeza-bump-nobump, awaiting test results.

[OCA-git-bot]

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