diff --git a/swh/web/browse/views/origin.py b/swh/web/browse/views/origin.py --- a/swh/web/browse/views/origin.py +++ b/swh/web/browse/views/origin.py @@ -4,13 +4,13 @@ # See top-level LICENSE file for more information from django.shortcuts import redirect, render +from django.urls import resolve from swh.web.browse.browseurls import browse_route from swh.web.browse.snapshot_context import ( browse_snapshot_branches, browse_snapshot_content, browse_snapshot_directory, - browse_snapshot_log, browse_snapshot_releases, get_snapshot_context, ) @@ -20,6 +20,15 @@ from swh.web.common.utils import format_utc_iso_date, parse_iso8601_date_to_utc, reverse +def redirect_to_new_route(request, new_route): + request_path = resolve(request.path_info) + # Send all the url_args and the request_args as query params + # eg /origin//log?path=test + # will be send as /log?url=&path=test + args = {**request_path.kwargs, **{k: v for k, v in request.GET.items()}} + return redirect(reverse(new_route, query_params=args), permanent=True,) + + @browse_route( r"origin/directory/", view_name="browse-origin-directory", ) @@ -111,17 +120,15 @@ r"origin/log/", view_name="browse-origin-log", ) def origin_log_browse(request): - """Django view that produces an HTML display of revisions history (aka + """ + This route is deprecated; use /browse/snapshot/log instead + + Django view that produces an HTML display of revisions history (aka the commit log) associated to a software origin. The URL that points to it is :http:get:`/browse/origin/log/` """ - return browse_snapshot_log( - request, - origin_url=request.GET.get("origin_url"), - snapshot_id=request.GET.get("snapshot"), - timestamp=request.GET.get("timestamp"), - ) + return redirect_to_new_route(request, "browse-snapshot-log") @browse_route( @@ -130,7 +137,10 @@ view_name="browse-origin-log-legacy", ) def origin_log_browse_legacy(request, origin_url, timestamp=None): - """Django view that produces an HTML display of revisions history (aka + """ + This route is deprecated; use /browse/snapshot/log instead + + Django view that produces an HTML display of revisions history (aka the commit log) associated to a software origin. The URLs that point to it are @@ -138,12 +148,7 @@ :http:get:`/browse/origin/(origin_url)/visit/(timestamp)/log/` """ - return browse_snapshot_log( - request, - origin_url=origin_url, - snapshot_id=request.GET.get("snapshot"), - timestamp=timestamp, - ) + return redirect_to_new_route(request, "browse-snapshot-log",) @browse_route( diff --git a/swh/web/browse/views/snapshot.py b/swh/web/browse/views/snapshot.py --- a/swh/web/browse/views/snapshot.py +++ b/swh/web/browse/views/snapshot.py @@ -13,10 +13,23 @@ browse_snapshot_directory, browse_snapshot_log, browse_snapshot_releases, + get_snapshot_context, ) +from swh.web.common.exc import BadInputExc from swh.web.common.utils import reverse +def get_snapshot_from_request(request): + snapshot = request.GET.get("snapshot") + if snapshot: + return snapshot + if request.GET.get("origin_url") is None: + raise BadInputExc("An origin URL must be provided as a query parameter.") + return get_snapshot_context( + origin_url=request.GET.get("origin_url"), timestamp=request.GET.get("timestamp") + )["snapshot_id"] + + @browse_route( r"snapshot/(?P[0-9a-f]+)/", view_name="browse-snapshot", @@ -115,17 +128,34 @@ @browse_route( r"snapshot/(?P[0-9a-f]+)/log/", + r"snapshot/log/", view_name="browse-snapshot-log", checksum_args=["snapshot_id"], ) -def snapshot_log_browse(request, snapshot_id): +def snapshot_log_browse(request, snapshot_id=None): """Django view that produces an HTML display of revisions history (aka the commit log) collected in a snapshot. The url that points to it is :http:get:`/browse/snapshot/(snapshot_id)/log/` """ - return browse_snapshot_log(request, snapshot_id=snapshot_id) + if snapshot_id is None: + # This case happens when redirected from /origin/log + snapshot_id = get_snapshot_from_request(request) + # Redirect to the same route with snapshot_id + return redirect( + reverse( + "browse-snapshot-log", + url_args={"snapshot_id": snapshot_id}, + query_params=request.GET, + ), + ) + return browse_snapshot_log( + request, + snapshot_id=snapshot_id, + origin_url=request.GET.get("origin_url"), + timestamp=request.GET.get("timestamp"), + ) @browse_route( diff --git a/swh/web/misc/coverage.py b/swh/web/misc/coverage.py --- a/swh/web/misc/coverage.py +++ b/swh/web/misc/coverage.py @@ -186,7 +186,7 @@ "search_pattern": "https://bitbucket.org/", "visit_types": ["hg"], "count": "336,795", - } + }, ], } diff --git a/swh/web/tests/browse/views/test_origin.py b/swh/web/tests/browse/views/test_origin.py --- a/swh/web/tests/browse/views/test_origin.py +++ b/swh/web/tests/browse/views/test_origin.py @@ -832,21 +832,45 @@ for browse_context in ( "content", "directory", - "log", "branches", "releases", "visits", ): url = reverse(f"browse-origin-{browse_context}") - resp = check_html_get_response( client, url, status_code=400, template_used="error.html" ) assert_contains( - resp, "An origin URL must be provided as query parameter.", status_code=400 + resp, "An origin URL must be provided as query parameter.", status_code=400, ) +@given(new_origin()) +@pytest.mark.parametrize("browse_context", ["log"]) +def test_origin_view_redirects(client, browse_context, new_origin): + query_params = {"origin_url": new_origin.url} + url = reverse(f"browse-origin-{browse_context}", query_params=query_params) + + resp = check_html_get_response(client, url, status_code=301) + assert resp["location"] == reverse( + f"browse-snapshot-{browse_context}", query_params=query_params + ) + + +@given(new_origin()) +@pytest.mark.parametrize("browse_context", ["log"]) +def test_origin_view_legacy_redirects(client, browse_context, new_origin): + params = {"origin_url": new_origin.url, "timestamp": "2021-01-23T22:24:10Z"} + url = reverse( + f"browse-origin-{browse_context}-legacy", url_args=params, query_params=params + ) + + resp = check_html_get_response(client, url, status_code=301) + assert resp["location"] == reverse( + f"browse-snapshot-{browse_context}", query_params=params + ) + + def _origin_content_view_test_helper( client, archive_data, diff --git a/swh/web/tests/browse/views/test_snapshot.py b/swh/web/tests/browse/views/test_snapshot.py new file mode 100644 --- /dev/null +++ b/swh/web/tests/browse/views/test_snapshot.py @@ -0,0 +1,82 @@ +import re + +from dateutil import parser +import pytest + +from swh.web.common.utils import reverse +from swh.web.tests.django_asserts import assert_contains +from swh.web.tests.utils import check_html_get_response + + +@pytest.mark.parametrize("browse_context", ["log"]) +def test_snapshot_browse_with_id(client, browse_context, snapshot): + url = reverse( + f"browse-snapshot-{browse_context}", url_args={"snapshot_id": snapshot} + ) + + resp = check_html_get_response( + client, url, status_code=200, template_used="includes/snapshot-context.html" + ) + assert_contains(resp, f"swh:1:snp:{snapshot}") + + +@pytest.mark.parametrize("browse_context", ["log"]) +def test_snapshot_browse_with_id_and_origin( + client, browse_context, archive_data, origin +): + snapshot = archive_data.snapshot_get_latest(origin["url"]) + url = reverse( + f"browse-snapshot-{browse_context}", + url_args={"snapshot_id": snapshot["id"]}, + query_params={"origin_url": origin["url"]}, + ) + + resp = check_html_get_response( + client, url, status_code=200, template_used="includes/snapshot-context.html" + ) + assert_contains(resp, origin["url"]) + + +@pytest.mark.parametrize("browse_context", ["log"]) +def test_snapshot_browse_with_id_origin_and_timestamp( + client, browse_context, archive_data, origin_with_multiple_visits +): + visit = archive_data.origin_visit_get(origin_with_multiple_visits["url"])[0] + url = reverse( + f"browse-snapshot-{browse_context}", + url_args={"snapshot_id": visit["snapshot"]}, + query_params={"origin_url": visit["origin"], "timestamp": visit["date"]}, + ) + resp = check_html_get_response( + client, url, status_code=200, template_used="includes/snapshot-context.html" + ) + requested_time = parser.parse(visit["date"]).strftime("%d %B %Y, %H:%M") + assert_contains(resp, requested_time) + assert_contains(resp, visit["origin"]) + + +@pytest.mark.parametrize("browse_context", ["log"]) +def test_snapshot_browse_without_id(client, browse_context, archive_data, origin): + url = reverse( + f"browse-snapshot-{browse_context}", query_params={"origin_url": origin["url"]} + ) + # This will be redirected to /snapshot//log + resp = check_html_get_response(client, url, status_code=302,) + snapshot = archive_data.snapshot_get_latest(origin["url"]) + + assert resp.url == reverse( + f"browse-snapshot-{browse_context}", + url_args={"snapshot_id": snapshot["id"]}, + query_params={"origin_url": origin["url"]}, + ) + + +@pytest.mark.parametrize("browse_context", ["log"]) +def test_snapshot_browse_without_id_and_origin(client, browse_context): + url = reverse(f"browse-snapshot-{browse_context}") + resp = check_html_get_response(client, url, status_code=400,) + # assert_contains works only with a success response, using re.search instead + assert re.search( + "An origin URL must be provided as a query parameter", + resp.content.decode("utf-8"), + )