From f527df18113eb29530fdbfb94e065373d2b96593 Mon Sep 17 00:00:00 2001 From: Samuel Paccoud - DINUM Date: Sun, 22 Sep 2024 09:06:35 +0200 Subject: [PATCH] =?UTF-8?q?fixup!=20=E2=99=BB=EF=B8=8F(api)=20refactor=20g?= =?UTF-8?q?etting=20versions=20to=20expose=20pagination?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/backend/core/models.py | 25 +++-- .../documents/test_api_document_versions.py | 96 ++++++++++++++----- .../core/tests/test_models_documents.py | 10 +- 3 files changed, 93 insertions(+), 38 deletions(-) diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 87ecf38b..13d72820 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -422,7 +422,7 @@ def get_versions_slice(self, from_version_id="", min_datetime=None, page_size=No {"KeyMarker": self.file_key, "VersionIdMarker": from_version_id} ) - max_keys = ( + real_page_size = ( min(page_size, settings.DOCUMENT_VERSIONS_PAGE_SIZE) if page_size else settings.DOCUMENT_VERSIONS_PAGE_SIZE @@ -431,7 +431,9 @@ def get_versions_slice(self, from_version_id="", min_datetime=None, page_size=No response = default_storage.connection.meta.client.list_object_versions( Bucket=default_storage.bucket_name, Prefix=self.file_key, - MaxKeys=max_keys, + # compensate the latest version that we exclude below and get one more to + # know if there are more pages + MaxKeys=real_page_size + 2, **markers, ) @@ -448,19 +450,22 @@ def get_versions_slice(self, from_version_id="", min_datetime=None, page_size=No } for version in response.get("Versions", []) if version["LastModified"] >= min_last_modified + and version["IsLatest"] is False ] + results = versions[:real_page_size] - count = len(versions) - is_truncated = response["IsTruncated"] and count == len( - response.get("Versions", []) - ) + count = len(results) + if count == len(versions): + is_truncated = False + next_version_id_marker = "" + else: + is_truncated = True + next_version_id_marker = versions[count - 1]["version_id"] return { - "next_version_id_marker": response["NextVersionIdMarker"] - if is_truncated - else "", + "next_version_id_marker": next_version_id_marker, "is_truncated": is_truncated, - "versions": versions, + "versions": results, "count": count, } diff --git a/src/backend/core/tests/documents/test_api_document_versions.py b/src/backend/core/tests/documents/test_api_document_versions.py index 9d59a8a7..69181218 100644 --- a/src/backend/core/tests/documents/test_api_document_versions.py +++ b/src/backend/core/tests/documents/test_api_document_versions.py @@ -98,8 +98,9 @@ def test_api_document_versions_list_authenticated_related_success(via, mock_user assert content["count"] == 0 # Add a new version to the document - document.content = "new content" - document.save() + for i in range(3): + document.content = f"new content {i:d}" + document.save() response = client.get( f"/api/v1.0/documents/{document.id!s}/versions/", @@ -107,7 +108,8 @@ def test_api_document_versions_list_authenticated_related_success(via, mock_user assert response.status_code == 200 content = response.json() - assert content["count"] == 1 + # The current version is not listed + assert content["count"] == 2 @pytest.mark.parametrize("via", VIA) @@ -142,7 +144,7 @@ def test_api_document_versions_list_authenticated_related_pagination( role=random.choice(models.RoleChoices.choices)[0], ) - for i in range(3): + for i in range(4): document.content = f"after {i:d}" document.save() @@ -152,6 +154,7 @@ def test_api_document_versions_list_authenticated_related_pagination( content = response.json() assert content["is_truncated"] is False + # The current version is not listed assert content["count"] == 3 assert content["next_version_id_marker"] == "" all_version_ids = [version["version_id"] for version in content["versions"]] @@ -208,6 +211,9 @@ def test_api_document_versions_retrieve_anonymous(reach): restricted or authenticated link reach. """ document = factories.DocumentFactory(link_reach=reach) + document.content = "new content" + document.save() + version_id = document.get_versions_slice()["versions"][0]["version_id"] url = f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/" @@ -231,6 +237,9 @@ def test_api_document_versions_retrieve_authenticated_unrelated(reach): client.force_login(user) document = factories.DocumentFactory(link_reach=reach) + document.content = "new content" + document.save() + version_id = document.get_versions_slice()["versions"][0]["version_id"] response = client.get( @@ -254,6 +263,10 @@ def test_api_document_versions_retrieve_authenticated_related(via, mock_user_tea client.force_login(user) document = factories.DocumentFactory() + document.content = "new content" + document.save() + + assert len(document.get_versions_slice()["versions"]) == 1 version_id = document.get_versions_slice()["versions"][0]["version_id"] if via == USER: @@ -262,6 +275,8 @@ def test_api_document_versions_retrieve_authenticated_related(via, mock_user_tea mock_user_teams.return_value = ["lasuite", "unknown"] factories.TeamDocumentAccessFactory(document=document, team="lasuite") + time.sleep(1) # minio stores datetimes with the precision of a second + # Versions created before the document was shared should not be seen by the user response = client.get( f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", @@ -269,11 +284,26 @@ def test_api_document_versions_retrieve_authenticated_related(via, mock_user_tea assert response.status_code == 404 - # Create a new version should make it available to the user - time.sleep(1) # minio stores datetimes with the precision of a second - document.content = "new content" + # Create a new version should not make it available to the user because + # only the current version is available to the user but it is excluded + # from the list + document.content = "new content 1" document.save() + assert len(document.get_versions_slice()["versions"]) == 2 + version_id = document.get_versions_slice()["versions"][0]["version_id"] + + response = client.get( + f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", + ) + + assert response.status_code == 404 + + # Adding one more version should make the previous version available to the user + document.content = "new content 2" + document.save() + + assert len(document.get_versions_slice()["versions"]) == 3 version_id = document.get_versions_slice()["versions"][0]["version_id"] response = client.get( @@ -281,7 +311,7 @@ def test_api_document_versions_retrieve_authenticated_related(via, mock_user_tea ) assert response.status_code == 200 - assert response.json()["content"] == "new content" + assert response.json()["content"] == "new content 1" def test_api_document_versions_create_anonymous(): @@ -349,10 +379,15 @@ def test_api_document_versions_create_authenticated_related(via, mock_user_teams def test_api_document_versions_update_anonymous(): """Anonymous users should not be allowed to update a document version.""" access = factories.UserDocumentAccessFactory() - version_id = access.document.get_versions_slice()["versions"][0]["version_id"] + document = access.document + document.content = "new content" + document.save() + + assert len(document.get_versions_slice()["versions"]) == 1 + version_id = document.get_versions_slice()["versions"][0]["version_id"] response = APIClient().put( - f"/api/v1.0/documents/{access.document_id!s}/versions/{version_id:s}/", + f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", {"foo": "bar"}, format="json", ) @@ -370,7 +405,12 @@ def test_api_document_versions_update_authenticated_unrelated(): client.force_login(user) access = factories.UserDocumentAccessFactory() - version_id = access.document.get_versions_slice()["versions"][0]["version_id"] + document = access.document + document.content = "new content" + document.save() + + assert len(document.get_versions_slice()["versions"]) == 1 + version_id = document.get_versions_slice()["versions"][0]["version_id"] response = client.put( f"/api/v1.0/documents/{access.document_id!s}/versions/{version_id:s}/", @@ -392,7 +432,6 @@ def test_api_document_versions_update_authenticated_related(via, mock_user_teams client.force_login(user) document = factories.DocumentFactory() - version_id = document.get_versions_slice()["versions"][0]["version_id"] if via == USER: factories.UserDocumentAccessFactory(document=document, user=user) @@ -400,6 +439,14 @@ def test_api_document_versions_update_authenticated_related(via, mock_user_teams mock_user_teams.return_value = ["lasuite", "unknown"] factories.TeamDocumentAccessFactory(document=document, team="lasuite") + time.sleep(1) # minio stores datetimes with the precision of a second + + document.content = "new content" + document.save() + + assert len(document.get_versions_slice()["versions"]) == 1 + version_id = document.get_versions_slice()["versions"][0]["version_id"] + response = client.put( f"/api/v1.0/documents/{document.id!s}/versions/{version_id!s}/", {"foo": "bar"}, @@ -434,6 +481,9 @@ def test_api_document_versions_delete_authenticated(reach): client.force_login(user) document = factories.DocumentFactory(link_reach=reach) + document.content = "new content" + document.save() + version_id = document.get_versions_slice()["versions"][0]["version_id"] response = client.delete( @@ -470,13 +520,7 @@ def test_api_document_versions_delete_reader_or_editor(via, role, mock_user_team document.save() versions = document.get_versions_slice()["versions"] - assert len(versions) == 2 - - version_id = versions[1]["version_id"] - response = client.delete( - f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", - ) - assert response.status_code == 403 + assert len(versions) == 1 version_id = versions[0]["version_id"] response = client.delete( @@ -485,7 +529,7 @@ def test_api_document_versions_delete_reader_or_editor(via, role, mock_user_team assert response.status_code == 403 versions = document.get_versions_slice()["versions"] - assert len(versions) == 2 + assert len(versions) == 1 @pytest.mark.parametrize("via", VIA) @@ -510,19 +554,25 @@ def test_api_document_versions_delete_administrator_or_owner(via, mock_user_team # Create a new version should make it available to the user time.sleep(1) # minio stores datetimes with the precision of a second - document.content = "new content" + document.content = "new content 1" document.save() versions = document.get_versions_slice()["versions"] - assert len(versions) == 2 + assert len(versions) == 1 - version_id = versions[1]["version_id"] + version_id = versions[0]["version_id"] response = client.delete( f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", ) # 404 because the version was created before the user was given access to the document assert response.status_code == 404 + document.content = "new content 2" + document.save() + + versions = document.get_versions_slice()["versions"] + assert len(versions) == 2 + version_id = versions[0]["version_id"] response = client.delete( f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/", diff --git a/src/backend/core/tests/test_models_documents.py b/src/backend/core/tests/test_models_documents.py index fb6143db..a52aad25 100644 --- a/src/backend/core/tests/test_models_documents.py +++ b/src/backend/core/tests/test_models_documents.py @@ -287,7 +287,7 @@ def test_models_documents_get_versions_slice_pagination(settings): from_version_id=response["next_version_id_marker"] ) assert response["is_truncated"] is False - assert len(response["versions"]) == 3 + assert len(response["versions"]) == 2 assert response["next_version_id_marker"] == "" # - Get custom max versions @@ -300,7 +300,7 @@ def test_models_documents_get_versions_slice_pagination(settings): def test_models_documents_get_versions_slice_min_datetime(): """ The "get_versions_slice" method should filter out versions anterior to - the from_datetime passed in argument. + the from_datetime passed in argument and the current version. """ document = factories.DocumentFactory() from_dt = [] @@ -311,14 +311,14 @@ def test_models_documents_get_versions_slice_min_datetime(): response = document.get_versions_slice(min_datetime=from_dt[2]) - assert len(response["versions"]) == 4 + assert len(response["versions"]) == 3 for version in response["versions"]: assert version["last_modified"] > from_dt[2] - response = document.get_versions_slice(min_datetime=from_dt[5]) + response = document.get_versions_slice(min_datetime=from_dt[4]) assert len(response["versions"]) == 1 - assert response["versions"][0]["last_modified"] > from_dt[5] + assert response["versions"][0]["last_modified"] > from_dt[4] def test_models_documents_version_duplicate():