-
-
Notifications
You must be signed in to change notification settings - Fork 304
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
[13.0][FIX] Compatibility with multicompany #327
[13.0][FIX] Compatibility with multicompany #327
Conversation
Hi @sbidoul, |
0894222
to
1fe2deb
Compare
@@ -4,7 +4,7 @@ | |||
<field name="name">Mis Report Instance multi company</field> | |||
<field name="model_id" ref="model_mis_report_instance" /> | |||
<field name="domain_force"> | |||
['|',('company_id','=',False),('company_id','in',company_ids)] | |||
['|',('company_id','=',False),('company_id','in',company_ids), '|', ('company_ids', '=', False), ('company_ids', 'in', company_ids)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
['|',('company_id','=',False),('company_id','in',company_ids), '|', ('company_ids', '=', False), ('company_ids', 'in', company_ids)] | |
['&', '|',('company_id','=',False),('company_id','in',company_ids), '|', ('company_ids', '=', False), ('company_ids', 'in', company_ids)] |
and run pre-commit
- The mis.report.instance should allow to define as many companies as the user is allowed to operate on when logged. - The company is only a required field if the flag 'multi_company' is not set.
1fe2deb
to
3072118
Compare
…tor and fix tests
bc9f39e
to
a208aed
Compare
@sbidoul What do you think? does this make sense to you? |
Thanks a lot for this! I'll look into it as soon as possible. |
@sbidoul Any chance you can consider this changes? Thanks! |
@LoisRForgeFlow yeah I absolutely need to find an hour to focus on this. I'll keep it on the topmost part of my stack. |
@sbidoul Hi, just a friendly reminder. It would be great if you can give your opinion on this! |
I've reserved some time for MIS builder maintenance mid august. I'll get to it then. Sorry again for the delay. |
self.company_ids = False | ||
|
||
@api.depends("multi_company", "company_id", "company_ids") | ||
def _compute_query_company_ids(self): | ||
for rec in self: | ||
if rec.multi_company: | ||
rec.query_company_ids = rec.company_ids or rec.company_id | ||
# If no companies defined use active companies. | ||
rec.query_company_ids = rec.company_ids or self.env.companies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better here to do the intersection of rec.company_ids and self.env.companies. Because in the report we print the list of companies used in the report and we should not print companies to which the user does not have access and therefore ignored by the record rules.
@@ -163,6 +163,7 @@ | |||
<field | |||
name="company_id" | |||
groups="base.group_multi_company" | |||
attrs="{'required': [('multi_company', '=', False)]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, invisible if multi_company is True ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a better UX, let's take the opportunity to move the multi_company checkbox before the company_id field.
) | ||
multi_company = fields.Boolean( | ||
string="Multiple companies", | ||
help="Check if you wish to specify " | ||
"children companies to be searched for data.", | ||
"several companies to be searched for data.", | ||
default=False, | ||
) | ||
company_ids = fields.Many2many( | ||
comodel_name="res.company", | ||
string="Companies", | ||
help="Select companies for which data will be searched.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
help="Select companies for which data will be searched.", | |
help="Select companies for which data will be searched (search all companies if empty).", |
default=False, | ||
) | ||
company_ids = fields.Many2many( | ||
comodel_name="res.company", | ||
string="Companies", | ||
help="Select companies for which data will be searched.", | ||
domain=lambda self: [("id", "in", self.env.companies.ids)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hesitate about this domain.
This domain hides companies that are not in the user context. In a way this is good because this is what the report will filter on. But OTOH it would be good to show the user that the report can work also on other companies if they select them in their context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove this domain and add query_company_ids
to the view as Effective companies
.
So I implemented my suggestions in #374 - apologies again for the late review. |
Superseded by #374 |
companies as the user is allowed to operate on when logged.