fixup! ♻️(api) refactor getting versions to expose pagination

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,
         }
 

src/backend/core/tests/documents/test_api_document_versions.py

@@ -98,16 +98,18 @@ 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/",
     )
 
     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,26 +275,43 @@ 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}/",
     )
 
     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(
         f"/api/v1.0/documents/{document.id!s}/versions/{version_id:s}/",
     )
 
     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,14 +432,21 @@ 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)
     elif via == TEAM:
         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}/",

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():