Page MenuHomeSoftware Heritage
Differential D6441

Deprecate /browse/origin/log/ URLs
ClosedPublic
Actions

Authored by jayeshv on Oct 8 2021, 12:33 PM.

Details

Reviewers
anlambert
Group Reviewers
Reviewers
Maniphest Tasks
T3608: Deprecate most of the /browse/origin/.* URLs
Commits
rDWAPPS0c04ede6c05a: Deprecate /origin/log route
Summary

This fix deprecates and redirects /origin/log?origin_url=<url> and

/origin/<url>/log to /snapshot/log/
- Generic function to redirect to a new route.
- Generic logic to forward url args and query params.
- Initial tests for snapshot view

Related to T3608

Diff Detail

Repository
rDWAPPS Web applications
Branch
directory_route
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24652
Build 38474: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 38473: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jayeshv requested review of this revision.Oct 8 2021, 12:54 PM
jayeshv updated this revision to Diff 23417.Oct 8 2021, 4:22 PM

WIP: Many URLs defined in the origin view (urlconf) are redundant.
This fix redirects /origin/branches?origin_url=<url> and
/origin/<url>/branches to /snapshot-branches/

jayeshv updated this revision to Diff 23418.Oct 8 2021, 4:31 PM

Diff for log routes

swh-public-ci added a comment.Oct 8 2021, 4:44 PM

Build has FAILED

Patch application report for D6441 (id=23417)

Rebasing onto 264c0bc84b...

Current branch diff-target is up to date.
Changes applied before test
commit 3bbb4320b179ab47b453e26a36f3b290c5ee57f4
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Oct 8 16:19:35 2021 +0200

    test-changes

commit f5b3cd19b787479ad090eba7f6db824783ed0978
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Oct 8 16:01:40 2021 +0200

    branch routes

commit 3c2da2ad33cf614c5e014a61457a4fed56554bd6
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Oct 8 12:39:33 2021 +0200

    Deprecate most of the /browse/origin/.* URLs
    
    Many URLs defined in the origin view (urlconf) are redundant.
    This fix redirects /origin/logs?origin_url=<url> and
    /origin/<url>/log to /snapshot-log/
    - Generic decorator to deprecate a route
    - Generic logic to forward url args and query params.
    
    Related to T3608

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1151/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1151/console

Harbormaster failed remote builds in B24333: Diff 23417!Oct 8 2021, 4:44 PM
swh-public-ci added a comment.Oct 8 2021, 4:46 PM

Build is green

Patch application report for D6441 (id=23418)

Rebasing onto 264c0bc84b...

Current branch diff-target is up to date.
Changes applied before test
commit 3c2da2ad33cf614c5e014a61457a4fed56554bd6
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Oct 8 12:39:33 2021 +0200

    Deprecate most of the /browse/origin/.* URLs
    
    Many URLs defined in the origin view (urlconf) are redundant.
    This fix redirects /origin/logs?origin_url=<url> and
    /origin/<url>/log to /snapshot-log/
    - Generic decorator to deprecate a route
    - Generic logic to forward url args and query params.
    
    Related to T3608

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1152/ for more details.

Harbormaster completed remote builds in B24334: Diff 23418.Oct 8 2021, 4:46 PM
anlambert requested changes to this revision.Oct 11 2021, 1:22 PM
anlambert added a subscriber: anlambert.

I added some inline comments to handle.

Also we should remove the use of /browse/origin/log/ URLs in codebase, grep for "browse-origin-log" to find them.

swh/web/browse/views/origin.py
22–43

Could you remove the decorator wrapping and simply call redirect_to_new_route in views implementation ?
It feels weird to have functions without any instructions in them.

swh/web/browse/views/snapshot.py
130

Please use r"snapshot/log/ instead.

142

if snasphot_id is None

143–159

origin_url and timestamp query parameters must be passed to browse_snapshot_log to
ensure same behavior as before.

swh/web/tests/browse/views/test_origin.py
845–846

Please move the redirection check in a separate test.

This revision now requires changes to proceed.Oct 11 2021, 1:22 PM
jayeshv added inline comments.Oct 11 2021, 1:38 PM
swh/web/browse/views/origin.py
22–43

Ok, do you think adding an extra line in the doc string in each deprecated function would be enough?. It would be like "This route is deprecated, use <alternate name> instead" ?

anlambert added inline comments.Oct 11 2021, 1:44 PM
swh/web/browse/views/origin.py
22–43

