From 9a14ae9bdaa34fb984b4d0388b8d8b7a6257f2ec Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 12 Nov 2024 16:27:41 +0100 Subject: [PATCH] Addons: resolve URLs for file tree diff API response (#11749) Resolve all the filename into URLs, so the frontend can use these links without manipulating them. We don't want re-implement the resolver in the frontend. Now, we return the filename, the URL for the current version, and the URL for the source version of the filename changed. Closes https://github.com/readthedocs/addons/issues/409 --- readthedocs/filetreediff/__init__.py | 20 +++++-- readthedocs/proxito/tests/test_hosting.py | 55 +++++++++++++------ readthedocs/proxito/views/hosting.py | 64 +++++++++++++++++++++-- 3 files changed, 116 insertions(+), 23 deletions(-) diff --git a/readthedocs/filetreediff/__init__.py b/readthedocs/filetreediff/__init__.py index 65c3fd22ee1..0d1c3d9ae81 100644 --- a/readthedocs/filetreediff/__init__.py +++ b/readthedocs/filetreediff/__init__.py @@ -69,10 +69,24 @@ def get_diff(version_a: Version, version_b: Version) -> FileTreeDiff | None: if file_a.main_content_hash != file_b.main_content_hash: files_modified.append(file_path) + def sortpath(filename): + """ + Function to use as `key=` argument for `sorted`. + + It sorts the file names by the less deep directories first. + However, it doesn't group the results by directory. + + Ideally, this should sort file names by hierarchy (less deep directory + first), groupping them by directory and alphabetically. We should update + this function to achieve that goal if we find a simple way to do it. + """ + parts = filename.split("/") + return len(parts), parts + return FileTreeDiff( - added=files_added, - deleted=files_deleted, - modified=files_modified, + added=sorted(files_added, key=sortpath), + deleted=sorted(files_deleted, key=sortpath), + modified=sorted(files_modified, key=sortpath), outdated=outdated, ) diff --git a/readthedocs/proxito/tests/test_hosting.py b/readthedocs/proxito/tests/test_hosting.py index fc1e23c12f0..5dd2cd754f0 100644 --- a/readthedocs/proxito/tests/test_hosting.py +++ b/readthedocs/proxito/tests/test_hosting.py @@ -925,27 +925,52 @@ def test_file_tree_diff(self, get_manifest): ], ), ] - r = self.client.get( - reverse("proxito_readthedocs_docs_addons"), - { - "url": "https://project--123.dev.readthedocs.build/en/123/", - "client-version": "0.6.0", - "api-version": "1.0.0", - }, - secure=True, - headers={ - "host": "project--123.dev.readthedocs.build", - }, - ) + with self.assertNumQueries(30): + r = self.client.get( + reverse("proxito_readthedocs_docs_addons"), + { + "url": "https://project--123.dev.readthedocs.build/en/123/", + "client-version": "0.6.0", + "api-version": "1.0.0", + }, + secure=True, + headers={ + "host": "project--123.dev.readthedocs.build", + }, + ) assert r.status_code == 200 filetreediff_response = r.json()["addons"]["filetreediff"] assert filetreediff_response == { "enabled": True, "outdated": False, "diff": { - "added": [{"file": "new-file.html"}], - "deleted": [{"file": "deleted.html"}], - "modified": [{"file": "tutorial/index.html"}], + "added": [ + { + "filename": "new-file.html", + "urls": { + "version_a": "https://project--123.dev.readthedocs.build/en/123/new-file.html", + "version_b": "https://project.dev.readthedocs.io/en/latest/new-file.html", + }, + }, + ], + "deleted": [ + { + "filename": "deleted.html", + "urls": { + "version_a": "https://project--123.dev.readthedocs.build/en/123/deleted.html", + "version_b": "https://project.dev.readthedocs.io/en/latest/deleted.html", + }, + }, + ], + "modified": [ + { + "filename": "tutorial/index.html", + "urls": { + "version_a": "https://project--123.dev.readthedocs.build/en/123/tutorial/index.html", + "version_b": "https://project.dev.readthedocs.io/en/latest/tutorial/index.html", + }, + }, + ], }, } diff --git a/readthedocs/proxito/views/hosting.py b/readthedocs/proxito/views/hosting.py index dc6a89110bb..9f4d92ee4de 100644 --- a/readthedocs/proxito/views/hosting.py +++ b/readthedocs/proxito/views/hosting.py @@ -529,7 +529,10 @@ def _v1(self, project, version, build, filename, url, request): if version: response = self._get_filetreediff_response( - request=request, project=project, version=version + request=request, + project=project, + version=version, + resolver=resolver, ) if response: data["addons"]["filetreediff"].update(response) @@ -614,7 +617,7 @@ def _v1(self, project, version, build, filename, url, request): return data - def _get_filetreediff_response(self, *, request, project, version): + def _get_filetreediff_response(self, *, request, project, version, resolver): """ Get the file tree diff response for the given version. @@ -641,9 +644,60 @@ def _get_filetreediff_response(self, *, request, project, version): "enabled": True, "outdated": diff.outdated, "diff": { - "added": [{"file": file} for file in diff.added], - "deleted": [{"file": file} for file in diff.deleted], - "modified": [{"file": file} for file in diff.modified], + "added": [ + { + "filename": filename, + "urls": { + "version_a": resolver.resolve_version( + project=project, + filename=filename, + version=version, + ), + "version_b": resolver.resolve_version( + project=project, + filename=filename, + version=latest_version, + ), + }, + } + for filename in diff.added + ], + "deleted": [ + { + "filename": filename, + "urls": { + "version_a": resolver.resolve_version( + project=project, + filename=filename, + version=version, + ), + "version_b": resolver.resolve_version( + project=project, + filename=filename, + version=latest_version, + ), + }, + } + for filename in diff.deleted + ], + "modified": [ + { + "filename": filename, + "urls": { + "version_a": resolver.resolve_version( + project=project, + filename=filename, + version=version, + ), + "version_b": resolver.resolve_version( + project=project, + filename=filename, + version=latest_version, + ), + }, + } + for filename in diff.modified + ], }, }