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

[13.0][MIG] stock_account #2376

Merged
merged 5 commits into from
Apr 16, 2021
Merged

Conversation

MiquelRForgeFlow
Copy link
Contributor

Migration of stock_account.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-mig-stock_account-script branch from 640d390 to a154932 Compare January 21, 2021 10:59
@MiquelRForgeFlow
Copy link
Contributor Author

Ready @pedrobaeza

@StefanRijnhart
Copy link
Member

What a data model nightmare creating these valuation records. Have you got any statistics about the duration of this process, iterating over every done stock move?

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-mig-stock_account-script branch from a154932 to fffa2ad Compare February 17, 2021 18:41
@MiquelRForgeFlow
Copy link
Contributor Author

I have added the tracking_disable=True to speed up a bit.

@StefanRijnhart
Copy link
Member

Is the performance now reasonable? Have you got an indication?

@MiquelRForgeFlow
Copy link
Contributor Author

MiquelRForgeFlow commented Feb 19, 2021

@StefanRijnhart A client bbdd with 258500 stock moves (221880 done moves) -> 7 hours 22 min (duration of this post-migration)

@kos94ok-3D
Copy link

@MiquelRForgeFlow Is product.product call tracking?

@MiquelRForgeFlow
Copy link
Contributor Author

No, because I disabled tracking as you can see in the code.

@kos94ok-3D
Copy link

kos94ok-3D commented Feb 19, 2021

I ask why you added it

@MiquelRForgeFlow
Copy link
Contributor Author

Because the tracking is not needed and can generate side issues.

@kos94ok-3D
Copy link

Ok, What do you think about use SQL instead of call _compute_value_svl.
I call _compute_value_svl because the product doesn`t have in cache correct value when I create SVL by SQL

@MiquelRForgeFlow
Copy link
Contributor Author

I don't know. Maybe doable by putting some explicit flush() calls to avoid the cache issue?

@kos94ok-3D
Copy link

I tried but don`t work

@kos94ok-3D
Copy link

@MiquelRForgeFlow It has to save ~30% time:kos94ok-3D@8c5405f

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-mig-stock_account-script branch 2 times, most recently from 112fd61 to 2d3f2c4 Compare February 19, 2021 15:28
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-mig-stock_account-script branch from 2d3f2c4 to c09e2f4 Compare February 19, 2021 18:24
@kos94ok-3D
Copy link

Hi, @pedrobaeza. Do you have a planned date for review it?

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-mig-stock_account-script branch from 2509528 to c7b3e3b Compare March 29, 2021 14:27
Copy link

@kos94ok-3D kos94ok-3D left a comment

Choose a reason for hiding this comment

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

I think we have to fill it
изображение
It was done in my version:
изображение
And will be good if we link SVL to Price history

@AaronHForgeFlow
Copy link

@kos94ok-3D How did you manage to get the account move information to introduce it in the layer? I think it would be great if we can preserve that information.

@kos94ok-3D
Copy link

@kos94ok-3D How did you manage to get the account move information to introduce it in the layer? I think it would be great if we can preserve that information.

def _get_related_account_move(env, svl_vals):
    """ Return Account move related to Stock Valuation Layer"""
    domain = []
    if svl_vals.get('stock_move_id'):
        domain = expression.AND([domain, [('stock_move_id', '=', svl_vals['stock_move_id'])]])
    if svl_vals.get('old_product_price_history_id'):
        env.cr.execute("""
            SELECT create_date
            FROM product_price_history
            WHERE id = %s
        """, (svl_vals['old_product_price_history_id'],))
        create_date = env.cr.fetchone()[0]
        domain = expression.AND([domain, [('create_date', '=', create_date)]])
    if not domain:
        return env['account.move']
    account_moves = env['account.move'].search(domain)
    if len(account_moves) > 1:
        return env['account.move']
    return account_moves


@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-mig-stock_account-script branch from c7b3e3b to 8110eea Compare April 6, 2021 15:48
@MiquelRForgeFlow
Copy link
Contributor Author

@kos94ok-3D If we add your account move filling method, it will affect badly the performance or not?

@kos94ok-3D
Copy link

@MiquelRForgeFlow I think my function needs to rewrite to SQL, I sent it just as an example.

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-mig-stock_account-script branch 2 times, most recently from 4e42b2a to a0bc755 Compare April 9, 2021 16:54
@MiquelRForgeFlow
Copy link
Contributor Author

@kos94ok-3D @AaronHForgeFlow Added the account move information in a neat way :)

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-mig-stock_account-script branch 2 times, most recently from ddff843 to c621dcd Compare April 9, 2021 17:15
Copy link

@kos94ok-3D kos94ok-3D left a comment

Choose a reason for hiding this comment

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

Required because old_product_price_history_id not filled if first row does not content it
изображение

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-mig-stock_account-script branch from c621dcd to 087c253 Compare April 9, 2021 18:03
@kos94ok-3D
Copy link

It is ok for me

@AaronHForgeFlow
Copy link

For the fifo case the quantities of the layers is well migrated for the test I have performed so far. However, for the outgoing stock moves, the value in the layers is not correct. For example in standard Odoo 13:

layer_in_1: qty: 10, unit_value: 25, total_value: 250
layer_in_2: qty: 10, unit_value: 15, total_value: 150

the layer out takes 10 units from layer_in_1 and 2 units from layer_in_2:
layer_out_1: qty: 12 unit_value: 23.33 total_value: 280

In a migrated database:

layer_in_1: qty: 10, unit_value: 25, total_value: 250
layer_in_2: qty: 10, unit_value: 15, total_value: 150

layer_out_1: qty: 12 unit_value: 20 total_value: 240

I will propose the changes that fixes this use case here.

Thanks all for the work.

Copy link

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

I think we can merge this now, and then if any issue appears create another PR for that case.

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.

6 participants