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

[15.0][FIX] base_tier_validation:Tier definition #917

Open
wants to merge 1 commit into
base: 15.0
Choose a base branch
from
Open

[15.0][FIX] base_tier_validation:Tier definition #917

wants to merge 1 commit into from

Conversation

Pani-k-folk
Copy link

@Pani-k-folk Pani-k-folk commented Jul 8, 2024 •
edited
Loading

Fix: Modify list of specific users to show only those who belong to that particular company

@OCA-git-bot
Copy link
Contributor

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

@Pani-k-folk Pani-k-folk changed the title [15.0][ Fix ] base tier validation tier definition [15.0][ Fix ] base_tier_validation : tier definition Jul 8, 2024
@Pani-k-folk Pani-k-folk changed the title [15.0][ Fix ] base_tier_validation : tier definition [15.0][ Fix ] base_tier_validation : Tier definition Jul 8, 2024
@Pani-k-folk Pani-k-folk force-pushed the 15.0-fix-base_tier_validation-tier_definition branch from 6a33ab7 to e2e81ed Compare July 8, 2024 11:50
@Pani-k-folk Pani-k-folk changed the title [15.0][ Fix ] base_tier_validation : Tier definition [15.0][Fix] base_tier_validation : Tier definition Jul 8, 2024
@Pani-k-folk Pani-k-folk changed the title [15.0][Fix] base_tier_validation : Tier definition [15.0][Fix] base_tier_validation:Tier definition Jul 8, 2024
@Pani-k-folk Pani-k-folk changed the title [15.0][Fix] base_tier_validation:Tier definition [15.0][FIX] base_tier_validation:Tier definition Jul 8, 2024
@Pani-k-folk
[UPD] Tier_definition
05c0afd 
[UPD] tier_definition

[UPD] tier_definition

[UPD] tier_definition
@Pani-k-folk Pani-k-folk force-pushed the 15.0-fix-base_tier_validation-tier_definition branch from e2e81ed to 05c0afd Compare July 8, 2024 12:04
@Pani-k-folk
Copy link
Author

Hi @LoisRForgeFlow, Can you review this. This is the first OCA that I've attempted to modify. Hopefully, it will be beneficial to everyone. ( = w=)

FrancoMaxime
FrancoMaxime approved these changes Jul 15, 2024
View reviewed changes
Copy link
Member

@FrancoMaxime FrancoMaxime left a comment

Choose a reason for hiding this comment

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

LGTM: code review

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

LoisRForgeFlow
LoisRForgeFlow requested changes Jul 22, 2024
View reviewed changes
Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

Can you fix your commit message to follow the guidelines:

[TAG] module_name: short description of change

LoisRForgeFlow
LoisRForgeFlow requested changes Jul 22, 2024
View reviewed changes
Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The fix is also not correct, look at this example in runbot:

image

image

The check company is based in the company_id field that for users is not like in many other models, in users is just the default company, the companies that can be acceses are multiple in a user (company_ids). Thefore if you add the check_company you cannot create a tier definition in company B for a user with access in both A and B company.

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

@LoisRForgeFlow LoisRForgeFlow LoisRForgeFlow requested changes

@newtratip newtratip newtratip approved these changes

@FrancoMaxime FrancoMaxime FrancoMaxime approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Pani-k-folk @OCA-git-bot @LoisRForgeFlow @newtratip @FrancoMaxime