Yes this should be enough, you can reference the correct view to use in sphinx using :http:get:<url>, see how its is done for the vault.

jayeshv added inline comments.Oct 11 2021, 1:47 PM
swh/web/browse/views/origin.py
22–43

Ok, Thanks. In fact, I have that doc string added in the coming diffs (for branches and releases). So, I will move that change to this diff and keep the decorator

jayeshv updated this revision to Diff 23441.Oct 11 2021, 3:01 PM

Review changes

swh-public-ci added a comment.Oct 11 2021, 3:17 PM

Build is green

Patch application report for D6441 (id=23441)

Rebasing onto dc489d1df2...

First, rewinding head to replay your work on top of it...
Applying: Deprecate /origin/log route
Changes applied before test
commit b169d9dffbe99d0585d1bd2c965ba5ddc33c86d9
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon Oct 11 14:56:52 2021 +0200

    Deprecate /origin/log route
    
    This fix deprecates and redirects /origin/log?origin_url=<url> and
    /origin/<url>/log to /snapshot/log/
    - Generic decorator to deprecate a route
    - Generic logic to forward url args and query params.
    
    Related to T3608

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1155/ for more details.

Harbormaster completed remote builds in B24357: Diff 23441.Oct 11 2021, 3:17 PM
jayeshv marked 4 inline comments as done.Oct 11 2021, 3:21 PM
jayeshv added inline comments.Oct 11 2021, 3:44 PM
swh/web/tests/browse/views/test_origin.py
852

This should be assert "/snapshot/{browse_context}/" in resp.url
I will fix this with branch/release diff

anlambert requested changes to this revision.Oct 11 2021, 3:47 PM

It seems that the browse-snapshot-log view is missing tests so we should create the file swh/web/tests/browse/views/test_snapshot.py
and implement tests here.

Endpoint documentation must also be updated:

  • /browse/origin/directory/ must be marked as deprecated in docs/uri-scheme-browse-origin.rst
  • /browse/snapshot/log/ documentation must be added in docs/uri-scheme-browse-snapshot.rst indicating the origin_url query parameter is mandatory and that a redirection will be performed to browse the log from the latest snapshot of the origin.
swh/web/browse/views/snapshot.py
26

If origin_url is not provided, get_snapshot_context will fail so you should raise a BadInputExc in that case to get a 400 error page.

144

Could you do a redirection to the same view with the snapshot argument instead ?
This way, the latest snapshot id will be displayed in browser URL bar.

swh/web/tests/browse/views/test_origin.py
847–852

This test is not complete and can be improved, use this instead:

@given(origin())
@pytest.mark.parametrize("browse_context", ["log"])
def test_origin_view_redirects(client, browse_context, origin):
    query_params = {"origin_url": 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
    )
This revision now requires changes to proceed.Oct 11 2021, 3:47 PM
anlambert retitled this revision from Deprecate most of the /browse/origin/.* URLs to Deprecate /browse/origin/log/ URLs.Oct 11 2021, 3:48 PM
jayeshv updated this revision to Diff 23447.Oct 11 2021, 4:12 PM

Review changes

  • checking for origin_url in request params
  • better tests
  • redirect to /snapshot/log/<id>
swh-public-ci added a comment.Oct 11 2021, 4:26 PM

Build is green

Patch application report for D6441 (id=23447)

Rebasing onto d7c16735a4...

Current branch diff-target is up to date.
Changes applied before test
commit 47f58ef7fcc396761e6d08e784351356f51a5f22
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon Oct 11 16:10:53 2021 +0200

    Deprecate /origin/log route
    
    This fix deprecates and redirects /origin/log?origin_url=<url> and
    /origin/<url>/log to /snapshot/log/
    - Generic decorator to deprecate a route
    - Generic logic to forward url args and query params.
    
    Related to T3608

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1158/ for more details.

Harbormaster completed remote builds in B24364: Diff 23447.Oct 11 2021, 4:26 PM
jayeshv edited the summary of this revision. (Show Details)Oct 11 2021, 4:48 PM
jayeshv marked 5 inline comments as done.Oct 12 2021, 10:25 AM
anlambert requested changes to this revision.Oct 12 2021, 10:50 AM

Still some work to do before I can accept the diff:

  • add missing tests
  • remove the use of the browse-origin-log view in codebase (grep for it)
swh/web/browse/views/origin.py
22–28

