Skip to content

WIP [FIX] theme-like dependencies of the theme must be handled as theme#409

Closed
yelizariev wants to merge 1 commit intoOCA:11.0from
yelizariev:11.0-website_multi_theme-work-with-dependencies
Closed

WIP [FIX] theme-like dependencies of the theme must be handled as theme#409
yelizariev wants to merge 1 commit intoOCA:11.0from
yelizariev:11.0-website_multi_theme-work-with-dependencies

Conversation

@yelizariev
Copy link
Member

No description provided.

@yelizariev yelizariev changed the title [FIX] theme-like dependecies of the theme have be handled as theme WIP [FIX] theme-like dependecies of the theme have be handled as theme Jan 26, 2018
@yelizariev
Copy link
Member Author

yelizariev commented Jan 26, 2018

I become to understand how @yajo's module works 😄 and why it doesn't work on some specific themes

I faced a following problem with some theme:

Style error                        
The style compilation failed, see the error below. Your recent actions may be the cause, please try reverting the changes you made.

The theme depends on other modules which have their own templates. Those templates don't work without main theme templates -- it shows less files compilation error.

In order to fix that we have to treat those dependenies as a part of main theme (deactivate original template, make duplicated of them, etc). This is what this PR does.

Additionally it filters out non-qweb views from processing.

This PR needs more documentation on how to convert such themes to multi-themes.
Additionally, we can make website.theme model editable from UI. I already did it in website_multi_company (depends on website_multi_theme)

@yelizariev yelizariev changed the title WIP [FIX] theme-like dependecies of the theme have be handled as theme WIP [FIX] theme-like dependencies of the theme must be handled as theme Jan 27, 2018
@pedrobaeza pedrobaeza requested a review from yajo January 27, 2018 13:39
@pedrobaeza
Copy link
Member

Little flake8 details:

website_multi_theme/models/website_theme.py:38:80: E501 line too long (106 > 79 characters)
website_multi_theme/models/website_theme.py:46:80: E501 line too long (98 > 79 characters)

@yelizariev yelizariev force-pushed the 11.0-website_multi_theme-work-with-dependencies branch from 0fddcfa to 62bbe92 Compare January 28, 2018 09:38
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very interesting fix indeed, thanks!

'ir.module.module',
string="Dependencies",
help="Theme-like dependencies. "
"Add modules here if you got error \"The style compilation failed\"."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, if I get your point, this new field would define other addons which would become part of this "theme", and thus their asset views would become single-website too. Am I right? 🤔

If you faced this problem on any of the supported themes, then I think you should add this definition into the corresponding data/themes_*.xml file.

Also in demo/themes.xml would be a way of demoing this new feature too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You understand me correctly.

The problem of this PR is that there is no easy way to check it. All official modules works without those updates. So, seems that the only way to demonstrate it is to create new dummy theme with specific structure

@yelizariev
Copy link
Member Author

yelizariev commented Jan 29, 2018

I think I found another problem. When we have following scenario:

  • Theme1

    • depends on dep_theme_1
    • depends on dep_theme_2
  • Theme2

    • depends on dep_theme_1
    • depends on dep_theme_2

-- _find_and_deactivate_views doesn't work correctly, because view_id could be already deactivated as a part of another theme.


What are possible solutions to make such updates testable from CI point of view? Can we have new OCA repo with themes\modules only for tests. Could those themes be in dependencies of main module or we need to add new module, say website_multi_theme_test?

@yajo
Copy link
Member

yajo commented Jan 29, 2018

I think the common pattern in such case is adding a new module called website_multi_theme_example

@pedrobaeza
Copy link
Member

Why not putting them in demo data? I don't like extra module pattern, but it's needed if there's associated code that can't be hidden in non demo DBs, but that's not the case, isn't it?

@yelizariev
Copy link
Member Author

yelizariev commented Jan 29, 2018

