[16.0][MIG] bi_view_editor

[huguesdk]

BVEEditor widget

the BVEEditor widget (in the “query builder” tab) has been converted to owl. this is quite a big change, almost a rewrite. it should work in exactly the same way as in 15.0 (except for one slight difference, as explained below, and little styling changes).

the only difference that i’m aware of is that if a model is expanded in the left column and the model filter is changed, in 15.0 all models are collapsed, while now the ones that are still visible stay expanded if they were. i don’t know if the previous behavior was desired or was due to a technical limitation, and i think the current way makes sense, so i left it like that. doing it with owl in the same way as in 15.0 would be possible but cumbersome, because parent components cannot access their children components in an easy way (at least i didn’t find how to do it except by making children register themselves with their parent using a callback).

i tried to keep the existing logic as much as possible to lower the amount of changes. for example, the FieldList component still manages the data of the field list, and has .get() and .set() methods called directly by the main widget, which in a reactive framework like owl is a little weird.

this is the first time i work with owl (learning it along the way), so there are probably things that are not done exactly like they should. for example, i didn’t succeed in using custom events to notify parent components of changes, so i used callbacks instead. all advice and comments are welcome.

known issues

in 16.0, translations are handled differently than before. the ir.translation table doesn’t exist anymore. because of this, i removed the translation smart button that was available on created bi views. the translations can be edited directly on the values themselves (name of the bi view and field names in the details tab).

additionally, there are 2 things that don’t work anymore like they do in 15.0 because of how translations are handled:

1. translations of the name of a bi view and of the field descriptions do not work as expected: the translated strings are selected (by the user’s language) when the view is generated (and stored as their en_US value) instead of when it is displayed.
2. translated fields are not supported in bi views.

the 1st issue is due to how the models and fields are created: only one value is set as their name field, while internally the field is a json object. currently, the user’s language defines the value for the en_US language, regardless of which language it actually is.

the 2nd issue is due to the fact that the code handles translated fields as regular fields while they need to be handled in a special way. here is how to reproduce the problem:

1. create a new bi view.
2. give it a name (or it won’t save, as it’s required).
3. add the “country” → “country code” and “country” → “country name” fields.
4. click on “generate bi view”.
5. click on “open bi view”.
6. problem: the “country name” column displays empty values.
7. click on the pivot button on the top right.
8. click on a “total” cell and select “country name”.
9. the values displayed are all “[object Object]”.
10. click on any of those values and select “country code”.
11. problem: the following error is raised:

    psycopg2.errors.InvalidTextRepresentation: invalid input syntax for type json
    LINE 4: ...       WHERE ("x_bve_test0"."x_bve_t1_name" = '{''en_US'...
                                                             ^
    DETAIL: Token "'" is invalid.
    CONTEXT: JSON data, line 1: {'...
    

any help to solve this is welcome.

edit

translated fields are now correctly supported.

[legalsylvain]

Hi @huguesdk. Do you know @PierrickBrun from akretion is working on it since some weeks ?

#755

[huguesdk]

@legalsylvain ouch, this is unfortunate. i started working on this a while ago, and didn’t check since then. 🤦 @PierrickBrun, since you dived into the code on your side, what do you think of this version?

[PierrickBrun]

I feel like this version is more finished than mine (ie. I did not look into translations at all atm, the drag/drop functionnality, the css on your side looks better)

about the owl migration of the widget, it looks like we took similar paths. I made a first attempt at using a store to put the fields but I did not like that the children components would directly make changes to it so I used callbacks, like you did

maybe we can use this one and take things that may be better in mine to complete it ?

I will squash my commits to make it more readable

[huguesdk]

@ivantodorovich should i just squash all commits after (and including) the migration commit to one?

[huguesdk]

i also think that this is a quite big and useless dependency. what do you suggest? put it in base.menu_board_root by default and add a glue module (auto-install) that depend on spreadsheet_dashboard and moves the menu entry there?

what do you think, @PierrickBrun?

[ivantodorovich]

should i just squash all commits after (and including) the migration commit to one?

Yes, you can do that. If all the changes are strictly due to the migration process, then it makes sense to have all of them in a single "migration" commit.

Though, if you have other improvements you can decide to put them in separate commits (optional, they can be on the migration commit, too). However if you do this, please write more descriptive commit messages and try to stick to the guidelines for formatting it. Messages like "add comment" don't say much 😓

[Camille0907]

I'm not totally at ease with owl, but LGTM from what I understand :)

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[huguesdk]

@ivantodorovich:

• removed dependency on spreadsheet_dashboard.
• added glue module to move menu items in case spreadsheet_dashboard is installed.
• squashed all commits to one.

please note that in the odoo master branch, the legacy base.menu_board_root menu has been removed, so eventually, either the dependency on spreadsheet_dashboard will be necessary, or a simple custom dashboard menu will need to be defined in this module.

[ivantodorovich]

thanks!

[HviorForgeFlow]

/ocabot merge nobump

[OCA-git-bot]

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-761-by-HviorForgeFlow-bump-nobump, awaiting test results.

[OCA-git-bot]

@HviorForgeFlow your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-761-by-HviorForgeFlow-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

[HviorForgeFlow]

@huguesdk seems that pre-commit is failing https://github.com/OCA/reporting-engine/actions/runs/6781745670/job/18432619470#step:7:19

[OCA-git-bot]

@HviorForgeFlow your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-761-by-HviorForgeFlow-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

[huguesdk]

@HviorForgeFlow i’ve just added the missing file. could you please retry?

edit: i’m surprised that pre-commit forces the __init__.py file to be there. the module is working well without it. maybe it was necessary in previous versions and not anymore?

[HviorForgeFlow]

/ocabot merge nobump

[OCA-git-bot]

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-761-by-HviorForgeFlow-bump-nobump, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at 79e0d02. Thanks a lot for contributing to OCA. ❤️