Page MenuHomeSoftware Heritage
Differential D6449

Navigation to directory(code) is broken in snapshot view
ClosedPublic
Actions

Authored by jayeshv on Oct 11 2021, 1:05 PM.

Details

Reviewers
anlambert
Group Reviewers
Reviewers
Maniphest Tasks
T3641: Top level tabs not working in snapshot view
Commits
rDWAPPSd7c16735a46d: Fix navigation to directory(code) in snapshot view.
Summary

URL to the code (directory) view was missing in case the snapshot is
missing an origin cotext. Added an extra parameter called directory_url in
the snapshot_context to fix this.

realated to T3644

Diff Detail

Repository
rDWAPPS Web applications
Branch
T3641
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24358
Build 38016: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 38015: arc lint + arc unit

Event Timeline

jayeshv created this revision.Oct 11 2021, 1:05 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptOct 11 2021, 1:05 PM
swh-public-ci added a comment.Oct 11 2021, 1:26 PM

Build is green

Patch application report for D6449 (id=23429)

Rebasing onto 264c0bc84b...

Current branch diff-target is up to date.
Changes applied before test
commit 0616a1711d235135afc66a972f42bca71903851f
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon Oct 11 12:35:02 2021 +0200

    Navigation to directory(code) is broken in snapshot view
    
    URL to the code (directory) view was missing in case the snapshot is
    missing an origin cotext. Added an extra parameter called directory_url in
    the snapshot_context to fix this.
    
    realated to T3644

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

Harbormaster completed remote builds in B24345: Diff 23429.Oct 11 2021, 1:26 PM
jayeshv requested review of this revision.Oct 11 2021, 1:26 PM
anlambert requested changes to this revision.Oct 11 2021, 1:33 PM
anlambert added a subscriber: anlambert.

Looks good but some small changes are required.

swh/web/browse/snapshot_context.py
483

There is still some use of visit_info.url in a template so you should rather do
visit_info["url"] = directory_url = visit_url

swh/web/tests/browse/test_snapshot_context.py
234

same here

This revision now requires changes to proceed.Oct 11 2021, 1:33 PM
jayeshv updated this revision to Diff 23434.Oct 11 2021, 1:58 PM

Review changes

vlorentz added a subscriber: vlorentz.EditedOct 11 2021, 2:05 PM

Please use the imperative form in diff and commit titles. For example (respectively):

Fix navigation to directory(code) in snapshot view

and

Add an extra parameter called directory_url in the snapshot_context

To fix missing URL to the code (directory) view in case the snapshot is missing an origin context.

Related to T3644

or just this:

Fix missing URL to the code (directory) view in case the snapshot is missing an origin context.

Related to T3644

Two other nitpicks:

  1. it's easier for everyone to have the diff title and message match the commit message most of the time
  2. Use "Fixes Txxxx" or "Closes Txxxx" instead of "Related to Txxxx" for commits that completely fix an issue, it gives more details on the relation between the commit and the task.

See https://docs.softwareheritage.org/devel/contributing/git-style-guide.html

jayeshv added a comment.Oct 11 2021, 2:14 PM
In D6449#167375, @vlorentz wrote:

Please use the imperative form in diff and commit titles. For example (respectively):

Fix navigation to directory(code) in snapshot view

and

Add an extra parameter called directory_url in the snapshot_context

To fix missing URL to the code (directory) view in case the snapshot is missing an origin context.

Related to T3644

or just this:

Fix missing URL to the code (directory) view in case the snapshot is missing an origin context.

Related to T3644

Two other nitpicks:

  1. it's easier for everyone to have the diff title and message match the commit message most of the time
  2. Use "Fixes Txxxx" or "Closes Txxxx" instead of "Related to Txxxx" for commits that completely fix an issue, it gives more details on the relation between the commit and the task.

See https://docs.softwareheritage.org/devel/contributing/git-style-guide.html

In D6449#167375, @vlorentz wrote:

Please use the imperative form in diff and commit titles. For example (respectively):

Fix navigation to directory(code) in snapshot view

and

Add an extra parameter called directory_url in the snapshot_context

To fix missing URL to the code (directory) view in case the snapshot is missing an origin context.

Related to T3644

or just this:

