[19.0][IMP] base_tier_validation: warn at definition save if reviewer lacks model ACL#31
Open
bosd wants to merge 3 commits into
Open
Conversation
… model ACL Make misconfigured tier definitions visible at config time instead of silently producing reviews nobody can act on. When the admin sets a reviewer (individual user or group) that has no ir.model.access read on the tier's target model, an onchange warning explains the problem and names the affected user(s) -- but does not block the save. Legitimate workflows (the user is about to be added to the group, ir.rules grant per-record access, ...) still go through. Skipped for review_type='field': the reviewer only resolves at validation time from a field on the document, so we can't check it in advance. The chatter notification at request_validation time (separate PR) catches that case. The helper deliberately checks only model-level ACL: per-record ir.rule restrictions can grant or revoke access at runtime, so the warning is worded as "may not be able to read" rather than asserting a definite block. False positives are preferable to silent failure. Tests cover all three branches (individual, group, field).
Contributor
|
Hi @LoisRForgeFlow, |
OCA-git-bot
added
mod:base_tier_validation
added 2 commits
May 14, 2026 09:27
Two issues in the onchange tests: - Test setup unlinked the public tier.validation.tester ACL after the .new() definition had already been built and the onchange had already called check_access once. The ir.model.access ormcache retained the earlier "allowed" decision, so the second onchange call still saw the reviewer as having access. Revoke the ACL before constructing the .new() record, and call ir.model.access.call_cache_clearing_methods() to be sure subsequent check_access invocations re-evaluate. Extract to a helper used by all three onchange tests so they exercise the same path. - On .new() records, the stored related ``self.model`` field can still be empty depending on cache state, which made the onchange silently return None in the failing CI environment. Read the model name directly off ``self.model_id.model`` (the m2o navigation) so it's always populated when ``model_id`` has been set.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Catch the "invalid tier definition" misconfiguration at config time instead of letting it fail silently later.
When an admin sets a reviewer (individual user or group) that has no
ir.model.accessread on the tier's target model, anonchangewarning explains the problem and names the affected user(s). The warning is non-blocking — the record can still be saved. Legitimate workflows (admin is about to add the user to the group,ir.rulegrants per-record access on demand, multi-company quirks) keep working; the warning just makes it impossible to misconfigure this silently.Scope
individualreviewer_idgroupreviewer_group_id.user_idsfieldrequest_validationtime (separate PR) catches that case.Tradeoff (worth calling out for review)
check_access('read')is a model-level check. Per-recordir.rulecan grant or revoke access at runtime. So:ir.rulewould have allowed access on specific records → admin sees the warning and ignores it. Fine; non-blocking.ir.ruleblocks access on some records → still silently broken for those. The chatter PR catches this.The message is worded "may not be able to read" on purpose. Better than silent failure.
Test plan
Three tests cover the three branches:
test_definition_onchange_warns_when_reviewer_lacks_access(individual)test_definition_onchange_warns_when_group_member_lacks_access(group)test_definition_onchange_skips_field_review_type(field — no warning)Each unlinks the tester model's
ir.model.access, builds atier.definition.new(...)with the relevant reviewer config, and asserts the onchange returns / does not return a warning dict.Follow-up
A separate PR will add a chatter notification on
request_validationfor the cases this onchange can't cover (fieldreviewer, dynamic group membership that changed after the definition was saved,ir.rule-only restrictions).