diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d40c041e..ba74f1ce4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,6 +87,7 @@ and this project adheres to `503` instead of denying access like a permission failure, so clients retry instead of giving up. The built-in endpoints can also answer JSON on `Accept: application/json` +- ✨(backend) call YHubService to seed initial document content - ✨(backend) reset the yhub connections of a document and its descendants when an access or the link configuration changes - ✨(backend) add a service to call the yhub REST API diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index d6b2b422c..5bf57ebd8 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -7,6 +7,7 @@ from base64 import b64decode from os.path import splitext from django.conf import settings +from django.db import transaction from django.db.models import Q from django.utils.functional import lazy from django.utils.text import slugify @@ -23,6 +24,7 @@ from core.services.converter_services import ( ConversionError, Converter, ) +from core.services.yhub_services import YHubError, YHubService from core.utils.analytics import PosthogEventName, posthog_capture from core.utils.treebeard import create_tree_node_with_retry @@ -482,12 +484,37 @@ class ServerCreateDocumentSerializer(serializers.Serializer): {"content": ["Could not convert content"]} ) from err - document = create_tree_node_with_retry( - lambda: models.Document.add_root( - title=validated_data["title"], - creator=user, + with transaction.atomic(): + document = create_tree_node_with_retry( + lambda: models.Document.add_root( + title=validated_data["title"], + creator=user, + ) ) - ) + + if user: + # Associate the document with the pre-existing user + models.DocumentAccess.objects.create( + document=document, + role=models.RoleChoices.OWNER, + user=user, + ) + else: + # The user doesn't exist in our database: we need to invite him/her + models.Invitation.objects.create( + document=document, + email=email, + role=models.RoleChoices.OWNER, + ) + + # the accesses exist by now, so the owner has access to the very + # first version of the document the collaboration server saves + try: + YHubService(user=user).create_ydoc(document, document_content) + except YHubError as err: + raise serializers.ValidationError( + {"content": ["Could not save the document content"]} + ) from err posthog_capture(PosthogEventName.DOC_CREATED, user, {}, document=document) posthog_capture( @@ -500,24 +527,6 @@ class ServerCreateDocumentSerializer(serializers.Serializer): document=document, ) - if user: - # Associate the document with the pre-existing user - models.DocumentAccess.objects.create( - document=document, - role=models.RoleChoices.OWNER, - user=user, - ) - else: - # The user doesn't exist in our database: we need to invite him/her - models.Invitation.objects.create( - document=document, - email=email, - role=models.RoleChoices.OWNER, - ) - - document.content = document_content - document.save() - if validated_data.get("send_notification_email", True): self._send_email_notification(document, validated_data, email, language) return document diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 1918ab604..a7f3b3204 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -71,6 +71,7 @@ from core.services.search_indexers import ( get_document_indexer, get_visited_document_ids_of, ) +from core.services.yhub_services import YHubError, YHubService from core.tasks.access import reset_service_connections_in_cascade from core.tasks.mail import send_ask_for_access_mail from core.utils.analytics import PosthogEventName, posthog_capture @@ -694,8 +695,13 @@ class DocumentViewSet( """ Check if a file has been uploaded with a doc or a children is created. If a file is present and the conversion upload enabled, the file is converted - using the converter service and the validated_data in the serializer are filled - with the converted file and the file name. + using the converter service and the title in the serializer is filled with + the file name. + + Return the converted content, as a raw Yjs update the collaboration + server can be seeded with, or None when no file was uploaded. The + content itself is not stored by Django, it is saved by the + collaboration server. """ uploaded_file = serializer.validated_data.pop("file", None) @@ -704,49 +710,72 @@ class DocumentViewSet( {"file": ["file upload is not allowed"]} ) - # If a file is uploaded, convert it to Yjs format and set as content - if uploaded_file: - try: - file_content = uploaded_file.read() + if not uploaded_file: + return None - converter = Converter() - converted_content = converter.convert( - file_content, - content_type=uploaded_file.content_type, - accept=mime_types.YJS, - ) - serializer.validated_data["content"] = converted_content - serializer.validated_data["title"] = uploaded_file.name - logger.info("conversion ended successfully") + # If a file is uploaded, convert it to Yjs format + try: + file_content = uploaded_file.read() - posthog_capture( - PosthogEventName.DOC_IMPORTED, - self.request.user, - {"content_type": uploaded_file.content_type}, - ) - except ConversionError as err: - logger.error("could not convert file content with error: %s", err) - raise drf.exceptions.ValidationError( - {"file": ["Could not convert file content"]} - ) from err + converter = Converter() + converted_content = converter.convert( + file_content, + content_type=uploaded_file.content_type, + accept=mime_types.YJS, + ) + serializer.validated_data["title"] = uploaded_file.name + logger.info("conversion ended successfully") + + posthog_capture( + PosthogEventName.DOC_IMPORTED, + self.request.user, + {"content_type": uploaded_file.content_type}, + ) + except ConversionError as err: + logger.error("could not convert file content with error: %s", err) + raise drf.exceptions.ValidationError( + {"file": ["Could not convert file content"]} + ) from err + + return converted_content + + def _create_collaboration_document(self, document, update): + """ + Seed a freshly created document with the content it was imported from. + + The collaboration server owns the content from there on, so a failure + here leaves a document that lost what was uploaded: it is reported to + the caller, who is left to create it again. + """ + try: + YHubService(user=self.request.user).create_ydoc(document, update) + except YHubError as err: + logger.error("could not save the imported content with error: %s", err) + raise drf.exceptions.ValidationError( + {"file": ["Could not save the imported file content"]} + ) from err def perform_create(self, serializer): """Set the current user as creator and owner of the newly created object.""" - self._apply_uploaded_file_conversion(serializer) + update = self._apply_uploaded_file_conversion(serializer) - obj = create_tree_node_with_retry( - lambda: models.Document.add_root( - creator=self.request.user, - **serializer.validated_data, + with transaction.atomic(): + obj = create_tree_node_with_retry( + lambda: models.Document.add_root( + creator=self.request.user, + **serializer.validated_data, + ) ) - ) - serializer.instance = obj - models.DocumentAccess.objects.create( - document=obj, - user=self.request.user, - role=models.RoleChoices.OWNER, - ) + serializer.instance = obj + models.DocumentAccess.objects.create( + document=obj, + user=self.request.user, + role=models.RoleChoices.OWNER, + ) + + if update is not None: + self._create_collaboration_document(obj, update) posthog_capture( PosthogEventName.DOC_CREATED, self.request.user, {}, document=obj @@ -1019,14 +1048,18 @@ class DocumentViewSet( ) serializer.is_valid(raise_exception=True) - self._apply_uploaded_file_conversion(serializer) + update = self._apply_uploaded_file_conversion(serializer) - child_document = create_tree_node_with_retry( - lambda: document.add_child( - creator=request.user, - **serializer.validated_data, + with transaction.atomic(): + child_document = create_tree_node_with_retry( + lambda: document.add_child( + creator=request.user, + **serializer.validated_data, + ) ) - ) + + if update is not None: + self._create_collaboration_document(child_document, update) # Set the created instance to the serializer serializer.instance = child_document diff --git a/src/backend/core/services/converter_services.py b/src/backend/core/services/converter_services.py index a5edd4195..7a550d18c 100644 --- a/src/backend/core/services/converter_services.py +++ b/src/backend/core/services/converter_services.py @@ -2,7 +2,6 @@ import logging import typing -from base64 import b64encode from django.conf import settings @@ -137,7 +136,13 @@ class YdocConverter: return response def convert(self, data, content_type=mime_types.MARKDOWN, accept=mime_types.YJS): - """Convert a Markdown text into our internal format using an external microservice.""" + """ + Convert a Markdown text into our internal format using an external microservice. + + A Yjs document is returned as the raw update the collaboration server + expects. It is base64 encoded only by the callers storing it in the + text content of a document. + """ if not data: raise ValidationError("Input data cannot be empty") @@ -146,7 +151,7 @@ class YdocConverter: try: response = self._request(url, data, content_type, accept) if accept == mime_types.YJS: - return b64encode(response.content).decode("utf-8") + return response.content if accept in {mime_types.MARKDOWN, "text/html"}: return response.text if accept == mime_types.JSON: diff --git a/src/backend/core/tests/documents/test_api_documents_children_create.py b/src/backend/core/tests/documents/test_api_documents_children_create.py index d5e25f7bb..679edca8f 100644 --- a/src/backend/core/tests/documents/test_api_documents_children_create.py +++ b/src/backend/core/tests/documents/test_api_documents_children_create.py @@ -314,8 +314,11 @@ def test_api_documents_create_document_children_race_condition(): assert document.numchild == 2 +@patch("core.api.viewsets.YHubService") @patch("core.services.converter_services.Converter.convert") -def test_api_documents_children_create_with_docx_file_success(mock_convert, settings): +def test_api_documents_children_create_with_docx_file_success( + mock_convert, mock_yhub, settings +): """ Authenticated users should be able to create children document by uploading a DOCX file. The file should be converted to YJS format and the title should be set from filename. @@ -327,7 +330,7 @@ def test_api_documents_children_create_with_docx_file_success(mock_convert, sett settings.CONVERSION_UPLOAD_ENABLED = True # Mock the conversion - converted_yjs = "base64encodedyjscontent" + converted_yjs = b"\x01\x02raw yjs update" mock_convert.return_value = converted_yjs # Create a fake DOCX file @@ -350,7 +353,10 @@ def test_api_documents_children_create_with_docx_file_success(mock_convert, sett assert Document.objects.count() == 2 children = Document.objects.get(pk=response.json()["id"]) assert children.title == "My Important Document.docx" - assert children.content == converted_yjs + # the content is saved by the collaboration server, not by Django + assert children.content is None + mock_yhub.assert_called_once_with(user=user) + mock_yhub.return_value.create_ydoc.assert_called_once_with(children, converted_yjs) # Verify the converter was called correctly mock_convert.assert_called_once_with( diff --git a/src/backend/core/tests/documents/test_api_documents_create_for_owner.py b/src/backend/core/tests/documents/test_api_documents_create_for_owner.py index b72bc8458..c27d57b16 100644 --- a/src/backend/core/tests/documents/test_api_documents_create_for_owner.py +++ b/src/backend/core/tests/documents/test_api_documents_create_for_owner.py @@ -20,18 +20,32 @@ from core.api.serializers import ServerCreateDocumentSerializer from core.models import Document, Invitation, User from core.services import mime_types from core.services.converter_services import ConversionError, YdocConverter +from core.services.yhub_services import ( + ServiceUnavailableError as YHubServiceUnavailableError, +) from core.utils.analytics import PosthogEventName pytestmark = pytest.mark.django_db +# the converter returns a raw Yjs update, saved by the collaboration server +CONVERTED_CONTENT = b"Converted document content" + + +@pytest.fixture(autouse=True, name="mock_yhub") +def mock_yhub_fixture(): + """No test of this module should reach the collaboration server.""" + with patch("core.api.serializers.YHubService") as mock: + yield mock + + @pytest.fixture def mock_convert_md(): """Mock YdocConverter.convert to return a converted content.""" with patch.object( YdocConverter, "convert", - return_value="Converted document content", + return_value=CONVERTED_CONTENT, ) as mock: yield mock @@ -172,7 +186,7 @@ def test_api_documents_create_for_owner_invalid_sub(): @override_settings(SERVER_TO_SERVER_API_TOKENS=["DummyToken"]) -def test_api_documents_create_for_owner_existing(mock_convert_md): +def test_api_documents_create_for_owner_existing(mock_convert_md, mock_yhub): """ It should be possible to create a document on behalf of a pre-existing user by passing their sub and email. @@ -204,7 +218,11 @@ def test_api_documents_create_for_owner_existing(mock_convert_md): assert response.json() == {"id": str(document.id)} assert document.title == "My Document" - assert document.content == "Converted document content" + # the content is saved by the collaboration server, not by Django + assert document.content is None + mock_yhub.return_value.create_ydoc.assert_called_once_with( + document, CONVERTED_CONTENT + ) assert document.creator == user assert document.accesses.filter(user=user, role="owner").exists() @@ -240,7 +258,7 @@ def test_api_documents_create_for_owner_existing(mock_convert_md): @override_settings(SERVER_TO_SERVER_API_TOKENS=["DummyToken"]) -def test_api_documents_create_for_owner_new_user(mock_convert_md): +def test_api_documents_create_for_owner_new_user(mock_convert_md, mock_yhub): """ It should be possible to create a document on behalf of new users by passing their unknown sub and email address. @@ -270,7 +288,11 @@ def test_api_documents_create_for_owner_new_user(mock_convert_md): assert response.json() == {"id": str(document.id)} assert document.title == "My Document" - assert document.content == "Converted document content" + # the content is saved by the collaboration server, not by Django + assert document.content is None + mock_yhub.return_value.create_ydoc.assert_called_once_with( + document, CONVERTED_CONTENT + ) assert document.creator is None assert document.accesses.exists() is False @@ -344,7 +366,7 @@ def test_api_documents_create_for_owner_without_notification_email(mock_convert_ OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION=True, ) def test_api_documents_create_for_owner_existing_user_email_no_sub_with_fallback( - mock_convert_md, + mock_convert_md, mock_yhub ): """ It should be possible to create a document on behalf of a pre-existing user for @@ -378,7 +400,11 @@ def test_api_documents_create_for_owner_existing_user_email_no_sub_with_fallback assert response.json() == {"id": str(document.id)} assert document.title == "My Document" - assert document.content == "Converted document content" + # the content is saved by the collaboration server, not by Django + assert document.content is None + mock_yhub.return_value.create_ydoc.assert_called_once_with( + document, CONVERTED_CONTENT + ) assert document.creator == user assert document.accesses.filter(user=user, role="owner").exists() @@ -444,7 +470,7 @@ def test_api_documents_create_for_owner_existing_user_email_no_sub_no_fallback( OIDC_ALLOW_DUPLICATE_EMAILS=True, ) def test_api_documents_create_for_owner_new_user_no_sub_no_fallback_allow_duplicate( - mock_convert_md, + mock_convert_md, mock_yhub ): """ When a user does not match an existing sub and fallback to matching on email is @@ -476,7 +502,11 @@ def test_api_documents_create_for_owner_new_user_no_sub_no_fallback_allow_duplic assert response.json() == {"id": str(document.id)} assert document.title == "My Document" - assert document.content == "Converted document content" + # the content is saved by the collaboration server, not by Django + assert document.content is None + mock_yhub.return_value.create_ydoc.assert_called_once_with( + document, CONVERTED_CONTENT + ) assert document.creator is None assert document.accesses.exists() is False @@ -669,21 +699,20 @@ def test_api_documents_create_for_owner_with_converter_exception( @override_settings(SERVER_TO_SERVER_API_TOKENS=["DummyToken"]) @pytest.mark.usefixtures("mock_convert_md") -def test_api_documents_create_for_owner_access_before_content(): +def test_api_documents_create_for_owner_access_before_content(mock_yhub): """ - Accesses must exist before content is saved to object storage so the owner - has access to the very first version of the document. + Accesses must exist before the content is sent to the collaboration server + so the owner has access to the very first version of the document. """ user = factories.UserFactory() accesses_at_save_time = [] - original_save_content = Document.save_content - - def capturing_save_content(self, content): + def capturing_create_ydoc(document, _update): accesses_at_save_time.extend( - list(self.accesses.values_list("user__sub", "role")) + list(document.accesses.values_list("user__sub", "role")) ) - return original_save_content(self, content) + + mock_yhub.return_value.create_ydoc.side_effect = capturing_create_ydoc data = { "title": "My Document", @@ -692,16 +721,15 @@ def test_api_documents_create_for_owner_access_before_content(): "email": user.email, } - with patch.object(Document, "save_content", capturing_save_content): - response = APIClient().post( - "/api/v1.0/documents/create-for-owner/", - data, - format="json", - HTTP_AUTHORIZATION="Bearer DummyToken", - ) + response = APIClient().post( + "/api/v1.0/documents/create-for-owner/", + data, + format="json", + HTTP_AUTHORIZATION="Bearer DummyToken", + ) assert response.status_code == 201 - # The owner access must already exist when save_content is called + # The owner access must already exist when the content is saved assert (str(user.sub), "owner") in accesses_at_save_time @@ -729,3 +757,33 @@ def test_api_documents_create_for_owner_with_empty_content(): "This field may not be blank.", ], } + + +@override_settings(SERVER_TO_SERVER_API_TOKENS=["DummyToken"]) +@pytest.mark.usefixtures("mock_convert_md") +def test_api_documents_create_for_owner_collaboration_server_unavailable(mock_yhub): + """ + A document whose content could not be saved by the collaboration server + should not be created at all, neither should its access. + """ + user = factories.UserFactory() + mock_yhub.return_value.create_ydoc.side_effect = YHubServiceUnavailableError( + "Failed to connect to the yhub service" + ) + + response = APIClient().post( + "/api/v1.0/documents/create-for-owner/", + { + "title": "My Document", + "content": "Document content", + "sub": str(user.sub), + "email": user.email, + }, + format="json", + HTTP_AUTHORIZATION="Bearer DummyToken", + ) + + assert response.status_code == 400 + assert response.json() == {"content": ["Could not save the document content"]} + assert Document.objects.exists() is False + assert len(mail.outbox) == 0 diff --git a/src/backend/core/tests/documents/test_api_documents_create_with_file.py b/src/backend/core/tests/documents/test_api_documents_create_with_file.py index ecafeb028..89066c965 100644 --- a/src/backend/core/tests/documents/test_api_documents_create_with_file.py +++ b/src/backend/core/tests/documents/test_api_documents_create_with_file.py @@ -2,7 +2,6 @@ Tests for Documents API endpoint in impress's core app: create with file upload """ -from base64 import b64decode, binascii from io import BytesIO from unittest.mock import patch @@ -16,6 +15,9 @@ from core.services.converter_services import ( ConversionError, ServiceUnavailableError, ) +from core.services.yhub_services import ( + ServiceUnavailableError as YHubServiceUnavailableError, +) from core.utils.analytics import PosthogEventName pytestmark = pytest.mark.django_db @@ -40,8 +42,9 @@ def test_api_documents_create_with_file_anonymous(): assert not Document.objects.exists() +@patch("core.api.viewsets.YHubService") @patch("core.services.converter_services.Converter.convert") -def test_api_documents_create_with_docx_file_success(mock_convert, settings): +def test_api_documents_create_with_docx_file_success(mock_convert, mock_yhub, settings): """ Authenticated users should be able to create documents by uploading a DOCX file. The file should be converted to YJS format and the title should be set from filename. @@ -53,7 +56,7 @@ def test_api_documents_create_with_docx_file_success(mock_convert, settings): settings.CONVERSION_UPLOAD_ENABLED = True # Mock the conversion - converted_yjs = "base64encodedyjscontent" + converted_yjs = b"\x01\x02raw yjs update" mock_convert.return_value = converted_yjs # Create a fake DOCX file @@ -73,9 +76,13 @@ def test_api_documents_create_with_docx_file_success(mock_convert, settings): assert response.status_code == 201 document = Document.objects.get() assert document.title == "My Important Document.docx" - assert document.content == converted_yjs + # the content is saved by the collaboration server, not by Django + assert document.content is None assert document.accesses.filter(role="owner", user=user).exists() + mock_yhub.assert_called_once_with(user=user) + mock_yhub.return_value.create_ydoc.assert_called_once_with(document, converted_yjs) + # Verify the converter was called correctly mock_convert.assert_called_once_with( file_content, @@ -134,8 +141,11 @@ def test_api_documents_create_with_docx_file_disabled(mock_convert, settings): mock_capture.assert_not_called() +@patch("core.api.viewsets.YHubService") @patch("core.services.converter_services.Converter.convert") -def test_api_documents_create_with_markdown_file_success(mock_convert, settings): +def test_api_documents_create_with_markdown_file_success( + mock_convert, mock_yhub, settings +): """ Authenticated users should be able to create documents by uploading a Markdown file. """ @@ -146,7 +156,7 @@ def test_api_documents_create_with_markdown_file_success(mock_convert, settings) settings.CONVERSION_UPLOAD_ENABLED = True # Mock the conversion - converted_yjs = "base64encodedyjscontent" + converted_yjs = b"\x01\x02raw yjs update" mock_convert.return_value = converted_yjs # Create a fake Markdown file @@ -166,9 +176,12 @@ def test_api_documents_create_with_markdown_file_success(mock_convert, settings) assert response.status_code == 201 document = Document.objects.get() assert document.title == "readme.md" - assert document.content == converted_yjs + # the content is saved by the collaboration server, not by Django + assert document.content is None assert document.accesses.filter(role="owner", user=user).exists() + mock_yhub.return_value.create_ydoc.assert_called_once_with(document, converted_yjs) + # Verify the converter was called correctly mock_convert.assert_called_once_with( file_content, @@ -204,7 +217,7 @@ def test_api_documents_create_with_file_and_explicit_title(mock_convert, setting settings.CONVERSION_UPLOAD_ENABLED = True # Mock the conversion - converted_yjs = "base64encodedyjscontent" + converted_yjs = b"\x01\x02raw yjs update" mock_convert.return_value = converted_yjs # Create a fake DOCX file @@ -212,7 +225,10 @@ def test_api_documents_create_with_file_and_explicit_title(mock_convert, setting file = BytesIO(file_content) file.name = "Uploaded Document.docx" - with patch("core.api.viewsets.posthog_capture") as mock_capture: + with ( + patch("core.api.viewsets.posthog_capture") as mock_capture, + patch("core.api.viewsets.YHubService"), + ): response = client.post( "/api/v1.0/documents/", { @@ -412,12 +428,13 @@ def test_api_documents_create_with_file_null_value(mock_convert, settings): ) +@patch("core.api.viewsets.YHubService") @patch("core.services.converter_services.Converter.convert") def test_api_documents_create_with_file_preserves_content_format( - mock_convert, settings + mock_convert, mock_yhub, settings ): """ - Verify that the converted content is stored correctly in the document. + Verify that the converted content reaches the collaboration server as it is. """ user = factories.UserFactory() client = APIClient() @@ -425,8 +442,8 @@ def test_api_documents_create_with_file_preserves_content_format( settings.CONVERSION_UPLOAD_ENABLED = True - # Mock the conversion with realistic base64-encoded YJS data - converted_yjs = "AQMEBQYHCAkKCwwNDg8QERITFBUWFxgZGhscHR4fICA=" + # Mock the conversion with a raw Yjs update, not encodable as text + converted_yjs = b"\x01\x03\x04\x05\x06\x07" mock_convert.return_value = converted_yjs # Create a fake DOCX file @@ -446,8 +463,9 @@ def test_api_documents_create_with_file_preserves_content_format( assert response.status_code == 201 document = Document.objects.get() - # Verify the content is stored as returned by the converter - assert document.content == converted_yjs + # The update is sent untouched, it is not base64 encoded on the way + mock_yhub.return_value.create_ydoc.assert_called_once_with(document, converted_yjs) + assert document.content is None # The successful conversion should be tracked in PostHog mock_capture.assert_any_call( @@ -464,12 +482,6 @@ def test_api_documents_create_with_file_preserves_content_format( assert mock_capture.call_count == 2 - # Verify it's valid base64 (can be decoded) - try: - b64decode(converted_yjs) - except binascii.Error: - pytest.fail("Content should be valid base64-encoded data") - @patch("core.services.converter_services.Converter.convert") def test_api_documents_create_with_file_unicode_filename(mock_convert, settings): @@ -483,7 +495,7 @@ def test_api_documents_create_with_file_unicode_filename(mock_convert, settings) settings.CONVERSION_UPLOAD_ENABLED = True # Mock the conversion - converted_yjs = "base64encodedyjscontent" + converted_yjs = b"\x01\x02raw yjs update" mock_convert.return_value = converted_yjs # Create a file with Unicode characters in the name @@ -491,7 +503,10 @@ def test_api_documents_create_with_file_unicode_filename(mock_convert, settings) file = BytesIO(file_content) file.name = "文档-télécharger-документ.docx" - with patch("core.api.viewsets.posthog_capture") as mock_capture: + with ( + patch("core.api.viewsets.posthog_capture") as mock_capture, + patch("core.api.viewsets.YHubService"), + ): response = client.post( "/api/v1.0/documents/", { @@ -580,3 +595,39 @@ def test_api_documents_create_with_file_extension_not_allowed(settings): } mock_capture.assert_not_called() + + +@patch("core.api.viewsets.YHubService") +@patch("core.services.converter_services.Converter.convert") +def test_api_documents_create_with_file_collaboration_server_unavailable( + mock_convert, mock_yhub, settings +): + """ + A document whose content could not be saved by the collaboration server + should not be created at all, the uploaded file would be lost. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + settings.CONVERSION_UPLOAD_ENABLED = True + + mock_convert.return_value = b"\x01\x02raw yjs update" + mock_yhub.return_value.create_ydoc.side_effect = YHubServiceUnavailableError( + "Failed to connect to the yhub service" + ) + + file = BytesIO(b"fake docx content") + file.name = "document.docx" + + response = client.post( + "/api/v1.0/documents/", + { + "file": file, + }, + format="multipart", + ) + + assert response.status_code == 400 + assert response.json() == {"file": ["Could not save the imported file content"]} + assert not Document.objects.exists() diff --git a/src/backend/core/tests/external_api/test_external_api_documents.py b/src/backend/core/tests/external_api/test_external_api_documents.py index 66e0bbe7f..e03a9286a 100644 --- a/src/backend/core/tests/external_api/test_external_api_documents.py +++ b/src/backend/core/tests/external_api/test_external_api_documents.py @@ -276,7 +276,7 @@ def test_external_api_documents_create_with_markdown_file_success( settings.CONVERSION_UPLOAD_ENABLED = True # Mock the conversion - converted_yjs = "base64encodedyjscontent" + converted_yjs = b"\x01\x02raw yjs update" mock_convert.return_value = converted_yjs # Create a fake Markdown file @@ -284,7 +284,10 @@ def test_external_api_documents_create_with_markdown_file_success( file = BytesIO(file_content) file.name = "readme.md" - with patch("core.api.viewsets.posthog_capture") as mock_capture: + with ( + patch("core.api.viewsets.posthog_capture") as mock_capture, + patch("core.api.viewsets.YHubService") as mock_yhub, + ): response = client.post( "/external_api/v1.0/documents/", { @@ -299,9 +302,13 @@ def test_external_api_documents_create_with_markdown_file_success( document = models.Document.objects.get(id=data["id"]) assert document.title == "readme.md" - assert document.content == converted_yjs + # the content is saved by the collaboration server, not by Django + assert document.content is None assert document.accesses.filter(role="owner", user=user_specific_sub).exists() + mock_yhub.assert_called_once_with(user=user_specific_sub) + mock_yhub.return_value.create_ydoc.assert_called_once_with(document, converted_yjs) + # Verify the converter was called correctly mock_convert.assert_called_once_with( file_content, diff --git a/src/backend/core/tests/test_services_converter_services.py b/src/backend/core/tests/test_services_converter_services.py index f19a120f6..5747e3e9a 100644 --- a/src/backend/core/tests/test_services_converter_services.py +++ b/src/backend/core/tests/test_services_converter_services.py @@ -1,6 +1,5 @@ """Test y-provider services.""" -from base64 import b64decode from unittest.mock import MagicMock, patch import jwt @@ -97,7 +96,8 @@ def test_convert_full_integration(mock_post, settings): result = converter.convert("test markdown") - assert b64decode(result) == expected_content + # the raw update is returned + assert result == expected_content mock_post.assert_called_once_with( "http://test.com/conversion-endpoint/",