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

[16.0][OU-ADD] payment #3918

Closed
wants to merge 1 commit into from
Closed

[16.0][OU-ADD] payment #3918

wants to merge 1 commit into from

Conversation

remytms
Copy link
Contributor

@remytms remytms commented Jun 19, 2023

Changes:

  • acquirer is now provider
  • some fields and xmlids moved from payment to account_payment

Details in the upgrade_analysis_work file.

Testing is partial, because there is no migration script for the account module. Therefore the upgrade fails. But at least, the migration script for payment does not fail.

@remytms remytms changed the title [OU-WIP] payment [16.0][OU-WIP] payment Jun 20, 2023
@remytms remytms force-pushed the 16-add-payment branch 9 times, most recently from de9ae8c to eac477a Compare June 20, 2023 16:04
@remytms remytms changed the title [16.0][OU-WIP] payment [16.0][OU-ADD] payment Jun 20, 2023
@remytms remytms marked this pull request as ready for review June 20, 2023 16:43
@remytms
Copy link
Contributor Author

remytms commented Jun 20, 2023

@legalsylvain Can you reference this PR with the OCABot ? :)

@legalsylvain
Copy link
Contributor

/ocabot migration payment

Comment on lines 375 to 389
DEL payment.acquirer: payment.payment_acquirer_adyen (noupdate)
DEL payment.acquirer: payment.payment_acquirer_alipay (noupdate)
DEL payment.acquirer: payment.payment_acquirer_authorize (noupdate)
DEL payment.acquirer: payment.payment_acquirer_buckaroo (noupdate)
DEL payment.acquirer: payment.payment_acquirer_mollie (noupdate)
DEL payment.acquirer: payment.payment_acquirer_ogone (noupdate)
DEL payment.acquirer: payment.payment_acquirer_paypal (noupdate)
DEL payment.acquirer: payment.payment_acquirer_payulatam (noupdate)
DEL payment.acquirer: payment.payment_acquirer_payumoney (noupdate)
DEL payment.acquirer: payment.payment_acquirer_sepa_direct_debit (noupdate)
DEL payment.acquirer: payment.payment_acquirer_sips (noupdate)
DEL payment.acquirer: payment.payment_acquirer_stripe (noupdate)
DEL payment.acquirer: payment.payment_acquirer_test (noupdate)
DEL payment.acquirer: payment.payment_acquirer_transfer (noupdate)
# NOTHING TO DO: replaced by payment.provider element (see below)
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure about this? The user could have put modifications in these records that will not be carried over to the newly created records.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure there is no change by the user on theses data. It contains data like this:

    <record id="payment_acquirer_adyen" model="payment.acquirer">
        <field name="name">Adyen</field>
        <field name="display_as">Credit Card (powered by Adyen)</field>
        <field name="image_128" type="base64" file="payment_adyen/static/src/img/adyen_icon.png"/>
        <field name="module_id" ref="base.module_payment_adyen"/>
        <field name="description" type="html">
            <p>
                A payment gateway to accept online payments via credit cards, debit cards and bank
                transfers.
            </p>
            <ul class="list-inline">
                <li class="list-inline-item"><i class="fa fa-check"/>Online Payment</li>
                <li class="list-inline-item"><i class="fa fa-check"/>Payment Status Tracking</li>
            </ul>
        </field>
        <!-- https://www.adyen.com/payment-methods -->
        <field name="payment_icon_ids"
               eval="[(6, 0, [
                   ref('payment.payment_icon_cc_bancontact'),
                   ref('payment.payment_icon_cc_maestro'),
                   ref('payment.payment_icon_cc_mastercard'),
                   ref('payment.payment_icon_cc_visa'),
                   ref('payment.payment_icon_cc_discover'),
                   ref('payment.payment_icon_cc_diners_club_intl'),
                   ref('payment.payment_icon_cc_jcb'),
                   ref('payment.payment_icon_cc_unionpay'),
               ])]"/>
    </record>

to

    <record id="payment_provider_adyen" model="payment.provider">
        <field name="name">Adyen</field>
        <field name="display_as">Credit Card (powered by Adyen)</field>
        <field name="image_128" type="base64" file="payment_adyen/static/description/icon.png"/>
        <field name="module_id" ref="base.module_payment_adyen"/>
        <!-- https://www.adyen.com/payment-methods -->
        <field name="payment_icon_ids"
               eval="[(6, 0, [
                   ref('payment.payment_icon_cc_bancontact'),
                   ref('payment.payment_icon_cc_maestro'),
                   ref('payment.payment_icon_cc_mastercard'),
                   ref('payment.payment_icon_cc_visa'),
                   ref('payment.payment_icon_cc_discover'),
                   ref('payment.payment_icon_cc_diners_club_intl'),
                   ref('payment.payment_icon_cc_jcb'),
                   ref('payment.payment_icon_cc_unionpay'),
               ])]"/>
    </record>

Copy link
Member

Choose a reason for hiding this comment

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

Users can absolutely modify these records.

Screenshot from 2023-07-03 18-09-56

In any case, currently after migration, you just get duplicates of everything:

> select id, name from payment_provider where cast(name->'en_US' as text) ilike '%mollie%';
+----+---------------------+
| id | name                |
|----+---------------------|
| 5  | {"en_US": "Mollie"} |
| 23 | {"en_US": "Mollie"} |
+----+---------------------+
SELECT 2

Copy link
Contributor Author

@remytms remytms Jul 7, 2023

Choose a reason for hiding this comment

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

Hum, indeed. I did not watch other field of the model. I will look deeper at this.

Thanks for pointing this out. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that I fixed it. :)

Renamed xmlids, added payment_provider in noupdate_changes, and loaded it.

Changes:

- acquirer is now provider
- some fields and xmlids moved from payment to account_payment

Details in the upgrade_analysis_work file.

Loads noupdate_changes.
Copy link
Contributor

@stefan-tecnativa stefan-tecnativa left a comment

Choose a reason for hiding this comment

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

LGTM! As account has already been merged, this can go on as well? TT42266

@pedrobaeza

@pedrobaeza
Copy link
Member

Hi, @remytms, thanks for the migration.

I have redone it in #4193 (as I can't push here) for being totally correct. Please take notes for other scripts you do of the following comments:

  • No need to comment each line on the work file. If several of them apply the same comment, just group them and put the comment at the end of the block. It's also OK to reorder the lines to make the blocks larger.
  • The same applies if there are non consecutive lines and both refers to the same action (model removed and model added for example): reorder them to go together in the same block with the same comment.
  • There's no need anymore to use update_module_moved_fields. It's covered natively in OpenUpgrade till version 14, and by Odoo since them: https://github.com/OCA/openupgradelib/blob/d0aee908031876a95dc36c7a9a2c847186155aa2/openupgradelib/openupgrade.py#L3039-L3040
  • There were some XML-IDs renamings that are not real, as they don't exist in target (Alipay, Ogone, etc). I have remove them.
  • And for the remaining noupdate old records, you have to try to remove them with the corresponding method.
  • For new noupdate records, there's nothing to do, so noupdate changes are not needed. They are new!
  • And finally, for the noupdate changes, only real changes should be put. If not, any customization done by the user is lost, so better to keep changes at minimum.

@pedrobaeza pedrobaeza closed this Oct 27, 2023
@remytms
Copy link
Contributor Author

remytms commented Oct 28, 2023

Hi @pedrobaeza

Thanks for your comments. I take good note of your advice. :)

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.

7 participants