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

[17.0][OU-ADD] payment: Migration scripts #4584

Closed
wants to merge 2 commits into from

Conversation

remi-filament
Copy link
Contributor

@remi-filament remi-filament commented Oct 11, 2024

Take over payment migration from PR #4466 upon @duong77476-viindoo request (#4466 (comment))

Extra tasks performed :

  • Remove payment.payment_method_acss_debit from noupdate_changes since this module does not exist anymore
  • Rename model payment.icon to payment.method
  • Reload payment methods

@rvalyi
Copy link
Member

rvalyi commented Oct 11, 2024

for reference, the 5 key commits for v17: https://github.com/akretion/odoo-module-diff-analysis/tree/main/17.0/payment

@remi-filament remi-filament force-pushed the 17.0-payment branch 2 times, most recently from d7cf356 to 0944bd0 Compare October 18, 2024 09:10
haumenphai and others added 2 commits October 18, 2024 11:11
Remove payment.payment_method_acss_debit as does not exist anymore
Move payment.icon to payment.method
Reload payment methods and links to providers
@remi-filament
Copy link
Contributor Author

Thanks @rvalyi it helps understanding where the changes come from (although in that case many changes were on data so there are more commits to take into account).
I added migration script to transform payment.icon in payment.method, even if I am not sure it was worth the effort since they changed pretty much everything, it would probably have been better to just drop everything and let ORM recreate proper values...

@legalsylvain
Copy link
Contributor

legalsylvain commented Oct 24, 2024

/ocabot migration payment

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Oct 24, 2024
@OCA-git-bot

This comment was marked as outdated.

Copy link
Member

@hbrunn hbrunn left a comment

Choose a reason for hiding this comment

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

Totally agree with renaming payment.icon to payment.method, as that will be the base for whatever migrations of modules depending on payment do.

Not so sure yet about the full reload of the payment methods, shouldn't we only force load new fields like primary_payment_method_id? Or the other way around, not touch active and sequence?

Comment on lines +15 to +25
for payment_token in PaymentToken.search([("payment_method_id", "=", False)]):
payment_token.payment_method_id = (
PaymentMethod._get_from_code(payment_token.provider_id.code)
or unknown_payment_method
).id

for transaction in PaymentTransaction.search([("payment_method_id", "=", False)]):
transaction.payment_method_id = (
PaymentMethod._get_from_code(transaction.provider_id.code)
or unknown_payment_method
).id
Copy link
Member

Choose a reason for hiding this comment

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

can we group those by provider_id and do only one write per provider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for you review @hbrunn
Sorry I did not come back on this before, I have had a crazy loaded month !
Thanks for fixing this in 4ec754c

"payment_provider",
"payment_icon_ids",
"payment_method_ids",
),
Copy link
Member

Choose a reason for hiding this comment

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

for this to work we'll also have to rename payment_icon_payment_provider_rel I think, and in there payment_icon_id to payment_method_id (I wonder if we should actually add this to rename_fields, seems a common enough task)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right of course, however since link between provider and methods completely changed in v17, I thought that maybe it was better to let ORM recreate the full table.
Now I see that you may have extra payment provider (not provided by Odoo) that would need to keep their corresponding methods, so it makes sense to migrate those as you did in 4ec754c

@hbrunn
Copy link
Member

hbrunn commented Dec 9, 2024

@remi-filament I've implemented my suggestions in #4692

@hbrunn hbrunn closed this Dec 9, 2024
@remi-filament
Copy link
Contributor Author

Not so sure yet about the full reload of the payment methods, shouldn't we only force load new fields like primary_payment_method_id? Or the other way around, not touch active and sequence?

The problem here is that before you only had 3 fields in data : sequence, name, image (https://github.com/odoo/odoo/blob/16.0/addons/payment/data/payment_icon_data.xml)
Now you have a lot more, and payment method data becomes noupdate=1 (https://github.com/odoo/odoo/blob/17.0/addons/payment/data/payment_method_data.xml).

Note that active and primary_payment_method_id did not exist before in v16.

Therefore I believe there were 2 ways to handle this :

  1. drop all existing payment methods from v16 and let ORM recreate them (and maybe keep only the ones created by modules outside Odoo core ?)
  2. try to transform old payment.icon XML-IDs to new payment.method XML-IDs and set force update on these new XML-IDs and update data afterwards (which is what I tried to achieve here, letting Odoo create all the new methods that did not map to icons in v16).

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