mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-03-22 13:47:19 +08:00
Jira: don't lock conversation to ticket (#12472)
Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
@@ -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(
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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"""
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user