[10.0][IMP][website_multi_theme] make customize_show views part of multi-theme by yelizariev · Pull Request #443 · OCA/website

yelizariev · 2018-04-11T22:08:28Z

The PR allows making customization independently per website

yelizariev

Some explanation of the updates

yelizariev · 2018-04-11T22:09:58Z

+     License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl.html). -->
+<odoo>
+    <!-- Default theme. It's basically a workaround for "Multi-customization"
+         feature which requires theme be set -->


I don't see other solution. At least until we make refactoring discussed #409

yelizariev · 2018-04-11T22:12:20Z

+        string="Copied from",
+        help="View from where this one was copied for multi-website"
+    )
+    copy_ids = fields.One2many(


Those two fields are left after initial draft of this update, but I think it could useful later

yelizariev · 2018-04-11T22:13:37Z

-                xmlid,
-                self.display_name,
-            ),
+            "name": '%s (Multi-Website)' % pattern.name,


It's renamed, because this name is used in Customize menu (grayed text)

Maybe, the name can be changed to '%s (Website #%s)' % (pattern.name, self.id) , because Multi-website sounds like the same per different websites

I don't want to use self.display_name, because website could be renamed

yelizariev · 2018-04-11T22:14:10Z

-            ),
+            "name": '%s (Multi-Website)' % pattern.name,
            "website_id": self.id,
+            "origin_view_id": pattern.id,


It's kind of replacement to removed data in name

yelizariev · 2018-04-11T22:19:38Z

+                    # used
+                    copied_parent = self._find_duplicate_view_for_website(
+                        parent_view, website
+                    )


It's not directly part of this update, though there is a theoretical situation when several Customized views inherit each other, so they need to use a copied view too. But I need to make real multi-footer in my module website_multi_company to apply some extension to make website.footer_custom copyable too

yelizariev · 2018-04-11T22:20:18Z

+
+            if refs or one.asset_ids:
+                # add common_refs only for installed themes
+                refs += common_refs


This is the key idea of the update

In general I recommend to use |=, to avoid duplicates in the recordsets.

yelizariev · 2018-04-11T22:20:55Z

        id="layout_pattern"
        inherit_id="website.layout"
-        name="Template for replacing assets_frontend"
+        name="Main layout"


it's renamed because it's used in Customize menu

