From 941b6dd340f0c75a01ed03adabc88a3fa1463ff0 Mon Sep 17 00:00:00 2001 From: Rohit Malhotra Date: Fri, 16 Jan 2026 11:12:27 -0800 Subject: [PATCH] Jira: don't lock conversation to ticket (#12472) Co-authored-by: openhands --- enterprise/integrations/jira/jira_manager.py | 4 - enterprise/integrations/jira/jira_view.py | 105 +------- .../tests/unit/integrations/jira/conftest.py | 16 -- .../integrations/jira/test_jira_manager.py | 37 --- .../unit/integrations/jira/test_jira_view.py | 233 +----------------- 5 files changed, 2 insertions(+), 393 deletions(-) diff --git a/enterprise/integrations/jira/jira_manager.py b/enterprise/integrations/jira/jira_manager.py index 1c49b417aa..05541a6995 100644 --- a/enterprise/integrations/jira/jira_manager.py +++ b/enterprise/integrations/jira/jira_manager.py @@ -7,7 +7,6 @@ import httpx from fastapi import Request from integrations.jira.jira_types import JiraViewInterface from integrations.jira.jira_view import ( - JiraExistingConversationView, JiraFactory, JiraNewConversationView, ) @@ -309,9 +308,6 @@ class JiraManager(Manager): Check if a job is requested and handle repository selection. """ - if isinstance(jira_view, JiraExistingConversationView): - return True - try: # Get user repositories user_repos: list[Repository] = await self._get_repositories( diff --git a/enterprise/integrations/jira/jira_view.py b/enterprise/integrations/jira/jira_view.py index c410175606..22229eed00 100644 --- a/enterprise/integrations/jira/jira_view.py +++ b/enterprise/integrations/jira/jira_view.py @@ -2,7 +2,7 @@ from dataclasses import dataclass from integrations.jira.jira_types import JiraViewInterface, StartingConvoException from integrations.models import JobContext -from integrations.utils import CONVERSATION_URL, get_final_agent_observation +from integrations.utils import CONVERSATION_URL from jinja2 import Environment from storage.jira_conversation import JiraConversation from storage.jira_integration_store import JiraIntegrationStore @@ -10,14 +10,9 @@ from storage.jira_user import JiraUser from storage.jira_workspace import JiraWorkspace from openhands.core.logger import openhands_logger as logger -from openhands.core.schema.agent import AgentState -from openhands.events.action import MessageAction -from openhands.events.serialization.event import event_to_dict from openhands.server.services.conversation_service import ( create_new_conversation, - setup_init_conversation_settings, ) -from openhands.server.shared import ConversationStoreImpl, config, conversation_manager from openhands.server.user_auth.user_auth import UserAuth from openhands.storage.data_models.conversation_metadata import ConversationTrigger @@ -101,87 +96,6 @@ class JiraNewConversationView(JiraViewInterface): return f"I'm on it! {self.job_context.display_name} can [track my progress here|{conversation_link}]." -@dataclass -class JiraExistingConversationView(JiraViewInterface): - job_context: JobContext - saas_user_auth: UserAuth - jira_user: JiraUser - jira_workspace: JiraWorkspace - selected_repo: str | None - conversation_id: str - - def _get_instructions(self, jinja_env: Environment) -> tuple[str, str]: - """Instructions passed when conversation is first initialized""" - - user_msg_template = jinja_env.get_template('jira_existing_conversation.j2') - user_msg = user_msg_template.render( - issue_key=self.job_context.issue_key, - user_message=self.job_context.user_msg or '', - issue_title=self.job_context.issue_title, - issue_description=self.job_context.issue_description, - ) - - return '', user_msg - - async def create_or_update_conversation(self, jinja_env: Environment) -> str: - """Update an existing Jira conversation""" - - user_id = self.jira_user.keycloak_user_id - - try: - conversation_store = await ConversationStoreImpl.get_instance( - config, user_id - ) - - try: - await conversation_store.get_metadata(self.conversation_id) - except FileNotFoundError: - raise StartingConvoException('Conversation no longer exists.') - - provider_tokens = await self.saas_user_auth.get_provider_tokens() - # Should we raise here if there are no providers? - providers_set = list(provider_tokens.keys()) if provider_tokens else [] - - conversation_init_data = await setup_init_conversation_settings( - user_id, self.conversation_id, providers_set - ) - - # Either join ongoing conversation, or restart the conversation - agent_loop_info = await conversation_manager.maybe_start_agent_loop( - self.conversation_id, conversation_init_data, user_id - ) - - final_agent_observation = get_final_agent_observation( - agent_loop_info.event_store - ) - agent_state = ( - None - if len(final_agent_observation) == 0 - else final_agent_observation[0].agent_state - ) - - if not agent_state or agent_state == AgentState.LOADING: - raise StartingConvoException('Conversation is still starting') - - _, user_msg = self._get_instructions(jinja_env) - user_message_event = MessageAction(content=user_msg) - await conversation_manager.send_event_to_conversation( - self.conversation_id, event_to_dict(user_message_event) - ) - - return self.conversation_id - except Exception as e: - logger.error( - f'[Jira] Failed to create conversation: {str(e)}', exc_info=True - ) - raise StartingConvoException(f'Failed to create conversation: {str(e)}') - - def get_response_msg(self) -> str: - """Get the response message to send back to Jira""" - conversation_link = CONVERSATION_URL.format(self.conversation_id) - return f"I'm on it! {self.job_context.display_name} can [continue tracking my progress here|{conversation_link}]." - - class JiraFactory: """Factory for creating Jira views based on message content""" @@ -197,23 +111,6 @@ class JiraFactory: if not jira_user or not saas_user_auth or not jira_workspace: raise StartingConvoException('User not authenticated with Jira integration') - conversation = await integration_store.get_user_conversations_by_issue_id( - job_context.issue_id, jira_user.id - ) - - if conversation: - logger.info( - f'[Jira] Found existing conversation for issue {job_context.issue_id}' - ) - return JiraExistingConversationView( - job_context=job_context, - saas_user_auth=saas_user_auth, - jira_user=jira_user, - jira_workspace=jira_workspace, - selected_repo=None, - conversation_id=conversation.conversation_id, - ) - return JiraNewConversationView( job_context=job_context, saas_user_auth=saas_user_auth, diff --git a/enterprise/tests/unit/integrations/jira/conftest.py b/enterprise/tests/unit/integrations/jira/conftest.py index 6838198c12..905e9009f1 100644 --- a/enterprise/tests/unit/integrations/jira/conftest.py +++ b/enterprise/tests/unit/integrations/jira/conftest.py @@ -7,7 +7,6 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest from integrations.jira.jira_manager import JiraManager from integrations.jira.jira_view import ( - JiraExistingConversationView, JiraNewConversationView, ) from integrations.models import JobContext @@ -194,21 +193,6 @@ def new_conversation_view( ) -@pytest.fixture -def existing_conversation_view( - sample_job_context, sample_user_auth, sample_jira_user, sample_jira_workspace -): - """JiraExistingConversationView instance for testing""" - return JiraExistingConversationView( - job_context=sample_job_context, - saas_user_auth=sample_user_auth, - jira_user=sample_jira_user, - jira_workspace=sample_jira_workspace, - selected_repo='test/repo1', - conversation_id='conv-123', - ) - - @pytest.fixture def mock_agent_loop_info(): """Mock agent loop info""" diff --git a/enterprise/tests/unit/integrations/jira/test_jira_manager.py b/enterprise/tests/unit/integrations/jira/test_jira_manager.py index 9240af3728..7177a1e5c2 100644 --- a/enterprise/tests/unit/integrations/jira/test_jira_manager.py +++ b/enterprise/tests/unit/integrations/jira/test_jira_manager.py @@ -12,7 +12,6 @@ from fastapi import Request from integrations.jira.jira_manager import JiraManager from integrations.jira.jira_types import JiraViewInterface from integrations.jira.jira_view import ( - JiraExistingConversationView, JiraNewConversationView, ) from integrations.models import Message, SourceType @@ -540,15 +539,6 @@ class TestReceiveMessage: class TestIsJobRequested: """Test job request validation.""" - @pytest.mark.asyncio - async def test_is_job_requested_existing_conversation(self, jira_manager): - """Test job request validation for existing conversation.""" - mock_view = MagicMock(spec=JiraExistingConversationView) - message = Message(source=SourceType.JIRA, message={}) - - result = await jira_manager.is_job_requested(message, mock_view) - assert result is True - @pytest.mark.asyncio async def test_is_job_requested_new_conversation_with_repo_match( self, jira_manager, sample_job_context, sample_user_auth @@ -659,33 +649,6 @@ class TestStartJob: mock_register.assert_called_once() jira_manager.send_message.assert_called_once() - @pytest.mark.asyncio - async def test_start_job_success_existing_conversation( - self, jira_manager, sample_jira_workspace - ): - """Test successful job start for existing conversation.""" - mock_view = MagicMock(spec=JiraExistingConversationView) - mock_view.jira_user = MagicMock() - mock_view.jira_user.keycloak_user_id = 'test_user' - mock_view.job_context = MagicMock() - mock_view.job_context.issue_key = 'PROJ-123' - mock_view.jira_workspace = sample_jira_workspace - mock_view.create_or_update_conversation = AsyncMock(return_value='conv_123') - mock_view.get_response_msg = MagicMock(return_value='Job started successfully') - - jira_manager.send_message = AsyncMock() - jira_manager.token_manager.decrypt_text.return_value = 'decrypted_key' - - with patch( - 'integrations.jira.jira_manager.register_callback_processor' - ) as mock_register: - await jira_manager.start_job(mock_view) - - mock_view.create_or_update_conversation.assert_called_once() - # Should not register callback for existing conversation - mock_register.assert_not_called() - jira_manager.send_message.assert_called_once() - @pytest.mark.asyncio async def test_start_job_missing_settings_error( self, jira_manager, sample_jira_workspace diff --git a/enterprise/tests/unit/integrations/jira/test_jira_view.py b/enterprise/tests/unit/integrations/jira/test_jira_view.py index ee14bf803f..4348030d22 100644 --- a/enterprise/tests/unit/integrations/jira/test_jira_view.py +++ b/enterprise/tests/unit/integrations/jira/test_jira_view.py @@ -2,18 +2,15 @@ Tests for Jira view classes and factory. """ -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, patch import pytest from integrations.jira.jira_types import StartingConvoException from integrations.jira.jira_view import ( - JiraExistingConversationView, JiraFactory, JiraNewConversationView, ) -from openhands.core.schema.agent import AgentState - class TestJiraNewConversationView: """Tests for JiraNewConversationView""" @@ -80,162 +77,9 @@ class TestJiraNewConversationView: assert 'conv-123' in response -class TestJiraExistingConversationView: - """Tests for JiraExistingConversationView""" - - def test_get_instructions(self, existing_conversation_view, mock_jinja_env): - """Test _get_instructions method""" - instructions, user_msg = existing_conversation_view._get_instructions( - mock_jinja_env - ) - - assert instructions == '' - assert 'TEST-123' in user_msg - assert 'Test Issue' in user_msg - assert 'Fix this bug @openhands' in user_msg - - @patch('integrations.jira.jira_view.ConversationStoreImpl.get_instance') - @patch('integrations.jira.jira_view.setup_init_conversation_settings') - @patch('integrations.jira.jira_view.conversation_manager') - @patch('integrations.jira.jira_view.get_final_agent_observation') - async def test_create_or_update_conversation_success( - self, - mock_get_observation, - mock_conversation_manager, - mock_setup_init, - mock_store_impl, - existing_conversation_view, - mock_jinja_env, - mock_conversation_store, - mock_conversation_init_data, - mock_agent_loop_info, - ): - """Test successful existing conversation update""" - # Setup mocks - mock_store_impl.return_value = mock_conversation_store - mock_setup_init.return_value = mock_conversation_init_data - mock_conversation_manager.maybe_start_agent_loop = AsyncMock( - return_value=mock_agent_loop_info - ) - mock_conversation_manager.send_event_to_conversation = AsyncMock() - - # Mock agent observation with RUNNING state - mock_observation = MagicMock() - mock_observation.agent_state = AgentState.RUNNING - mock_get_observation.return_value = [mock_observation] - - result = await existing_conversation_view.create_or_update_conversation( - mock_jinja_env - ) - - assert result == 'conv-123' - mock_conversation_manager.send_event_to_conversation.assert_called_once() - - @patch('integrations.jira.jira_view.ConversationStoreImpl.get_instance') - async def test_create_or_update_conversation_no_metadata( - self, mock_store_impl, existing_conversation_view, mock_jinja_env - ): - """Test conversation update with no metadata""" - mock_store = AsyncMock() - mock_store.get_metadata.side_effect = FileNotFoundError( - 'No such file or directory' - ) - mock_store_impl.return_value = mock_store - - with pytest.raises( - StartingConvoException, match='Conversation no longer exists' - ): - await existing_conversation_view.create_or_update_conversation( - mock_jinja_env - ) - - @patch('integrations.jira.jira_view.ConversationStoreImpl.get_instance') - @patch('integrations.jira.jira_view.setup_init_conversation_settings') - @patch('integrations.jira.jira_view.conversation_manager') - @patch('integrations.jira.jira_view.get_final_agent_observation') - async def test_create_or_update_conversation_loading_state( - self, - mock_get_observation, - mock_conversation_manager, - mock_setup_init, - mock_store_impl, - existing_conversation_view, - mock_jinja_env, - mock_conversation_store, - mock_conversation_init_data, - mock_agent_loop_info, - ): - """Test conversation update with loading state""" - mock_store_impl.return_value = mock_conversation_store - mock_setup_init.return_value = mock_conversation_init_data - mock_conversation_manager.maybe_start_agent_loop = AsyncMock( - return_value=mock_agent_loop_info - ) - - # Mock agent observation with LOADING state - mock_observation = MagicMock() - mock_observation.agent_state = AgentState.LOADING - mock_get_observation.return_value = [mock_observation] - - with pytest.raises( - StartingConvoException, match='Conversation is still starting' - ): - await existing_conversation_view.create_or_update_conversation( - mock_jinja_env - ) - - @patch('integrations.jira.jira_view.ConversationStoreImpl.get_instance') - async def test_create_or_update_conversation_failure( - self, mock_store_impl, existing_conversation_view, mock_jinja_env - ): - """Test conversation update failure""" - mock_store_impl.side_effect = Exception('Store error') - - with pytest.raises( - StartingConvoException, match='Failed to create conversation' - ): - await existing_conversation_view.create_or_update_conversation( - mock_jinja_env - ) - - def test_get_response_msg(self, existing_conversation_view): - """Test get_response_msg method""" - response = existing_conversation_view.get_response_msg() - - assert "I'm on it!" in response - assert 'Test User' in response - assert 'continue tracking my progress here' in response - assert 'conv-123' in response - - class TestJiraFactory: """Tests for JiraFactory""" - @patch('integrations.jira.jira_view.integration_store') - async def test_create_jira_view_from_payload_existing_conversation( - self, - mock_store, - sample_job_context, - sample_user_auth, - sample_jira_user, - sample_jira_workspace, - jira_conversation, - ): - """Test factory creating existing conversation view""" - mock_store.get_user_conversations_by_issue_id = AsyncMock( - return_value=jira_conversation - ) - - view = await JiraFactory.create_jira_view_from_payload( - sample_job_context, - sample_user_auth, - sample_jira_user, - sample_jira_workspace, - ) - - assert isinstance(view, JiraExistingConversationView) - assert view.conversation_id == 'conv-123' - @patch('integrations.jira.jira_view.integration_store') async def test_create_jira_view_from_payload_new_conversation( self, @@ -341,83 +185,8 @@ class TestJiraViewEdgeCases: ): await new_conversation_view.create_or_update_conversation(mock_jinja_env) - @patch('integrations.jira.jira_view.ConversationStoreImpl.get_instance') - @patch('integrations.jira.jira_view.setup_init_conversation_settings') - @patch('integrations.jira.jira_view.conversation_manager') - @patch('integrations.jira.jira_view.get_final_agent_observation') - async def test_existing_conversation_empty_observations( - self, - mock_get_observation, - mock_conversation_manager, - mock_setup_init, - mock_store_impl, - existing_conversation_view, - mock_jinja_env, - mock_conversation_store, - mock_conversation_init_data, - mock_agent_loop_info, - ): - """Test existing conversation with empty observations""" - mock_store_impl.return_value = mock_conversation_store - mock_setup_init.return_value = mock_conversation_init_data - mock_conversation_manager.maybe_start_agent_loop = AsyncMock( - return_value=mock_agent_loop_info - ) - mock_get_observation.return_value = [] # Empty observations - - with pytest.raises( - StartingConvoException, match='Conversation is still starting' - ): - await existing_conversation_view.create_or_update_conversation( - mock_jinja_env - ) - def test_new_conversation_view_attributes(self, new_conversation_view): """Test new conversation view attribute access""" assert new_conversation_view.job_context.issue_key == 'TEST-123' assert new_conversation_view.selected_repo == 'test/repo1' assert new_conversation_view.conversation_id == 'conv-123' - - def test_existing_conversation_view_attributes(self, existing_conversation_view): - """Test existing conversation view attribute access""" - assert existing_conversation_view.job_context.issue_key == 'TEST-123' - assert existing_conversation_view.selected_repo == 'test/repo1' - assert existing_conversation_view.conversation_id == 'conv-123' - - @patch('integrations.jira.jira_view.ConversationStoreImpl.get_instance') - @patch('integrations.jira.jira_view.setup_init_conversation_settings') - @patch('integrations.jira.jira_view.conversation_manager') - @patch('integrations.jira.jira_view.get_final_agent_observation') - async def test_existing_conversation_message_send_failure( - self, - mock_get_observation, - mock_conversation_manager, - mock_setup_init, - mock_store_impl, - existing_conversation_view, - mock_jinja_env, - mock_conversation_store, - mock_conversation_init_data, - mock_agent_loop_info, - ): - """Test existing conversation when message sending fails""" - mock_store_impl.return_value = mock_conversation_store - mock_setup_init.return_value = mock_conversation_init_data - mock_conversation_manager.maybe_start_agent_loop = AsyncMock( - return_value=mock_agent_loop_info - ) - mock_conversation_manager.send_event_to_conversation = AsyncMock( - side_effect=Exception('Send error') - ) - - # Mock agent observation with RUNNING state - mock_observation = MagicMock() - mock_observation.agent_state = AgentState.RUNNING - mock_get_observation.return_value = [mock_observation] - - with pytest.raises( - StartingConvoException, match='Failed to create conversation' - ): - await existing_conversation_view.create_or_update_conversation( - mock_jinja_env - )