I think it will be better to add a view_redirect parameter to the decorator instead of storing the deprecation_mapping dict in it.
It will make the code more readable and the decorator more generic.

swh/web/browse/views/snapshot.py
22

request.GET is already a dictionary so that instruction is not needed.pt

27

We need a test for this.

135–160

That view is not covered by tests, we must add some in a new tests/browse/views/test_snapshot.py file.

This revision now requires changes to proceed.Oct 12 2021, 10:50 AM
jayeshv updated this revision to Diff 23739.Oct 21 2021, 2:58 PM

Changes requested in the review

  • Removed the decorator for a redirect function
  • Added tests for snapshot view
jayeshv edited the summary of this revision. (Show Details)Oct 21 2021, 3:10 PM
swh-public-ci added a comment.Oct 21 2021, 3:18 PM

Build is green

Patch application report for D6441 (id=23739)

Rebasing onto a8cfc14b4a...

First, rewinding head to replay your work on top of it...
Applying: Deprecate /origin/log route
Changes applied before test
commit 116a4aae0b8115d7f86b4c0717dd47a5a37b8c5a
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Oct 21 14:54:22 2021 +0200

    Deprecate /origin/log route
    
    This fix deprecates and redirects /origin/log?origin_url=<url> and
    /origin/<url>/log to /snapshot/log/
    - Generic function to redirect to a new route.
    - Generic logic to forward url args and query params.
    - Initial tests for snapshot view.
    
    Related to T3608

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1200/ for more details.

Harbormaster completed remote builds in B24627: Diff 23739.Oct 21 2021, 3:18 PM
jayeshv marked 4 inline comments as done.Oct 21 2021, 3:20 PM
jayeshv updated this revision to Diff 23745.Oct 21 2021, 4:17 PM

Removed unused code

anlambert requested changes to this revision.Oct 21 2021, 4:28 PM

This is moving forward, great ! I added a couple of inline comments to handle.

Also could you add documentation for the new /browse/snapshot/log/ endpoint here
and mark the /browse/origin/logas deprecated (see example here) ?

swh/web/browse/views/snapshot.py
27

This call will fail if the request.GET dict contains parameters unexpected by the get_snapshot_context function.
You should explicitly provide the origin_url and timestamp parameter instead.

swh/web/tests/browse/views/test_snapshot.py
19

Use this instead:

from swh.web.tests.django_asserts import assert_contains

assert_contains(resp, f"swh:1:snp:{snapshot}")

Same thing for other tests.

This revision now requires changes to proceed.Oct 21 2021, 4:28 PM
swh-public-ci added a comment.Oct 21 2021, 4:32 PM

Build is green

Patch application report for D6441 (id=23745)

Rebasing onto a8cfc14b4a...

First, rewinding head to replay your work on top of it...
Applying: Deprecate /origin/log route
Changes applied before test
commit a006ec12220115be99f794dbdd24214462660987
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Oct 21 16:16:07 2021 +0200

    Deprecate /origin/log route
    
    This fix deprecates and redirects /origin/log?origin_url=<url> and
    /origin/<url>/log to /snapshot/log/
    - Generic function to redirect to a new route.
    - Generic logic to forward url args and query params.
    - Initial tests for snapshot view.
    
    Related to T3608

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1201/ for more details.

Harbormaster completed remote builds in B24632: Diff 23745.Oct 21 2021, 4:32 PM
jayeshv updated this revision to Diff 23749.Oct 21 2021, 4:44 PM

Requested changes

jayeshv marked 2 inline comments as done.Oct 21 2021, 4:45 PM
swh-public-ci added a comment.Oct 21 2021, 4:59 PM

Build is green

Patch application report for D6441 (id=23749)

Rebasing onto a8cfc14b4a...

First, rewinding head to replay your work on top of it...
Applying: Deprecate /origin/log route
Changes applied before test
commit aa9ed3401503f2093334c1e27c92a503f807a2be
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Oct 21 16:43:19 2021 +0200

    Deprecate /origin/log route
    
    This fix deprecates and redirects /origin/log?origin_url=<url> and
    /origin/<url>/log to /snapshot/log/
    - Generic function to redirect to a new route.
    - Generic logic to forward url args and query params.
    - Initial tests for snapshot view.
    
    Related to T3608

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1202/ for more details.

Harbormaster completed remote builds in B24636: Diff 23749.Oct 21 2021, 4:59 PM
jayeshv updated this revision to Diff 23755.Oct 21 2021, 5:48 PM

Documentation updates

