[Ubercart] Migrate payments

I think we can revisit this now right?

Hi, I haven't improved the functionality much, but I've rebuilt the patch to work with the latest dev version of commerce_migrate. I've adjusted the file locations based on the new structure, and changed the currency from fixed to a variable_get similar to the order_product migrate.
Note that this still needs work...

- If you don't have a payment gateway set up, it doesn't import, right now it throws the following error:

Drupal\Core\Entity\EntityMalformedException: Required payment field "payment_gateway" is empty. in Drupal\commerce_payment\Entity\Payment->preSave() (line 282 of /modules/contrib/commerce/modules/payment/src/Entity/Payment.php). [error]
Required payment field "payment_gateway" is empty. (/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:777)                                                                                                                [error]

So basically we need to decide what to do here, could we set up a dummy payment gateway temporarily and set all payments to a gateway of "Other"?

- there are some test things in the migrate template yml
- There are some fields commented out which I don't know if we need or what to do with
- There are references to "refunded_amount". In U6 you dont refund payment items, you create a new payment item with a negative value so this isn't relevant. Can we just remove it?

Correct, in the days of U6, refunds initiated from ubercart didn't exist. The payment module was more like a log so anything less than 0 is a refund, correct.

The most important thing right now to get this working is a decision on the payment gateway issue. Can we make the migrate module require an "other" type payment gateway on install or something? I don't see migrating payment gateways being feasible really

I think for the payment gateway question, I would suggest we add a hook_requirements stating you must configure a payment gateway before enabling the module. Then add an extra layer of protection by throwing an exception in the migration if the gateway was removed and no longer exists.

It still leaves a lingering question of what we want to do for these payments, since we don't want to errantly let someone refund the thing via their new payment gateway. Maybe we need to build a require a 'migrated' payment gateway (based on manual) that is hidden/disabled so no new payments can use it. This new module should retain the legacy payment gateway's 'name/title', in case we want to see how/where it was sourced from. This gateway shouldn't exist in commerce_migrate because we want to disable migrate later on. It could be a required contrib module. Don't know. I prefer a new gateway over manual, because the site could accept new manual payments later on and I wouldn't want to confuse those new payments with the earlier migrated payments.

1. +++ b/modules/ubercart/src/Plugin/migrate/source/d6/Payment.php
   @@ -0,0 +1,97 @@
   +    // Ubercart 6 stores credit card information in a hash. Since this probably
   +    // isn't necessary so I removed it here.
   

   Comment suggestion: Ubercart 6 stores credit card information in a hash. Remove it.

2. +++ b/modules/ubercart/src/Plugin/migrate/source/d6/Payment.php
   @@ -0,0 +1,97 @@
   +    unset($data['cc_data']);
   

   I don't see any examples of this in the source test data. Do we need to test that?

I was thinking about my response in #20. I think we can use the manual gateway. And extend/use entity_generate from migrate_plus to generate the manual payment_gateway in D8 based on the name of what it was in D6. This would allow us to import payments into a 'authorize.net' or 'paypal' manual gateway. If a site needed to accept real/actual manual payments, they could easily just build one itself.

If we need to patch migrate_plus to support generation of config entities with entity_generate, we can do that.

OK, so you are saying that we need to require the manual gateway module (is this a core or contrib module?) and thus with this we can import payments without actually processing them and match the single payment gateway to all payment lines?
In most projects I've worked on there are multiple payment gateways (uc_sermepa + uc_paypal), would it be possible to import multiple gateways and associate them on a per payment basis?

Checking the database model in D6, there is no relationship between gateways and payments, just "method" which we already match to "payment_method". Maybe, at least for D6, a single payment gateway would be fine, called say "Others", and we could simply import the methods as is. The methods I have on a site are:

Credit Card
paypal_ec
Other

@willeaton manual is part of commerce core.

@heddn, how should I go about this? Set up the payment gateway in a preprocess of the payment migrate or create a new gateway migration which sets up manual gateways for each gateway previously installed?

re #26: I suggest using entity_generate from migrate_plus to build the gateway. If you need to patch the process plugin to get it working, coordinate with me. There's a couple issues in the queue that has aspirations and we could probably fit it into one of them. Hopefully entity_generate "Just Works (tm)"

I'll work on this the next few days. I'll probably need some support on this though

OK, here is my updated patch so far. Points done:

• Written code to handle refunds (untested)
• Gone through the lines and removed comments and lines we are not going to migrate
• Lines are simply migrated to a manual payment gateway, there seems to be no way to migrate even the name of the previous gateway, or its comments

Points still left to do:

• Set up the entity_generate to pre-create the payment gateways
• Create multiple payment gateways for each found in D6
• Test refund payments

OK, here is a new updated patch. Points done:

• Written code to handle refunds
• Gone through the lines and removed comments and lines we are not going to migrate
• Lines are simply migrated to a manual payment gateway using the same name as before, there seems to be no way to migrate its comments
• Set up the entity_generate to pre-create the payment gateways (separate migration)
• Create multiple payment gateways for each found in D6
• Tested refund payments and they work but could fail quite easily with weird data

Points still left to do:

• Create payment migrate tests

As far as Im concerned, this is a working migration now. Time to test. Could someone please help me write the tests?

Not sure why it failed, seems to be an error with the test environment to me. Can anyone help so we can close this?

Hi, I can write some documentation in about 12 hours, probably best that I do it. As for the tests I can probably provide the data in the fixture but I still need to learn how to write tests.... :-)

There is no need for any uc_variables added to the fixture to my knowledge, I haven't added these.

Hi!

AFAIKT, the configuration is an array on the payment gateway entity. So, removing this.