Solution requires references to theme modules, which cannot be created via xml. So, not just one example module, but at least 4 example modules (2 themes, 1 theme-dependency and 1 module website_multi_theme_example)

@pedrobaeza
Copy link
Member

Agh, too much work I think. We should leave it as is IMO.

@yajo
Copy link
Member

yajo commented Jan 29, 2018

I neither like that pattern, so I'm open to suggestions, but the case at hand is quite specific. Let's put a real scenario:

  1. Theme for website 1 depends on website_anchor_smooth_scroll addon because it uses its features.
  2. Theme for website 2 does not use nor want that addon.

Since the mocked assets are just from the website_1_theme addon, those of website_anchor_smooth_scroll would appear always. This patch would bundle that addon's assets into website_1_theme's and thus it would produce a full package of assets, views, etc.

I'm starting to think on alternatives:

  • Allow the addon to apply several themes per website. Then, convert (in de example) website_anchor_smooth_scroll into a multi-website theme, and the website admin should enable both manually.
  • Use Odoo's builtin dependency resolution and get up to the website addon. Assume that all dependencies from the curren theme up to website belong to current theme, and pack'em all always. This is nice, because you could build a bridge addon that includes i.e. website_sale, and that would make only 1 of your websites have a shop.

Thoughts?

@yelizariev
Copy link
Member Author

yelizariev commented Jan 29, 2018

In my multi-company module I implemented something in between -- it allows to admin to choose only dependencies of the theme, but not built-in dependencies:

https://github.com/it-projects-llc/website-addons/blob/e5864598a4ffca70c34bc40ee9fa126f8372ad89/website_multi_company/models/module.py#L28-L32


The idea about changing multi_theme_id to multi_theme_ids in website model and remove dependency_ids in multi-theme model (i.e. allow choose several themes per website) seems good from technical point of view (no need to make tricks in _find_and_deactivate_views). But I am not sure about user point of view:

  • Admin / User can expect that he can activate any set of themes (or mini-themes), but those might be incompatible
  • Theme makers probably prefer to make a single ready-to-use theme, rather than kit of mini-themes to avoid compatibility problems, simplify customer support service, increase sales, etc.

@yajo
Copy link
Member

yajo commented Jan 29, 2018

Theme makers probably prefer to make a single ready-to-use theme, rather than kit of mini-themes to avoid compatibility problems, simplify customer support service, increase sales, etc.

That's true, although this addon was able to build a bridge between a theme that didn't support multithemes (a.k.a. almost any paid theme out there) and the multiwebsite support. If we don't go the way of multiple themes per website, I cannot figure out how to retain that feature (which is critical IMHO).

So I still feel that's the best way to go. Besides, you can test that easily with demo data by bundling 2 demo themes and enabling both.

Developing multithemes is gonna be a hard thing in any case until Odoo gives sane support for it upstream. I guess we should just add proper docs and go on with it.

You can still keep this PR and convert those dependency addons into themes that get always applied behind the scenes, no matter if selected or not by the user.

So, do you agree on going that path?

@yelizariev
Copy link
Member Author

That's true, although this addon was able to build a bridge between a theme that didn't support multithemes (a.k.a. almost any paid theme out there) and the multiwebsite support. If we don't go the way of multiple themes per website, I cannot figure out how to retain that feature (which is critical IMHO).

I'm sorry, but I hardly understand what do you mean here. Which addon and which feature do you reference here? From my point of view both ways allow to implement same thing.

So I still feel that's the best way to go. Besides, you can test that easily with demo data by bundling 2 demo themes and enabling both.

That's a good point for multiple themes per website way.

