[16.0][IMP] choose between brand or company values in layout

[Kev-Roche]

Inspired of #180 and could replace it.
Here a way to select and use brand fields on reports and avoid the missing field error if a module add a new field on res.company

[OCA-git-bot]

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

[bosd]

Functional test! :+1
Thanks for this fix!

[bosd]

Actually some fields are still not overridden.

'report_header'
'company_details'
'report_layout_id'

To elaborate.
The error is gone. A report is generated. But it is rather a mixture of the report set on the company level and the brand level.

The colors are a mixture of both settings.
The tag line/header is not filled with the "brand" one.
Background image not working.

The adress still show the company adress. Instead of the adress of the Brand.

[bosd]

Oopsie part of this bug is actually fixed in #195

[metaminux]

Hi @bosd

Can you mark this as resolved to allow merging again ?

@OCA/brand-maintainers Can someone approve this PR ? Please...

[bosd]

Hi @bosd

Can you mark this as resolved to allow merging again ?

@OCA/brand-maintainers Can someone approve this PR ? Please...

Yeah, I could do that. But it is actually not resolved. So maybe better fix the functionality before merging.

[sbejaoui]

Apologies for the delayed review; I’ve been quite busy lately.

I understand the issue we’re trying to address here, but I have some concerns about the user experience.

IMO, we should continue adding fields to the brand model and let it evolve in line with how odoo evolves the company model. The purpose of this repository is to make the layout branded, and this solution may prevent us from noticing new fields added in each version that could break the layout.

For instance, in v16, the field is_company_detail_empty was introduced. With the proposed solution, this field would return True if it’s empty in the company object, even if the user filled it at the brand level. This issue would go unnoticed, and users might not understand why the brand details are not visible.

To summarize, I suggest keeping things as they are. During each migration, we can easily identify the layout changes introduced by odoo as they will inevitably breaks the layout, allowing us to address them appropriately.

[Kev-Roche]

Apologies for the delayed review; I’ve been quite busy lately.

I understand the issue we’re trying to address here, but I have some concerns about the user experience.

IMO, we should continue adding fields to the brand model and let it evolve in line with how odoo evolves the company model. The purpose of this repository is to make the layout branded, and this solution may prevent us from noticing new fields added in each version that could break the layout.

For instance, in v16, the field is_company_detail_empty was introduced. With the proposed solution, this field would return True if it’s empty in the company object, even if the user filled it at the brand level. This issue would go unnoticed, and users might not understand why the brand details are not visible.

To summarize, I suggest keeping things as they are. During each migration, we can easily identify the layout changes introduced by odoo as they will inevitably breaks the layout, allowing us to address them appropriately.

Thank you for taking the time to review this. I appreciate your feedback and would like to address your concerns.

• Conflict with evolving layouts:
  The issue isn’t limited to migrations. Whenever new fields are added to the company model, via custom or OCA modules, they can conflict with the layout if not anticipated. For example, in the Cooperative module, added fields could create conflicts or unexpected behaviors. The current approach requires manual adjustments for each new field, whereas the present PR minimizes such conflicts and ensures smoother integration.

• Ease of maintenance:
  My approach simplifies maintenance by centralizing field handling at the brand level. This avoids repetitive manual adjustments and ensures consistency across layouts, even when new fields are introduced.

To summarize, this PR proactively addresses conflicts during both migrations and modules installation, reducing bugs and maintenance effort while providing a more robust and user-friendly experience.

[bosd]

@Kev-Roche Can you (or ocabot) please rebase, So we can test it against the latest code on a fresh runboat?

[bosd]

@OCA-git-bot I know we're not friends... But here goes nothing..

/ocabot rebase

[OCA-git-bot]

Sorry @bosd you are not allowed to rebase.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

[Kev-Roche]

@Kev-Roche Can you (or ocabot) please rebase, So we can test it against the latest code on a fresh runboat?

Done !

[bosd]

Thanks, To keep you guys no longer waiting. here is my initial response.
But might need more time for thorough testing.

This following fields are still not made brand specific.

'company_details'
'report_layout_id'

