feat: added output management integration by Venkatakrishnan774 · Pull Request #161 · SAP/cloud-sdk-python

Venkatakrishnan774 · 2026-06-14T12:21:39Z

Disclaimer: Do not include SAP-internal or customer-specific information in this PR (e.g. internal system URLs, customer names, tenant IDs, or confidential configurations). This is a public repository.

Description

Output Management is a BTP shared service which has features like sending documents to external system in various channels like EDI/EMAIL/Print. As part of this, we are mainly concentrating on the email use case where one can send email via our framework along with attachments if necessary.

We are exposing capabilities via util methods of sending email as BTP destination based and via integrated MCP hub. When using the MCP Hub way, we are giving the capability to construct the required payload and invoke the util functions. When using the BTP destination way, we are giving the options to input the destination details and the payloads.

Related Issue

Closes #<issue_number>

(Link to the GitHub issue this PR addresses)

Type of Change

Please check the relevant option:

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update
Code refactoring
Dependency update

How to Test

Describe how reviewers can test your changes:

Step 1
Step 2
Expected result

Checklist

Before submitting your PR, please review and check the following:

I have read the Contributing Guidelines
I have verified that my changes solve the issue
I have added/updated automated tests to cover my changes
All tests pass locally
I have verified that my code follows the Code Guidelines
I have updated documentation (if applicable)
I have added type hints for all public APIs
My code does not contain sensitive information (credentials, tokens, etc.)
I have followed Conventional Commits for commit messages

Breaking Changes

If this PR introduces breaking changes, please describe:

What breaks
Migration path for users
Alternative approaches considered

Additional Notes

Add any additional context, screenshots, or information that would help reviewers.

cla-assistant · 2026-06-14T12:22:00Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ Venkatakrishnan774
❌ lokeshkumarkuntal
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

betinacosta · 2026-06-18T12:45:44Z

Hi. Could you give a more detailed description of this new feature for future reference? Also, is necessary that the title follows commit convention, for exemple: feat: added output management integration.

betinacosta · 2026-06-18T13:24:52Z

Tests seems to be failing. Could you fix it?

betinacosta · 2026-06-18T13:31:45Z

+        self,
+        http_session: requests.Session,
+        base_url: str,
+        destination: any = None,


If these parameters are optional, please use:

destination: Optional[any] = None

or even

destination: Optional[Destination] = None

betinacosta · 2026-06-18T13:33:37Z

+        http_session: requests.Session,
+        base_url: str,
+        destination: any = None,
+        destination_instance: str = None,


destination_instance: Optional[str] = None

betinacosta · 2026-06-18T13:37:26Z

