Membership is handled automatically ... leave it alone!

Comment out unused code
Add note re Membership Dates

CRM/GoCardlessUtils.php

@@ -234,14 +234,17 @@ public static function completeRedirectFlowCiviCore($deets)
           $installments = $result['installments'];
         }
 
-      } elseif (!empty($deets['membershipID'])) {
-        // This is a membership. Load the interval from the type.
-        $result = civicrm_api3('Membership', 'getsingle',
-          ['id' => $deets['membershipID'], 'api.MembershipType.getsingle' => []]
-        );
-        $interval_unit = $result['api.MembershipType.getsingle']['duration_unit'];
-        $interval = $result['api.MembershipType.getsingle']['duration_interval'];
       }
+      // This section of code might be useful if we implement one-off payment, but until then
+      // all cases should have a recurring contribution.
+      // elseif (!empty($deets['membershipID'])) {
+      //   // This is a membership. Load the interval from the type.
+      //   $result = civicrm_api3('Membership', 'getsingle',
+      //     ['id' => $deets['membershipID'], 'api.MembershipType.getsingle' => []]
+      //   );
+      //   $interval_unit = $result['api.MembershipType.getsingle']['duration_unit'];
+      //   $interval = $result['api.MembershipType.getsingle']['duration_interval'];
+      // }
       else {
         // Something is wrong.
         throw new Exception("Failed to find interval details");
@@ -322,19 +325,6 @@ public static function completeRedirectFlowCiviCore($deets)
         ]);
       }
 
-      if (!empty($deets['membershipID'])) {
-        // Calculate the end date for the membership, although hopefully this will be renewed automatically.
-        // People expect their membership to start immediately, although the payment might not come through for a couple of days.
-        // Use today's date as the start date, and contribution date + N * interval for end date.
-        // Update membership dates.
-        civicrm_api3("Membership" ,"create" , [
-          'id'         => $deets['membershipID'],
-          'end_date'   => date('Y-m-d', strtotime($subscription->start_date . " + $interval $interval_unit")),
-          'start_date' => date('Y-m-d'),
-          'join_date'  => date('Y-m-d'),
-          'status_id'  => 'Pending', // https://github.com/artfulrobot/uk.artfulrobot.civicrm.gocardless/issues/28
-        ]);
-      }
       CRM_Core_Error::debug_log_message(__FUNCTION__ . ": CiviCRM updated successfully (Contribution ID $deets[contributionID]).", FALSE, 'GoCardless', PEAR_LOG_INFO);
     }
     catch (Exception $e) {

MembershipDates.md

@@ -0,0 +1,18 @@
+# Membership Dates
+
+In a simple world, someone fills in a membership form, pays by credit card and their membership is active immediately.
+
+In the DD world, things happen on different dates:
+1. `setup_date` - fill in a membership form, complete a DD mandate
+   - Recurring Contribution created with status Pending
+   - Contribution created with status Pending
+   - Membership created  with status Pending, `join_date = start_date = setup_date`
+2. `charge_date` - first payment charged
+3. `webhook_date` - GC fires webhook and notifies of `charge_date`
+   - Recurring Contribution updated to In Progress
+   - Contribution updated to status Completed, `receive_date` updated to `charge_date`
+   - Membership updated to status New, `join_date` unchanged, `start_date` updated to `webhook_date`, `end_date` updated to `start_date` + membership_length
+
+This appears to be identical date behaviour to creating a membership with a pending cheque payment and then later recording the cheque as being received.  The membership will start when the payment is recorded and the contribution status set to Completed.  The end date is recalculated to be a year (or other membership length) from the start date so that members get a full year of benefit.
+
+However ... some might want the membership to start as soon as the mandate is setup, before waiting for the first payment.  The current date logic is handled by core so this extension would need to override that in a couple of places to implement a different scheme.  Since that is not specific to this payment processor, it might be better to do this as an enhancement to core, or a separate extension.