PaddleHQ / paddle-python-sdk

Python SDK for working with the Paddle API in server-side apps.
https://developer.paddle.com/
Apache License 2.0
11 stars 2 forks source link

fix: notification settings collection import #12

Closed gerlv closed 2 weeks ago

gerlv commented 2 weeks ago

SDK incorrectly imported Notification entity instead of NotificationSetting when pulling a list of notification settings.

Verified it by installing the package form the head repo and running the same command - no error.

Error log:

In [10]: client.notification_settings.list()
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[10], line 1
----> 1 client.notification_settings.list()

File ~/env/paddle_billing/Resources/NotificationSettings/NotificationSettingsClient.py:23, in NotificationSettingsClient.list(self)
     20 self.response = self.client.get_raw('/notification-settings')
     21 parser        = ResponseParser(self.response)
---> 23 return NotificationSettingCollection.from_list(parser.get_data())

File ~/env/paddle_billing/Entities/Collections/NotificationSettingCollection.py:12, in NotificationSettingCollection.from_list(cls, items_data, paginator)
      8 @classmethod
      9 def from_list(cls, items_data: list, paginator: Paginator | None = None) -> NotificationSettingCollection:
     10     from paddle_billing.Entities.Notification import Notification
---> 12     items: list[Notification] = [Notification.from_dict(item) for item in items_data]
     14     return NotificationSettingCollection(items, paginator)

File ~/env/paddle_billing/Entities/Collections/NotificationSettingCollection.py:12, in <listcomp>(.0)
      8 @classmethod
      9 def from_list(cls, items_data: list, paginator: Paginator | None = None) -> NotificationSettingCollection:
     10     from paddle_billing.Entities.Notification import Notification
---> 12     items: list[Notification] = [Notification.from_dict(item) for item in items_data]
     14     return NotificationSettingCollection(items, paginator)

File ~/env/paddle_billing/Entities/Notification.py:31, in Notification.from_dict(cls, data)
     27 @classmethod
     28 def from_dict(cls, data: dict) -> Notification:
     29     return Notification(
     30         id                      = data['id'],
---> 31         type                    = EventTypeName(data['type']),
     32         status                  = NotificationStatus(data['status']),
     33         payload                 = Event.from_dict(data['payload']),
     34         occurred_at             = datetime.fromisoformat(data['occurred_at']),
     35         origin                  = NotificationOrigin(data['origin']),
     36         times_attempted         = data['times_attempted'],
     37         notification_setting_id = data['notification_setting_id'],
     38         delivered_at            = datetime.fromisoformat(data['delivered_at'])    if data.get('delivered_at')    else None,
     39         replayed_at             = datetime.fromisoformat(data['replayed_at'])     if data.get('replayed_at')     else None,
     40         last_attempt_at         = datetime.fromisoformat(data['last_attempt_at']) if data.get('last_attempt_at') else None,
     41         retry_at                = datetime.fromisoformat(data['retry_at'])        if data.get('retry_at')        else None,
     42     )

File ~/env/lib/python3.11/enum.py:695, in EnumType.__call__(cls, value, names, module, qualname, type, start, boundary)
    670 """
    671 Either returns an existing member, or creates a new enum class.
    672 
   (...)
    692 `type`, if set, will be mixed in as the first base class.
    693 """
    694 if names is None:  # simple value lookup
--> 695     return cls.__new__(cls, value)
    696 # otherwise, functional API: we're creating a new Enum type
    697 return cls._create_(
    698         value,
    699         names,
   (...)
    704         boundary=boundary,
    705         )

File ~/env/lib/python3.11/enum.py:1111, in Enum.__new__(cls, value)
   1109 ve_exc = ValueError("%r is not a valid %s" % (value, cls.__qualname__))
   1110 if result is None and exc is None:
-> 1111     raise ve_exc
   1112 elif exc is None:
   1113     exc = TypeError(
   1114             'error in %s._missing_: returned %r instead of None or a valid member'
   1115             % (cls.__name__, result)
   1116             )

ValueError: 'url' is not a valid EventTypeName
davidgrayston-paddle commented 2 weeks ago

Hi @gerlv

Thank you for taking the time to contribute this fix to the SDK.

If possible, please can you update the test to cover this change.

If you are unable to update the test at this time, please let us know and we will make the change.

Here is a suggested change:

diff --git a/tests/Functional/Resources/NotificationSettings/test_NotificationSettingsClient.py b/tests/Functional/Resources/NotificationSettings/test_NotificationSettingsClient.py
index 5fd65e2..1a6d936 100644
--- a/tests/Functional/Resources/NotificationSettings/test_NotificationSettingsClient.py
+++ b/tests/Functional/Resources/NotificationSettings/test_NotificationSettingsClient.py
@@ -200,12 +200,13 @@ class TestNotificationSettingsClient:
         expected_url,
     ):
         expected_url = f"{test_client.base_url}{expected_url}"
-        mock_requests.get(expected_url, status_code=expected_response_status)
+        mock_requests.get(expected_url, status_code=expected_response_status, text=expected_response_body)

         response     = test_client.client.notification_settings.list()
         last_request = mock_requests.last_request

         assert isinstance(response, NotificationSettingCollection)
+        assert all(isinstance(item, NotificationSetting) for item in response.items), "Not all items are NotificationSetting"
         assert last_request is not None
         assert last_request.method            == 'GET'
         assert test_client.client.status_code == expected_response_status
gerlv commented 2 weeks ago
assert all(isinstance(item, NotificationSetting) for item in response.items), "Not all items are NotificationSetting"

Thank you, done! Also verified it passes locally: == 216 passed in 3.05s ====

davidgrayston-paddle commented 2 weeks ago

@gerlv thank you for contributing this fix, we will release this shortly