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.

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

[FIX] [16.0] project_administrator_restricted_visibility: Restricted Project Manager always can manage its own projects #1393

Conversation

Shide
Copy link

@Shide Shide commented Dec 16, 2024

If you change default visibility to "Invited internal users", Project Administrator with restricted access cannot create projects.

This fix adds a new rule to allow them to create projects always (only create).

Once the project is created, the creator of the projects adds automatically to followers and other rules are applied.

MT-7868 @moduon @rafaelbn @Gelojr @fcvalgar @yajo @ernestotejeda please review if you want 😄

@OCA-git-bot
Copy link
Contributor

Hi @edlopen, @rafaelbn,
some modules you are maintaining are being modified, check this out!

@pedrobaeza pedrobaeza added this to the 16.0 milestone Dec 16, 2024
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

I don't think this is a good idea, as they should only create projects with the corresponding allowed permissions. Adding a wildcard is not the option.

@Shide
Copy link
Author

Shide commented Dec 16, 2024

But it's not a wildcard, default visibility should be freely updated at anyone what the customer wants without an impact on any role. When the project is created, the first that Odoo does is to put the creation of the project on the followers (and then the other rules are applied).
This doesn't change anything on the normal flow (except this corner case) and permissions

@pedrobaeza
Copy link
Member

Yes, but it should be updated by a manager, not a restricted visibility one.

@Shide
Copy link
Author

Shide commented Dec 17, 2024

But the goal of this module is to make that project admins (that can create projects in any scenario), could do the same things as always, but see only projects that are followers with visibility of internal users (the other visibility options gives internal users all options).

Why Restricted Administrator cannot create projects? This role will be almost the same as Internal User.

There is no specific rule for visibility = followers nor check the user is the project manager.
image

Other rule options

  • [('user_id', '=', user.id)]: The user that are creating the project will be the project responsible.
  • [('privacy_visibility', '=', 'followers'), ('message_partner_ids', 'in', [user.partner_id.id])]: Visibility followers with the user as follower (the responsible in the creation must be the creation user)

Unsolvable issue (even with the 1=1 rule)

If defaults are:

  • Visibility = internal users
  • Project Responsible = another user

Raises the same error and there is no way to handle this because the follower is the responsible at the first time.

Recap

  • The [(1, '=', 1)] on Create rule does the same as the other option rules.
  • The [(1, '=', 1)] rule it's simple to maintain and it's very clear (means full permissions to create projects and that's all)
  • The edge case where the responsible of the project are set by default to an specific user is unsolvable in any case. (This scenario means that only full administrators can create projects, no matter what). With this set-up of defaults, you could give the ability to create projects only to full administrators.

@Shide Shide marked this pull request as draft December 18, 2024 09:01
@yajo
Copy link
Member

yajo commented Dec 18, 2024

  • [('user_id', '=', user.id)]: The user that are creating the project will be the project responsible.

This rule is better because it's more UX-consistent than the current one. It can be applied to all CRUD operations, so it will make sure that a project you created you can read it later. Otherwise, the restricted admin could create a project but then never read it, resulting in a false sense of it not having been created.

@Shide Shide force-pushed the 16.0-fix_project_manager_create_visibility-project_administrator_restricted_visibility branch from bf5fa7a to a03329a Compare December 19, 2024 07:12
@Shide Shide changed the title [FIX] [16.0] project_administrator_restricted_visibility: Project Manager always can create projects [FIX] [16.0] project_administrator_restricted_visibility: Restricted Project Manager always can manage its own projects Dec 19, 2024
@Shide Shide marked this pull request as ready for review December 19, 2024 07:23
@Shide Shide requested a review from pedrobaeza December 19, 2024 07:23
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

This looks much better now. I still found one issue though.

@Shide Shide force-pushed the 16.0-fix_project_manager_create_visibility-project_administrator_restricted_visibility branch from a03329a to f2c5014 Compare December 19, 2024 08:00
@Shide Shide requested a review from yajo December 19, 2024 08:02
Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

👍🏼

@pedrobaeza I'm going to merge after all comments solved, OK?

@pedrobaeza
Copy link
Member

Please wait till my confirmation, as I want to make sure this doesn't provoke any problem in our use cases.

@pedrobaeza
Copy link
Member

@victoralmau can you please check the impact in our customers?

@victoralmau
Copy link
Member

Reviewing the whole thread I think the same as pedro, I don't like this solution because without this module (standard) this is a problem that already happens and would be solved in another way (create it with other visibility and changing it later for example) and therefore, you are changing the behavior taking advantage of this permission.

Having clarified the above, I BELIEVE that it will not have any side effect and I will not oppose it either.

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

@yajo
Copy link
Member

yajo commented Dec 27, 2024

I hope this video lets you understand better why we need this fix and why we think it fits with this modules' users expectations:

Fixing.Project.Visibility.Issues.in.Pull.Request.mp4

I BELIEVE that it will not have any side effect and I will not oppose it either.

So, do you agree to merge?

Thanks.

@rafaelbn
Copy link
Member

Hello @victoralmau ! Could you please kindly review the video of @yajo after you comment? 😄

Thank you! ❤️ and Happy new year! 🥳

@victoralmau
Copy link
Member

Hello again,

Thanks for the explanation video, I understand (and understood) what was happening, and that is why I do not oppose this change and I think it will not have any side effect. Currently it would be possible to create a project with the visibility “Invited portal users and all internal users”, become a follower of that project and then change the visibility to “Invited internal users” (indeed it could not be done currently with the Restricted Administrator permission because it is intentional).

Best regards.

@Shide
Copy link
Author

Shide commented Jan 8, 2025

So, taking in consideration the @victoralmau response, @pedrobaeza can you approve this PR?

@rafaelbn
Copy link
Member

rafaelbn commented Jan 8, 2025

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1393-by-rafaelbn-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 87c86f2 into OCA:16.0 Jan 8, 2025
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@Shide Shide deleted the 16.0-fix_project_manager_create_visibility-project_administrator_restricted_visibility branch January 8, 2025 12:43
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.

7 participants