Fix missing URL to the code (directory) view in case the snapshot is missing an origin context.

Related to T3644

Two other nitpicks:

  1. it's easier for everyone to have the diff title and message match the commit message most of the time
  2. Use "Fixes Txxxx" or "Closes Txxxx" instead of "Related to Txxxx" for commits that completely fix an issue, it gives more details on the relation between the commit and the task.

See https://docs.softwareheritage.org/devel/contributing/git-style-guide.html

Thanks. I will address these comments as well.

anlambert requested changes to this revision.EditedOct 11 2021, 2:17 PM

Looks good for the code but still some issues in the commit message (typo + formatting),
I would have done like this for instance.

browse/snapshot_context: Fix missing code URL in some snapshot views

URL to the code (directory) view was missing when the snapshot is missing an origin context. 

Add an extra field called directory_url in the snapshot context to fix this.

Related to T3644
This revision now requires changes to proceed.Oct 11 2021, 2:17 PM
swh-public-ci added a comment.Oct 11 2021, 2:21 PM

Build was aborted

Patch application report for D6449 (id=23434)

Rebasing onto dc489d1df2...

First, rewinding head to replay your work on top of it...
Applying: URL to the code (directory) view was missing in case the snapshot is
Changes applied before test
commit d7edf052645d36831d198f2037530c57d2713838
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon Oct 11 13:57:16 2021 +0200

    URL to the code (directory) view was missing in case the snapshot is
    missing an origin cotext. Added an extra parameter called directory_url in
    the snapshot_context to fix this.
    
    realated to T3644

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

Harbormaster failed remote builds in B24350: Diff 23434!Oct 11 2021, 2:21 PM
jayeshv updated this revision to Diff 23442.Oct 11 2021, 3:15 PM

Review changes and changes to the commit message

jayeshv updated this revision to Diff 23443.Oct 11 2021, 3:20 PM

Commit message changes to include "Fixes <ticket>"

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

Build is green

Patch application report for D6449 (id=23442)

Rebasing onto dc489d1df2...

Current branch diff-target is up to date.
Changes applied before test
commit 4284400015d1454284e41c40967acf3e80ac3dbf
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon Oct 11 15:07:24 2021 +0200

    Fix navigation to directory(code) in snapshot view.
    
    Add an extra parameter called directory_url in the snapshot_context.
    To fix the missing URL to the code (directory) view in case the
    snapshot is missing an origin context.
    
    Related to T3644

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

Harbormaster completed remote builds in B24358: Diff 23442.Oct 11 2021, 3:31 PM
anlambert accepted this revision.Oct 11 2021, 3:35 PM

Looks good to me, thanks !

This revision is now accepted and ready to land.Oct 11 2021, 3:35 PM
swh-public-ci added a comment.Oct 11 2021, 3:36 PM

Build is green

Patch application report for D6449 (id=23443)

Rebasing onto dc489d1df2...

Current branch diff-target is up to date.
Changes applied before test
commit d7c16735a46d0ea78f0fa48b16f66dc30bab006d
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon Oct 11 15:19:26 2021 +0200

    Fix navigation to directory(code) in snapshot view.
    
    Add an extra parameter called directory_url in the snapshot_context.
    To fix the missing URL to the code (directory) view in case the
    snapshot is missing an origin context.
    
    Fixes T3644

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

Harbormaster completed remote builds in B24359: Diff 23443.Oct 11 2021, 3:36 PM
Closed by commit rDWAPPSd7c16735a46d: Fix navigation to directory(code) in snapshot view. (authored by jayeshv). · Explain WhyOct 11 2021, 3:39 PM
This revision was automatically updated to reflect the committed changes.
jayeshv added a commit: rDWAPPSd7c16735a46d: Fix navigation to directory(code) in snapshot view..

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
web/
browse/
4 lines82%
Loading...
common/
2 lines100%
Loading...
templates/
includes/
4 lines--
tests/
browse/
5 lines100%
Loading...

Diff 23442

View Options

swh/web/browse/snapshot_context.py

Loading...
View Options

swh/web/common/typing.py

Loading...
View Options

swh/web/templates/includes/snapshot-context.html

Loading...
View Options

swh/web/tests/browse/test_snapshot_context.py

Loading...
About · Developer information · Legal