Skip to content

Commit 01b9fec

Browse files
committed
PR review comments
1 parent b49646d commit 01b9fec

16 files changed

Lines changed: 264 additions & 43 deletions

File tree

.github/workflows/ci.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,8 @@ jobs:
7676
cp sinch-sdk-mockserver/features/numbers/callback-configuration.feature ./tests/e2e/numbers/features/
7777
cp sinch-sdk-mockserver/features/numbers/numbers.feature ./tests/e2e/numbers/features/
7878
cp sinch-sdk-mockserver/features/numbers/webhooks.feature ./tests/e2e/numbers/features/
79+
cp sinch-sdk-mockserver/features/sms/delivery-reports.feature ./tests/e2e/sms/features/
80+
cp sinch-sdk-mockserver/features/sms/delivery-reports_servicePlanId.feature ./tests/e2e/sms/features/
7981
8082
- name: Wait for mock server
8183
run: .github/scripts/wait-for-mockserver.sh

sinch/core/clients/sinch_client_configuration.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -233,20 +233,24 @@ def validate_authentication_parameters(self):
233233
Validates that sufficient authentication parameters are provided.
234234
This should be called before making actual API requests.
235235
"""
236+
if self.service_plan_id and not self.sms_api_token:
237+
raise ValueError(
238+
"The sms_api_token is required when using service_plan_id"
239+
)
236240
if self._authentication_method is None or self._authentication_method == "project_auth":
237241
# Default to project_auth and validate parameters
238242
if not self.project_id:
239243
raise ValueError(
240-
"Project authentication requires 'project_id'"
244+
"The project_id is required"
241245
)
242246
if not self.key_id or not self.key_secret:
243247
raise ValueError(
244-
"Project authentication requires 'key_id' and 'key_secret'"
248+
"The key_id and key_secret are required"
245249
)
246250
elif self._authentication_method == "sms_auth":
247251
if not self.service_plan_id or not self.sms_api_token:
248252
raise ValueError(
249-
"SMS authentication requires both 'service_plan_id' and 'sms_api_token'"
253+
"The service_plan_id and sms_api_token are required"
250254
)
251255

252256
def get_sms_origin_for_auth(self):

sinch/domains/sms/api/v1/base/base_sms.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,12 @@ def _request(self, endpoint_class, request_data):
1717
"""
1818
# Use service_plan_id for SMS auth, project_id for project auth
1919
if self._sinch.configuration.authentication_method == "sms_auth":
20-
endpoint_id = self._sinch.configuration.service_plan_id
20+
path_identifier = self._sinch.configuration.service_plan_id
2121
else:
22-
endpoint_id = self._sinch.configuration.project_id
22+
path_identifier = self._sinch.configuration.project_id
2323

2424
endpoint = endpoint_class(
25-
project_id=endpoint_id,
25+
project_id=path_identifier,
2626
request_data=request_data,
2727
)
2828

sinch/domains/sms/api/v1/delivery_reports_apis.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,12 +64,12 @@ def list(
6464
) -> Paginator[RecipientDeliveryReport]:
6565
# Use service_plan_id for SMS auth, project_id for project auth
6666
if self._sinch.configuration.authentication_method == "sms_auth":
67-
endpoint_id = self._sinch.configuration.service_plan_id
67+
path_identifier = self._sinch.configuration.service_plan_id
6868
else:
69-
endpoint_id = self._sinch.configuration.project_id
69+
path_identifier = self._sinch.configuration.project_id
7070

