From 608284309a601fc558afd0f23eb8b3ee4cf5ebc7 Mon Sep 17 00:00:00 2001 From: Hugo Osvaldo Barrera Date: Fri, 18 Mar 2022 18:33:56 +0100 Subject: [PATCH] Use a JSONField for BasePayment.extra_data --- CHANGELOG.rst | 5 ++ docs/backends.rst | 2 +- payments/cybersource/__init__.py | 18 +++---- payments/cybersource/forms.py | 4 +- payments/cybersource/test_cybersource.py | 14 ++--- payments/mercadopago/__init__.py | 6 +-- payments/mercadopago/test_mercadopago.py | 1 + payments/models.py | 54 ++++++------------- payments/paypal/__init__.py | 18 +++---- payments/paypal/test_paypal.py | 36 ++++++------- payments/sofort/__init__.py | 4 +- payments/sofort/test_sofort.py | 26 +++++---- payments/stripe/__init__.py | 6 +-- payments/stripe/forms.py | 2 +- payments/stripe/test_stripe.py | 1 + payments/test_core.py | 10 ++-- .../testmain/migrations/0001_initial.py | 2 +- 17 files changed, 95 insertions(+), 114 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 01561ac69..998c009b2 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -8,6 +8,11 @@ v3.0.0 ------ - **BREAKING**: Dropped support for Django 2.2, 3.0, 3.1 and 4.0. Supported versions of Django are 3.2 (LTS), 4.1 and 4.2. +- **BREAKING** ``BasePayment.extra_data`` is now a JSONField and django will + handle the serialisation. Due to this, usage of the ``BasePayment.attrs`` + proxy has been deprecated. A migration needs to be generated to update this + column in place. Application code needs to be updated from + ``payment.extra_data.field`` to ``payment.extra_data["field"]``. - Stripe backends now sends order_id in the metadata parameter. - A new ``StripeProviderV3`` has been added using the latest Stripe API. - Added support for Python 3.11, Django 4.1 and Django 4.2. diff --git a/docs/backends.rst b/docs/backends.rst index af478c58e..9ddfba7b4 100644 --- a/docs/backends.rst +++ b/docs/backends.rst @@ -108,7 +108,7 @@ about the payment or the order, such as an order number, additional customer information, or a special comment or request from the customer. This can be accomplished by passing your data to the :class:`Payment` instance:: - >>> payment.attrs.merchant_defined_data = {'01': 'foo', '02': 'bar'} + >>> payment.extra_data["merchant_defined_data"] = {'01': 'foo', '02': 'bar'} Fingerprinting:: diff --git a/payments/cybersource/__init__.py b/payments/cybersource/__init__.py index da87cb068..b328feae3 100644 --- a/payments/cybersource/__init__.py +++ b/payments/cybersource/__init__.py @@ -170,11 +170,11 @@ def charge(self, payment, data): else: params = self._prepare_preauth(payment, data) response = self._make_request(payment, params) - payment.attrs.capture = self._capture + payment.extra_data["capture"] = self._capture payment.transaction_id = response.requestID if response.reasonCode == AUTHENTICATE_REQUIRED: xid = response.payerAuthEnrollReply.xid - payment.attrs.xid = xid + payment.extra_data["xid"] = xid payment.change_status( PaymentStatus.WAITING, message=_("3-D Secure verification in progress") ) @@ -276,8 +276,8 @@ def _get_params_for_new_payment(self, payment): "merchantReferenceCode": payment.id, } try: - fingerprint_id = payment.attrs.fingerprint_session_id - except AttributeError: + fingerprint_id = payment.extra_data["fingerprint_session_id"] + except KeyError: pass else: params["deviceFingerprintID"] = fingerprint_id @@ -288,7 +288,7 @@ def _get_params_for_new_payment(self, payment): def _make_request(self, payment, params): response = self.client.service.runTransaction(**params) - payment.attrs.last_response = self._serialize_response(response) + payment.extra_data["last_response"] = self._serialize_response(response) return response def _prepare_payer_auth_validation_check(self, payment, card_data, pa_response): @@ -297,7 +297,7 @@ def _prepare_payer_auth_validation_check(self, payment, card_data, pa_response): check_service.signedPARes = pa_response params = self._get_params_for_new_payment(payment) params["payerAuthValidateService"] = check_service - if payment.attrs.capture: + if payment.extra_data["capture"]: service = self.client.factory.create("data:CCCreditService") service._run = "true" params["ccCreditService"] = service @@ -440,8 +440,8 @@ def _prepare_items(self, payment): def _prepare_merchant_defined_data(self, payment): try: - merchant_defined_data = payment.attrs.merchant_defined_data - except AttributeError: + merchant_defined_data = payment.extra_data["merchant_defined_data"] + except KeyError: return None else: data = self.client.factory.create("data:MerchantDefinedData") @@ -471,7 +471,7 @@ def _serialize_response(self, response): def process_data(self, payment, request): xid = request.POST.get("MD") - if xid != payment.attrs.xid: + if xid != payment.extra_data["xid"]: return redirect(payment.get_failure_url()) if payment.status in [PaymentStatus.CONFIRMED, PaymentStatus.PREAUTH]: return redirect(payment.get_success_url()) diff --git a/payments/cybersource/forms.py b/payments/cybersource/forms.py index 2ad01002a..3fda8deaa 100644 --- a/payments/cybersource/forms.py +++ b/payments/cybersource/forms.py @@ -41,7 +41,7 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) if self.provider.org_id: try: - fingerprint_id = self.payment.attrs.fingerprint_session_id + fingerprint_id = self.payment.extra_data["fingerprint_session_id"] except KeyError: fingerprint_id = str(uuid4()) self.fields["fingerprint"] = FingerprintInput( @@ -57,7 +57,7 @@ def clean(self): if not self.errors: if self.provider.org_id: fingerprint = cleaned_data["fingerprint"] - self.payment.attrs.fingerprint_session_id = fingerprint + self.payment.extra_data["fingerprint_session_id"] = fingerprint if not self.payment.transaction_id: try: self.provider.charge(self.payment, cleaned_data) diff --git a/payments/cybersource/test_cybersource.py b/payments/cybersource/test_cybersource.py index fdbf07a47..85b23a072 100644 --- a/payments/cybersource/test_cybersource.py +++ b/payments/cybersource/test_cybersource.py @@ -41,10 +41,10 @@ class Payment(Mock): transaction_id = None captured_amount = 0 message = "" - - class attrs: - fingerprint_session_id = "fake" - merchant_defined_data: dict[str, str] = {} + extra_data = { + "fingerprint_session_id": "fake", + "merchant_defined_data": {}, + } def get_process_url(self): return "http://example.com" @@ -153,7 +153,7 @@ def test_provider_redirects_on_success_captured_payment( ): transaction_id = 1234 xid = "abc" - self.payment.attrs.xid = xid + self.payment.extra_data["xid"] = xid response = MagicMock() response.requestID = transaction_id @@ -188,7 +188,7 @@ def test_provider_redirects_on_success_preauth_payment( ) transaction_id = 1234 xid = "abc" - self.payment.attrs.xid = xid + self.payment.extra_data["xid"] = xid response = MagicMock() response.requestID = transaction_id @@ -218,7 +218,7 @@ def test_provider_redirects_on_success_preauth_payment( def test_provider_redirects_on_failure(self, mocked_request, mocked_redirect): transaction_id = 1234 xid = "abc" - self.payment.attrs.xid = xid + self.payment.extra_data["xid"] = xid response = MagicMock() response.requestID = transaction_id diff --git a/payments/mercadopago/__init__.py b/payments/mercadopago/__init__.py index 0f1533e8e..89cd7fff6 100644 --- a/payments/mercadopago/__init__.py +++ b/payments/mercadopago/__init__.py @@ -74,7 +74,7 @@ def create_preference(self, payment: BasePayment): if payment.transaction_id: raise ValueError("This payment already has a preference.") - payment.attrs.external_reference = uuid4().hex + payment.extra_data["external_reference"] = uuid4().hex payload = { "auto_return": "all", @@ -89,7 +89,7 @@ def create_preference(self, payment: BasePayment): } for item in payment.get_purchased_items() ], - "external_reference": payment.attrs.external_reference, + "external_reference": payment.extra_data["external_reference"], "back_urls": { "success": self.get_return_url(payment), "pending": self.get_return_url(payment), @@ -218,7 +218,7 @@ def poll_for_updates(self, payment: BasePayment): """ data = self.client.payment().search( { - "external_reference": payment.attrs.external_reference, + "external_reference": payment.extra_data["external_reference"], } ) diff --git a/payments/mercadopago/test_mercadopago.py b/payments/mercadopago/test_mercadopago.py index 7a0527ec8..787942067 100644 --- a/payments/mercadopago/test_mercadopago.py +++ b/payments/mercadopago/test_mercadopago.py @@ -29,6 +29,7 @@ class Payment(Mock): captured_amount = 0 transaction_id: str | None = None billing_email = "john@doe.com" + extra_data: dict = {} def change_status(self, status, message=""): self.status = status diff --git a/payments/models.py b/payments/models.py index 922156bf3..8da98e375 100644 --- a/payments/models.py +++ b/payments/models.py @@ -1,6 +1,6 @@ from __future__ import annotations -import json +import warnings from typing import Iterable from uuid import uuid4 @@ -15,30 +15,6 @@ from .core import provider_factory -class PaymentAttributeProxy: - def __init__(self, payment): - self._payment = payment - super().__init__() - - def __getattr__(self, item): - data = json.loads(self._payment.extra_data or "{}") - try: - return data[item] - except KeyError as e: - raise AttributeError(*e.args) from e - - def __setattr__(self, key, value): - if key == "_payment": - return super().__setattr__(key, value) - try: - data = json.loads(self._payment.extra_data) - except ValueError: - data = {} - data[key] = value - self._payment.extra_data = json.dumps(data) - return None - - class BasePayment(models.Model): """ Represents a single transaction. Each instance has one or more PaymentItem. @@ -80,7 +56,7 @@ class BasePayment(models.Model): billing_email = models.EmailField(blank=True) billing_phone = PhoneNumberField(blank=True) customer_ip_address = models.GenericIPAddressField(blank=True, null=True) - extra_data = models.TextField(blank=True, default="") + extra_data = models.JSONField(blank=True, default=dict) message = models.TextField(blank=True, default="") token = models.CharField(max_length=36, blank=True, default="") captured_amount = models.DecimalField(max_digits=9, decimal_places=2, default="0.0") @@ -226,14 +202,18 @@ def refund(self, amount=None): @property def attrs(self): - """A JSON-serialised wrapper around `extra_data`. - - This property exposes a a dict or list which is serialised into the `extra_data` - text field. Usage of this wrapper is preferred over accessing the underlying - field directly. - - You may think of this as a `JSONField` which is saved to the `extra_data` - column. - """ - # TODO: Deprecate in favour of JSONField when we drop support for django 2.2. - return PaymentAttributeProxy(self) + warnings.warn( + "Using BasePayment.attrs is deprecated. Use BasePayment.extra_data instead", + DeprecationWarning, + stacklevel=2, + ) + return self.extra_data + + @attrs.setter + def attrs(self, value): + warnings.warn( + "Using BasePayment.attrs is deprecated. Use BasePayment.extra_data instead", + DeprecationWarning, + stacklevel=2, + ) + self.extra_data = value diff --git a/payments/paypal/__init__.py b/payments/paypal/__init__.py index e7f2d0e58..fa3906969 100644 --- a/payments/paypal/__init__.py +++ b/payments/paypal/__init__.py @@ -82,14 +82,14 @@ def __init__( super().__init__(capture=capture) def set_response_data(self, payment, response, is_auth=False): - extra_data = json.loads(payment.extra_data or "{}") + extra_data = payment.extra_data or {} if is_auth: extra_data["auth_response"] = response else: extra_data["response"] = response if "links" in response: extra_data["links"] = {link["rel"]: link for link in response["links"]} - payment.extra_data = json.dumps(extra_data) + payment.extra_data = extra_data payment.save() def set_response_links(self, payment, response): @@ -97,19 +97,19 @@ def set_response_links(self, payment, response): related_resources = transaction["related_resources"][0] resource_key = "sale" if self._capture else "authorization" links = related_resources[resource_key]["links"] - extra_data = json.loads(payment.extra_data or "{}") + extra_data = payment.extra_data or {} extra_data["links"] = {link["rel"]: link for link in links} - payment.extra_data = json.dumps(extra_data) + payment.extra_data = extra_data payment.save() def set_error_data(self, payment, error): - extra_data = json.loads(payment.extra_data or "{}") + extra_data = payment.extra_data or {} extra_data["error"] = error - payment.extra_data = json.dumps(extra_data) + payment.extra_data = extra_data payment.save() def _get_links(self, payment): - extra_data = json.loads(payment.extra_data or "{}") + extra_data = payment.extra_data or {} return extra_data.get("links", {}) @authorize @@ -144,7 +144,7 @@ def post(self, payment, *args, **kwargs): return data def get_last_response(self, payment, is_auth=False): - extra_data = json.loads(payment.extra_data or "{}") + extra_data = payment.extra_data or {} if is_auth: return extra_data.get("auth_response", {}) return extra_data.get("response", {}) @@ -249,7 +249,7 @@ def process_data(self, payment, request): except PaymentError: return redirect(failure_url) self.set_response_links(payment, executed_payment) - payment.attrs.payer_info = executed_payment["payer"]["payer_info"] + payment.extra_data["payer_info"] = executed_payment["payer"]["payer_info"] if self._capture: payment.captured_amount = payment.total payment.change_status(PaymentStatus.CONFIRMED) diff --git a/payments/paypal/test_paypal.py b/payments/paypal/test_paypal.py index 28fc7149d..6de04c1b4 100644 --- a/payments/paypal/test_paypal.py +++ b/payments/paypal/test_paypal.py @@ -1,6 +1,5 @@ from __future__ import annotations -import json from datetime import date from decimal import Decimal from unittest import TestCase @@ -46,16 +45,14 @@ class Payment(Mock): variant = VARIANT transaction_id = None message = "" - extra_data = json.dumps( - { - "links": { - "approval_url": None, - "capture": {"href": "http://capture.com"}, - "refund": {"href": "http://refund.com"}, - "execute": {"href": "http://execute.com"}, - } + extra_data = { + "links": { + "approval_url": None, + "capture": {"href": "http://capture.com"}, + "refund": {"href": "http://refund.com"}, + "execute": {"href": "http://execute.com"}, } - ) + } def change_status(self, status, message=""): self.status = status @@ -225,23 +222,22 @@ def test_provider_renews_access_token(self, mocked_post): mocked_post.side_effect = [HTTPError(response=response401), response, response] self.payment.created = timezone.now() - self.payment.extra_data = json.dumps( - { - "auth_response": { - "access_token": "expired_token", - "token_type": "token type", - "expires_in": 99999, - } + self.payment.extra_data = { + "auth_response": { + "access_token": "expired_token", + "token_type": "token type", + "expires_in": 99999, } - ) + } + self.provider.create_payment(self.payment) - payment_response = json.loads(self.payment.extra_data)["auth_response"] + payment_response = self.payment.extra_data["auth_response"] self.assertEqual(payment_response["access_token"], new_token) class TestPaypalCardProvider(TestCase): def setUp(self): - self.payment = Payment(extra_data="") + self.payment = Payment(extra_data={}) self.provider = PaypalCardProvider(secret=SECRET, client_id=CLIENT_ID) def test_provider_raises_redirect_needed_on_success_captured_payment(self): diff --git a/payments/sofort/__init__.py b/payments/sofort/__init__.py index 9bcf30fa9..7a53bfda3 100644 --- a/payments/sofort/__init__.py +++ b/payments/sofort/__init__.py @@ -99,7 +99,7 @@ def process_data(self, payment, request): else: payment.captured_amount = payment.total payment.change_status(PaymentStatus.CONFIRMED) - payment.extra_data = json.dumps(doc) + payment.extra_data = doc sender_data = doc["transactions"]["transaction_details"]["sender"] holder_data = sender_data["holder"] first_name, last_name = holder_data.rsplit(" ", 1) @@ -112,7 +112,7 @@ def process_data(self, payment, request): def refund(self, payment, amount=None): if amount is None: amount = payment.captured_amount - doc = json.loads(payment.extra_data) + doc = payment.extra_data sender_data = doc["transactions"]["transaction_details"]["sender"] refund_request = render_to_string( "payments/sofort/refund_transaction.xml", diff --git a/payments/sofort/test_sofort.py b/payments/sofort/test_sofort.py index 071cf0492..337d4e904 100644 --- a/payments/sofort/test_sofort.py +++ b/payments/sofort/test_sofort.py @@ -1,6 +1,5 @@ from __future__ import annotations -import json from unittest import TestCase from unittest.mock import MagicMock from unittest.mock import Mock @@ -99,21 +98,20 @@ def test_provider_redirects_on_failure( @patch("xmltodict.parse") @patch("requests.post") def test_provider_refunds_payment(self, mocked_post, mocked_parser): - self.payment.extra_data = json.dumps( - { - "transactions": { - "transaction_details": { - "status": "ok", - "sender": { - "holder": "John Doe", - "country_code": "EN", - "bic": "1234", - "iban": "abcd", - }, - } + self.payment.extra_data = { + "transactions": { + "transaction_details": { + "status": "ok", + "sender": { + "holder": "John Doe", + "country_code": "EN", + "bic": "1234", + "iban": "abcd", + }, } } - ) + } + mocked_parser.return_value = {} self.provider.refund(self.payment) self.assertEqual(self.payment.status, PaymentStatus.REFUNDED) diff --git a/payments/stripe/__init__.py b/payments/stripe/__init__.py index 05930f30e..8e7acf640 100644 --- a/payments/stripe/__init__.py +++ b/payments/stripe/__init__.py @@ -60,19 +60,19 @@ def capture(self, payment, amount=None): except stripe.InvalidRequestError as e: payment.change_status(PaymentStatus.REFUNDED) raise PaymentError("Payment already refunded") from e - payment.attrs.capture = json.dumps(charge) + payment.extra_data["capture"] = json.dumps(charge) return Decimal(amount) / 100 def release(self, payment): charge = stripe.Charge.retrieve(payment.transaction_id) charge.refund() - payment.attrs.release = json.dumps(charge) + payment.extra_data["release"] = json.dumps(charge) def refund(self, payment, amount=None): amount = int((amount or payment.total) * 100) charge = stripe.Charge.retrieve(payment.transaction_id) charge.refund(amount=amount) - payment.attrs.refund = json.dumps(charge) + payment.extra_data["refund"] = json.dumps(charge) return Decimal(amount) / 100 diff --git a/payments/stripe/forms.py b/payments/stripe/forms.py index a4ef39d58..d59e59039 100644 --- a/payments/stripe/forms.py +++ b/payments/stripe/forms.py @@ -87,7 +87,7 @@ def clean(self): def save(self): self.payment.transaction_id = self.charge.id - self.payment.attrs.charge = json.dumps(self.charge) + self.payment.extra_data["charge"] = json.dumps(self.charge) self.payment.change_status(PaymentStatus.PREAUTH) if self.provider._capture: self.payment.capture() diff --git a/payments/stripe/test_stripe.py b/payments/stripe/test_stripe.py index b9cd05067..d05e1ed73 100644 --- a/payments/stripe/test_stripe.py +++ b/payments/stripe/test_stripe.py @@ -30,6 +30,7 @@ class Payment(Mock): captured_amount = 0 transaction_id = None billing_email = "john@doe.com" + extra_data: dict = {} def change_status(self, status, message=""): self.status = status diff --git a/payments/test_core.py b/payments/test_core.py index 8c9190dd6..3188abf1f 100644 --- a/payments/test_core.py +++ b/payments/test_core.py @@ -43,11 +43,11 @@ class Payment(BasePayment): class TestBasePayment(TestCase): def test_payment_attributes(self): - payment = Payment(extra_data='{"attr1": "test1", "attr2": "test2"}') - self.assertEqual(payment.attrs.attr1, "test1") - self.assertEqual(payment.attrs.attr2, "test2") - self.assertEqual(getattr(payment.attrs, "attr5", None), None) - self.assertEqual(hasattr(payment.attrs, "attr7"), False) + payment = Payment(extra_data={"attr1": "test1", "attr2": "test2"}) + self.assertEqual(payment.extra_data["attr1"], "test1") + self.assertEqual(payment.extra_data["attr2"], "test2") + self.assertEqual(payment.extra_data.get("attr5", None), None) + self.assertEqual("attr7" in payment.extra_data, False) def test_capture_with_wrong_status(self): payment = Payment(variant="default", status=PaymentStatus.WAITING) diff --git a/testapp/testapp/testmain/migrations/0001_initial.py b/testapp/testapp/testmain/migrations/0001_initial.py index fc5fc439c..64d865258 100644 --- a/testapp/testapp/testmain/migrations/0001_initial.py +++ b/testapp/testapp/testmain/migrations/0001_initial.py @@ -85,7 +85,7 @@ class Migration(migrations.Migration): "customer_ip_address", models.GenericIPAddressField(blank=True, null=True), ), - ("extra_data", models.TextField(blank=True, default="")), + ("extra_data", models.JSONField(blank=True, default=dict)), ("message", models.TextField(blank=True, default="")), ("token", models.CharField(blank=True, default="", max_length=36)), (