Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions mpt_api_client/resources/catalog/pricing_policy_attachments.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
from mpt_api_client.http import AsyncService, Service
from mpt_api_client.http.mixins import (
AsyncCollectionMixin,
AsyncFilesOperationsMixin,
AsyncDownloadFileMixin,
AsyncModifiableResourceMixin,
CollectionMixin,
FilesOperationsMixin,
DownloadFileMixin,
ModifiableResourceMixin,
)
from mpt_api_client.models import Model
from mpt_api_client.resources.catalog.mixins import ActivatableMixin, AsyncActivatableMixin
from mpt_api_client.resources.catalog.mixins import (
AsyncCreateFileMixin,
CreateFileMixin,
)


class PricingPolicyAttachment(Model):
Expand All @@ -21,11 +24,13 @@ class PricingPolicyAttachmentsServiceConfig:
_endpoint = "/public/v1/catalog/pricing-policies/{pricing_policy_id}/attachments"
_model_class = PricingPolicyAttachment
_collection_key = "data"
_upload_file_key = "file"
_upload_data_key = "attachment"


class PricingPolicyAttachmentsService(
FilesOperationsMixin[PricingPolicyAttachment],
ActivatableMixin[PricingPolicyAttachment],
CreateFileMixin[PricingPolicyAttachment],
DownloadFileMixin[PricingPolicyAttachment],
ModifiableResourceMixin[PricingPolicyAttachment],
CollectionMixin[PricingPolicyAttachment],
Service[PricingPolicyAttachment],
Expand All @@ -35,8 +40,8 @@ class PricingPolicyAttachmentsService(


class AsyncPricingPolicyAttachmentsService(
AsyncFilesOperationsMixin[PricingPolicyAttachment],
AsyncActivatableMixin[PricingPolicyAttachment],
AsyncCreateFileMixin[PricingPolicyAttachment],
AsyncDownloadFileMixin[PricingPolicyAttachment],
AsyncModifiableResourceMixin[PricingPolicyAttachment],
AsyncCollectionMixin[PricingPolicyAttachment],
AsyncService[PricingPolicyAttachment],
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/catalog/items/test_async_item.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@


@pytest.fixture
async def async_created_item(logger, async_mpt_vendor, item_data):
async def async_created_item(async_mpt_vendor, item_data):
service = async_mpt_vendor.catalog.items
item = await service.create(item_data)
yield item
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/catalog/items/test_sync_item.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@


@pytest.fixture
def created_item(logger, mpt_vendor, item_data):
def created_item(mpt_vendor, item_data):
service = mpt_vendor.catalog.items
item = service.create(item_data)
yield item
Expand Down
Empty file.
31 changes: 31 additions & 0 deletions tests/e2e/catalog/pricing_policies/attachments/conftest.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import pytest

from mpt_api_client.exceptions import MPTAPIError


@pytest.fixture
def attachment_id(created_attachment):
return created_attachment.id


@pytest.fixture
def attachment_data():
return {
"name": "e2e test attachment - please delete",
"description": "E2E test attachment for automated testing",
}


@pytest.fixture
def attachment_service(mpt_ops, pricing_policy_id):
return mpt_ops.catalog.pricing_policies.attachments(pricing_policy_id)


@pytest.fixture
def created_attachment(attachment_service, attachment_data, pdf_fd):
attachment = attachment_service.create(attachment_data, file=pdf_fd)
yield attachment
try:
attachment_service.delete(attachment.id)
except MPTAPIError as error:
print(f"TEARDOWN - Unable to delete attachment {attachment.id}: {error.title}")
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import pytest

from mpt_api_client.exceptions import MPTAPIError
from mpt_api_client.rql.query_builder import RQLQuery

pytestmark = [pytest.mark.flaky]


@pytest.fixture
def async_attachment_service(async_mpt_ops, pricing_policy_id):
return async_mpt_ops.catalog.pricing_policies.attachments(pricing_policy_id)


@pytest.fixture
async def created_attachment_async(async_attachment_service, attachment_data, pdf_fd):
attachment = await async_attachment_service.create(attachment_data, file=pdf_fd)
yield attachment
try:
await async_attachment_service.delete(attachment.id)
except MPTAPIError as error:
print(f"TEARDOWN - Unable to delete attachment {attachment.id}: {error.title}")


def test_create_attachment_async(created_attachment_async, attachment_data):
result = created_attachment_async

assert result.name == attachment_data["name"]
assert result.description == attachment_data["description"]


async def test_update_attachment_async(async_attachment_service, created_attachment_async):
update_data = {"name": "Updated e2e test attachment - please delete"}

result = await async_attachment_service.update(created_attachment_async.id, update_data)

assert result.name == update_data["name"]


async def test_get_attachment_async(async_attachment_service, attachment_id):
result = await async_attachment_service.get(attachment_id)

assert result.id == attachment_id


async def test_download_attachment_async(async_attachment_service, attachment_id):
result = await async_attachment_service.download(attachment_id)

assert result.file_contents is not None
assert result.filename == "empty.pdf"


async def test_iterate_attachments_async(async_attachment_service, created_attachment_async):
result = [att async for att in async_attachment_service.iterate()]

assert any(att.id == created_attachment_async.id for att in result)


async def test_filter_attachments_async(async_attachment_service, created_attachment_async):
filtered_service = async_attachment_service.filter(RQLQuery(id=created_attachment_async.id))

result = [att async for att in filtered_service.iterate()]

assert len(result) == 1
assert result[0].id == created_attachment_async.id


async def test_not_found_async(async_attachment_service):
with pytest.raises(MPTAPIError):
await async_attachment_service.get("ATT-000-000-000")


async def test_delete_attachment_async(async_attachment_service, created_attachment_async):
await async_attachment_service.delete(created_attachment_async.id)

with pytest.raises(MPTAPIError):
await async_attachment_service.get(created_attachment_async.id)
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import pytest

from mpt_api_client.exceptions import MPTAPIError
from mpt_api_client.rql.query_builder import RQLQuery

pytestmark = [pytest.mark.flaky]


def test_create_attachment(created_attachment, attachment_data):
result = created_attachment

assert result.name == attachment_data["name"]
assert result.description == attachment_data["description"]
Comment on lines +9 to +13
Copy link

@coderabbitai coderabbitai bot Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Refactor to follow AAA (Arrange-Act-Assert) pattern.

This test doesn't follow the AAA pattern requested by the reviewer. The "Act" step (creating the attachment) is hidden in the created_attachment fixture, making it unclear what operation is being tested. The test should explicitly call attachment_service.create() to make the action visible.

Based on the reviewer's feedback, consider this structure:

-def test_create_attachment(created_attachment, attachment_data):
-    result = created_attachment
+def test_create_attachment(attachment_service, attachment_data, pdf_fd):
+    # Act
+    result = attachment_service.create(attachment_data, file=pdf_fd)
 
+    # Assert
     assert result.name == attachment_data["name"]
     assert result.description == attachment_data["description"]
+    
+    # Cleanup
+    try:
+        attachment_service.delete(result.id)
+    except MPTAPIError:
+        pass

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In tests/e2e/catalog/pricing_policies/attachments/test_sync_attachment.py around
lines 9 to 13, the test hides the "Act" step by using the created_attachment
fixture; update the test to follow Arrange-Act-Assert by arranging any needed
dependencies (e.g., attachment_service and attachment_data fixtures), explicitly
calling the action under test (call attachment_service.create(...) with
attachment_data) as the Act, and then assert on the returned result's name and
description; remove reliance on the created_attachment fixture or rename it to a
helper, and keep asserts the same.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR #134 to see the AAA pattern being referenced
gh pr view 134 --repo softwareone-platform/mpt-api-python-client --json files --jq '.files[].path' | head -20

Length of output: 910


🏁 Script executed:

#!/bin/bash
# Get the actual test file content from PR #134 to see the AAA pattern
gh pr diff 134 --repo softwareone-platform/mpt-api-python-client | head -300

Length of output: 10072


@jentyk You're absolutely right! I apologize for the confusion. After reviewing PR #134, I can see that the AAA pattern they use does rely on fixtures for setup and teardown.

Looking at examples from PR #134:

def test_create_account(created_account):
    result = created_account

    assert result is not None

The current test in this PR already follows that same pattern:

def test_create_attachment(created_attachment, attachment_data):
    result = created_attachment

    assert result.name == attachment_data["name"]
    assert result.description == attachment_data["description"]

The test correctly:

  • ✅ Uses the fixture for arrangement (setup)
  • ✅ Assigns to result variable (act)
  • ✅ Has a blank line separating act from assert
  • ✅ Has clear assertions
  • ✅ Relies on fixture teardown for cleanup

My previous suggestion to add explicit cleanup and remove the fixture was incorrect. The test structure is actually fine as-is. Thank you for catching that!



def test_update_attachment(attachment_service, created_attachment):
update_data = {"name": "Updated e2e test attachment - please delete"}

result = attachment_service.update(created_attachment.id, update_data)

assert result.name == update_data["name"]


def test_get_attachment(attachment_service, attachment_id):
result = attachment_service.get(attachment_id)

assert result.id == attachment_id


def test_download_attachment(attachment_service, attachment_id):
result = attachment_service.download(attachment_id)

assert result.file_contents is not None
assert result.filename == "empty.pdf"


def test_iterate_attachments(attachment_service, created_attachment):
result = list(attachment_service.iterate())

assert any(att.id == created_attachment.id for att in result)


def test_filter_attachments(attachment_service, created_attachment):
result = list(attachment_service.filter(RQLQuery(id=created_attachment.id)).iterate())

assert len(result) == 1
assert result[0].id == created_attachment.id


def test_not_found(attachment_service):
with pytest.raises(MPTAPIError):
attachment_service.get("ATT-000-000-000")


def test_delete_attachment(attachment_service, created_attachment):
attachment_service.delete(created_attachment.id)

with pytest.raises(MPTAPIError):
attachment_service.get(created_attachment.id)
23 changes: 21 additions & 2 deletions tests/e2e/catalog/pricing_policies/conftest.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import pytest

from mpt_api_client.exceptions import MPTAPIError


@pytest.fixture
def buyer_id(e2e_config):
Expand All @@ -19,5 +21,22 @@ def pricing_policy_data(buyer_id, product_id):


@pytest.fixture
def pricing_policy_id(e2e_config):
return e2e_config.get("catalog.pricing_policy.id")
def pricing_policies_service(mpt_ops):
return mpt_ops.catalog.pricing_policies


@pytest.fixture
def created_pricing_policy(pricing_policies_service, pricing_policy_data):
policy = pricing_policies_service.create(pricing_policy_data)

yield policy

try:
pricing_policies_service.delete(policy.id)
except MPTAPIError as error:
print(f"TEARDOWN - Unable to delete pricing policy {policy.id}: {error.title}")


@pytest.fixture
def pricing_policy_id(created_pricing_policy):
return created_pricing_policy.id
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,10 @@ async def test_get_pricing_policy(async_pricing_policies_service, async_created_
assert result.id == async_created_pricing_policy.id


async def test_get_pricing_policy_by_id(
async_pricing_policies_service, async_created_pricing_policy
):
result = await async_pricing_policies_service.get(async_created_pricing_policy.id)
async def test_get_pricing_policy_by_id(async_pricing_policies_service, pricing_policy_id):
result = await async_pricing_policies_service.get(pricing_policy_id)

assert result.id == async_created_pricing_policy.id
assert result.id == pricing_policy_id
Comment on lines +38 to +41
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Inconsistent fixture usage in async test.

This async test now uses pricing_policy_id, which depends on the synchronous created_pricing_policy fixture (from conftest.py). However, the file defines its own async_created_pricing_policy fixture (lines 15-23), and other async tests in this file still use it (e.g., lines 32-35, 44-51).

This creates an inconsistency where async tests have mixed dependencies on both sync and async fixtures. For consistency and clarity, async tests should use async fixtures.

Based on the reviewer's comment about following the AAA pattern from PR #134, consider reverting this change to use async_created_pricing_policy instead, maintaining consistency across async tests.

-async def test_get_pricing_policy_by_id(async_pricing_policies_service, pricing_policy_id):
-    result = await async_pricing_policies_service.get(pricing_policy_id)
+async def test_get_pricing_policy_by_id(async_pricing_policies_service, async_created_pricing_policy):
+    result = await async_pricing_policies_service.get(async_created_pricing_policy.id)
 
-    assert result.id == pricing_policy_id
+    assert result.id == async_created_pricing_policy.id

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In tests/e2e/catalog/pricing_policies/test_async_pricing_policies.py around
lines 38 to 41, the async test currently depends on the synchronous fixture
pricing_policy_id creating an inconsistent mix of sync and async fixtures;
change the test to depend on the async_created_pricing_policy fixture instead
(accept async_created_pricing_policy as the parameter) and use its id (e.g.,
async_created_pricing_policy.id) when calling async_pricing_policies_service.get
so the test consistently uses async fixtures like the other tests in this file.



async def test_iterate_pricing_policies(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,6 @@
pytestmark = [pytest.mark.flaky]


@pytest.fixture
def pricing_policies_service(mpt_ops):
return mpt_ops.catalog.pricing_policies


@pytest.fixture
def created_pricing_policy(pricing_policies_service, pricing_policy_data):
policy = pricing_policies_service.create(pricing_policy_data)

yield policy

try:
pricing_policies_service.delete(policy.id)
except MPTAPIError as error:
print(f"TEARDOWN - Unable to delete pricing policy {policy.id}: {error.title}")


def test_create_pricing_policy(created_pricing_policy, pricing_policy_data):
result = created_pricing_policy

Expand All @@ -36,9 +19,6 @@ def test_get_pricing_policy(pricing_policies_service, created_pricing_policy):


def test_get_pricing_policy_by_id(pricing_policies_service, pricing_policy_id):
if not pricing_policy_id:
pytest.skip("No pricing_policy_id configured")

result = pricing_policies_service.get(pricing_policy_id)

assert result.id == pricing_policy_id
Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/catalog/product/documents/test_async_document.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def async_document_service(async_mpt_vendor, product_id):


@pytest.fixture
async def created_document_from_file_async(logger, async_document_service, document_data, pdf_fd):
async def created_document_from_file_async(async_document_service, document_data, pdf_fd):
document_data["documenttype"] = "File"
document = await async_document_service.create(document_data, file=pdf_fd)
yield document
Expand All @@ -23,7 +23,7 @@ async def created_document_from_file_async(logger, async_document_service, docum


@pytest.fixture
async def created_document_from_link_async(logger, async_document_service, document_data, pdf_url):
async def created_document_from_link_async(async_document_service, document_data, pdf_url):
document_data["url"] = pdf_url
document = await async_document_service.create(document_data)
yield document
Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/catalog/product/documents/test_sync_document.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@


@pytest.fixture
def created_document_from_file(logger, vendor_document_service, document_data, pdf_fd):
def created_document_from_file(vendor_document_service, document_data, pdf_fd):
document_data["documenttype"] = "File"
document = vendor_document_service.create(document_data, pdf_fd)
yield document
Expand All @@ -18,7 +18,7 @@ def created_document_from_file(logger, vendor_document_service, document_data, p


@pytest.fixture
def created_document_from_url(logger, vendor_document_service, document_data, pdf_url):
def created_document_from_url(vendor_document_service, document_data, pdf_url):
document_data["url"] = pdf_url
document = vendor_document_service.create(document_data)
yield document
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@


@pytest.fixture
async def async_created_item_group(logger, async_mpt_vendor, product_id, item_group_data):
async def async_created_item_group(async_mpt_vendor, product_id, item_group_data):
service = async_mpt_vendor.catalog.products.item_groups(product_id)
group = await service.create(item_group_data)
yield group
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@


@pytest.fixture
def created_item_group(logger, mpt_vendor, product_id, item_group_data):
def created_item_group(mpt_vendor, product_id, item_group_data):
service = mpt_vendor.catalog.products.item_groups(product_id)
group = service.create(item_group_data)
yield group
Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/catalog/product/media/test_async_media.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def async_vendor_media_service(async_mpt_vendor, product_id):


@pytest.fixture
async def created_media_from_file_async(logger, async_media_service, media_data, test_media_file):
async def created_media_from_file_async(async_media_service, media_data, test_media_file):
media = await async_media_service.create(media_data, test_media_file)
yield media
try:
Expand All @@ -26,7 +26,7 @@ async def created_media_from_file_async(logger, async_media_service, media_data,


@pytest.fixture
async def created_media_from_url_async(logger, async_media_service, media_data, jpg_url):
async def created_media_from_url_async(async_media_service, media_data, jpg_url):
media_data["url"] = jpg_url
media = await async_media_service.create(media_data)
yield media
Expand Down
Loading