[13.0][FIX] Compatibility with multicompany #327

JordiBForgeFlow · 2020-10-27T09:37:24Z

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.

OCA-git-bot · 2020-10-27T09:38:03Z

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

MiquelRForgeFlow · 2020-10-27T11:22:28Z

mis_builder/security/mis_builder_security.xml

@@ -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)]


Suggested change

['|',('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.

…tor and fix tests

LoisRForgeFlow · 2021-01-14T14:28:08Z

To ease the review, I will showcase a bit the changes:

When the companies field is empty, the same report can be used for all companies, and you just need to activate more companies if you want to aggregate:
Base report instance:

If we open this report with only one company active we get the result for each one:

Then we can activate the second and third company to get the aggregated result:

Regarding _onchange_company the new behavior can be observer in the gif below:

LoisRForgeFlow · 2021-01-14T14:30:46Z

@sbidoul What do you think? does this make sense to you?

sbidoul · 2021-01-14T14:46:23Z

To ease the review, I will showcase a bit the changes:

Thanks a lot for this! I'll look into it as soon as possible.

LoisRForgeFlow · 2021-04-15T11:12:07Z

@sbidoul Any chance you can consider this changes? Thanks!

sbidoul · 2021-04-16T06:11:11Z

@LoisRForgeFlow yeah I absolutely need to find an hour to focus on this. I'll keep it on the topmost part of my stack.

LoisRForgeFlow · 2021-07-22T07:31:03Z

@sbidoul Hi, just a friendly reminder. It would be great if you can give your opinion on this!

sbidoul · 2021-07-23T07:32:27Z

I've reserved some time for MIS builder maintenance mid august. I'll get to it then. Sorry again for the delay.

sbidoul · 2021-08-13T14:25:14Z

mis_builder/models/mis_report_instance.py

            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


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.

sbidoul · 2021-08-13T14:27:54Z

mis_builder/views/mis_report_instance.xml

@@ -163,6 +163,7 @@
                                <field
                                    name="company_id"
                                    groups="base.group_multi_company"
+                                    attrs="{'required': [('multi_company', '=', False)]}"


Also, invisible if multi_company is True ?

For a better UX, let's take the opportunity to move the multi_company checkbox before the company_id field.

mis_builder/models/mis_report_instance.py

sbidoul · 2021-08-13T14:55:08Z

mis_builder/models/mis_report_instance.py

    )
    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.",


Suggested change

help="Select companies for which data will be searched.",

help="Select companies for which data will be searched (search all companies if empty).",

sbidoul · 2021-08-13T14:59:38Z

mis_builder/models/mis_report_instance.py

        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)],


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.

I'll remove this domain and add query_company_ids to the view as Effective companies.

sbidoul · 2021-08-13T16:44:06Z

So I implemented my suggestions in #374 - apologies again for the late review.

sbidoul · 2021-09-30T15:44:52Z

Superseded by #374

JordiBForgeFlow force-pushed the 13.0-mis_builder-fix-multicompany branch 2 times, most recently from 0894222 to 1fe2deb Compare October 27, 2020 11:13

MiquelRForgeFlow reviewed Oct 27, 2020

View reviewed changes

[13.0][FIX] Compatibility with multicompany

3072118

- 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.

LoisRForgeFlow force-pushed the 13.0-mis_builder-fix-multicompany branch from 1fe2deb to 3072118 Compare January 14, 2021 08:23

sbidoul added the 13.0 label Jan 14, 2021

[13.0][FIX] mis_builder: improve UX using new v13 multi-company selec…

a208aed

…tor and fix tests

LoisRForgeFlow force-pushed the 13.0-mis_builder-fix-multicompany branch from bc9f39e to a208aed Compare January 14, 2021 14:26

sbidoul mentioned this pull request Aug 13, 2021

MIS builder 4.0 #371

Closed

3 tasks

sbidoul reviewed Aug 13, 2021

View reviewed changes

mis_builder/models/mis_report_instance.py Show resolved Hide resolved

sbidoul reviewed Aug 13, 2021

View reviewed changes

This was referenced Aug 13, 2021

[13.0] multi company improvements #374

Merged

Call for maintainer not working as expected OCA/oca-github-bot#153

Closed

sbidoul added this to the 4.0 milestone Aug 14, 2021

sbidoul closed this Sep 30, 2021

LoisRForgeFlow deleted the 13.0-mis_builder-fix-multicompany branch September 30, 2021 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[13.0][FIX] Compatibility with multicompany #327

[13.0][FIX] Compatibility with multicompany #327

JordiBForgeFlow commented Oct 27, 2020

OCA-git-bot commented Oct 27, 2020

MiquelRForgeFlow Oct 27, 2020

LoisRForgeFlow commented Jan 14, 2021

LoisRForgeFlow commented Jan 14, 2021 •

edited

Loading

sbidoul commented Jan 14, 2021

LoisRForgeFlow commented Apr 15, 2021

sbidoul commented Apr 16, 2021

LoisRForgeFlow commented Jul 22, 2021

sbidoul commented Jul 23, 2021

sbidoul Aug 13, 2021

sbidoul Aug 13, 2021

sbidoul Aug 13, 2021

sbidoul Aug 13, 2021

sbidoul Aug 13, 2021

sbidoul Aug 13, 2021

sbidoul commented Aug 13, 2021

sbidoul commented Sep 30, 2021

	['\|',('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)]

	help="Select companies for which data will be searched.",
	help="Select companies for which data will be searched (search all companies if empty).",

[13.0][FIX] Compatibility with multicompany #327

[13.0][FIX] Compatibility with multicompany #327

Conversation

JordiBForgeFlow commented Oct 27, 2020

OCA-git-bot commented Oct 27, 2020

MiquelRForgeFlow Oct 27, 2020

Choose a reason for hiding this comment

LoisRForgeFlow commented Jan 14, 2021

LoisRForgeFlow commented Jan 14, 2021 • edited Loading

sbidoul commented Jan 14, 2021

LoisRForgeFlow commented Apr 15, 2021

sbidoul commented Apr 16, 2021

LoisRForgeFlow commented Jul 22, 2021

sbidoul commented Jul 23, 2021

sbidoul Aug 13, 2021

Choose a reason for hiding this comment

sbidoul Aug 13, 2021

Choose a reason for hiding this comment

sbidoul Aug 13, 2021

Choose a reason for hiding this comment

sbidoul Aug 13, 2021

Choose a reason for hiding this comment

sbidoul Aug 13, 2021

Choose a reason for hiding this comment

sbidoul Aug 13, 2021

Choose a reason for hiding this comment

sbidoul commented Aug 13, 2021

sbidoul commented Sep 30, 2021

LoisRForgeFlow commented Jan 14, 2021 •

edited

Loading