Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[17.0][MIG] auth_saml #634

Open
wants to merge 56 commits into
base: 17.0
Choose a base branch
from
Open

[17.0][MIG] auth_saml #634

wants to merge 56 commits into from

Conversation

astirpe
Copy link
Member

@astirpe astirpe commented Apr 8, 2024

No description provided.

@astirpe astirpe marked this pull request as ready for review April 8, 2024 14:14
@astirpe astirpe mentioned this pull request Apr 8, 2024
Closed
@vincent-hatakeyama
Copy link
Contributor

/ocabot migration auth_saml

@OCA-git-bot
Copy link
Contributor

Sorry @vincent-hatakeyama you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

vincent-hatakeyama
vincent-hatakeyama approved these changes Apr 24, 2024
View reviewed changes
Copy link
Contributor

@vincent-hatakeyama vincent-hatakeyama left a comment

Choose a reason for hiding this comment

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

I’’ve tested the module successfully with a local keycloak.

@vincent-hatakeyama
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Sorry @vincent-hatakeyama you are not allowed to merge.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@vincent-hatakeyama
Copy link
Contributor

No idea what the issue is as I am the maintainer of the module.

@vincent-hatakeyama vincent-hatakeyama mentioned this pull request Apr 29, 2024
Open
19 tasks
@vincent-hatakeyama
Copy link
Contributor

@pedrobaeza Sorry to bother you but I have no idea who to contact about the bot not recognizing me as the module maintainer anymore. I thought of contacting oca/server-auth-maintainers but there’s no such group apparently.

@pedrobaeza
Copy link
Member

IIRC, @legalsylvain was the one developing the feature of looking into the previous branch to allow maintainers to merge. Anyway, you should respect the OCA rules, requiring 2 reviews for the merge, unless this gets blocked:

https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#review

@legalsylvain
Copy link
Contributor

@pedrobaeza Sorry to bother you but I have no idea who to contact about the bot not recognizing me as the module maintainer anymore. I thought of contacting oca/server-auth-maintainers but there’s no such group apparently.

You can find the info here : https://github.com/OCA/repo-maintainer-conf/blob/master/conf/repo/server.yml#L1
And here : https://github.com/OCA/repo-maintainer-conf/blob/master/conf/psc/tools.yml#L1

No idea what the issue is as I am the maintainer of the module.

Indeed... https://github.com/OCA/server-auth/blob/16.0/auth_saml/__manifest__.py#L10

IIRC, @legalsylvain was the one developing the feature of looking into the previous branch to allow maintainers to merge.

@sbidoul what is the value of MAINTAINER_CHECK_ODOO_RELEASES in production ? (Should be a list from 8.0 to 17.0)

https://github.com/OCA/oca-github-bot/blob/master/environment.sample#L86

@legalsylvain
Copy link
Contributor

No idea what the issue is as I am the maintainer of the module.

Investigated. You propose a change in a file you dont maintain. (Requirements.txt at repo foot)
That's a limitation of the current algorythm.

@vincent-hatakeyama
Copy link
Contributor

No idea what the issue is as I am the maintainer of the module.

Investigated. You propose a change in a file you dont maintain. (Requirements.txt at repo foot) That's a limitation of the current algorythm.

I believe that change is from previous commits kept by the migration process. The file is automatically generated, I suppose the changes should be removed in the migration commit so that the file is regenerated by a bot with the content?

@pedrobaeza
Copy link
Member

I think requirements.txt and test_requirements.txt should be excluded from the algorithm.

@sbidoul
Copy link
Member

sbidoul commented Apr 30, 2024

I think requirements.txt and test_requirements.txt should be excluded from the algorithm.

This makes sense.

PCatinean
PCatinean reviewed May 20, 2024
View reviewed changes
auth_saml/security/ir.model.access.csv Outdated
@@ -0,0 +1,6 @@
id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
access_auth_saml_provider,auth_saml_provider,model_auth_saml_provider,base.group_system,1,1,1,1
access_auth_res_users_saml,auth_res_users_saml,model_res_users_saml,base.group_system,1,1,1,1
Copy link

Choose a reason for hiding this comment

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

Wanted to point this out in the previous versions but shouldn't we set this to base.group_erp_manager?

Setting up the the saml provider is definitely for base.group_system but managing the user entries should be done by "Access Rights" group imo

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, #652 need to ported to 17.0.

@gurneyalex gurneyalex added this to the 17.0 milestone May 21, 2024
@gurneyalex
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 17.0-ocabot-merge-pr-634-by-gurneyalex-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request May 21, 2024
@OCA-git-bot
Merge PR #634 into 17.0
366037d 
Signed-off-by gurneyalex
@OCA-git-bot
Copy link
Contributor

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). 🤖

@OCA-git-bot
Copy link
Contributor

@gurneyalex your merge command was aborted due to failed check(s), which you can inspect on this commit of 17.0-ocabot-merge-pr-634-by-gurneyalex-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@vincent-hatakeyama vincent-hatakeyama mentioned this pull request Jun 12, 2024
Closed
@vincent-hatakeyama
Copy link
Contributor

