[REV] website: revert commit ability to restrict groups on website menu

[Gorash]

[REV] website: revert commit ability to restrict groups on website menu

revert b5091c7

fp asks to remove the groups on the menus to simplify the
model and allow the menu to be cached in the future. The user can change
the templates, use the big menu, add custom t-if, groups...

task-2737973

[robodoo]

Pull request status dashboard.

[rdeodoo]

What does and use the big menu refers to?

This is just reverting b5091c7, that should be indicated in the commit message. Also, this is a task not an opw?

I am obviously not going to be excited about this. This was an awaited feature from the community (and website team).
The goal of this feature is to be able to allow a controller like /shop to be hidden from visitor for instance:

1. You add the group on the view from the controller
2. You add the group on the menu

Do you confirm it was decided to remove this for "the future"?

[Gorash]

What does and use the big menu refers to?

To put t-if inside I think

This is just reverting

Ok, I will change the message by a revert message.

Do you confirm it was decided to remove this for "the future"?

It's an fp request, he says it has no use and needs to be removed, and it annoys him because he can't cache the whole menu because of it.

[rdeodoo]

I will try to do a quick summary here, as I think this is wrong (not too wrong tho, but more wrong that good).
Even if I think this makes sense with what we target (le boucher du coin).

This feature was requested by the community, and is now used by the PS Tech.
Of course, they can do that with a custo if we remove it.

You suggest an alternative by using a mega menu, I guess to write groups and t-if inside the template, but it does not makes sens:

1. It will make the mega menu not fully editable?
2. It will need the use of.. a mega menu.. which is not what the user might want? It's like telling a client to buy a truck instead of a car 🤷
3. It will also be broken when you will introduce your cached menu after this PR?

As a reminder, this is the only way to hide a menu from a controller, like /shop.
You can quickly add a group to the website.products view and the same group on the menu and voila, you have a fully working way of hidding a controller to only some users, menu included.
It is advanced, but not that stupid..

On top of that, note that this is also partly reverting what was done at 49d64a8 in slides -> Hide a forum if you don't have access to it.
See, the feature is not totally useless ;-)

Also, worth mentionning that it will still work for website.page's menu, as for a menu (linked to a page) to be visible, its page need to also be visible, which checks if the page's view has a group.
But I guess with what is coming after this PR (caching all the menu template) it will also break the feature for website.page
It seems louch, as far as I can tell, wasn't it FP that wanted the groups on page to match other builders?
The feature doesn't makes that much sense if we have hidden pages but not hidden related menus..

On the other hand, I have to admit and be coherent with how I generally think when it comes to the website builder: we aim to provide the best solution for "le boucher du coin", for which this feature has no benefit.

Why don't we keep this (very) advanced feature (I admit), alongside your incoming menu cache system, with something like:

• Menu is fully cached
• On website.menu write(), if adding a group on it, set t-cache=False on the website.submenu template?
  Or anything else that goes that way.

I'll r+ once I got @fpodoo confirmation given this summary

[fpodoo]

The rationale is the following:

• this feature is currently only useable by experts / developers (you have to go in backend, in developer mode, ...)
• but experts / developers can also do dynamic menus in the XML template anyway (I actually find it more convenient that way, more powerful); that's what we do for more advanced menus like on odoo.com
• because this feature is 1/ not easily accessible and 2/ slow down every page; we save performance by removing it.

[Gorash]

@fpodoo Regarding performance, for the menu, I saw no change by removing the group_ids, since there is no group in base, and otherwise the accesses are cached.

[rdeodoo]

because this feature [..] slow down every page; we save performance by removing it.

It is not? Everything is cached and regardless of the menu complexity (having groups, having multiple nested level etc), there will only be one and only one sql query regarding the website.menu table.
That's what we made sure with f95af92 and b98d9c9

As far as I understand this one, this is for "future perf improvement" for when we will have the qweb t-cache and we will avoid that query on website.menu, not querying at all that table?

