[10.0][MIG] hr_expense - #1001
Conversation
|
Hey @remi-filament, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
| # Set correct account corresponding to product subject to this expense | ||
| cr.execute("SELECT distinct he.product_id, pp.product_tmpl_id from hr_expense he, product_product pp where he.product_id=pp.id") | ||
| product_ids = cr.fetchall() | ||
| for product in product_ids: |
There was a problem hiding this comment.
This can go wrong in several ways. How about making this company aware by fetching the product's account through the ORM. If you browse the product with force_company=company.id in the context, you get the correct account automatically that you can write on the expenses. Update the expenses per product per company in one big sweep to prevent all too many lookups or write operations.
There was a problem hiding this comment.
Initially I tried to call a function to get the account associated to each product from the product addon (which retrieves it from ir_property table) but did not manage to figure out how to do that...
That would have been much cleaner I agree.
Regarding the specifics of your comment, I am not sure about what you propose since the account is not linked to the company, there are so many accounts per company. However the account is linked to the product template and therefore should be retrieved from there in my view...
| am.id as id, am.date as date | ||
| FROM account_move am | ||
| ) AS subquery | ||
| WHERE hr_expense_sheet.account_move_id = subquery.id |
There was a problem hiding this comment.
More readable, if not faster:
UPDATE hr_expense_sheet hes
SET accounting_date = am.date
FROM account_move am
WHERE hes.account_move_id = am.id;
|
|
||
| @openupgrade.migrate() | ||
| def migrate(env, version): | ||
| openupgrade.copy_columns(env.cr, column_copies) |
There was a problem hiding this comment.
Is this necessary? We like to preserve data in OpenUpgrade, but you are literally copying the data to the related sheet already in the post script.
There was a problem hiding this comment.
You are right, I was not sure whether the lines would not have disappeared before running post-migration, which is why I wanted to make sure they were not lost.
From your comment I understand that this is not the case so I will drop this pre-migration file.
| cr.execute( | ||
| '''UPDATE hr_expense_sheet | ||
| SET state='submit' | ||
| WHERE state='draft' |
There was a problem hiding this comment.
Glancing over the code in the Odoo module, it seems that hr.expense records in draft state can stay that way, and don't need a sheet because submitting the expense will create the sheet: https://github.com/odoo/odoo/blob/10.0/addons/hr_expense/models/hr_expense.py#L103
| cr.execute( | ||
| '''UPDATE hr_expense | ||
| SET state='reported' | ||
| WHERE state='submit' or state='approved' |
There was a problem hiding this comment.
New state approve for old state approved, I think.
| cr.execute( | ||
| '''UPDATE hr_expense | ||
| SET state='done' | ||
| WHERE state='post' or state='done' |
There was a problem hiding this comment.
But the post state is still possible, isn't it?
| # Done - Update states to fit with new definition | ||
|
|
||
| hr_expense / hr.expense / website_message_ids (one2many): DEL relation: mail.message | ||
| # Not present if no website..., I do not know what should be done here... |
There was a problem hiding this comment.
You can ignore this. You can write a comment # Nothing to do.
|
Hi @remi-filament, thank you for your contribution. I have some comments and I am looking forward to your follow-up. Did you send your contribution-license-agreement to the OCA already? Let me know if you have any questions about all that. |
|
Hi @StefanRijnhart , thank you for your comments, really useful ! |
|
Hey @remi-filament, Appreciation of efforts, |
- Cut line to remains under 120 char - Add a limit 1 on select to get only one account - Rework the last update command
|
@StefanRijnhart I have reviewed your comments in details, and I think we need to clarify the new states that are now in use for hr_expense modules. Therefore the products and accounts are defined in hr_expense still when the accounting information are covered in hr_expense_sheet now. The states have also changed accordingly. These were the states present before on hr_expense in V9:
And we now have on hr_expense:
And on hr_expense_sheet:
This is however my undestanding of the changes performed, explaining the mapping I tried to make. Please let me know your view. |
|
Thanks! merging this, so the work can continue on #1130 |
3 participants
Description of the issue/feature this PR addresses:
hr_expense module migration for V10
Current behavior before PR:
Migration of hr_expense module not initialized
Desired behavior after PR is merged:
Migration of hr_expense module is performed
--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr