Added missing BillingAgreementId key in webhook

[mgorunuch]

Webhook resource contains billing_agreement_id. Exists in type: PAYMENT.SALE.COMPLETED when a subscription is paid.

What does this PR do?

• Added new key in existed type.

Where should the reviewer start?

• only one row

How should this be manually tested?

• create subscription
• pay subscription
• receive PAYMENT.SALE.COMPLETED webhook

Any background context you want to provide?

• not, I'm not.

[plutov]

Please use "ID" instead of "Id" as we do in other identifiers.

[mgorunuch]

done

[thomasf]

The docs here says that Resource is the PaymentResource type and this is a Sale resource (according to to the paypal event) so should not be the same struct.

[thomasf]

a webhook simulator event for reference

{
  "id": "WH-2WR32451HC0233532-67976317FL4543714",
  "create_time": "2014-10-23T17:23:52Z",
  "resource_type": "sale",
  "event_type": "PAYMENT.SALE.COMPLETED",
  "summary": "A successful sale payment was made for $ 0.48 USD",
  "resource": {
    "parent_payment": "PAY-1PA12106FU478450MKRETS4A",
    "update_time": "2014-10-23T17:23:04Z",
    "amount": {
      "total": "0.48",
      "currency": "USD"
    },
    "payment_mode": "ECHECK",
    "create_time": "2014-10-23T17:22:56Z",
    "clearing_time": "2014-10-30T07:00:00Z",
    "protection_eligibility_type": "ITEM_NOT_RECEIVED_ELIGIBLE,UNAUTHORIZED_PAYMENT_ELIGIBLE",
    "protection_eligibility": "ELIGIBLE",
    "links": [
      {
        "href": "https://api.paypal.com/v1/payments/sale/80021663DE681814L",
        "rel": "self",
        "method": "GET"
      },
      {
        "href": "https://api.paypal.com/v1/payments/sale/80021663DE681814L/refund",
        "rel": "refund",
        "method": "POST"
      },
      {
        "href": "https://api.paypal.com/v1/payments/payment/PAY-1PA12106FU478450MKRETS4A",
        "rel": "parent_payment",
        "method": "GET"
      }
    ],
    "id": "80021663DE681814L",
    "state": "completed"
  },
  "links": [
    {
      "href": "https://api.paypal.com/v1/notifications/webhooks-events/WH-2WR32451HC0233532-67976317FL4543714",
      "rel": "self",
      "method": "GET",
      "encType": "application/json"
    },
    {
      "href": "https://api.paypal.com/v1/notifications/webhooks-events/WH-2WR32451HC0233532-67976317FL4543714/resend",
      "rel": "resend",
      "method": "POST",
      "encType": "application/json"
    }
  ],
  "event_version": "1.0"
}

[thomasf]

every resource_type need it's own struct, otherwise it's just a big mess

[thomasf]

If you really need this in the package please create an PaymentSaleResource and do not mix resource types for events. There are at least 10 different resource types just under payment and throwing all of the resource type fields in one struct makes very little organisational sense.

[thomasf]

This is how I have arranged my webhook handlers for paypal.

