New linter: oe_structure element without specific id

[yajo]

Odoo website builder will replace a view if it contains something like this and the user edits that section:

<div class="oe_structure"/>

However, since Odoo v12, if it is in this other format, instead of creating a COW view that completely replaces the previous one, it will create an extension view that replaces this specific element in the parent view:

<div id="oe_structure_SOME_HOOK_HERE" class="oe_structure"/>

This provides a more resilient upgrade path because your module can "safely"* update the base view which contains logic, while user customizations are kept separately.

IMHO this is a good candidate for a linter.

---

* You know that in Odoo nothing is exactly "safe", but it's better. :shrug:

[moylop260]

Hi @Yajo

What tags should raise an error if an id is not defined?

Should it be consider qweb <template views or only normal ones or both?

[yajo]

What tags should raise an error if an id is not defined?

Usually any HTML block content, such as div, content, etc. However I think you can ignore the tag, because it would make no sense to add a oe_structure class to i.e. a span, i, b, etc., wouldn't even make sense.

So we can assume that if any element has oe_structure, it should have an id with that format. Otherwise you should either add the id or remove the class.

Should it be consider qweb <template views or only normal ones or both?

Usually only on templates, but again I think it's not important because outside of templates it wouldn't make sense anyway. Same logic as above.

[moylop260]

@yajo

I looked for grep -r 'class="oe_structure"' . --include=*.xml -A2 in odoo modules and they are a lot of these cases and the following one only declaring class:

• https://github.com/odoo/odoo/blob/bbf8255bbb758c060ec8f58cd0dc4cc10e14a3aa/addons/event/report/event_event_templates.xml#L55

I'm not totally convinced to do this lint seeing the output

Could you check it, please?

[yajo]

Yes, they do that and it's horrible. 😄

If the div is declared like <div class="oe_structure">, Odoo will let the user modify it. Then it will create a copy of the whole view, including user's modifications. If you update the base view, the modified copy will not be updated. If the base view had any logic, it won't update.

If the div is declared like <div id="oe_structure_SOMETHING" class="oe_structure">, Odoo will also let the user modify it. But then it will create a new view that extends the parent view with an <xpath expr="//div[@id='oe_structure_SOMETHING']">. This is much more maintainable because it lets you update the base view while preserving user's modifications.

Since Odoo always uses this bad practice, it's common to imitate them. That's why the linter would help.

[moylop260]

@xmo-odoo I think this might interest you

[github-actions[bot]]

There hasn't been any activity on this issue in the past 6 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 issue to never become stale, please ask a PSC member to apply the "no stale" label.

[antonag32]

Hello @yajo

Just to verify the behavior you intended:

1. Should this lint only apply to elements with no children?
2. Should the id for this elements always start with oe_structure?

[luisg123v]

Should the id for this elements always start with oe_structure?

Maybe not mandatory, but it's a good idea to follow Odoo's way:

oe_structure_<template_id>_<some_unique_id>

(suggestion taken from @imanie383).

Whether it's mandatory or not, if we agree on a specific format, it should be mentioned in the error message.

I'll reopen this issue to address mentioned points.

[antonag32]

Should the id for this elements always start with oe_structure?

Maybe not mandatory, but it's a good idea to follow Odoo's way:

oe_structure_<template_id>_<some_unique_id>

(suggestion taken from @imanie383).

Whether it's mandatory or not, if we agree on a specific format, it should be mentioned in the error message.

I'll reopen this issue to address mentioned points.

Haven't checked the source code but was wondering if maybe it was necessary for the desired behavior (creating a view that inherits the base view) and any other ids (like <div id="wrap"...) will be ignored and the behavior won't apply to them.

I planned it to leave the format as configurable (with the default being the odoo way), but if the id needs to start with oe_structure that would need to be taken into account.

[antonag32]

Also, is using t-attf-id valid? Or must it be id? :thinking:

[imanie383]

Also, is using t-attf-id valid? Or must it be id? thinking

https://github.com/odoo/odoo/blob/25edcc89273c389f318c434ce9a9e0b531b8e599/addons/web_editor/models/ir_ui_view.py#L41

[antonag32]

Also, is using t-attf-id valid? Or must it be id? thinking

https://github.com/odoo/odoo/blob/25edcc89273c389f318c434ce9a9e0b531b8e599/addons/web_editor/models/ir_ui_view.py#L41

Thanks, that confirms my suspicion. The lint will now ensures all ids contain oe_structure. However I am still not sure about t-attf-id. Is t-attf-id converted to id before the xpath evaluation?

Is this valid, or should the linter complain?

<div class="oe_structure" t-attf-id="oe_structure_blog_single_header_#{blog.id}" />

Also, should we ignore the following special case (anything with id="wrap")?

div class="oe_structure" id="wrap"

[yajo]

Is this valid, or should the linter complain?

<div class="oe_structure" t-attf-id="oe_structure_blog_single_header_#{blog.id}" />

This should make the linter fail anyway because it's a div with class oe_structure and without oe_structure in its id (because t-attf-id isn't enough).

However, that's an invalid example and it makes no sense.

The use case for this is to have a customizable static section in a qweb view.

If you want to have a section customizable per record (not statically), it is better to add a field to the model and just expose it as an HTML-editable t-field. Your example would become:

<div t-field="blog.header"/>

Also, should we ignore the following special case (anything with id="wrap")?

div class="oe_structure" id="wrap"

This one is also not realistic.

If you made such a template, you'd be basically creating an empty page. It makes no sense to do so, because Odoo can already do that by just clicking on add page, so why would you create a module with an empty page?

Even if it had any sense, you could easily solve the problem with:

<div id="wrap">
  <div class="oe_structure" id="oe_structure_body"/>
</div>

... or you could deactivate the linter inline, just like usual (because we can do that, right? 🤔).

[moylop260]

Second part from https://github.com/OCA/odoo-pre-commit-hooks/pull/81