[IMP] rename_fields : rename field also on custom view like the one used on dashboards#298
[IMP] rename_fields : rename field also on custom view like the one used on dashboards#298daiduongnguyen-odoo wants to merge 1 commit intoOCA:masterfrom
Conversation
daiduongnguyen-odoo
changed the title
daiduongnguyen-odoo
changed the title
|
Hmm, why it said key error? Something is not right |
|
|
daiduongnguyen-odoo
force-pushed
the
imp_rename_field
branch
3 times, most recently
from
9bd2dc3 to
f659e7d
Compare
|
I haved test 2 cases where we gonna use rename_fields function
|
pedrobaeza
left a comment
pedrobaeza
left a comment
There was a problem hiding this comment.
I doubt this is going to be performant enough
openupgradelib/openupgrade.py
Outdated
| ) | ||
|
|
||
|
|
||
| def rename_field_on_dashboard(env, model, old_field, new_field): |
There was a problem hiding this comment.
This should be a private method.
There was a problem hiding this comment.
Maybe you 're right
daiduongnguyen-odoo
force-pushed
the
imp_rename_field
branch
from
f659e7d to
b88c457
Compare
|
What about performance? |
daiduongnguyen-odoo
force-pushed
the
imp_rename_field
branch
2 times, most recently
from
93acdbc to
d2e297c
Compare
I have a few changes, if you have time please review |
|
|
||
| def _rename_field_on_dashboard(env, model, old_field, new_field, | ||
| dashboard_view_data): | ||
| for r in dashboard_view_data: |
There was a problem hiding this comment.
Do the search here and only if the view contains old_field.
There was a problem hiding this comment.
Thanks for the review and suggestion, I'm quite busy at the moment so i will comeback with this ASAP. In the meantime, feel free to add some modification to this PR, i don't mind.
Kind regard,
There was a problem hiding this comment.
@pedrobaeza Forgive me, but is all you are suggesting is (in my simplistic ways)
if old_field not in r.arch:
continue
There was a problem hiding this comment.
More or less, for improving drastically the performance. The previous search shouldn't contain non matched results.
There was a problem hiding this comment.
Done i use ilike search for ir.ui.view.custom that contain the old_field , is that ok? , also i rename the method and handle some of your comment, please review if you have time
|
This PR has the |
pedrobaeza
left a comment
pedrobaeza
left a comment
There was a problem hiding this comment.
Pending the scope of the search for better performance
daiduongnguyen-odoo
force-pushed
the
imp_rename_field
branch
2 times, most recently
from
55648d9 to
60a1ba2
Compare
daiduongnguyen-odoo
force-pushed
the
imp_rename_field
branch
from
60a1ba2 to
2ab6c90
Compare
|
My fear on this is that it affects a lot the performance. Have you performed a migration with and without this and measure the time for both? |
I did, but i haven't measured only test the script (it still lacking some cases). Next time i will try to measure and let you know |
|
At the end, it's to see: before the change, the migration lasts 20 minutes; after the change, it lasts 21 minutes. That's acceptable. But if you check it and it takes 40 minutes, then the performance should be analyzed. |
Yes, understood, will keep that in mind |
6 participants
close #190