The thing is, if you remove this, where do we provide the payment gateway config? We are creating a new payment gateway for each payment type used in the source site, with specific configuration for these gateways to not appear in the checkout later on (internal only). The config I pass here are the values of the required fields you have to fill in when creating a payment gateway via the UI. Not sure how it could still work without these fields. Obviously I know very little about config_entity migrations so I may be wrong :-)

My proposed documentation for this migration below. Please advise where best to add this, its not short.

This file handles the migration of the Drupal 6 Ubercart payment log. In Ubercart 2 of Drupal 6, the payment table is more like a log than anything else. There is a dedicated table with 1 row per payment. A payment can have a positive or negative value (refunds), and merely references a text value of the payment gateway. There is no attached functionality or relationships between a payment entry and the payment gateway.

In Drupal 8 commerce 2 a payment is directly associated to a payment gateway. There is one line per payment however refunds are associated to a specific payment. It is not possible for a single refund to have a higher value than that of a single payment. For example, you cannot have a 2 payments of 40€ totalling 80€ and associate 80€ of a refund to one of the payments. You would do a full refund of the two entries. This is the big difference between Ubercart 2 and Commerce 2.

Firstly, in order to set up the payment entries in Commerce 2, we have to create manual payment gateways. This is handled in a separate migrate (see d6_ubercart_payment_gateway). We have chosen to associate old payment entries with manual payment gateways, instead of associating them to the new payment modules for 2 reasons. Firstly we can’t guarantee that the module exists in Drupal 8 for the old payment gateway, nor that it has a migration prepared. Secondly, in Commerce 2, refunds associated to a real payment gateway might trigger an actual refund of the money. Imagine doing a migration and actually triggering a refund of all the money you ever charged your customers. The payment gateways are set to manual mode and are set to not appear in the checkout by default. The idea is that they exist for historic purposes, they are not intended for use after migration.

Once the payment gateways are migrated, and the orders and stores too, the payments are migrated. Its fairly straight forward, for every payment line in Ubercart 2, we add a payment line in Commerce 2. The complication comes when there is a refund. This migration plugin attempts to associate a refund to a previously migrated payment. This process is unconventional and not encouraged by the migrate team, but has been implemented for lack of a better solution. When the migration finds a refund, it will query the payments that have already been migrated for that order, find one that is larger than the refund value, and use that payment id as the destination for the refund instead of its own line.

Several things are assumed.
1. No order has a higher amount refunded than paid. This would have to be fixed manually.
2. No order has a refund before the first payment. This would have to be fixed manually.
3. No single refund is larger than all individual payments made. This would have to be fixed manually.

Looking really good. Just a couple small nits. Plus, it looks like we want an IS update? Which would help, since this has gone a couple directions.

Plus, we should add some developer docs as per #58.

1. +++ b/modules/ubercart/migrations/d6_ubercart_order_payment.yml
   @@ -0,0 +1,45 @@
   +  # typ payment_manuak
   +  #payment_gateway - test /test2
   

   Copy/pasta

2. +++ b/modules/ubercart/migrations/d6_ubercart_payment_gateway.yml
   @@ -0,0 +1,18 @@
   +  plugin:
   +    plugin: default_value
   +    default_value: manual
   

   static values in source would be more clear that there isn't any source for this.

3. +++ b/modules/ubercart/src/Plugin/migrate/source/d6/OrderPayment.php
   @@ -0,0 +1,125 @@
   +    // If this is a refund we need ot attach it to a payment.
   

   ot = to.

4. +++ b/modules/ubercart/src/Plugin/migrate/source/d6/OrderPayment.php
   @@ -0,0 +1,125 @@
   +      // Take first payment and see if its a positive amount.
   

   Nit: it is or it's.

5. +++ b/modules/ubercart/src/Plugin/migrate/source/d6/OrderPayment.php
   @@ -0,0 +1,125 @@
   +      // Final check that it is a positive amount otherwise fail.
   

   Finally, check that is...

1. --- /dev/null
   +++ b/modules/ubercart/migrations/d6_ubercart_payment.yml
   
   @@ -0,0 +1,42 @@
   +  constants:
   +    type: payment_manual
   
   --- /dev/null
   +++ b/modules/ubercart/migrations/d6_ubercart_payment_gateway.yml
   
   @@ -0,0 +1,18 @@
   +  id:
   +    plugin: machine_name
   +    source: method
   

   This doesn't seem to match. Is it supposed to? I'd expect the type to be the same for the payment and for the payment gateway. Maybe it shouldn't?

2. +++ b/modules/ubercart/src/Plugin/migrate/destination/d6/CommercePayment.php
   @@ -0,0 +1,142 @@
   +   * Builds an user entity destination.
   

   Copy/pasta.

3. +++ b/modules/ubercart/src/Plugin/migrate/destination/d6/CommercePayment.php
   @@ -0,0 +1,142 @@
   +          $diff = Calculator::subtract($paid_amount, $total_refund_amount);
   

   This depends on commerce_payment module. Do we need to add that to the list of requirements in .info.yml or should we somehow do a check if the payment module is enabled before installing this config.

I just thought of another option. Rather than doing the info.yml entry, what if you put it into optional config and did something like what is discussed in http://lightning.acquia.com/blog/optional-config-weirdness-drupal-8. And find a stable config dependency in payment you can depend upon.

I feel bad about throwing this back on such small nits.

+++ b/modules/ubercart/tests/src/Kernel/PaymentTest.php
@@ -0,0 +1,68 @@
+      try {
+        // If this test passes, a PluginNotFoundException os thrown and we'll
+        // never reach fail().

s/os/is/

I think you can do $this->expectException(PluginNotFoundExceptionption::class);. See https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html#writ...

All feedback addressed.