diff --git a/src/sentry/integrations/perforce/client.py b/src/sentry/integrations/perforce/client.py index 52058ce5ece2b9..27186ce5ecd8c8 100644 --- a/src/sentry/integrations/perforce/client.py +++ b/src/sentry/integrations/perforce/client.py @@ -2,10 +2,14 @@ import logging from collections.abc import Sequence +from contextlib import contextmanager from typing import Any from P4 import P4, P4Exception +from sentry.integrations.models.integration import Integration +from sentry.integrations.models.organization_integration import OrganizationIntegration +from sentry.integrations.services.integration import RpcIntegration, RpcOrganizationIntegration from sentry.integrations.source_code_management.commit_context import ( CommitContextClient, FileBlameInfo, @@ -14,6 +18,7 @@ from sentry.integrations.source_code_management.repository import RepositoryClient from sentry.models.pullrequest import PullRequest, PullRequestComment from sentry.models.repository import Repository +from sentry.shared_integrations.exceptions import ApiError, ApiUnauthorized, IntegrationError logger = logging.getLogger(__name__) @@ -29,42 +34,137 @@ class PerforceClient(RepositoryClient, CommitContextClient): def __init__( self, - p4port: str | None = None, - user: str | None = None, - password: str | None = None, - client: str | None = None, - ssl_fingerprint: str | None = None, + integration: Integration | RpcIntegration, + org_integration: OrganizationIntegration | RpcOrganizationIntegration | None = None, ): """ Initialize Perforce client. Args: - p4port: P4PORT string (e.g., 'ssl:host:port', 'tcp:host:port', or 'host:port') - user: Perforce username - password: Perforce password OR P4 ticket (both are supported) - client: Client/workspace name - ssl_fingerprint: SSL trust fingerprint for secure connections - """ - self.p4port = p4port - self.ssl_fingerprint = ssl_fingerprint - self.user = user or "" - self.password = password - self.client_name = client + integration: Integration instance containing credentials in metadata + org_integration: Organization integration instance (required for API compatibility) + """ + self.integration = integration + self.org_integration = org_integration self.P4 = P4 self.P4Exception = P4Exception + # Extract configuration from integration.metadata + if not org_integration: + raise IntegrationError("Organization Integration is required for Perforce") + + metadata = integration.metadata + self.p4port = metadata.get("p4port", "localhost:1666") + self.user = metadata.get("user", "") + self.password = metadata.get("password") + self.auth_type = metadata.get( + "auth_type", "password" + ) # Default to password for backwards compat + self.client_name = metadata.get("client") + self.ssl_fingerprint = metadata.get("ssl_fingerprint") + + @contextmanager def _connect(self): - """Create and connect a P4 instance with SSL support.""" - pass + """ + Context manager for P4 connections with automatic cleanup. + + Yields a connected P4 instance and ensures disconnection on exit. + + Uses P4Python API: + - p4.connect(): https://www.perforce.com/manuals/p4python/Content/P4Python/python.programming.html#python.programming.connecting + - p4.run_trust(): https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_trust.html + - p4.run_login(): https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_login.html + + Example: + with self._connect() as p4: + result = p4.run("info") + """ + p4 = self.P4() + p4.port = self.p4port + p4.user = self.user + p4.password = self.password + + if self.client_name: + p4.client = self.client_name + + p4.exception_level = 1 # Only errors raise exceptions + + # Connect to Perforce server + try: + p4.connect() + except self.P4Exception as e: + error_msg = str(e) + # Provide helpful error message for connection failures + if "SSL" in error_msg or "trust" in error_msg.lower(): + raise ApiError( + f"Failed to connect to Perforce (SSL issue): {error_msg}. " + f"Ensure ssl_fingerprint is correct. Obtain with: p4 -p {self.p4port} trust -y" + ) + raise ApiError(f"Failed to connect to Perforce: {error_msg}") - def _disconnect(self, p4): - """Disconnect P4 instance.""" - pass + # Assert SSL trust after connection (if needed) + # This must be done after p4.connect() but before p4.run_login() + if self.ssl_fingerprint and self.p4port.startswith("ssl:"): + try: + p4.run_trust("-i", self.ssl_fingerprint) + except self.P4Exception as trust_error: + try: + p4.disconnect() + except Exception: + pass + raise ApiError( + f"Failed to establish SSL trust: {trust_error}. " + f"Ensure ssl_fingerprint is correct. Obtain with: p4 -p {self.p4port} trust -y" + ) + + # Authenticate based on auth_type + # - password: Requires run_login() to exchange password for session ticket + # - ticket: Already authenticated via p4.password, no login needed + if self.password and self.auth_type == "password": + try: + p4.run_login() + except self.P4Exception as login_error: + try: + p4.disconnect() + except Exception: + pass + raise ApiUnauthorized( + f"Failed to authenticate with Perforce: {login_error}. " + "Verify your password is correct." + ) + elif self.password and self.auth_type == "ticket": + # Ticket authentication: p4.password is already set to the ticket + # Verify ticket works by running a test command + try: + p4.run("info") + except self.P4Exception as e: + try: + p4.disconnect() + except Exception: + pass + raise ApiUnauthorized( + f"Failed to authenticate with Perforce ticket: {e}. " + "Verify your P4 ticket is valid. Obtain a new ticket with: p4 login -p" + ) + + try: + yield p4 + finally: + # Ensure cleanup + try: + if p4.connected(): + p4.disconnect() + except Exception as e: + # Log disconnect failures as they may indicate connection leaks + logger.warning("Failed to disconnect from Perforce: %s", e, exc_info=True) def check_file(self, repo: Repository, path: str, version: str | None) -> object | None: """ Check if a file exists in the depot. + Uses p4 files command to list file(s) in the depot. + API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_files.html + Args: repo: Repository object containing depot path (includes stream if specified) path: File path relative to depot @@ -73,16 +173,128 @@ def check_file(self, repo: Repository, path: str, version: str | None) -> object Returns: File info dict if exists, None otherwise """ - return None + with self._connect() as p4: + try: + depot_path = self.build_depot_path(repo, path) + result = p4.run("files", depot_path) + + # Verify result contains actual file data (not just warnings) + # When exception_level=1, warnings are returned in result list + if result and len(result) > 0 and "depotFile" in result[0]: + return result[0] + return None + + except self.P4Exception: + return None + + def build_depot_path(self, repo: Repository, path: str, stream: str | None = None) -> str: + """ + Build full depot path from repo config and file path. + + Handles both relative and absolute paths: + - Relative: "depot/app/file.py" or "app/file.py" → "//depot/app/file.py" + - Absolute: "//depot/app/file.py" → "//depot/app/file.py" (unchanged) + - With stream: "app/file.py" + stream="main" → "//depot/main/app/file.py" + + Args: + repo: Repository object + path: File path (may include #revision for file revisions like "file.cpp#1") + stream: Optional stream name to insert after depot (e.g., "main", "dev") + + Returns: + Full depot path with #revision preserved if present + """ + # Extract file revision if present (# syntax only) + revision = None + path_without_rev = path + + if "#" in path: + path_without_rev, revision = path.rsplit("#", 1) + + # If already absolute depot path, use as-is + if path_without_rev.startswith("//"): + full_path = path_without_rev + else: + depot_root = repo.config.get("depot_path", repo.name).rstrip("/") + + # Normalize depot_root to ensure it starts with // + if not depot_root.startswith("//"): + depot_root = f"//{depot_root}" + + # Strip depot name from path if it duplicates depot_root + # e.g., depot_root="//depot", path="depot/app/file.py" → "app/file.py" + depot_name = depot_root.lstrip("/") # "depot" + if path_without_rev.startswith(depot_name + "/") or path_without_rev == depot_name: + path_without_rev = path_without_rev[len(depot_name) :].lstrip("/") + + # Remove leading slashes from relative path + path_without_rev = path_without_rev.lstrip("/") + + # Handle Perforce streams: insert stream after depot + # Format: //depot/stream/path/to/file + if stream: + full_path = f"{depot_root}/{stream}/{path_without_rev}" + else: + full_path = f"{depot_root}/{path_without_rev}" + + # Add file revision back if present + if revision: + full_path = f"{full_path}#{revision}" + + return full_path def get_depots(self) -> list[dict[str, Any]]: """ List all depots accessible to the user. + Uses p4 depots command to display a list of all depots. + API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_depots.html + Returns: List of depot info dictionaries """ - return [] + with self._connect() as p4: + depots = p4.run("depots") + return [ + { + "name": depot.get("name"), + "type": depot.get("type"), + "description": depot.get("desc", ""), + } + for depot in depots + ] + + def get_user(self, username: str) -> dict[str, Any] | None: + """ + Get user information from Perforce. + + Uses p4 user command to fetch user details including email and full name. + API docs: https://www.perforce.com/manuals/cmdref/Content/CmdRef/p4_user.html + + Args: + username: Perforce username + + Returns: + User info dictionary with Email and FullName fields, or None if not found + + Raises: + P4Exception: For connection or transient errors that may be retryable + """ + with self._connect() as p4: + result = p4.run("user", "-o", username) + if result and len(result) > 0: + user_info = result[0] + # p4 user -o returns a template for non-existent users + # Check if user actually exists by verifying Update field is set + if not user_info.get("Update"): + return None + return { + "email": user_info.get("Email", ""), + "full_name": user_info.get("FullName", ""), + "username": user_info.get("User", username), + } + # User not found - return None (not an error condition) + return None def get_changes( self, depot_path: str, max_changes: int = 20, start_cl: str | None = None @@ -121,7 +333,7 @@ def get_file( ) -> str: """ Get file contents from Perforce depot. - Required by abstract base class but not used (CODEOWNERS feature removed). + Required by abstract base class but not used (CODEOWNERS). """ raise NotImplementedError("get_file is not supported for Perforce") diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index 8313cb2ed6d963..d4c013f6b384c4 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/src/sentry/integrations/perforce/integration.py @@ -1,9 +1,12 @@ from __future__ import annotations +import hashlib import logging from collections.abc import Mapping, Sequence from typing import Any +from django import forms +from django.http import HttpRequest, HttpResponseBase from django.utils.translation import gettext_lazy as _ from sentry.integrations.base import ( @@ -15,13 +18,15 @@ ) from sentry.integrations.models.integration import Integration from sentry.integrations.perforce.client import PerforceClient +from sentry.integrations.pipeline import IntegrationPipeline from sentry.integrations.services.repository import RpcRepository from sentry.integrations.source_code_management.commit_context import CommitContextIntegration from sentry.integrations.source_code_management.repository import RepositoryIntegration from sentry.models.repository import Repository from sentry.organizations.services.organization.model import RpcOrganization from sentry.pipeline.views.base import PipelineView -from sentry.shared_integrations.exceptions import ApiError +from sentry.shared_integrations.exceptions import ApiError, ApiUnauthorized, IntegrationError +from sentry.web.helpers import render_to_response logger = logging.getLogger(__name__) @@ -66,6 +71,85 @@ ) +class PerforceInstallationForm(forms.Form): + """Form for Perforce installation configuration.""" + + p4port = forms.CharField( + label=_("P4PORT (Server Address)"), + help_text=_( + "Perforce server address in P4PORT format. " + "Examples: 'ssl:perforce.company.com:1666' (encrypted), " + "'perforce.company.com:1666' or 'tcp:perforce.company.com:1666' (plaintext). " + "SSL is strongly recommended for production use." + ), + widget=forms.TextInput(attrs={"placeholder": "ssl:perforce.company.com:1666"}), + ) + user = forms.CharField( + label=_("Perforce Username"), + help_text=_( + "Username for authenticating with Perforce. " + "Required for both password and ticket authentication." + ), + widget=forms.TextInput(attrs={"placeholder": "sentry-bot"}), + ) + auth_type = forms.ChoiceField( + label=_("Authentication Type"), + choices=[ + ("password", _("Password")), + ("ticket", _("P4 Ticket")), + ], + initial="password", + help_text=_( + "Select whether you're providing a password or a P4 ticket. " + "Tickets are obtained via 'p4 login -p' and don't require re-authentication." + ), + ) + password = forms.CharField( + label=_("Password / Ticket"), + help_text=_( + "Your Perforce password or P4 authentication ticket " + "(depending on the authentication type selected above)." + ), + widget=forms.PasswordInput(attrs={"placeholder": "••••••••"}), + ) + client = forms.CharField( + label=_("Perforce Client/Workspace (Optional)"), + help_text=_("Optional: Specify a client workspace name"), + widget=forms.TextInput(attrs={"placeholder": "sentry-workspace"}), + required=False, + ) + ssl_fingerprint = forms.CharField( + label=_("SSL Fingerprint (Required for SSL)"), + help_text=_( + "SSL fingerprint for secure connections. " + "Required when using 'ssl:' protocol. " + "Obtain with: p4 -p ssl:host:port trust -y" + ), + widget=forms.TextInput( + attrs={"placeholder": "AB:CD:EF:01:23:45:67:89:AB:CD:EF:01:23:45:67:89:AB:CD:EF:01"} + ), + required=False, + ) + web_url = forms.URLField( + label=_("Helix Swarm URL (Optional)"), + help_text=_("Optional: URL to Helix Swarm web viewer for browsing files"), + widget=forms.URLInput(attrs={"placeholder": "https://swarm.company.com"}), + required=False, + assume_scheme="https", + ) + + def clean_p4port(self): + """Strip off trailing / and whitespace from p4port""" + return self.cleaned_data["p4port"].strip().rstrip("/") + + def clean_web_url(self): + """Strip off trailing / from web_url""" + web_url = self.cleaned_data.get("web_url", "") + if web_url: + return web_url.rstrip("/") + return web_url + + class PerforceIntegration(RepositoryIntegration, CommitContextIntegration): """ Integration for Perforce/Helix Core version control system. @@ -84,8 +168,16 @@ def __init__( def get_client(self) -> PerforceClient: """Get the Perforce client instance.""" - if self._client is None: - self._client = PerforceClient() + if self._client is not None: + return self._client + + if not self.org_integration: + raise IntegrationError("Organization Integration not found") + + self._client = PerforceClient( + integration=self.model, + org_integration=self.org_integration, + ) return self._client def on_create_or_update_comment_error(self, api_error: ApiError, metrics_base: str) -> bool: @@ -97,6 +189,13 @@ def on_create_or_update_comment_error(self, api_error: ApiError, metrics_base: s def source_url_matches(self, url: str) -> bool: """Check if URL is from this Perforce server.""" + if url.startswith("p4://"): + return True + + web_url = self.model.metadata.get("web_url") + if web_url and url.startswith(web_url): + return True + return False def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None: @@ -112,8 +211,28 @@ def check_file(self, repo: Repository, filepath: str, branch: str | None = None) Returns: Formatted URL if the file exists, None otherwise + + Raises: + ApiUnauthorized: For authentication failures (should be shown to user) + IntegrationError: For configuration errors (should be shown to user) """ - return None + try: + client = self.get_client() + # Use client's check_file to verify file exists on P4 server + result = client.check_file(repo, filepath, branch) + if result is None: + return None + # File exists, return formatted URL + return self.format_source_url(repo, filepath, branch) + except (ApiUnauthorized, IntegrationError): + # Re-raise auth/config errors so they're visible to users + raise + except ApiError: + # Re-raise API errors for visibility + raise + except Exception: + # For other errors (e.g., file not found), return None + return None def format_source_url(self, repo: Repository, filepath: str, branch: str | None) -> str: """ @@ -131,7 +250,32 @@ def format_source_url(self, repo: Repository, filepath: str, branch: str | None) Returns: Formatted URL (p4:// or Swarm web viewer URL) """ - return "" + # Use client's build_depot_path to handle both relative and absolute paths correctly + client = self.get_client() + full_path = client.build_depot_path(repo, filepath, branch) + + # If Swarm web viewer is configured, use it + # Web URL is stored in Integration.metadata + web_url = self.model.metadata.get("web_url") + + if web_url: + # Extract file revision from filepath if present (e.g., "file.cpp#1") + revision = None + path_without_rev = full_path + if "#" in full_path: + path_without_rev, revision = full_path.rsplit("#", 1) + + # Swarm format: /files/?v= + if revision: + url = f"{web_url}/files{path_without_rev}?v={revision}" + else: + url = f"{web_url}/files{full_path}" + return url + + # Default: p4:// protocol URL with file revision (#) syntax + # Strip leading // from full_path to avoid p4://// + url = f"p4://{full_path.lstrip('/')}" + return url def extract_branch_from_source_url(self, repo: Repository, url: str) -> str: """ @@ -151,7 +295,42 @@ def extract_source_path_from_source_url(self, repo: Repository, url: str) -> str Returns just the file path without revision info. """ - return "" + depot_path = repo.config.get("depot_path", repo.name) + + # Handle Swarm web viewer URLs + # Web URL is stored in Integration.metadata + web_url = self.model.metadata.get("web_url") + if web_url and url.startswith(web_url): + # Strip Swarm base URL and /files prefix + # e.g., "https://swarm.example.com/files//depot/path/file.cpp" -> "//depot/path/file.cpp" + url = url[len(web_url) :] + if url.startswith("/files"): + url = url[6:] # Remove "/files" + + # Remove p4:// prefix + if url.startswith("p4://"): + url = url[5:] + + # Remove revision specifier (#revision) + if "#" in url: + url = url.split("#")[0] + + # Remove query parameters (for web viewers) + if "?" in url: + url = url.split("?")[0] + + # Normalize both paths by stripping leading slashes for comparison + # depot_path is typically "//depot" from config + # url after stripping prefix is "depot/path/file.cpp" + normalized_depot = depot_path.lstrip("/") + normalized_url = url.lstrip("/") + + # Remove depot prefix to get relative path + # Ensure exact match by checking for separator or exact equality + if normalized_url.startswith(normalized_depot + "/") or normalized_url == normalized_depot: + return normalized_url[len(normalized_depot) :].lstrip("/") + + return url def get_repositories( self, query: str | None = None, page_number_limit: int | None = None @@ -159,14 +338,64 @@ def get_repositories( """ Get list of depots/streams from Perforce server. + Args: + query: Optional search query to filter depot names + page_number_limit: Maximum number of repositories to return + Returns: - List of repository dictionaries + List of repository dictionaries (limited by page_number_limit if provided) """ - return [] + try: + client = self.get_client() + depots = client.get_depots() + + repositories = [] + for depot in depots: + depot_name = depot["name"] + depot_path = f"//{depot_name}" + + # Filter by query if provided + if query and query.lower() not in depot_name.lower(): + continue + + repositories.append( + { + "name": depot_name, + "identifier": depot_path, + "default_branch": None, # Perforce uses depot paths, not branch refs + } + ) + + # Apply pagination limit if specified + if page_number_limit and len(repositories) >= page_number_limit: + break + + return repositories + + except ApiError: + # Re-raise authentication/connection errors so user sees them + raise + except Exception as e: + raise IntegrationError(f"Failed to fetch repositories from Perforce: {str(e)}") def has_repo_access(self, repo: RpcRepository) -> bool: """Check if integration can access the depot.""" - return False + try: + client = self.get_client() + depot_path = repo.config.get("depot_path", repo.name) + + # Verify depot exists by checking depot list instead of listing all files + # Using "..." wildcard would fetch metadata for all files in large repos + depots = client.get_depots() + + # Extract depot name from path (e.g., "//depot" -> "depot") + depot_name = depot_path.lstrip("/").split("/")[0] + + # Check if depot exists in the list + return any(depot["name"] == depot_name for depot in depots) + + except Exception: + return False def get_unmigratable_repositories(self) -> list[RpcRepository]: """Get repositories that can't be migrated. Perforce doesn't need migration.""" @@ -175,7 +404,9 @@ def get_unmigratable_repositories(self) -> list[RpcRepository]: def get_organization_config(self) -> list[dict[str, Any]]: """ Get configuration form fields for organization-level settings. - These fields will be displayed in the integration settings UI. + + Returns the form schema (field definitions, labels, help text, types). + Current values are provided separately via get_config_data(). """ return [ { @@ -194,12 +425,23 @@ def get_organization_config(self) -> list[dict[str, Any]]: "help": "Username for authenticating with Perforce. Required for both password and ticket authentication.", "required": True, }, + { + "name": "auth_type", + "type": "choice", + "label": "Authentication Type", + "choices": [ + ["password", "Password"], + ["ticket", "P4 Ticket"], + ], + "help": "Select whether you're providing a password or a P4 ticket. Tickets are obtained via 'p4 login -p' and don't require re-authentication.", + "required": True, + }, { "name": "password", "type": "secret", - "label": "Password or P4 Ticket", + "label": "Password / Ticket", "placeholder": "••••••••", - "help": "Perforce password OR P4 authentication ticket. Tickets are obtained via 'p4 login -p' and are more secure than passwords. Both are supported in this field.", + "help": "Your Perforce password or P4 authentication ticket (depending on the authentication type selected above).", "required": True, }, { @@ -228,6 +470,45 @@ def get_organization_config(self) -> list[dict[str, Any]]: }, ] + def get_config_data(self) -> Mapping[str, Any]: + """ + Get current configuration values for the integration. + + This is called by the serializer to populate the form fields with existing values. + Since we store credentials in integration.metadata (not org_integration.config), + we override the base implementation to read from metadata. + + Returns: + Dictionary of current configuration values that will be used to populate + the form fields defined in get_organization_config() + """ + return self.model.metadata + + def update_organization_config(self, data: Mapping[str, Any]) -> None: + """ + Update organization configuration by saving to integration.metadata. + + Since each organization has its own private Perforce integration instance, + we store credentials in integration.metadata instead of org_integration.config. + + Only updates fields that are present in data, preserving existing values + for fields not included in the update. + + Args: + data: Updated configuration data from the form (only changed fields) + """ + from sentry.integrations.services.integration import integration_service + + # Update integration metadata with new values + metadata = dict(self.model.metadata) # Create a mutable copy + metadata.update(data) # Only update fields present in data + + # Update the integration with new metadata + integration_service.update_integration( + integration_id=self.model.id, + metadata=metadata, + ) + class PerforceIntegrationProvider(IntegrationProvider): """Provider for Perforce integration.""" @@ -252,28 +533,63 @@ def build_integration(self, state: Mapping[str, Any]) -> IntegrationData: """ Build integration data from installation state. + Each organization gets its own private Perforce integration instance, + even if multiple orgs connect to the same Perforce server. This ensures + credentials and configuration are isolated per organization. + + Credentials are stored in Integration.metadata since each integration + is unique per organization (external_id includes org_id). + Args: state: Installation state from pipeline Returns: Integration data dictionary + + Raises: + IntegrationError: If organization_id is not provided """ + # Validate organization_id is present + organization_id = state.get("organization_id") + if not organization_id: + raise IntegrationError("Organization ID is required for Perforce integration") + # Use p4port if available, otherwise fall back to host:port for legacy + installation_data = state.get("installation_data", {}) p4port = ( - state.get("p4port") or f"{state.get('host', 'localhost')}:{state.get('port', '1666')}" + installation_data.get("p4port") + or state.get("p4port") + or f"{state.get('host', 'localhost')}:{state.get('port', '1666')}" ) + # Create unique external_id per organization to ensure private integrations + # Use hash to avoid exceeding 64-character external_id limit with long hostnames + # Format: "perforce-org-{org_id}-{hash}" where hash is first 8 chars of SHA256(p4port) + p4port_hash = hashlib.sha256(p4port.encode("utf-8")).hexdigest()[:8] + external_id = f"perforce-org-{organization_id}-{p4port_hash}" + + # Store credentials in Integration.metadata + metadata = { + "p4port": p4port, + "user": installation_data.get("user", ""), + "auth_type": installation_data.get("auth_type", "password"), # Default to password + "password": installation_data.get("password", ""), + } + + # Add optional fields if provided + if installation_data.get("client"): + metadata["client"] = installation_data["client"] + + if installation_data.get("ssl_fingerprint"): + metadata["ssl_fingerprint"] = installation_data["ssl_fingerprint"] + + if installation_data.get("web_url"): + metadata["web_url"] = installation_data["web_url"] + return { "name": state.get("name", f"Perforce ({p4port})"), - "external_id": p4port, - "metadata": { - "p4port": p4port, - "user": state.get("user"), - "password": state.get("password"), - "client": state.get("client"), - "ssl_fingerprint": state.get("ssl_fingerprint"), - "web_url": state.get("web_url"), - }, + "external_id": external_id, + "metadata": metadata, } def post_install( @@ -283,7 +599,12 @@ def post_install( *, extra: dict[str, Any], ) -> None: - """Actions after installation.""" + """ + Actions after installation. + + Configuration is now stored in Integration.metadata (set by build_integration), + so no additional setup is needed per organization. + """ pass def setup(self) -> None: @@ -302,16 +623,115 @@ def setup(self) -> None: class PerforceInstallationView: """ Installation view for Perforce configuration. - - This is a simple pass-through view. The actual configuration - happens in the Settings tab after installation via get_organization_config(). + Collects and validates Perforce server credentials during installation. """ - def dispatch(self, request, pipeline): + def dispatch(self, request: HttpRequest, pipeline: IntegrationPipeline) -> HttpResponseBase: """ - Handle installation request. + Handle installation request with form validation. + Args: request: HTTP request object pipeline: Installation pipeline + + Returns: + HTTP response (form render or redirect to next step) """ - return pipeline.next_step() + if request.method == "POST": + form = PerforceInstallationForm(request.POST) + if form.is_valid(): + form_data = form.cleaned_data + + # Verify connection to Perforce server before completing installation + try: + client = PerforceClient( + integration=type( + "obj", + (object,), + { + "metadata": { + "p4port": form_data.get("p4port"), + "user": form_data.get("user"), + "password": form_data.get("password"), + "auth_type": form_data.get("auth_type", "password"), + "client": form_data.get("client"), + "ssl_fingerprint": form_data.get("ssl_fingerprint"), + } + }, + )(), + org_integration=type("obj", (object,), {})(), + ) + # Test connection by fetching depot list + client.get_depots() + + pipeline.get_logger().info( + "perforce.setup.connection-verified", + extra={ + "p4port": form_data.get("p4port"), + "user": form_data.get("user"), + }, + ) + except ApiUnauthorized as e: + form.add_error( + None, + f"Authentication failed: {e}. Please check your username and password.", + ) + return render_to_response( + template="sentry/integrations/perforce-config.html", + context={"form": form}, + request=request, + ) + except ApiError as e: + form.add_error( + None, + f"Failed to connect to Perforce server: {e}. Please verify your server address and SSL fingerprint.", + ) + return render_to_response( + template="sentry/integrations/perforce-config.html", + context={"form": form}, + request=request, + ) + except Exception as e: + pipeline.get_logger().error( + "perforce.setup.connection-verification-failed", + extra={ + "p4port": form_data.get("p4port"), + "error": str(e), + }, + exc_info=True, + ) + form.add_error( + None, + f"Unexpected error during connection verification: {e}", + ) + return render_to_response( + template="sentry/integrations/perforce-config.html", + context={"form": form}, + request=request, + ) + + # Bind configuration data to pipeline state + pipeline.bind_state("installation_data", form_data) + pipeline.bind_state("p4port", form_data.get("p4port")) + pipeline.bind_state("name", f"Perforce ({form_data.get('p4port')})") + # Include organization_id to create unique external_id per org + pipeline.bind_state("organization_id", pipeline.organization.id) + + pipeline.get_logger().info( + "perforce.setup.installation-config-view.success", + extra={ + "p4port": form_data.get("p4port"), + "user": form_data.get("user"), + "has_ssl_fingerprint": bool(form_data.get("ssl_fingerprint")), + "has_web_url": bool(form_data.get("web_url")), + }, + ) + return pipeline.next_step() + else: + form = PerforceInstallationForm() + + return render_to_response( + template="sentry/integrations/perforce-config.html", + context={"form": form}, + request=request, + ) diff --git a/src/sentry/tasks/post_process.py b/src/sentry/tasks/post_process.py index 4b7452819278f0..a6754f064fb7a7 100644 --- a/src/sentry/tasks/post_process.py +++ b/src/sentry/tasks/post_process.py @@ -1150,6 +1150,7 @@ def process_commits(job: PostProcessJob) -> None: IntegrationProviderSlug.GITHUB.value, IntegrationProviderSlug.GITLAB.value, IntegrationProviderSlug.GITHUB_ENTERPRISE.value, + IntegrationProviderSlug.PERFORCE.value, ], ) has_integrations = len(org_integrations) > 0 diff --git a/src/sentry/templates/sentry/integrations/perforce-config.html b/src/sentry/templates/sentry/integrations/perforce-config.html new file mode 100644 index 00000000000000..e7288bbe251c98 --- /dev/null +++ b/src/sentry/templates/sentry/integrations/perforce-config.html @@ -0,0 +1,42 @@ +{% extends "sentry/bases/modal.html" %} +{% load crispy_forms_tags %} +{% load sentry_assets %} +{% load i18n %} + +{% block css %} + +{% endblock %} + +{% block wrapperclass %} narrow auth {% endblock %} +{% block modal_header_signout %} {% endblock %} + +{% block title %} {% trans "Perforce Setup" %} | {{ block.super }} {% endblock %} + +{% block main %} +

{% trans "Configure Perforce/Helix Core Connection" %}

+

{% trans "Enter your Perforce server credentials to connect Sentry with your Perforce/Helix Core server." %}

+

{% trans "See the" %} {% trans "docs" %} {% trans "for more information." %}

+ +
+ {% csrf_token %} + + + {{ form|as_crispy_errors }} + + {% for field in form %} + {{ field|as_crispy_field }} + {% endfor %} + +
+
+ +
+
+
+{% endblock %} diff --git a/tests/sentry/integrations/perforce/__init__.py b/tests/sentry/integrations/perforce/__init__.py new file mode 100644 index 00000000000000..b13e3ff4975bb9 --- /dev/null +++ b/tests/sentry/integrations/perforce/__init__.py @@ -0,0 +1 @@ +# Perforce integration tests diff --git a/tests/sentry/integrations/perforce/test_integration.py b/tests/sentry/integrations/perforce/test_integration.py new file mode 100644 index 00000000000000..1a957e2c6a0c95 --- /dev/null +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -0,0 +1,588 @@ +import hashlib +from unittest.mock import patch + +from sentry.integrations.perforce.integration import ( + PerforceIntegration, + PerforceIntegrationProvider, +) +from sentry.models.repository import Repository +from sentry.testutils.cases import IntegrationTestCase + + +class PerforceIntegrationTest(IntegrationTestCase): + provider = PerforceIntegrationProvider + installation: PerforceIntegration + + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test", + metadata={}, + ) + self.installation = self.integration.get_installation(self.organization.id) + self.repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + def test_format_source_url_absolute_path(self): + """Test formatting URL with absolute depot path""" + url = self.installation.format_source_url( + repo=self.repo, filepath="//depot/app/services/processor.cpp", branch=None + ) + assert url == "p4://depot/app/services/processor.cpp" + + def test_format_source_url_relative_path(self): + """Test formatting URL with relative path - should prepend depot_path""" + url = self.installation.format_source_url( + repo=self.repo, filepath="app/services/processor.cpp", branch=None + ) + assert url == "p4://depot/app/services/processor.cpp" + + def test_format_source_url_path_starting_with_depot_name(self): + """Test that paths starting with depot name don't get duplicated (depot/app -> //depot/app not //depot/depot/app)""" + url = self.installation.format_source_url( + repo=self.repo, filepath="depot/app/services/processor.cpp", branch=None + ) + # Should strip "depot/" prefix and prepend "//depot/" -> "//depot/app/services/processor.cpp" + assert url == "p4://depot/app/services/processor.cpp" + + def test_format_source_url_absolute_path_starting_with_depot_name(self): + """Test that absolute paths with depot name are handled correctly (//depot/app stays as //depot/app)""" + url = self.installation.format_source_url( + repo=self.repo, filepath="//depot/app/services/processor.cpp", branch=None + ) + # Should preserve as-is (already absolute) + assert url == "p4://depot/app/services/processor.cpp" + + def test_format_source_url_with_revision_in_filename(self): + """ + Test formatting URL with revision in filename (from Symbolic transformer). + Perforce uses # for file revisions, which should be preserved. + """ + url = self.installation.format_source_url( + repo=self.repo, filepath="app/services/processor.cpp#1", branch=None + ) + assert url == "p4://depot/app/services/processor.cpp#1" + + def test_format_source_url_swarm_viewer(self): + """Test formatting URL for Swarm viewer with revision""" + integration_with_swarm = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-swarm", + metadata={ + "web_url": "https://swarm.example.com", + "p4port": "ssl:perforce.example.com:1666", + "user": "testuser", + "password": "testpass", + }, + ) + installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] + + url = installation.format_source_url( + repo=self.repo, filepath="app/services/processor.cpp#1", branch=None + ) + assert url == "https://swarm.example.com/files//depot/app/services/processor.cpp?v=1" + + def test_format_source_url_swarm_viewer_no_revision(self): + """Test formatting URL for Swarm viewer without revision""" + integration_with_swarm = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-swarm2", + metadata={ + "web_url": "https://swarm.example.com", + "p4port": "ssl:perforce.example.com:1666", + "user": "testuser", + "password": "testpass", + }, + ) + installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] + + url = installation.format_source_url( + repo=self.repo, filepath="app/services/processor.cpp", branch=None + ) + assert url == "https://swarm.example.com/files//depot/app/services/processor.cpp" + + def test_format_source_url_swarm_viewer_absolute_path(self): + """Test Swarm viewer with absolute path (//depot/...)""" + integration_with_swarm = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-swarm-abs", + metadata={ + "web_url": "https://swarm.example.com", + "p4port": "ssl:perforce.example.com:1666", + "user": "testuser", + "password": "testpass", + }, + ) + installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] + + url = installation.format_source_url( + repo=self.repo, filepath="//depot/app/services/processor.cpp", branch=None + ) + assert url == "https://swarm.example.com/files//depot/app/services/processor.cpp" + assert "depot/depot" not in url + + def test_format_source_url_swarm_viewer_depot_name_path(self): + """Test Swarm viewer with path starting with depot name (depot/...)""" + integration_with_swarm = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-swarm-depot", + metadata={ + "web_url": "https://swarm.example.com", + "p4port": "ssl:perforce.example.com:1666", + "user": "testuser", + "password": "testpass", + }, + ) + installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] + + url = installation.format_source_url( + repo=self.repo, filepath="depot/app/services/processor.cpp", branch=None + ) + assert url == "https://swarm.example.com/files//depot/app/services/processor.cpp" + assert "depot/depot" not in url + + def test_format_source_url_strips_leading_slash_from_relative_path(self): + """Test that leading slash is stripped from relative paths""" + url = self.installation.format_source_url( + repo=self.repo, filepath="/app/services/processor.cpp", branch=None + ) + assert url == "p4://depot/app/services/processor.cpp" + + def test_format_source_url_uses_repo_name_as_fallback_depot(self): + """Test that repo name is used as depot_path fallback""" + repo_without_config = Repository.objects.create( + name="//myproject", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={}, + ) + + url = self.installation.format_source_url( + repo=repo_without_config, filepath="app/services/processor.cpp", branch=None + ) + assert url == "p4://myproject/app/services/processor.cpp" + + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_check_file_absolute_depot_path(self, mock_check_file): + """Test check_file with absolute depot path (//depot/...)""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} + result = self.installation.check_file(self.repo, "//depot/app/services/processor.cpp") + assert result is not None + assert "//depot/app/services/processor.cpp" in result + + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_check_file_path_with_depot_name(self, mock_check_file): + """Test check_file with path starting with depot name (depot/...)""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} + result = self.installation.check_file(self.repo, "depot/app/services/processor.cpp") + assert result is not None + # Should not duplicate depot name in result + assert "//depot/app/services/processor.cpp" in result + assert "depot/depot" not in result + + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_check_file_relative_path(self, mock_check_file): + """Test check_file with normal relative path (app/...)""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} + result = self.installation.check_file(self.repo, "app/services/processor.cpp") + assert result is not None + assert "//depot/app/services/processor.cpp" in result + + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_check_file_with_revision_syntax_absolute(self, mock_check_file): + """Test check_file with #revision syntax on absolute path""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} + result = self.installation.check_file(self.repo, "//depot/app/services/processor.cpp#1") + assert result is not None + assert "#1" in result + + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_check_file_with_revision_syntax_depot_name(self, mock_check_file): + """Test check_file with #revision syntax on path with depot name""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} + result = self.installation.check_file(self.repo, "depot/app/services/processor.cpp#1") + assert result is not None + assert "#1" in result + assert "depot/depot" not in result + + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_check_file_with_revision_syntax_relative(self, mock_check_file): + """Test check_file with #revision syntax on relative path""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} + result = self.installation.check_file(self.repo, "app/services/processor.cpp#1") + assert result is not None + assert "#1" in result + + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") + def test_get_stacktrace_link(self, mock_check_file): + """Test get_stacktrace_link returns format_source_url result""" + mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} + filepath = "app/services/processor.cpp#1" + default_branch = "" # Perforce doesn't require default_branch (used for streams) + version = None + + url = self.installation.get_stacktrace_link(self.repo, filepath, default_branch, version) + # The # character is preserved as-is (not URL-encoded in this method) + assert url == "p4://depot/app/services/processor.cpp#1" + assert "#1" in url + + def test_integration_name(self): + """Test integration has correct name""" + assert self.installation.model.name == "Perforce" + + def test_integration_provider(self): + """Test integration has correct provider""" + assert self.installation.model.provider == "perforce" + + +class PerforceIntegrationCodeMappingTest(IntegrationTestCase): + """Tests for Perforce integration with code mappings""" + + provider = PerforceIntegrationProvider + + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test", + metadata={}, + ) + self.installation = self.integration.get_installation(self.organization.id) + self.repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + def test_format_source_url_from_code_mapping_output(self): + """ + Test that paths coming from code mapping (depot/ -> /) work correctly. + Code mapping strips 'depot/' and leaves 'app/services/processor.cpp', + which should be prepended with depot_path. + """ + # This is what code mapping would output after stripping "depot/" + code_mapped_path = "app/services/processor.cpp" + + url = self.installation.format_source_url( + repo=self.repo, filepath=code_mapped_path, branch=None + ) + + # Should prepend depot_path to create full depot path + assert url == "p4://depot/app/services/processor.cpp" + + def test_format_source_url_preserves_revision_in_filename(self): + """ + Test that #revision syntax in filename is preserved. + This tests the Symbolic transformer output format. + """ + # This is what Symbolic transformer outputs + symbolic_path = "app/services/processor.cpp#1" + + url = self.installation.format_source_url( + repo=self.repo, filepath=symbolic_path, branch=None + ) + + # The #1 should be preserved in the path + assert url == "p4://depot/app/services/processor.cpp#1" + + def test_format_source_url_python_path_without_revision(self): + """Test Python SDK paths without revision""" + # Python SDK typically doesn't include revisions + python_path = "app/services/processor.py" + + url = self.installation.format_source_url(repo=self.repo, filepath=python_path, branch=None) + + assert url == "p4://depot/app/services/processor.py" + + +class PerforceIntegrationDepotPathTest(IntegrationTestCase): + """Tests for depot path handling in different scenarios""" + + provider = PerforceIntegrationProvider + + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test", + ) + self.installation = self.integration.get_installation(self.organization.id) + + def test_multiple_depots(self): + """Test handling multiple depot configurations""" + depot_repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + myproject_repo = Repository.objects.create( + name="//myproject", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//myproject"}, + ) + + # Test depot + url1 = self.installation.format_source_url( + repo=depot_repo, filepath="app/services/processor.cpp", branch=None + ) + assert url1 == "p4://depot/app/services/processor.cpp" + + # Test myproject + url2 = self.installation.format_source_url( + repo=myproject_repo, filepath="app/services/processor.cpp", branch=None + ) + assert url2 == "p4://myproject/app/services/processor.cpp" + + def test_nested_depot_paths(self): + """Test handling nested depot paths""" + repo = Repository.objects.create( + name="//depot/game/project", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot/game/project"}, + ) + + url = self.installation.format_source_url(repo=repo, filepath="src/main.cpp", branch=None) + assert url == "p4://depot/game/project/src/main.cpp" + + def test_depot_path_with_trailing_slash(self): + """Test depot_path with trailing slash is handled correctly""" + repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot/"}, + ) + + url = self.installation.format_source_url( + repo=repo, filepath="app/services/processor.cpp", branch=None + ) + # Should not have double slashes in path portion + assert url == "p4://depot/app/services/processor.cpp" + + +class PerforceIntegrationWebViewersTest(IntegrationTestCase): + """Tests for web viewer URL formatting""" + + provider = PerforceIntegrationProvider + + def setUp(self): + super().setUp() + self.integration = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test", + metadata={}, + ) + self.installation = self.integration.get_installation(self.organization.id) + self.repo = Repository.objects.create( + name="//depot", + organization_id=self.organization.id, + integration_id=self.integration.id, + config={"depot_path": "//depot"}, + ) + + def test_swarm_extracts_revision_from_filename(self): + """Test Swarm viewer correctly extracts and formats revision from #revision syntax""" + integration_with_swarm = self.create_integration( + organization=self.organization, + provider="perforce", + name="Perforce", + external_id="perforce-test-swarm3", + metadata={ + "web_url": "https://swarm.example.com", + "p4port": "ssl:perforce.example.com:1666", + "user": "testuser", + "password": "testpass", + }, + ) + installation: PerforceIntegration = integration_with_swarm.get_installation(self.organization.id) # type: ignore[assignment] + + # Filename with revision + url = installation.format_source_url( + repo=self.repo, filepath="game/src/main.cpp#1", branch=None + ) + + # Should extract #1 and use it as v parameter + assert url == "https://swarm.example.com/files//depot/game/src/main.cpp?v=1" + + +class PerforceIntegrationEndToEndTest(IntegrationTestCase): + """ + End-to-end test covering the full integration lifecycle: + - Installation with credentials + - Configuration retrieval + - Partial updates + - Full updates + """ + + provider = PerforceIntegrationProvider + + def test_integration_lifecycle(self): + """Test complete integration lifecycle from installation to updates""" + + # Step 1: Simulate installation with build_integration + provider = PerforceIntegrationProvider() + state = { + "organization_id": self.organization.id, + "installation_data": { + "p4port": "ssl:perforce.example.com:1666", + "user": "sentry-bot", + "auth_type": "password", + "password": "initial_password", + "client": "sentry-workspace", + "ssl_fingerprint": "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD", + "web_url": "https://swarm.example.com", + }, + "name": "Perforce (ssl:perforce.example.com:1666)", + } + + integration_data = provider.build_integration(state) + + # Verify integration data structure + assert integration_data["name"] == "Perforce (ssl:perforce.example.com:1666)" + + # Verify external_id format: perforce-org-{org_id}-{hash} + # Hash is first 8 chars of SHA256(p4port) + p4port_hash = hashlib.sha256(b"ssl:perforce.example.com:1666").hexdigest()[:8] + expected_external_id = f"perforce-org-{self.organization.id}-{p4port_hash}" + assert integration_data["external_id"] == expected_external_id + + assert "metadata" in integration_data + + # Verify all credentials are in metadata + metadata = integration_data["metadata"] + assert metadata["p4port"] == "ssl:perforce.example.com:1666" + assert metadata["user"] == "sentry-bot" + assert metadata["auth_type"] == "password" + assert metadata["password"] == "initial_password" + assert metadata["client"] == "sentry-workspace" + assert ( + metadata["ssl_fingerprint"] + == "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD" + ) + assert metadata["web_url"] == "https://swarm.example.com" + + # Step 2: Create integration (simulating ensure_integration) + integration = self.create_integration( + organization=self.organization, + provider="perforce", + name=integration_data["name"], + external_id=integration_data["external_id"], + metadata=integration_data["metadata"], + ) + + # Step 3: Get installation and verify configuration retrieval + installation: PerforceIntegration = integration.get_installation(self.organization.id) # type: ignore[assignment] + + # Test get_organization_config returns form schema + org_config = installation.get_organization_config() + assert len(org_config) == 7 # 7 configuration fields (added auth_type) + field_names = {field["name"] for field in org_config} + assert field_names == { + "p4port", + "user", + "auth_type", + "password", + "ssl_fingerprint", + "client", + "web_url", + } + + # Verify field types + field_types = {field["name"]: field["type"] for field in org_config} + assert field_types["password"] == "secret" + assert field_types["p4port"] == "string" + assert field_types["user"] == "string" + + # Test get_config_data returns current values + config_data = installation.get_config_data() + assert config_data["p4port"] == "ssl:perforce.example.com:1666" + assert config_data["user"] == "sentry-bot" + assert config_data["password"] == "initial_password" + assert config_data["client"] == "sentry-workspace" + assert ( + config_data["ssl_fingerprint"] + == "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD" + ) + assert config_data["web_url"] == "https://swarm.example.com" + + # Step 4: Test partial update (only change password) + installation.update_organization_config({"password": "updated_password_123"}) + + # Refresh and verify password changed but other fields preserved + integration.refresh_from_db() + updated_config = installation.get_config_data() + assert updated_config["password"] == "updated_password_123" + assert updated_config["p4port"] == "ssl:perforce.example.com:1666" # Preserved + assert updated_config["user"] == "sentry-bot" # Preserved + assert updated_config["client"] == "sentry-workspace" # Preserved + assert ( + updated_config["ssl_fingerprint"] + == "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD" + ) # Preserved + assert updated_config["web_url"] == "https://swarm.example.com" # Preserved + + # Step 5: Test multiple field update + installation.update_organization_config( + { + "user": "new-user", + "client": "new-workspace", + "web_url": "https://new-swarm.example.com", + } + ) + + # Refresh and verify multiple fields changed + integration.refresh_from_db() + final_config = installation.get_config_data() + assert final_config["user"] == "new-user" + assert final_config["client"] == "new-workspace" + assert final_config["web_url"] == "https://new-swarm.example.com" + # Verify other fields still preserved + assert final_config["password"] == "updated_password_123" # From previous update + assert final_config["p4port"] == "ssl:perforce.example.com:1666" # Original value + assert ( + final_config["ssl_fingerprint"] + == "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD" + ) # Original value + + # Step 6: Verify empty optional fields don't break anything + installation.update_organization_config( + { + "client": "", # Clear client + "web_url": "", # Clear web_url + } + ) + + integration.refresh_from_db() + cleared_config = installation.get_config_data() + assert cleared_config["client"] == "" + assert cleared_config["web_url"] == "" + # Required fields still preserved + assert cleared_config["p4port"] == "ssl:perforce.example.com:1666" + assert cleared_config["user"] == "new-user" + assert cleared_config["password"] == "updated_password_123"