swh-public-ci added a comment.Oct 21 2021, 6:12 PM

Build was aborted

Patch application report for D6441 (id=23755)

Rebasing onto a8cfc14b4a...

First, rewinding head to replay your work on top of it...
Applying: Deprecate /origin/log route
Changes applied before test
commit 60dd6faae996e38b67109d37daac4b9617b2ae46
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Oct 21 17:46:53 2021 +0200

    Deprecate /origin/log route
    
    This fix deprecates and redirects /origin/log?origin_url=<url> and
    /origin/<url>/log to /snapshot/log/
    - Generic function to redirect to a new route.
    - Generic logic to forward url args and query params.
    - Initial tests for snapshot view.
    - Documentation updates
    
    Related to T3608

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1204/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1204/console

Harbormaster failed remote builds in B24642: Diff 23755!Oct 21 2021, 6:12 PM
swh-public-ci added a comment.Oct 22 2021, 9:26 AM

Build is green

Patch application report for D6441 (id=23755)

Rebasing onto a8cfc14b4a...

First, rewinding head to replay your work on top of it...
Applying: Deprecate /origin/log route
Changes applied before test
commit b30080209cc0285432c1408e42686335340311c8
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Oct 21 17:46:53 2021 +0200

    Deprecate /origin/log route
    
    This fix deprecates and redirects /origin/log?origin_url=<url> and
    /origin/<url>/log to /snapshot/log/
    - Generic function to redirect to a new route.
    - Generic logic to forward url args and query params.
    - Initial tests for snapshot view.
    - Documentation updates
    
    Related to T3608

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1205/ for more details.

Harbormaster completed remote builds in B24642: Diff 23755.Oct 22 2021, 9:26 AM
anlambert accepted this revision.Oct 22 2021, 11:29 AM

Looks good to me, thanks ! I added a last batch of nitpick comments to handle before landing this.

docs/uri-scheme-browse-snapshot.rst
182

s/origin_url/origin URL/

190

extra "in the" to remove

swh/web/browse/views/origin.py
123

Use http:get:`/browse/snapshot/log` to link to endpoint doc

140

same here

swh/web/browse/views/snapshot.py
138–139

The urls that point to it are :http:get:`/browse/snapshot/log` and

This revision is now accepted and ready to land.Oct 22 2021, 11:29 AM
jayeshv updated this revision to Diff 23766.Oct 22 2021, 11:50 AM

typo fixes; added doc links

jayeshv marked 5 inline comments as done.Oct 22 2021, 11:53 AM
swh-public-ci added a comment.Oct 22 2021, 12:03 PM

Build is green

Patch application report for D6441 (id=23766)

Rebasing onto 0da5060e6f...

Current branch diff-target is up to date.
Changes applied before test
commit 0c04ede6c05aff2dea636ad3c560acdeaf94b2cf
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Oct 22 11:45:28 2021 +0200

    Deprecate /origin/log route
    
    This fix deprecates and redirects /origin/log?origin_url=<url> and
    /origin/<url>/log to /snapshot/log/
    - Generic function to redirect to a new route.
    - Generic logic to forward url args and query params.
    - Initial tests for snapshot view.
    - Documentation updates
    
    Related to T3608

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1206/ for more details.

Harbormaster completed remote builds in B24652: Diff 23766.Oct 22 2021, 12:03 PM
Closed by commit rDWAPPS0c04ede6c05a: Deprecate /origin/log route (authored by jayeshv). · Explain WhyOct 22 2021, 12:06 PM
This revision was automatically updated to reflect the committed changes.
jayeshv added a commit: rDWAPPS0c04ede6c05a: Deprecate /origin/log route.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
docs/
6 lines--
21 lines--
swh/
web/
browse/
views/
35 lines82%
Loading...
39 lines70%
Loading...
misc/
2 lines96%
Loading...
tests/
browse/
views/
30 lines97%
Loading...
82 lines100%
Loading...

Diff 23766

View Options

docs/uri-scheme-browse-origin.rst

Loading...
View Options

docs/uri-scheme-browse-snapshot.rst

Loading...
View Options

swh/web/browse/views/origin.py

Loading...
View Options

swh/web/browse/views/snapshot.py

Loading...
View Options

swh/web/misc/coverage.py

Loading...
View Options

swh/web/tests/browse/views/test_origin.py

Loading...
View Options

swh/web/tests/browse/views/test_snapshot.py

Loading...
About · Developer information · Legal