Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MIG] Migrate mis_builder, mis_builder_budget, mis_builder_demo to 16.0 #472

Merged
merged 27 commits into from Apr 4, 2023

Conversation

adrienpeiffer
Copy link
Contributor

@adrienpeiffer adrienpeiffer commented Oct 3, 2022

@adrienpeiffer adrienpeiffer marked this pull request as draft October 3, 2022 11:13
@adrienpeiffer
Copy link
Contributor Author

odoo/odoo#101701

@sbidoul
Copy link
Member

sbidoul commented Oct 3, 2022

/ocabot migration mis_builder

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Oct 3, 2022
@OCA-git-bot
Copy link
Contributor

There's no issue in this repo with the title 'Migration to version 16.0' and the milestone 16.0, so not possible to add the comment.

@sbidoul
Copy link
Member

sbidoul commented Oct 3, 2022

/ocabot migration mis_builder

@OCA-git-bot OCA-git-bot mentioned this pull request Oct 3, 2022
3 tasks
@adrienpeiffer adrienpeiffer force-pushed the 16.0-mig-mis_builder-ape branch 2 times, most recently from 40e6d36 to ead84d2 Compare October 10, 2022 08:58
@FernandoRomera
Copy link

@adrienpeiffer
Merged:
OCA/server-ux#535
OCA/reporting-engine#690

Can you rebase, please?

@ovnicraft
Copy link
Member

@adrienpeiffer any news about this? I would like to know if more hands are needed here.

@sbidoul
Copy link
Member

sbidoul commented Jan 28, 2023

I had a quick look at this.

The PDF and XLSX exports work.

There are a few UX issues (default company, preview/export buttons on the report form are not visible).

The preview widget does not work. Does it need to be rewritten in OWL?

The analytic filters... well I don't immediately see how we could implement them in 16.0.
When using the journal items as data source, we can't do much filtering with the analytic distribution json field.
Maybe we could promote analytic lines as an official data source (like in mis_builder_analytic), and enable analytic filters only when that data source is selected? Not sure. Suggestions welcome.

@dzungtran89
Copy link
Contributor

hi @sbidoul , about the widget mis_report_widget, it needs to be migrated to owl in order to work with odoo 16, so I think we can do step by step:

  • Migrate the widget to owl to make the preview work at least -> I'll do this
  • About the filter, for the moment I can add the filter of analytic lines m2m

@sbidoul
Copy link
Member

sbidoul commented Jan 30, 2023

Migrate the widget to owl to make the preview work at least -> I'll do this

Thanks! Please do a PR to acsone:16.0-mig-mis_builder-ape

About the filter, for the moment I can add the filter of analytic lines m2m

I'm not sure how that would work?

@dzungtran89
Copy link
Contributor

dzungtran89 commented Jan 30, 2023

I'm not sure how that would work?

I think, because the analytic lines link to the move_line_id (m2o), so with this filter, we could get relevant move lines for the report

@sbidoul
Copy link
Member

sbidoul commented Jan 30, 2023

Hm, yes, but I'm afraid that will not work performance-wise.

@rafaelbn
Copy link
Member

rafaelbn commented Feb 2, 2023

Hello! Thank you! 😄

Could you please rebase to check and test?

Thank you!

@sbidoul
Copy link
Member

sbidoul commented Feb 2, 2023

@rafaelbn there is nothing to rebase. runboat is usable.

@sbidoul
Copy link
Member

sbidoul commented Feb 2, 2023

A few more thoughts about analytic filters.

I'm going to removing everything that concerns analytic tags and analytic groups. These concepts are gone for good.

The remaining analytic filters would be analytic account and analytic plan. Selecting an analytic plan would be mandatory when filtering on analytic accounts.

Maybe we can make the analytic filters visible only if analytic_account_id and analytic_plan_id are present in the move line source.

I'm still wondering what the analytic plan hierarchy is good for...

@sbidoul
Copy link
Member

sbidoul commented Mar 13, 2023

@dzungtran89 little steps that can be done:

  • a PR with one commit to remove code related to analytic tags (across all 3 modules)
  • a PR with the forward ports mentioned in the original description (across all 3 modules)
  • a PR with a SQL view model that emulates account.move.line based on the analytic lines with at least an additional column for the analytic plan