Later i will test more.

Thinking out loud. Would it be possible to just inherit the whole company model with all of its fields?

[bosd]

Functional test on local system in combination with account brand.
It's not working :(

The only branded item changed in the report was the logo.
Other fields stayed the same.

Steps to reproduce. install account_brand set brand policy to optional
Create a brand.
Usability improvement suggestion: Assign a default layout, as Empty is not allowed.

Details

Configure branded document.

For completeness my test company settings:

Details

Create a unbranded invoice:

branded invoice:

Result:
🟢 Logo, Layout and document colors changed. Fonts
🔴 Header and footer not added.
🔴 Company details incorrect

untested: paperformat

[bosd]

Suggested change

	"layout_background_image",
	"layout_background_image",
	"company_details",

This is fixes one of the not overriden fields.

[metaminux]

Hi @bosd, @sbejaoui and @Kev-Roche

This PR is a replacement for #180 which was opened to fix #167 and #179.
The latter is about using this module in a multi-company setup.

Odoo native behavior is about configuring a report layout for each company.
This module is about using brands to define report layouts, but I don't think every piece of information should be overridden...

To add some context, one of my customer is a corporate group that is manufacturing and selling products mainly in France, UK, USA and Canada. Those products are sold under 4 different brands. So, there are 4 brands used in 5 companies.
They want to have a common style for each brand in all companies.
In those conditions, we just want to use the "style" fields from brands and keep the others from each companies. That's why we choose to override only a subset of fields.
For instance, we do want the footer to be different and related to the actual company for a same brand. Addresses should also refer to companies, not brands...

Now, I'm aware this subset could seem arbitrary, as already explained in comments of the original PR (see #180 (comment) and further comments)
The original PR was replacing "at rendering time" the company fields by the brand ones, so every field overridden had to be defined in brands and there were no way to use information from actual company.
This one is way "smarter" (thanks to @Kev-Roche) because if no value is set at brand level, we use the company value.
Maybe this is a bit disturbing at first sight and should be made more visible to end users... but I think this could allow to use brand report layout in multi-company environment and avoid conflicts with other modules.

I hope this comment will help going forward on that PR and stay open to all your thoughts and advice.

[bosd]

@metaminux Aha,
I see we have different use cases.
Thanks for the feedback on your use case.

We are using it for manufacturers / subcontractors. They have one facility and produce for different brands / entity.
They want to keep their setup simple and flat. Because a multi company setup would create intercompany (warehouse) transactions.
Instead we use the module to sent out branded invoices. (accounting is not handled by odoo in most cases).
Each brand will have their own logo and company details, header footer and such.

This one is way "smarter" (thanks to @Kev-Roche) because if no value is set at brand level, we use the company value.

Yes, That would allow usage for both of our uses cases.

Maybe this is a bit disturbing at first sight and should be made more visible to end users... but I think this could allow to use brand report layout in multi-company environment and avoid conflicts with other modules.

It would be a good addition to update the docs with this new functionality.

    def _get_company_overriden_fields(self):
        return {

Here the subset of fields to override is strict.
As discussed, this could lead to issues on maintainability.
Would it be an option to just check on all available field?
IMO, it would make it easier to migration as well.

https://github.com/OCA/brand/pull/180/files#diff-af7160d79a4a7e74dfb1e70cd2d4f512bd7b5c5e2ffe73c4e05fb182c77bee2dR109-R121

In it's current form, with only the minor subset included. It is changing the behaviour of the module.
E.g. breaking our use case, which we had deployed at a handfull of clients for couple of years on different versions.

[christian-ramos-tecnativa]

Hello @Kev-Roche, as @bosd said, IMO retrieving the company values dynamically should cover every use case. Could you apply the change to test it? Thanks in advance

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[metaminux]

Hi everyone,

To avoid this PR being closed...

I'll try to add missing fields to _get_company_overriden_fields so that @bosd use case explained earlier is OK
We'll override the method in our customer specific module to fit our use case.

Regards,

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[bosd]

To avoid this PR being closed...

1 day left for auto closing. If i did not reply today :)