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.

Sign up for GitHub

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

Jump to bottom

[16.0][OU-ADD] analytic: Migration done #4052

Closed
wants to merge 1 commit into from
Closed

[16.0][OU-ADD] analytic: Migration done #4052

wants to merge 1 commit into from

Conversation

duong77476-viindoo
Copy link
Contributor

Need test with account also

@duong77476-viindoo duong77476-viindoo changed the title [WIP][16.0][OU-ADD] analytic: Migration partial done [16.0][OU-ADD] analytic: Migration partial done Jul 6, 2023
@duong77476-viindoo duong77476-viindoo force-pushed the v16_mig_analytic branch 2 times, most recently from b223310 to 2209155 Compare July 6, 2023 02:53
duong77476-viindoo
duong77476-viindoo commented Jul 6, 2023
View reviewed changes
openupgrade_scripts/scripts/analytic/16.0.1.1/upgrade_analysis_work.txt
Comment on lines +1 to +7
---Models in module 'analytic'---
obsolete model account.analytic.distribution
obsolete model account.analytic.tag
new model account.analytic.applicability
new model account.analytic.distribution.model
new model analytic.mixin [abstract]
# NOTHING TO DO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This analysis work file is base on newly generation at PR : #4042. I did not edit the original upgrade_analysis only ugrade_analysis_work

openupgrade_scripts/scripts/analytic/16.0.1.1/pre-migration.py Outdated
Comment on lines 22 to 42
"""
We would like to fill for group_id column before start renaming it into plan_id
"""
openupgrade.logged_query(
Copy link
Contributor Author

@duong77476-viindoo duong77476-viindoo Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think in production database it is quite rare to have analytic_account with empty analytic_group right? but still IMO, it need to be filled before changing it into plan

openupgrade_scripts/scripts/analytic/16.0.1.1/pre-migration.py Outdated
Comment on lines 53 to 78
Pre fill value for column root_plan_id using the logic in its compute method to avoid computed by ORM
"""
openupgrade.logged_query(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid computing using ORM for performance

openupgrade_scripts/scripts/analytic/16.0.1.1/upgrade_analysis_work.txt
Comment on lines +24 to +26
analytic / account.analytic.applicability / analytic_plan_id (many2one) : NEW relation: account.analytic.plan
analytic / account.analytic.applicability / applicability (selection) : NEW required, selection_keys: ['mandatory', 'optional', 'unavailable']
analytic / account.analytic.applicability / business_domain (selection) : NEW required, selection_keys: ['general']
Copy link
Contributor Author

@duong77476-viindoo duong77476-viindoo Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this one and the one i comment below should be handled in account module

openupgrade_scripts/scripts/analytic/16.0.1.1/upgrade_analysis_work.txt
Comment on lines +34 to +39
analytic / account.analytic.distribution.model / analytic_distribution (json) : NEW hasdefault: compute
analytic / account.analytic.distribution.model / analytic_distribution_search (json): NEW
analytic / account.analytic.distribution.model / analytic_precision (integer) : NEW hasdefault: default
analytic / account.analytic.distribution.model / company_id (many2one) : NEW relation: res.company, hasdefault: default
analytic / account.analytic.distribution.model / partner_category_id (many2one): NEW relation: res.partner.category
analytic / account.analytic.distribution.model / partner_id (many2one) : NEW relation: res.partner
Copy link
Contributor Author

@duong77476-viindoo duong77476-viindoo Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Same above should handle in account module

@duong77476-viindoo duong77476-viindoo changed the title [16.0][OU-ADD] analytic: Migration partial done [16.0][OU-ADD] analytic: Migration done Jul 6, 2023
@duong77476-viindoo
Copy link
Contributor Author

Anybody help me review pls :((

@legalsylvain
Copy link
Contributor

@remytms, could you take a look over there ? thanks
@duong77476 thanks for your contribution !

@legalsylvain
Copy link
Contributor

/ocabot migration analytic

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Jul 6, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Jul 6, 2023
Open
This was referenced Jul 6, 2023
Merged
Closed
@duong77476-viindoo duong77476-viindoo mentioned this pull request Jul 8, 2023
Merged
@duong77476-viindoo
Copy link
Contributor Author

\ocabot rebase

@duong77476-viindoo
Copy link
Contributor Author

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

Sorry @duong77476 you are not allowed to rebase.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@duong77476-viindoo duong77476-viindoo force-pushed the v16_mig_analytic branch 2 times, most recently from 53864fe to f42e59c Compare July 21, 2023 02:38
katyukha
katyukha reviewed Jul 29, 2023
View reviewed changes
Copy link

@katyukha katyukha left a comment

Choose a reason for hiding this comment

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

It seems that some chanes not related to OCA were added to this PR.

openupgrade_scripts/apriori.py Outdated Show resolved Hide resolved
openupgrade_scripts/apriori.py Outdated Show resolved Hide resolved
openupgrade_scripts/apriori.py Outdated Show resolved Hide resolved
openupgrade_scripts/apriori.py Outdated Show resolved Hide resolved
openupgrade_framework/odoo_patch/odoo/modules/loading.py Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
remi-filament
remi-filament approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@remi-filament remi-filament left a comment

Choose a reason for hiding this comment

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

Thanks @duong77476, tested OK on our database (where typically we did not use analytic groups :))

@OCA-git-bot
Copy link
Contributor

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). 🤖

@pedrobaeza pedrobaeza mentioned this pull request Oct 23, 2023
Merged
@pedrobaeza
Copy link
Member

Thanks for the migration scripts, Nguyễn. Unfortunately, you haven't allowed to push changes on your branch for the maintainers, so I have superseded this PR at #4188, respecting your attribution, rebasing on top of current branch, and adding the following changes:

  • Database structure changes should be always applied before business logic changes.
  • No need to implement literally the compute root_plan_id, as we are creating it for the first time. We just assign it on the accounts.
  • Restructured dummy plan creation, for having the ID available for several queries and not having to make a fuzzy select.
  • For a smooth migration, the default_applicability should be optional, so that users can still continue applying their accounts into documents.
  • Rename default plan to 'Legacy' for better understanding.

@pedrobaeza pedrobaeza closed this Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedrobaeza pedrobaeza pedrobaeza left review comments

@jcdrubay jcdrubay jcdrubay left review comments

@katyukha katyukha katyukha left review comments

@remi-filament remi-filament remi-filament approved these changes

@ngochung207 ngochung207 ngochung207 approved these changes

Assignees
No one assigned
Labels
approved Dependency OK ready to merge
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
8 participants
@duong77476-viindoo @legalsylvain @OCA-git-bot @pedrobaeza @katyukha @jcdrubay @ngochung207 @remi-filament