@rafaelbn
Copy link
Member

Hello @sbidoul ,

I'm going to removing everything that concerns analytic tags and analytic groups. These concepts are gone for good.

For a migration from 15 to 16:

  • To which object analytic tags will be migrated in order to maintain all reports working? Actually tags were dimension , important information for clients.
  • To which object analytic groups will be migrated in order to maintain all report working? Less important but exists.

Do you have an idea actually or we have still to analyse?

I'm worried about transition. Let me know any help. I will try to record a analysis.

Thank you!

@sbidoul
Copy link
Member

sbidoul commented Mar 14, 2023

Hi @rafaelbn if you have information about how the analytic concepts are migrated from 15 to 16 that would help. I very much doubt we can migrate reports that rely on analytic automatically, though.

@dzungtran89
Copy link
Contributor

Hi @sbidoul , thanks for your suggestions. The below are two forward ports

The reason of having two PRs is to avoid mixing up the commit history of two modules mis_builder and mis_builder_widget

@pedrobaeza
Copy link
Member

I let @sbidoul the honor of merging when he's ready.

@sbidoul
Copy link
Member

sbidoul commented Apr 1, 2023

Let's wait until mid next week to let people test?

@pedrobaeza
Copy link
Member

OK for me.

Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

Functional test locally and it worked like a charm!

@sbidoul
Copy link
Member

sbidoul commented Apr 2, 2023

There is probably a bug when changing the move line source in a report template after reports have been created for it. I'll look into it.

@sbidoul
Copy link
Member

sbidoul commented Apr 2, 2023

Is there a way to warn users about analytic account not being an option anymore? I think it would avoid some possible bug reports.

ValueError: Invalid field account.move.line.analytic_account_id in leaf ('analytic_account_id.code', '=', '03000')

More broadly it would be good to show a nicer error message is such case [invalid domain]. This can happen for many reasons. I created #557 to track this.

@sbidoul
Copy link
Member

sbidoul commented Apr 2, 2023

@CasVissers-360ERP a friendlier error message is now provided when invalid fields are used in expressions or filter domains. Let me know how it works for you.

@sbidoul
Copy link
Member

sbidoul commented Apr 3, 2023

Since the widget is getting a life of its own (thanks OWL for enabling this!), I refactored it's configuration a little bit and moved it to its own tab in the report configuration form.

@CasVissers-360ERP
Copy link

Functional review:

  1. I love the filtering widget and the option to choose a custom search view. Thanks a lot.
  2. The help view for KPI's looks a bit weird (nice2have)

image

3. Thanks for the error handling with incorrect domains. Hopefully saves some future inefficiencies.

Now on the analytic filtering:

I think I'll wait before doing the analytic extension modules I mentioned in #472 (comment) until I have a clearer view of the use cases.

Not sure if this is what you mean but our use-case:
A P&L split by business line, for business lines analytic accounts are used. We need to filter in the P&L using analytic accounts. Currently we do this on KPI's using the analytic account filter.

I was thinking odoo/odoo#116496 might help here, what if we added a filter option on KPI's: "Filter Analytic Accounts" a m2m to account.analytic.account.

When computing amounts, if analytic distribution is set and available on the model, search analytic distributions to find if the analytic account is used. If this is the case we look at the distribution and correct the amount. I know it can be performance heavy, but this is something we can mention in the view? "Using analytic accounts in KPI's might have an impact on performance". Future ideas/changes might fix the performance?


Anyhow from my side no blocking issues to merge this PR. Thanks

@sbidoul
Copy link
Member

sbidoul commented Apr 3, 2023

@CasVissers-360ERP I fixed the help page. Thanks for the testing.

I'm following odoo/odoo#116496 too. It would definitely help, in situations where each move line is allocated 100% in exactly one analytic account per plan. With proper indexing it may even be performant. I hope they land it.

For analytic distributions the only solution I see is a move-line-like SQL view on analytic lines.

@sbidoul
Copy link
Member

sbidoul commented Apr 4, 2023

Let's go :)

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 4fbf94b into OCA:16.0 Apr 4, 2023
5 checks passed
@OCA-git-bot
Copy link
Contributor

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

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.

None yet