yelizariev · 2018-04-11T22:24:31Z

        result = pattern.copy({
            "active": pattern.was_active,
            "arch_fs": False,
-            "customize_show": False,


@yajo is there any reason why this was set to False? Do you remember the case when it's required?

yelizariev · 2018-04-11T23:04:29Z

 * If you install any of the supported themes after installing this addon, you
  will have to press *Reload* in the website config wizard to make it notice
-  the change.
+  the change. The same is true when you install any module with ``customize_show="True"``


Tests are not passed in travis due to this new requirement: Reload must be done after installing website_blog_category https://github.com/OCA/website/blob/10.0/website_blog_category/templates/website_blog_template.xml#L25

What can we do here? @yajo @pedrobaeza

What about calling reload from the last demo data file read?

Do you mean demo file of website_blog_category?

I'd prefer to Reload after creating new ir.ui.view record with customize_show (via extending create method). What do you think, @pedrobaeza ?

That would make sense, yes.

I was talking about demo data file included in this addon. Or even maybe a normal data file.

yelizariev · 2018-04-11T23:06:58Z

        result = pattern.copy({
            "active": pattern.was_active,
            "arch_fs": False,
-            "customize_show": False,


Copied message to don't miss it:

@yajo is there any reason why this was set to False? Do you remember the case when it's required?

Can't remember, honestly 🙄

yelizariev · 2018-04-11T23:30:35Z

When you review PR yourself

yajo

OK, let me know if I understand what you are doing here:

Now all websites have a theme, even if it must be the default one. I like this approach, it reduces variability 😊
Converted assets inherit the customize_show attribute independently.
All views with customize_show=True are considered multiwebsite views. I'm not sure if we should instead duplicate all views belonging to addons that depend on website; after all, a user usually can customize any view in a website... 🤔

Some comments below. Thanks!

yajo · 2018-04-12T07:43:35Z

        result = pattern.copy({
            "active": pattern.was_active,
            "arch_fs": False,
-            "customize_show": False,


Can't remember, honestly 🙄

yajo · 2018-04-12T07:45:14Z

-
                    parent_view = copied_view.inherit_id
-                    parent_view_module = parent_view.model_data_id.module
                    copied_parent = None


This line makes no sense now, isn't it?

yajo · 2018-04-12T07:50:14Z

 class IrUiView(models.Model):
    _inherit = 'ir.ui.view'

-    multi_theme_generated = fields.Boolean(


You need a migration script to fill origin_view_id from preexisting views that had this boolean set to true.

I'll do that

yajo · 2018-04-12T07:51:29Z

        inverse_name="website_id",
-        domain=[("multi_theme_generated", "=", True),
+        domain=[("origin_view_id", "!=", False),
                "|", ("active", "=", True), ("active", "=", False)],


Maybe we should replace this by context={"active_test": True}

I cannot find an example of using such trick. Are you sure that context is used when we read the field in python via .multi_theme_view_ids and other cases? I'd prefer to keep as it's now

OK no problem, it was just an idea

yajo · 2018-04-12T07:54:09Z

+
+            if refs or one.asset_ids:
+                # add common_refs only for installed themes
+                refs += common_refs


In general I recommend to use |=, to avoid duplicates in the recordsets.

yajo · 2018-04-12T07:54:48Z

-            ])
-            existing = frozenset(one.mapped("asset_ids.name"))
+
+        common_refs = self.env["ir.model.data"]


This line is not needed; the next sentence will return an empty recordset when nothing is found.

The idea was to allow in possible next updates don't change git blame much.

For example,

Version1

common_refs = self.env["ir.model.data"]
common_refs |= A

Version 2

common_refs = self.env["ir.model.data"]
common_refs |= A
common_refs |= B

Version 3

common_refs = self.env["ir.model.data"]
common_refs |= A1
common_refs |= B
common_refs |= C

Version 5

common_refs = self.env["ir.model.data"]
common_refs |= B
common_refs |= C

yajo · 2018-04-12T07:58:06Z

             "website, and disabled otherwise. Usually used to load assets.",
    )

+    auto = fields.Boolean(


Move fields above, just like explained here.

Do you mean above _sql_constraints? It's actually not explained in the guidelines. It mentions only constrain methods. While _sql_constraints seems to be part of

Private attributes (_name, _description, _inherit, ...)

Yikes! forget this. I think I didn't expand the diff in between 😆

yajo · 2018-04-12T08:00:05Z

+                    <separator string="Copies"/>
+                    <field name="copy_ids"/>
+                </page>
+                <page string="Multi-website" attrs="{'invisible': [('origin_view_id', '=', False)]}">


2 pages called the same?

Better 1 page and 2 groups.

Remember to add a name="multiwebsite", to enable possible inheritance.

The idea is don't pass attributes twice in each node here:

<separator string="Copies"/> <field name="copy_ids"/>

I can add different names, if it's the concern

name="multiwebsite_origin_view" name="multiwebsite_copied_view"

yajo · 2018-04-12T08:00:57Z

                <label for="multi_theme_id"/>
                <div>
-                    <field name="multi_theme_id" class="oe_inline"/>
+                    <field name="multi_theme_id" class="oe_inline" required="1"/>


Why not require it in the model?

Because we have default website, which doesn't have multi_theme_id on installation and we cannot set it on installation, because Default theme is created after loading python files

It makes sense. Could you add a note here then please? It's not obvious...

yajo · 2018-04-12T08:01:35Z

 * Rafael Blasco <rafael.blasco@tecnativa.com>
 * Antonio Espinosa <antonio.espinosa@tecnativa.com>
 * Jairo Llopis <jairo.llopis@tecnativa.com>
+* Ivan Yelizariev <https://it-projects.info/team/yelizariev>


I can't believe you weren't here before!

yelizariev · 2018-04-12T17:21:41Z

Rebased and added some fixes. Few comments from @yajo's review are subject to discuss. I'll squash commits once we close all concerns or one can do it on merging (just fist commit message has to be keeped: [IMP] make customize_show views part of multi-theme)

yelizariev · 2018-04-12T17:23:30Z

About this idea:

All views with customize_show=True are considered multiwebsite views. I'm not sure if we should instead duplicate all views belonging to addons that depend on website; after all, a user usually can customize any view in a website... 🤔

The possible problem here is that those changes will be destroyed after switching a website to new theme. So, it's better to share them, than letting user face data lost

yelizariev · 2018-04-12T17:27:24Z

I'll do this later

a migration script to fill origin_view_id from preexisting views that had this boolean set to true.

rafaelbn · 2018-04-13T22:57:10Z

Thank you @yelizariev for this great contribution! 😄 👏 👏 👏

yelizariev · 2018-04-14T01:26:42Z

I added migration scripts and squashed the PR

Previous history:
https://github.com/yelizariev/website/commits/10.0-website_multi_company-multi-customize-before-squashing

yelizariev · 2018-04-14T01:39:59Z

And another commit yelizariev@1984fbf
(squashed)

yelizariev · 2018-04-16T23:47:56Z

One more update from @yajo's review yelizariev@0fd0ad3

and there is still the question about travis tests. Can we do something about it or just left it as is

ilmir-k · 2018-04-17T18:23:41Z

@yelizariev
During the tests I found out wrong behavior. When we use Default theme or Multiwebsite Demo Theme for websites everything is ok, but if we install and set Bootswatch Theme we miss the options for Customize

yelizariev · 2018-04-17T18:32:53Z

I think the error was introduced in a4ed42f#diff-93d51900ef6c23af9b0a13e1278dfad5R45 by @yajo

How to reproduce without this updates:

Install Bootswatch Theme
Click reload twice
RESULT: Multi-theme Bootswatch Theme is disappeared from the list

yelizariev · 2018-04-17T18:33:11Z

@yajo could you check and fix?

yajo · 2018-04-18T06:32:44Z

See #447

yelizariev · 2018-04-18T15:07:03Z

Rebased


ivann@ivann-360:/mnt/files/odoo/odoo-10.0/website$ git fetch OCA  pull/443/head
De https://github.com/OCA/website
 * branch            refs/pull/443/head -> FETCH_HEAD
ivann@ivann-360:/mnt/files/odoo/odoo-10.0/website$ git diff FETCH_HEAD..HEAD
diff --git a/website_multi_theme/models/website_theme.py b/website_multi_theme/models/website_theme.py
index 45f3ea4..a0190f2 100644
--- a/website_multi_theme/models/website_theme.py
+++ b/website_multi_theme/models/website_theme.py
@@ -65,10 +65,11 @@ class WebsiteTheme(models.Model):
                 # add common_refs only for installed themes
                 refs |= common_refs
 
-            views = self.env["ir.ui.view"].search([
-                ("id", "in", refs.mapped("res_id")),
-                ("type", "=", "qweb"),
-            ])
+            views = self.env["ir.ui.view"].with_context(active_test=False) \
+                .search([
+                    ("id", "in", refs.mapped("res_id")),
+                    ("type", "=", "qweb"),
+                ])
             existing = frozenset(
                 one
                 .mapped("asset_ids")

yelizariev · 2018-04-19T02:06:16Z

Update for the last question: yelizariev@3b61c5d

It took more time than I thought

@yajo your review is very needed

yelizariev · 2018-04-19T02:30:37Z

Other updates

LINT: yelizariev@52b962f
CI fix: yelizariev@6c5a852

rafaelbn · 2018-04-19T03:13:33Z

Please @yajo could you review here? Thank you!

yajo

Amazing work, now just minor details

yajo · 2018-04-19T08:57:15Z

        help="Indicates if the view was originally active before converting "
             "the single website theme that owns it to multi website mode.",
    )
+


I'd remove this white line

yajo · 2018-04-19T09:00:17Z

                )
-                website.multi_theme_view_ids.unlink()
-                continue
+                if not default_theme:


If I'm not wrong, this if will never pass because the above call to ref() doesn't have a raise_if_not_found=False.

OTOH I'd use the try-except-else option instead.

yajo · 2018-04-19T09:01:22Z

-                ("model", "=", "ir.ui.view"),
-            ])
+
+        common_refs = self.env["ir.model.data"]


You can delete this, the search below will return an empty recordset on his own if no results are found.

My explanation is here: #443 (comment)
You probably miss it. What do you think about it?

yelizariev · 2018-04-19T20:30:46Z

Tiny updates yelizariev@e028b32

yelizariev · 2018-04-20T15:41:26Z

@pedrobaeza ?

ilmir-k

👍

pedrobaeza · 2018-04-23T14:50:58Z

+    "version": "10.0.1.2.0",
    "category": "Website",
    "website": "https://www.tecnativa.com",
    "author": "Tecnativa, Odoo Community Association (OCA)",


@yelizariev your company deserves to be here

pedrobaeza · 2018-04-23T14:53:45Z

+    def create(self, vals):
+        res = super(IrModelData, self).create(vals)
+        if vals.get('model') == 'ir.ui.view' \
+           and not self.env.context.get('duplicate_view_for_website'):


Incorrect indendation and line split

Multi-line if-block is my weak side 😞

The best is to enclose in parenthesis and always indent second line 2 levels more:

if (vals.get('model') == 'ir.ui.view' and not self.env.context.get('duplicate_view_for_website')):

I'd prefer to put logical operator at the beginning of the line

Oh, it's W503 requirement. OK

I'm not the only one who thinks so.

W503 is ignored by default in latest version of pycodestyle.

OCA/maintainer-quality-tools#545

I said wrong. W504 is ignored by default

But people say that W504 is more logical

Well, we can discuss about that in the issue, but right now it's W503 what is used.

pedrobaeza · 2018-04-23T14:54:40Z

+        "origin_view_id",
+        string="Copies",
+        readonly=True,
+        help="Duplicates of this view"


I would prefer a more explicit name: multitheme_copy_ids and a longer explanation in help.

pedrobaeza · 2018-04-23T14:56:48Z

+           and not self.env.context.get('duplicate_view_for_website'):
+            view = self.env['ir.ui.view'].browse(vals.get('res_id'))
+            if view.customize_show:
+                # Reload theme to make new customize_show available right after


Put this explanation in the method docstring instead of here, as it's the purpose of the override.

pedrobaeza · 2018-04-23T14:58:20Z

+        views = super(IrUiView, self)._customize_template_get_views(
+            key, full=full, bundles=bundles
+        )
+


These empty lines don't add anything useful and PEP8 discourages its use. Better to strip them.

pedrobaeza · 2018-04-23T14:59:33Z

        comodel_name='website.theme',
        domain=[("asset_ids.view_id", "!=", False)],
        help="Multiwebsite-compatible theme for this website",
+        require=True,


Please don't add require fields in inherited classes, as this will make that tests that create records from this model fail when this dependent module is not loaded. Instead, put the require on the view.

I believe it's true for existing fields only, while here we create a new field.

No, it's the same. The sequence is the following in MQT:

This module is installed along with all the testable ones and its dependencies.

Then the SQL NOT NULL constrained is added.

On the second pass, modules are being tested while being loaded.

Any module adding a website will have the not null error until this module is loaded.

I got it. You are right.
It seems to be an odoo framework issue. Is there a way to fix it in odoo core?

pedrobaeza · 2018-04-23T15:01:16Z

+                               ('website_id', '=', False),
+                               ('customize_show', '=', True),
+                           ]).mapped('model_data_id')
+


Don't add too much empty lines...

yelizariev · 2018-04-23T15:39:41Z

New updates: yelizariev@3a24f70

yelizariev · 2018-04-23T15:51:45Z

And update about required=True yelizariev@b70f910

yelizariev · 2018-04-23T16:31:43Z

I suppose this is the last question here:

website_multi_theme/models/ir_model_data.py:33:17: W503 line break before binary operator

I made an issue in MQT OCA/maintainer-quality-tools#545

pedrobaeza · 2018-04-23T16:34:21Z

But that's because you haven't put the and in the previous line.

yelizariev · 2018-04-23T17:37:04Z

I know, but I'd prefer to put and at the beginning of the line.

pedrobaeza · 2018-04-23T17:44:17Z

Yeah, but if you want this merged before discussing about the change, you have to fix it. If not, we have to wait until the discussion is solved.

New field "auto" in "website.theme.asset" to distinguish assets created from theme and ones created in xml. It's needed, for example, for non-module themes (like Demo theme) be handled in _convert_assets (before this update such themes were skipped) [REF] field multi_theme_generated is replaced with origin_view_id [IMP] apply default theme after installation [IMP] reload themes on installing new modules with customize_show [CI] Website always have Multi-Theme now. Also, increase default timeout (10 sec) to 60 sec, because it may take more time now

yelizariev · 2018-04-23T18:10:11Z

I'd prefer to merge it now.
(Travis is green now)

rafaelbn · 2018-04-26T22:42:10Z

Thanks @yelizariev ! 👏 👏

yelizariev commented Apr 11, 2018

View reviewed changes

yajo requested changes Apr 12, 2018

View reviewed changes

yelizariev force-pushed the 10.0-website_multi_company-multi-customize branch from b21031f to 8aa5f94 Compare April 12, 2018 16:55

rafaelbn changed the title ~~[IMP] make customize_show views part of multi-theme~~ [10.0][IMP][website_multi_theme] make customize_show views part of multi-theme Apr 13, 2018

rafaelbn added this to the 10.0 milestone Apr 13, 2018

yelizariev force-pushed the 10.0-website_multi_company-multi-customize branch 2 times, most recently from 8fb65b0 to c5836fc Compare April 14, 2018 01:26

yelizariev force-pushed the 10.0-website_multi_company-multi-customize branch from c5836fc to b97a462 Compare April 14, 2018 01:39

yelizariev force-pushed the 10.0-website_multi_company-multi-customize branch from b97a462 to 0252aee Compare April 16, 2018 23:46

yelizariev force-pushed the 10.0-website_multi_company-multi-customize branch from 0252aee to 64c5033 Compare April 18, 2018 15:06

yelizariev force-pushed the 10.0-website_multi_company-multi-customize branch from 64c5033 to a3c2d6a Compare April 19, 2018 02:04

yelizariev force-pushed the 10.0-website_multi_company-multi-customize branch from a3c2d6a to f0bb1c1 Compare April 19, 2018 02:28

yelizariev force-pushed the 10.0-website_multi_company-multi-customize branch from f0bb1c1 to e9d2100 Compare April 19, 2018 02:33

rafaelbn added the needs review label Apr 19, 2018

yajo requested changes Apr 19, 2018

View reviewed changes

yelizariev force-pushed the 10.0-website_multi_company-multi-customize branch from e9d2100 to d6cd84a Compare April 19, 2018 20:31

yajo approved these changes Apr 20, 2018

View reviewed changes

ilmir-k approved these changes Apr 23, 2018

View reviewed changes

pedrobaeza reviewed Apr 23, 2018

View reviewed changes

yelizariev force-pushed the 10.0-website_multi_company-multi-customize branch from d6cd84a to 62eabef Compare April 23, 2018 14:54

pedrobaeza requested changes Apr 23, 2018

View reviewed changes

yelizariev force-pushed the 10.0-website_multi_company-multi-customize branch from 62eabef to ca86676 Compare April 23, 2018 15:38

yelizariev force-pushed the 10.0-website_multi_company-multi-customize branch from ca86676 to 3b9f640 Compare April 23, 2018 15:51

yelizariev force-pushed the 10.0-website_multi_company-multi-customize branch from 3b9f640 to 73d6a72 Compare April 23, 2018 17:49

pedrobaeza approved these changes Apr 23, 2018

View reviewed changes

pedrobaeza merged commit 082c66c into OCA:10.0 Apr 23, 2018

yelizariev mentioned this pull request Apr 24, 2018

10.0 website multi theme fixes #452

Merged

Uh oh!

Conversation

yelizariev commented Apr 11, 2018

Uh oh!

yelizariev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yelizariev commented Apr 11, 2018

Uh oh!

yajo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yajo left a comment •

edited

Loading

yelizariev commented Apr 12, 2018 •

edited

Loading

yelizariev commented Apr 12, 2018 •

edited

Loading

yelizariev commented Apr 12, 2018 •

edited

Loading