I am just waiting for PDE's feedback before r+ to see if something else should be shipped alongside this one.
Indeed, removing the group on menu makes the New > Forum screen broken UX-wise, as:

• User can add a group to the forum
• User can add a menu for the forum

-> If you set both, it doesn't makes sense after this PR as you have unreachable menus leading to 403 for non authorized user.
Note that the visibility feature on forum was introduced with 49d64a8

[rdeodoo]

@robodoo r+
Everything seems to have been processed here:

• There is no perf imp here
• This is about removing feature so later the menu can be cached when we will have qweb t-cache
• As confirmed with @pierre-pde , the UX inconsistency on the new forum popup is not a big deal, "Add to menu" and "Add groups" will lead to an always visible menu leading to 403 (as the menu won't have the group, reverted in this pr)
• There is a workaround to achieve what was done with groups -> Edit the ir.ui.view template to construct the menu and do the checks there (t-if, groups etc).

[robodoo]

Staging failed: ci/upgrade_enterprise on 3882eb049fd647fbaee3062822d729d0b5131638 (view more at http://runbot.odoo.com/runbot/build/12516236)

[Feyensv]

Do not forget to open (and r+) the PR of the corresponding upgrade branch (that's why the runbot is green here)

[tde-banana-odoo]

When removing fields, always have upgrade branches and reviewed PR... you just ate 3 mergebot builds in a freeze period -_-' .

[rdeodoo]

Whoops, sorry. I made sure there was an upgrade branch the first time I reviewed this PR, which there was.
I kinda forgot about it when I r+'d here today.

Do not forget to open (and r+) the PR of the corresponding upgrade branch (that's why the runbot is green here)

Thank you @Feyensv that explains why it went through despite having a green runbot (and an upgrade branch) indeed 👍

Anyway, @Gorash I rebased both your branches and opened the upgrade PR (not sure why it wasn't tho..), we can merge this after the freeze, it can wait another version, I'd not want to slow down or block a bit more @tde-banana-odoo during this freeze period in case something else goes wrong 😨

[tde-banana-odoo]

There was a branch and it was merged ? Weird ...

[rdeodoo]

Yup, created 6 days ago 2022-01-25 09:47:54 (give or take a few hours I guess) which actually made the runbot become green. (it was red before it was created, for the same reason as the staging, missing remove_field)

Edit: Gotta admit, I saw the warning about mismatch but I thought it could only introduce more error and not "avoid"/"remove" some error from the builds. I thought it could only makes the build red if it was supposed to be green (with rebased branch), not the other way around.
I therefor took the green build for granted.

I'll know, now :)

[tde-banana-odoo]

Well that's strange it was built based on a branch without pr and tried to merge it ... I suppose he does not know the branch contains commits not present on stable, something like that. Funny one.

[rdeodoo]

Waiting for upgrade PR to be ready

[rdeodoo]

@Gorash Don't forget https://github.com/odoo/upgrade/pull/3208#issuecomment-1034888824 so we can ship this one

[rdeodoo]

Build is red, most likely due to branches not sync'd, can you rebase both together with updated master so we can r+?

[rdeodoo]

I fixed the remaining ci issues in the upgrade PR, should be good.

@robodoo r+

(copy paste from previous r+)

Everything seems to have been processed here:

• There is no perf imp here
• This is about removing feature so later the menu can be cached when we will have qweb t-cache
• As confirmed with @pierre-pde , the UX inconsistency on the new forum popup is not a big deal, "Add to menu" and "Add groups" will lead to an always visible menu leading to 403 (as the menu won't have the group, reverted in this pr)
• There is a workaround to achieve what was done with groups -> Edit the ir.ui.view template to construct the menu and do the checks there (t-if, groups etc).

[robodoo]

Linked pull request(s) odoo/upgrade#3208 not ready. Linked PRs are not staged until all of them are ready.