func (wh *WebhookHandler) PayoutsBatchHandler() httpmiddlewares.AppHandler {
	h := func(w http.ResponseWriter, r *http.Request) error {
		ctx, cancel := context.WithTimeout(r.Context(), defaultRequestTimeout)
		defer cancel()
		logger := log.FromRequest(r)
		var event PaymentPayoutsBatchEvent // package local event type, not in the paypal package right now.
		ok, _ := httputils.ParseRequestData(ctx, w, r, &event)
		if !ok {
			return nil
		}

		logger.Debug().
			Str("event_type", event.EventType).
			Str("event_version", event.EventVersion).
			Interface("event", event).
			Msg("received paypal webhook event")

		{
			var invalid []string
			if event.ResourceType != "payouts" {
				invalid = append(invalid, fmt.Sprintf("expected resource_type payouts, got %s", event.ResourceType))
			}
			if event.EventVersion != "1.0" {
				invalid = append(invalid, "expected event_version 1.0")
			}
			if event.Resource.BatchHeader == nil ||
				event.Resource.BatchHeader.SenderBatchHeader == nil ||
				event.Resource.BatchHeader.SenderBatchHeader.SenderBatchID == "" {
				invalid = append(invalid, "sender batch id not set")
			}
			if len(invalid) > 0 {

				logger.Error().
					Str("event_version", event.EventVersion).
					Str("resource_type", event.ResourceType).
					Strs("errors", invalid).
					Interface("event", event).
					Msg("event validation failed")
				return &errors.ErrorResponse{
					Message:    "invalid event",
					StatusCode: 500,
					Detail:     invalid,
				}
			}
		}
		switch event.EventType {

		case "PAYMENT.PAYOUTSBATCH.DENIED":
			return wh.HandlePayoutsBatchDenied(ctx, w, r, event)

		case "PAYMENT.PAYOUTSBATCH.PROCESSING":
			return wh.HandlePayoutsBatchProcessing(ctx, w, r, event)

		case "PAYMENT.PAYOUTSBATCH.SUCCESS":
			return wh.HandlePayoutsBatchSuccess(ctx, w, r, event)

		default:
			logger.Error().
				Str("event_type", event.EventType).
				Interface("event", event).
				Msg("don't know how to handle event type.")
		}
		return nil
	}
	return h
}

One handler for every individual resource type which dispatches to sub handlers.

But the most important factor is that there isn't a single Resource type that contains all possible fields for all possible resources because that makes it really hard to use.

If the fields needed for this payment resource also were to be added to the Resource type it would need all this (and possibly more because I might have omitted information I don't need) and that is just for two additional resource types. There are probably even conflicting fields between different resource types.

type (
	PaymentPayoutsBatchEventResource struct {
		BatchHeader *PaymentPayoutsBatchEventBatchHeader `json:"batch_header"`
		Items       []paypal.PayoutItemResponse          `json:"items"`
		Links       []paypal.Link                        `json:"links"`
	}

	PaymentPayoutsBatchEventBatchHeader struct {
		paypal.BatchHeader
		CurrencyConversation CurrencyConversation `json:"currency_conversion"`
		Payments             int                  `json:"payments"`
		FundingSource        string               `json:"funding_source"`
	}

	PaymentPayoutsBatchEvent struct {
		paypal.Event
		Resource             PaymentPayoutsBatchEventResource `json:"resource"`
		CurrencyConversation CurrencyConversation             `json:"currency_conversion"`
	}

	CurrencyConversation struct {
		FromAmount   paypal.Currency `json:"from_amount"`
		ToAmount     paypal.Currency `json:"to_amount"`
		ExchangeRate string          `json:"exchange_rate"`
	}

	PaymentPayoutsItemEventResource struct {
		Error             paypal.ErrorResponse `json:"errors,omitempty"`
		Links             []paypal.Link        `json:"links"`
		PayoutBatchID     string               `json:"payout_batch_id,omitempty"`
		PayoutItem        paypal.PayoutItem    `json:"payout_item"`
		PayoutItemFee     paypal.AmountPayout  `json:"payout_item_fee,omitempty"`
		PayoutItemID      string               `json:"payout_item_id"`
		SenderBatchID     string               `json:"sender_batch_id"`
		TimeProcessed     time.Time            `json:"time_processed,omitempty"`
		TransactionID     string               `json:"transaction_id"`
		TransactionStatus string               `json:"transaction_status"`
		ActivityID        string               `json:"activity_id"`
	}

	PaymentPayoutsItemEvent struct {
		paypal.Event
		Resource PaymentPayoutsItemEventResource `json:"resource"`
	}
)

There should preferably never be any confusion about what type you are actually hanlding.

[thomasf]

If all the resource types are to be added to this module they should probably live in their own sub package because it's probably going to be a lot of types.