7171
endpoint = ListDeliveryReportsEndpoint(
72-
project_id=endpoint_id,
72+
project_id=path_identifier,
7373
request_data=ListDeliveryReportsRequest(
7474
page=page,
7575
page_size=page_size,

sinch/domains/sms/models/v1/internal/base/base_model_configuration.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,20 @@ def model_dump_for_query_params(self, exclude_none=True, by_alias=True):
1919
"""
2020
Serializes the model for use as query parameters.
2121
Converts list values to comma-separated strings for APIs that expect this format.
22+
Filters out empty values (empty strings and empty lists).
2223
"""
2324
data = self.model_dump(exclude_none=exclude_none, by_alias=by_alias)
25+
filtered_data = {}
2426
for key, value in data.items():
27+
if value == "":
28+
continue
29+
if isinstance(value, list) and len(value) == 0:
30+
continue
2531
if isinstance(value, list):
26-
data[key] = ",".join(str(item) for item in value)
27-
return data
32+
filtered_data[key] = ",".join(str(item) for item in value)
33+
else:
34+
filtered_data[key] = value
35+
return filtered_data
2836

2937

3038
class BaseModelConfigurationResponse(BaseModel):

tests/conftest.py

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -254,21 +254,27 @@ class MockSinchClient:
254254

255255
@pytest.fixture
256256
def mock_sinch_client_sms():
257-
class SMSMockConfiguration:
258-
sms_origin = "https://mock-sms-api.sinch.com"
259-
sms_origin_with_service_plan_id = "https://mock-sms-api.sinch.com"
260-
project_id = "test_project_id"
261-
service_plan_id = "test_service_plan_id"
262-
authentication_method = "project_auth"
263-
transport = MagicMock()
264-
transport.request = MagicMock()
265-
266-
def get_sms_origin_for_auth(self):
267-
"""Returns the appropriate SMS origin based on authentication method."""
268-
return self.sms_origin_with_service_plan_id if self.authentication_method == "sms_auth" else self.sms_origin
269-
257+
from sinch.core.clients.sinch_client_configuration import Configuration
258+
from sinch.core.ports.http_transport import HTTPTransport
259+
from sinch.core.token_manager import TokenManager
260+
261+
mock_transport = MagicMock(spec=HTTPTransport)
262+
mock_transport.request = MagicMock()
263+
264+
mock_token_manager = MagicMock(spec=TokenManager)
265+
266+
config = Configuration(
267+
transport=mock_transport,
268+
token_manager=mock_token_manager,
269+
project_id="test_project_id",
270+
service_plan_id="test_service_plan_id",
271+
sms_region="eu"
272+
)
273+
274+
config._authentication_method = "project_auth"
275+
270276
class MockSinchClient:
271-
configuration = SMSMockConfiguration()
277+
configuration = config
272278

273279
return MockSinchClient()
274280

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,6 @@
1-
from sinch import SinchClient
1+
from tests.e2e.shared_config import create_test_client
22

33

44
def before_all(context):
55
"""Initializes the Sinch client"""
6-
client_params = {
7-
'project_id': 'tinyfrog-jump-high-over-lilypadbasin',
8-
'key_id': 'keyId',
9-
'key_secret': 'keySecret',
10-
}
11-
context.sinch = SinchClient(**client_params)
12-
context.sinch.configuration.auth_origin = 'http://localhost:3011'
13-
context.sinch.configuration.numbers_origin = 'http://localhost:3013'
14-
context.sinch.configuration.sms_origin = 'http://localhost:3017'
6+
context.sinch = create_test_client()

tests/e2e/shared_config.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
from sinch import SinchClient
2+
3+
4+
def create_test_client():
5+
"""Creates a Sinch client with test configuration for all domains"""
6+
client_params = {
7+
'project_id': 'tinyfrog-jump-high-over-lilypadbasin',
8+
'key_id': 'keyId',
9+
'key_secret': 'keySecret',
10+
}
11+
client = SinchClient(**client_params)
12+
client.configuration.auth_origin = 'http://localhost:3011'
13+
client.configuration.numbers_origin = 'http://localhost:3013'
14+
client.configuration.sms_origin = 'http://localhost:3017'
15+
return client
16+
17+
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
from tests.e2e.shared_config import create_test_client
2+
3+
4+
def before_all(context):
5+
"""Initializes the Sinch client"""
6+
context.sinch = create_test_client()
Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
from datetime import datetime, timezone
2+
from behave import given, when, then
3+
from sinch.domains.sms.models.v1.response import BatchDeliveryReport, RecipientDeliveryReport
4+
5+
6+
@given('the SMS service "{service_name}" is available')
7+
def step_sms_service_available(context, service_name):
8+
"""Ensures the Sinch client is initialized"""
9+
assert hasattr(context, 'sinch') and context.sinch, 'Sinch client was not initialized'
10+
11+
12+
@given('the SMS service "{service_name}" is available and is configured for servicePlanId authentication')
13+
def step_sms_service_available_with_service_plan(context, service_name):
14+
"""Ensures the Sinch client is initialized with service_plan_id authentication"""
15+
from sinch import SinchClient
16+
17+
# Create a new client with service_plan_id authentication
18+
context.sinch = SinchClient(
19+
service_plan_id='CappyPremiumPlan',
20+
sms_api_token='HappyCappyToken',
21+
)
22+
context.sinch.configuration.auth_origin = 'http://localhost:3011'
23+
context.sinch.configuration.sms_origin = 'http://localhost:3017'
24+
context.sinch.configuration.sms_origin_with_service_plan_id = 'http://localhost:3017'
25+
26+
27+
@when('I send a request to retrieve a summary SMS delivery report')
28+
def step_retrieve_summary_delivery_report(context):
29+
"""Retrieve a summary SMS delivery report"""
30+
context.response = context.sinch.sms.delivery_reports.get(
31+
batch_id='01W4FFL35P4NC4K35SMSBATCH1',
32+
status=['DELIVERED', 'FAILED'],
33+
code=[15, 0]
34+
)
35+
36+
37+
@then('the response contains a summary SMS delivery report')
38+
def step_validate_summary_delivery_report(context):
39+
"""Validate summary SMS delivery report response"""
40+
data: BatchDeliveryReport = context.response
41+
assert data.batch_id == '01W4FFL35P4NC4K35SMSBATCH1'
42+
assert data.client_reference == 'reference_e2e'
43+
assert data.statuses is not None
44+
assert len(data.statuses) >= 2
45+
46+
status = data.statuses[0]
47+
assert status.code == 15
48+
assert status.count == 1
49+
assert status.recipients is None
50+
assert status.status == 'Failed'
51+
52+
status = data.statuses[1]
53+
assert status.code == 0
54+
assert status.count == 1
55+
assert status.recipients is None
56+
assert status.status == 'Delivered'
57+
58+
assert data.total_message_count == 2
59+
assert data.type == 'delivery_report_sms'
60+
61+
62+
@when('I send a request to retrieve a full SMS delivery report')
63+
def step_retrieve_full_delivery_report(context):
64+
"""Retrieve a full SMS delivery report"""
65+
context.response = context.sinch.sms.delivery_reports.get(
66+
batch_id='01W4FFL35P4NC4K35SMSBATCH1',
67+
report_type='full'
68+
)
69+
70+
71+
@then('the response contains a full SMS delivery report')
72+
def step_validate_full_delivery_report(context):
73+
"""Validate full SMS delivery report response"""
74+
data: BatchDeliveryReport = context.response
75+
assert data.batch_id == '01W4FFL35P4NC4K35SMSBATCH1'
76+
assert data.statuses is not None
77+
status = data.statuses[0]
78+
assert status.recipients is not None
79+
assert status.code == 0
80+
assert status.count == 1
81+
assert status.recipients[0] == '12017777777'
82+
assert status.status == 'Delivered'
83+
84+
85+
@when('I send a request to retrieve a recipient\'s delivery report')
86+
def step_retrieve_recipient_delivery_report(context):
87+
"""Retrieve a recipient's delivery report"""
88+
context.response = context.sinch.sms.delivery_reports.get_for_number(
89+
batch_id='01W4FFL35P4NC4K35SMSBATCH1',
90+
recipient='12017777777'
91+
)
92+
93+
94+
@then('the response contains the recipient\'s delivery report details')
95+
def step_validate_recipient_delivery_report(context):
96+
"""Validate recipient delivery report response"""
97+
data: RecipientDeliveryReport = context.response
98+
assert data.batch_id == '01W4FFL35P4NC4K35SMSBATCH1'
99+
assert data.recipient == '12017777777'
100+
assert data.client_reference == 'reference_e2e'
101+
assert data.status == 'Delivered'
102+
assert data.type == 'recipient_delivery_report_sms'
103+
assert data.code == 0
104+
assert data.at == datetime(2024, 6, 6, 13, 6, 27, 833000, tzinfo=timezone.utc)
105+
assert data.operator_status_at == datetime(2024, 6, 6, 13, 6, 0, tzinfo=timezone.utc)
106+
107+
108+
@when('I send a request to list the SMS delivery reports')
109+
def step_list_delivery_reports(context):
110+
"""List a page of SMS delivery reports"""
111+
context.response = context.sinch.sms.delivery_reports.list()
112+
113+
114+
@then('the response contains "{count}" SMS delivery reports')
115+
def step_validate_delivery_reports_count(context, count):
116+
"""Validate the count of SMS delivery reports in response"""
117+
expected_count = int(count)
118+
assert len(context.response.content()) == expected_count, \
119+
f'Expected {expected_count}, got {len(context.response.content())}'
120+
121+
122+
@when('I send a request to list all the SMS delivery reports')
123+
def step_list_all_delivery_reports(context):
124+
"""List all SMS delivery reports using iterator"""
125+
response = context.sinch.sms.delivery_reports.list(page_size=2)
126+
delivery_reports_list = []
127+
128+
for delivery_report in response.iterator():
129+
delivery_reports_list.append(delivery_report)
130+
131+
context.delivery_reports_list = delivery_reports_list
132+
133+
134+
@then('the SMS delivery reports list contains "{count}" SMS delivery reports')
135+
def step_validate_delivery_reports_list_count(context, count):
136+
"""Validate the count of SMS delivery reports in the full list"""
137+
expected_count = int(count)
138+
assert len(context.delivery_reports_list) == expected_count, \
139+
f'Expected {expected_count}, got {len(context.delivery_reports_list)}'
140+
141+
142+
@when('I iterate manually over the SMS delivery reports pages')
143+
def step_iterate_manually_delivery_reports(context):
144+
"""Manually iterate over SMS delivery reports pages"""
145+
context.list_response = context.sinch.sms.delivery_reports.list(page_size=2)
146+
147+
# Iterate through all pages
148+
context.delivery_reports_list = []
149+
context.pages_iteration = 0
150+
reached_last_page = False
151+
152+
while not reached_last_page:
153+
context.delivery_reports_list.extend(context.list_response.content())
154+
context.pages_iteration += 1
155+
if context.list_response.has_next_page:
156+
context.list_response = context.list_response.next_page()
157+
else:
158+
reached_last_page = True
159+
160+
161+
@then('the SMS delivery reports iteration result contains the data from "{count}" pages')
162+
def step_validate_delivery_reports_pages_count(context, count):
163+
"""Validate the count of pages in the iteration result"""
164+
expected_pages_count = int(count)
165+
assert context.pages_iteration == expected_pages_count, \
166+
f'Expected {expected_pages_count} pages, got {context.pages_iteration}'

0 commit comments

Comments
 (0)