+    def __init__(
+        self,
+        base_url: str,
+        destination: any = None,


destination: Optional[any] = None

or even

destination: Optional[Destination] = None

betinacosta · 2026-06-18T13:39:13Z

+        self,
+        base_url: str,
+        destination: any = None,
+        destination_instance: str = None,


destination_instance: Optional[str] = None

betinacosta · 2026-06-18T13:43:29Z

+
+        return None
+
+    def _execute_request(self, method: str, endpoint: str, **kwargs):


Missing return type annotation.

betinacosta · 2026-06-18T13:59:50Z

+        self._destination_instance = destination_instance
+
+        # Get sender-provider-subaccount-id from environment variable
+        self._sender_provider_subaccount_id = os.getenv("APPFND_CONHOS_SUBACCOUNTID")


We are now using CLOUD_SDK_CFG_ as prefix. You can algo take a look at src/sap_cloud_sdk/core/secret_resolver, it may help

betinacosta · 2026-06-18T14:02:39Z

+        # Get the destination object - it handles authentication automatically
+        http_destination = self._destination_credential_config.get_destination()
+
+        # Get the base URL from destinatiozxn


betinacosta · 2026-06-18T14:10:46Z

+
+        # If no ID found, use template key as fallback
+        if not doc_id:
+            doc_id = f"{notification_template_key}-{id(business_document)}"


Use id() as fallback can not be a good choice, once it is not stable across runs

betinacosta · 2026-06-18T14:27:50Z

+        # Build attachment config if URLs are provided
+        attachment_config = None
+        if attachment_urls:
+            from ..models.attachment_config import AttachmentConfig


This lazy imports seems to serve no purpose here. There is no circular dependency risk here.

betinacosta · 2026-06-18T14:32:16Z

+                from sap_cloud_sdk.destination import create_certificate_client, AccessStrategy
+                import tempfile
+                import base64
+                from cryptography.hazmat.primitives.serialization import pkcs12, Encoding, PrivateFormat, NoEncryption


This hides an undeclared runtime dependency from static analysis tools (ruff, ty), prevents the type checker from seeing the symbols, and means the ImportError is only discovered at call time rather than at startup.

betinacosta · 2026-06-18T14:32:50Z

+import logging
+from typing import Dict, Optional
+import os
+import json


Is this used?

betinacosta · 2026-06-18T14:34:15Z

+    CONTENT_TYPE_PDF = "application/pdf"
+
+
+class Status(Enum):


Shouldn't the values be lowercased?

betinacosta · 2026-06-18T14:34:25Z

+    CANCELLED = "CANCELLED"
+
+
+class FileFormat(Enum):


Shouldn't the values be lowercased?

betinacosta · 2026-06-18T14:34:33Z

+    XML = "XML"
+
+
+class Channel(Enum):


Shouldn't the values be lowercased?

betinacosta · 2026-06-18T14:36:06Z

+    API_OUTPUT_CONTROL = "/api/output-control-api/v1/"
+
+    # Headers
+    CONTENT_TYPE = "Content-Type"


Why CONTENT_TYPE and HEADER_CONTENT_TYPE have the same value?

betinacosta · 2026-06-18T14:37:44Z

+
+    # Headers
+    CONTENT_TYPE = "Content-Type"
+    APPLICATION_JSON = "application/json"


Same goes for APPLICATION_JSON and CONTENT_TYPE_JSON.

betinacosta · 2026-06-18T14:49:37Z

+
+        return None
+
+    def _execute_request(self, method: str, endpoint: str, **kwargs):


Also, is this method necessary? Its a wrapper for just one line.

betinacosta · 2026-06-18T15:00:14Z

+        destination_name: str,
+        cc: Optional[List[str]] = None,
+        template_language: str = "en",
+        access_strategy: str = "PROVIDER_ONLY",


access_strategy: Optional[str] = "PROVIDER_ONLY"

betinacosta · 2026-06-18T15:01:27Z

+        business_document: Dict[str, Any],
+        destination_name: str,
+        cc: Optional[List[str]] = None,
+        template_language: str = "en",


template_language: Optional[str] = "en"

betinacosta · 2026-06-18T15:19:44Z

+
+        try:
+            request_body = output_request.model_dump(by_alias=True, exclude_none=True)
+            logger.info(f"Request body: {request_body}")


Should we log the request body? I believe it may have sensitive information.

Venkatakrishnan774 · 2026-06-19T06:32:41Z

Hi. Could you give a more detailed description of this new feature for future reference? Also, is necessary that the title follows commit convention, for exemple: feat: added output management integration.

Done

Venkatakrishnan774 and others added 11 commits May 15, 2026 15:16

added output management

6816198

updated workaround

477e98b

updated workaround

0e0bdfd

updated workaround

21ec7d5

updated workaround

78ce649

adding a logger to test access to this repo

ef83886

adding a logger to test access to this repo

ad3c8d1

removed harcoded values

b817b56

added pre generated attachment section

8e1da26

added pre generated attachment section

6bfcf9d

added tests

68079ab

Venkatakrishnan774 requested a review from a team as a code owner June 14, 2026 12:21

Venkatakrishnan774 added 3 commits June 15, 2026 11:12

added mcp way to communicate

15606cd

added mcp way to communicate

59426a3

added test

9b90568

betinacosta reviewed Jun 18, 2026

View reviewed changes

Comment thread tests/outputmanagement/unit/__init__.py

betinacosta Jun 18, 2026

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

Tests seems to be failing. Could you fix it?

betinacosta reviewed Jun 18, 2026

View reviewed changes

Venkatakrishnan774 changed the title ~~Added output management integration~~ feat: added output management integration Jun 18, 2026

betinacosta reviewed Jun 18, 2026

View reviewed changes

fixed tests

d24cbf1


		return None

		def _execute_request(self, method: str, endpoint: str, **kwargs):

Conversation

Venkatakrishnan774 commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Type of Change

How to Test

Checklist

Breaking Changes

Additional Notes

Uh oh!

cla-assistant Bot commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

betinacosta commented Jun 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

betinacosta Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Venkatakrishnan774 commented Jun 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Venkatakrishnan774 commented Jun 14, 2026 •

edited

Loading

cla-assistant Bot commented Jun 14, 2026 •

edited

Loading

betinacosta Jun 18, 2026 •

edited

Loading