#667 needs to be cherry picked to fix the tests.

Taking #652 would avoid having to forward port those changes separately too.

@astirpe
Copy link
Member Author

astirpe commented Jun 13, 2024

@vincent-hatakeyama thank you! Would you check if it's all good now?

@bizzappdev
Copy link
Contributor

IMO Commits [MIG] auth_saml: migrate to V17 and [MIG] auth_saml: fix E501 Line too long should be squashed into one.

The code LGTM.

@vincent-hatakeyama
Copy link
Contributor

The tests now pass so that part is fine.

I think the lower test cover can be ignored.

I can’t merge the changes as I do not have the enough rights, but to me it’s ready to be merged in 17.0.

@sweiland57
Copy link

Will this be merged anytime soon ?

@leemannd
Copy link
Contributor

Hello, the migration looks good. Usually we are also requested to squash administrative/translation commits

Ivorra78 and others added 12 commits June 26, 2024 19:21
@Ivorra78 @astirpe
Added translation using Weblate (Spanish)
eeb799e 
Translated using Weblate (Spanish)

Currently translated at 100.0% (90 of 90 strings)

Translation: server-auth-16.0/server-auth-16.0-auth_saml
Translate-URL: https://translation.odoo-community.org/projects/server-auth-16-0/server-auth-16-0-auth_saml/es/
@OCA-git-bot @astirpe
[UPD] README.rst
65a6d70
@weblate @astirpe
Update translation files
281955a 
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: server-auth-16.0/server-auth-16.0-auth_saml
Translate-URL: https://translation.odoo-community.org/projects/server-auth-16-0/server-auth-16-0-auth_saml/
@tb-flyt @astirpe
Small bugfix, RelayState can be empty
ec00146
@OCA-git-bot @astirpe
[BOT] post-merge updates
4379dc2
@mymage @astirpe
Added translation using Weblate (Italian)
078340c 
Translated using Weblate (Italian)

Currently translated at 85.5% (77 of 90 strings)

Translation: server-auth-16.0/server-auth-16.0-auth_saml
Translate-URL: https://translation.odoo-community.org/projects/server-auth-16-0/server-auth-16-0-auth_saml/it/

Translated using Weblate (Italian)

Currently translated at 100.0% (90 of 90 strings)

Translation: server-auth-16.0/server-auth-16.0-auth_saml
Translate-URL: https://translation.odoo-community.org/projects/server-auth-16-0/server-auth-16-0-auth_saml/it/
@leemannd @astirpe
FIX auth_saml: password reset when deactivating the config settings
d410ac1
@OCA-git-bot @astirpe
[BOT] post-merge updates
d1c36b4
@astirpe
[MIG] auth_saml: pre-commit
70aed43
@astirpe
[MIG] auth_saml: migrate to V17
306979e
@vincent-hatakeyama @astirpe
[FIX] auth_saml: update signin method
f5aa0d4 
Updated the signin method to reflect changes in similar method signin
from auth_oauth.
Without the changes, the ORM crashes with
psycopg2.errors.InvalidSavepointSpecification when trying to signin.

Fixes OCA#664
@aronabencherif @astirpe
[IMP] auth_saml: Allow admin/managers to edit User SAML information
440b787 
As user in that group can already edit users, so it make sense to allow
them to see and edit that information rather than restrict it to
admin/system.
@astirpe
Copy link
Member Author

astirpe commented Jun 26, 2024

Commits squashed. Thanks!

@PCatinean
Copy link

@gurneyalex could you please help us once again with a merge command?

@oussjarrousse
Copy link

I would like to push this merge request above the finish line... It seems that just more test coverage is required at the moment. I am not familiar with Odoo testing. How do you suggest I contribute? Where should I start?

@oussjarrousse
Copy link

Hello @astirpe. Any help I can provide?

@astirpe
Copy link
Member Author

astirpe commented Aug 2, 2024

Can this one be merged?

@oussjarrousse thank you for asking, feel free to contribute in whatever you like!

@oussjarrousse
Copy link

oussjarrousse commented Aug 4, 2024 •
edited
Loading

@astirpe

Alright, It's the first time I write test code for Odoo, so I am still setting up the dev environment and figuring out where things are. If you have a good guide on that. I would appreciate it. I will keep you updated.

@oussjarrousse
Copy link

@astirpe

FYI, I managed to I setup the Dev Environment. I also managed to run all tests and test coverage locally 💪.
I will add tests to improve coverage and will create a pull request to your fork when I am ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincent-hatakeyama vincent-hatakeyama vincent-hatakeyama approved these changes

@gurneyalex gurneyalex gurneyalex approved these changes

@leemannd leemannd leemannd approved these changes

@PCatinean PCatinean PCatinean approved these changes

Assignees
No one assigned
Labels
approved ready to merge
Projects
None yet
Milestone
17.0
Development

Successfully merging this pull request may close these issues.

None yet