OK,there is another problem we need to solve with multiple themes per website way.
How to generate those website.theme records? Some kind of wizard? Because in my case, there are literally 19 mini-themes in dependencies, so some kind of automation is required rather than asking user to create 19 records of website.theme manually. Also, we need a way to specify batch of themes at once (Imagine that we have theme1 with 19 deps and theme2 with 25 deps and with want to set theme1+deps for website1, theme2+deps for website2, theme3+deps for website3, etc..

I'd like to have solution, that can be easily configured by working with UI and doesn't require extra actions from theme makers (because they will not read our docs and do what it says)


By the way, installing multiple themes at once requires removing exclusiveness for theme-modules. It's a part of website_multi_company now, which can be move to website_multi_theme

@pedrobaeza
Copy link
Member

By the way, installing multiple themes at once requires removing exclusiveness for theme-modules. It's a part of website_multi_company now, which can be move to website_multi_theme

Yes, please, this is something missing that it's needed for completeness

@yelizariev
Copy link
Member Author

yelizariev commented Jan 29, 2018

What if we change dependency_ids fields as following

dependency_ids = fields.Many2many('website.theme')

It allows to put demo data in xmls. It still requires a way to generate website.theme records, but it looks easier to do in this way -- just a button, that generates records depending of dependencies of converted_theme_addon

@yelizariev
Copy link
Member Author

  • remove exclusiveness for theme-modules.

@yajo
Copy link
Member

yajo commented Jan 29, 2018

I'm sorry, but I hardly understand what do you mean here. Which addon and which feature do you reference here? From my point of view both ways allow to implement same thing.

Let me reword: website_multi_theme is currently able to build a bridge between a theme that doesn't support multithemes (almost all themes out there) and the multiwebsite support. You can see these files, which basically build that bridge for several themes. If we don't go the way of multiple themes per website, I cannot figure out how to autobuild those bridges (which is critical for this addon to be useful).

I'd like to have solution, that can be easily configured by working with UI and doesn't require extra actions from theme makers (because they will not read our docs and do what it says)

It's hard to have a 100% automated system, but I guess that if you change dependency_ids to website.theme, then it's quite clear that a theme must be declared to be set as a dependency. I'm a bit mentally stuck on how to make this more automated than that and still consider it stable for production...

The only other option I can imagine is that when the user selects the theme, he gets a wizard with all the addons that he can enable in that website too. All the dependency graph between chosen theme and website is forced to be true, and all other installed addons that depend on website are opt-in. But that complicates the addon and its testing quite a few. I guess we should just hope they read the docs...

Well, I'm just trying to get to a good solution here. 🤔 Do you have a better idea?

@yelizariev
Copy link
Member Author

It could be semi-automated solution with instruction and warning in the wizard...

I still believe that base idea of the updates should be new field in website.theme model:

dependency_ids = fields.Many2many('website.theme')

I'd like to prepare some prototype for it and review posible problems during the development. After that it should be easier to discuss details.

@yajo
Copy link
Member

yajo commented Jan 30, 2018

Yep, I guess it all will become easier once we see the prototype, but it looks like a good design. Setting as WIP then.

yelizariev pushed a commit to yelizariev/website that referenced this pull request Apr 25, 2018
@yelizariev
Copy link
Member Author

yelizariev commented Apr 25, 2018

We can continue the discussion here: #454

@yelizariev yelizariev closed this Apr 25, 2018
yelizariev pushed a commit to yelizariev/website that referenced this pull request Apr 26, 2018
yelizariev pushed a commit to yelizariev/website that referenced this pull request Apr 26, 2018
yelizariev pushed a commit to yelizariev/website that referenced this pull request Apr 26, 2018
yelizariev pushed a commit to yelizariev/website that referenced this pull request Apr 26, 2018
yelizariev pushed a commit to yelizariev/website that referenced this pull request Apr 27, 2018
rafaelbn pushed a commit that referenced this pull request Apr 28, 2018
yelizariev pushed a commit to yelizariev/website that referenced this pull request May 1, 2018
yajo pushed a commit that referenced this pull request May 23, 2018
HviorForgeFlow pushed a commit to ForgeFlow/website that referenced this pull request Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants