Skip to content

[T-7772][15.0][ADD] model_access_restriction #24

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
Closed
wants to merge 2 commits into from
Closed

[T-7772][15.0][ADD] model_access_restriction #24

wants to merge 2 commits into from

Conversation

Tisho99
Copy link
Contributor

@Tisho99 Tisho99 commented Apr 10, 2025

No description provided.

@codecov Codecov
Copy link

codecov bot commented Apr 10, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.61017% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
..._restriction/models/ir_model_access_restriction.py 93.33% 1 Missing and 1 partial ⚠️
Files with missing lines Coverage Δ
model_access_restriction/__init__.py 100.00% <100.00%> (ø)
model_access_restriction/models/__init__.py 100.00% <100.00%> (ø)
model_access_restriction/models/models.py 100.00% <100.00%> (ø)
model_access_restriction/tests/__init__.py 100.00% <100.00%> (ø)
...restriction/tests/test_model_access_restriction.py 100.00% <100.00%> (ø)
..._restriction/models/ir_model_access_restriction.py 93.33% <93.33%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Tisho99 Tisho99 force-pushed the 15.0-T-7772 branch 2 times, most recently from 29b695f to abb5695 Compare April 10, 2025 08:15
@Tisho99
Copy link
Contributor Author

Tisho99 commented Apr 10, 2025

@manuel-florido Can you review this pre-commit error? It seems to be a configuration issue

@manuel-florido
Copy link
Contributor

Solved, please rebase 15.0

@Tisho99 Tisho99 force-pushed the 15.0-T-7772 branch 3 times, most recently from dd3c315 to 1213793 Compare April 10, 2025 09:59
sara-castello
sara-castello approved these changes Apr 14, 2025
View reviewed changes
Copy link

@sara-castello sara-castello left a comment

Choose a reason for hiding this comment

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

Functional review LGTM

@HaraldPanten
Copy link

@manuelregidor This module is ready for the technical review

manuelregidor
manuelregidor requested changes Apr 15, 2025
View reviewed changes
manuelregidor
manuelregidor requested changes Apr 15, 2025
View reviewed changes
@Tisho99 Tisho99 force-pushed the 15.0-T-7772 branch 2 times, most recently from 733dd50 to 5b24a39 Compare April 15, 2025 09:07
@Tisho99 Tisho99 requested a review from manuelregidor April 15, 2025 14:12
manuelregidor
manuelregidor approved these changes Apr 15, 2025
View reviewed changes
Copy link

@manuelregidor manuelregidor left a comment

Choose a reason for hiding this comment

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

Technical review. LGTM

@Tisho99
Copy link
Contributor Author

Tisho99 commented Apr 15, 2025

@ValentinVinagre Can you review?

@Sygel-bot
Copy link
Contributor

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

ValentinVinagre
ValentinVinagre reviewed Apr 17, 2025
View reviewed changes
Copy link
Contributor

@ValentinVinagre ValentinVinagre left a comment

Choose a reason for hiding this comment

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

Odoo indicates which groups you have to join if you don't have permission to perform an action. Has this been taken into account in this module?

@Tisho99 Tisho99 force-pushed the 15.0-T-7772 branch 2 times, most recently from f5672a1 to 147b705 Compare April 17, 2025 15:42
@Tisho99
Copy link
Contributor Author

Tisho99 commented Apr 17, 2025

@ValentinVinagre Can you review again?

I have applied all your suggestions

ValentinVinagre
ValentinVinagre requested changes Apr 23, 2025
View reviewed changes
Copy link
Contributor

@ValentinVinagre ValentinVinagre left a comment

Choose a reason for hiding this comment

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

I would like the missing permits to be implemented, let's give it a little more time and have them done.

model_access_restriction/models/ir_model_access_restriction.py
@api.constrains("groups")
def _check_groups(self):
if self.filtered(lambda r: not r.groups):
raise exceptions.ValidationError(
Copy link
Contributor

Choose a reason for hiding this comment

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

add to tests

model_access_restriction/tests/test_model_access_restriction.py
"groups": [(4, cls.group.id)],
}
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Add 2 tests to handle the 2 uncovered permissions "perm_write" and "perm_read".

model_access_restriction/tests/test_model_access_restriction.py
from odoo.tests.common import TransactionCase


class TestModelAccessRestriction(TransactionCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite understand the tests. Can you provide a brief explanation?
A group and a restriction model are created... but:
What are the base permissions?
Which user is being tested?
This would require further explanation. New users would need to be created to control the permissions associated with them, not the ones created by the system, as these can vary depending on the Odoo version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tested user is the root one, the base permissions are all

Which is being tested is if adding a new access restriction disables or not the permissions, assuming that the user has permissions without taking the restrictions into account

The default permissions won't change between versions because we are using the root user, the restrictions won't change because there is no one at installation

But anyway I'm going to review the tests to see what can be improved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ValentinVinagre

@manuelregidor
Copy link

@ValentinVinagre Reading and writing rights can be developed in a second iteration, as a new version of the module, as it is not part of the customer's requirements.

@ValentinVinagre
Copy link
Contributor

@ValentinVinagre Reading and writing rights can be developed in a second iteration, as a new version of the module, as it is not part of the customer's requirements.

I understand the point, I would like the module to be completed.

@Tisho99
Copy link
Contributor Author

Tisho99 commented Apr 23, 2025

Moved to oca: OCA/server-backend#352

@Tisho99 Tisho99 closed this Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ValentinVinagre ValentinVinagre ValentinVinagre requested changes

@manuelregidor manuelregidor manuelregidor approved these changes

@sara-castello sara-castello sara-castello approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@Tisho99 @manuel-florido @HaraldPanten @Sygel-bot @manuelregidor @ValentinVinagre @sara-castello