internal/frontend: display module importer count Display the count of importing modules in the header section of the main package page, along with the package count. Add a help tooltip to explain why we're doing this. Cq-Include-Trybots: luci.golang.try:x_pkgsite-gotip-linux-amd64_docker-screentest For golang/go#80242. Change-Id: If184d698aa8f2cee26921490dddad40d6a6a6964 Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/850925 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
diff --git a/internal/frontend/main.go b/internal/frontend/main.go index a08ede8..142ed2c 100644 --- a/internal/frontend/main.go +++ b/internal/frontend/main.go
@@ -59,10 +59,15 @@ // See https://golang.org/issue/42968. ModuleReadmeLinks []link - // ImportedByCount is the number of packages that import this path. - // When the count is > limit it will read as 'limit+'. This field - // is not supported when using a datasource proxy. - ImportedByCount string + // ImportedByPackages describes the number of packages that import this path, + // for example "1,234 packages". It is "? packages" if the count hasn't been + // computed. This field is not supported when using a datasource proxy. + ImportedByPackages string + + // ImportedByModules describes the number of modules that import this path, + // for example "1,234 modules". It is "? modules" if the count hasn't been + // computed. This field is not supported when using a datasource proxy. + ImportedByModules string DocBody safehtml.HTML DocOutline safehtml.HTML @@ -202,35 +207,49 @@ isStableVersion := semver.Major(um.Version) != "v0" && versionType == version.TypeRelease pr := message.NewPrinter(language.English) return &MainDetails{ - ExpandReadme: expandReadme, - Directories: unitDirectories(append(subdirectories, nestedModules...)), - Licenses: transformLicenseMetadata(unit.Licenses), - CommitTime: absoluteTime(um.CommitTime), - Readme: readme.HTML, - ReadmeOutline: readme.Outline, - ReadmeLinks: readme.Links, - DocLinks: docLinks, - ModuleReadmeLinks: modLinks, - DocOutline: docParts.Outline, - DocBody: docParts.Body, - DocSynopsis: synopsis, - GOOS: goos, - GOARCH: goarch, - BuildContexts: buildContexts, - SourceFiles: files, - RepositoryURL: um.SourceInfo.RepoURL(), - SourceURL: um.SourceInfo.DirectoryURL(internal.Suffix(um.Path, um.ModulePath)), - MobileOutline: docParts.MobileOutline, - NumImports: pr.Sprint(unit.NumImports), - ImportedByCount: pr.Sprint(unit.NumImportedBy), - IsPackage: unit.IsPackage(), - ModFileURL: um.SourceInfo.ModuleURL() + "/go.mod", - IsTaggedVersion: isTaggedVersion, - IsStableVersion: isStableVersion, - IsRedistributable: unit.IsRedistributable, + ExpandReadme: expandReadme, + Directories: unitDirectories(append(subdirectories, nestedModules...)), + Licenses: transformLicenseMetadata(unit.Licenses), + CommitTime: absoluteTime(um.CommitTime), + Readme: readme.HTML, + ReadmeOutline: readme.Outline, + ReadmeLinks: readme.Links, + DocLinks: docLinks, + ModuleReadmeLinks: modLinks, + DocOutline: docParts.Outline, + DocBody: docParts.Body, + DocSynopsis: synopsis, + GOOS: goos, + GOARCH: goarch, + BuildContexts: buildContexts, + SourceFiles: files, + RepositoryURL: um.SourceInfo.RepoURL(), + SourceURL: um.SourceInfo.DirectoryURL(internal.Suffix(um.Path, um.ModulePath)), + MobileOutline: docParts.MobileOutline, + NumImports: pr.Sprint(unit.NumImports), + ImportedByPackages: importedByDisplay(pr, "package", unit.NumImportedBy), + ImportedByModules: importedByDisplay(pr, "module", unit.NumImportedByModules), + IsPackage: unit.IsPackage(), + ModFileURL: um.SourceInfo.ModuleURL() + "/go.mod", + IsTaggedVersion: isTaggedVersion, + IsStableVersion: isStableVersion, + IsRedistributable: unit.IsRedistributable, }, nil } +// importedByDisplay returns the text describing n importing modules, +// or "? WORD" if n is negative (meaning the count hasn't been computed). +func importedByDisplay(pr *message.Printer, word string, n int) string { + switch { + case n < 0: + return pr.Sprintf("? %ss", word) + case n == 1: + return pr.Sprintf("1 %s", word) + default: + return pr.Sprintf("%d %ss", n, word) + } +} + func cleanDocumentation(docs []*internal.Documentation) []*internal.Documentation { // If there is more than one row but the first is all/all, ignore the others. // Should never happen; temporary fix until the DB is cleaned up.
diff --git a/internal/frontend/main_test.go b/internal/frontend/main_test.go index 129fe92..2c6e780 100644 --- a/internal/frontend/main_test.go +++ b/internal/frontend/main_test.go
@@ -11,6 +11,8 @@ "golang.org/x/pkgsite/internal" "golang.org/x/pkgsite/internal/testing/fakedatasource" "golang.org/x/pkgsite/internal/testing/sample" + "golang.org/x/text/language" + "golang.org/x/text/message" ) func TestGetImportedByCount(t *testing.T) { @@ -59,3 +61,21 @@ }) } } + +func TestImportedByDisplay(t *testing.T) { + pr := message.NewPrinter(language.English) + for _, test := range []struct { + n int + want string + }{ + {-1, "? modules"}, + {0, "0 modules"}, + {1, "1 module"}, + {2, "2 modules"}, + {15833, "15,833 modules"}, + } { + if got := importedByDisplay(pr, "module", test.n); got != test.want { + t.Errorf("importedByDisplay(%d) = %q, want %q", test.n, got, test.want) + } + } +}
diff --git a/internal/postgres/unit.go b/internal/postgres/unit.go index 4cc9884..dcfa421 100644 --- a/internal/postgres/unit.go +++ b/internal/postgres/unit.go
@@ -549,6 +549,8 @@ docSelect = "d.source," } + // TODO(jba): the imported-by counts should return -1 instead of + // 0 if the unit is not in search_documents. query := fmt.Sprintf(` SELECT r.file_path, @@ -567,7 +569,15 @@ -- Only package_path_id is needed b/c it is the PK for -- search_documents. WHERE package_path_id = $1 - ), 0) AS num_imported_by + ), 0) AS num_imported_by, + COALESCE(( + SELECT CASE + WHEN imported_by_module_count_updated_at IS NULL THEN -1 + ELSE imported_by_module_count + END + FROM search_documents + WHERE package_path_id = $1 + ), 0) AS num_imported_by_modules FROM units u LEFT JOIN readmes r ON r.unit_id = u.id @@ -594,6 +604,7 @@ &doc.Source, &u.NumImports, &u.NumImportedBy, + &u.NumImportedByModules, ) switch err { case sql.ErrNoRows:
diff --git a/internal/testing/pagecheck/pagecheck.go b/internal/testing/pagecheck/pagecheck.go index 08dee43..4f3a87e 100644 --- a/internal/testing/pagecheck/pagecheck.go +++ b/internal/testing/pagecheck/pagecheck.go
@@ -101,7 +101,7 @@ in(`[data-test-id="UnitHeader-importedby"]`, in("a", href(urlPath+"?tab=importedby"), - text(`Imported by: [0-9]+\+?`)))) + text(`Imported by: [0-9,]+ modules, [0-9,]+ packages`)))) if !isPackage { importsDetails = nil }
diff --git a/internal/unit.go b/internal/unit.go index 448ec9d..15e6d83 100644 --- a/internal/unit.go +++ b/internal/unit.go
@@ -49,7 +49,10 @@ LicenseContents []*licenses.License NumImports int NumImportedBy int - Licenses []*licenses.Metadata + // NumImportedByModules is the number of modules that import this unit, + // or -1 if it has not been computed. + NumImportedByModules int + Licenses []*licenses.Metadata // Note: IsRedistributable applies to the unit; // UnitMeta.ModuleInfo.IsRedistributable applies to the module. IsRedistributable bool
diff --git a/internal/worker/fetch_test.go b/internal/worker/fetch_test.go index cd26f1f..2427496 100644 --- a/internal/worker/fetch_test.go +++ b/internal/worker/fetch_test.go
@@ -76,6 +76,7 @@ Filepath: "bar/README", Contents: "Another README file for testing.", }, + NumImportedByModules: -1, } testCases := []struct { @@ -130,6 +131,7 @@ GOOS: "linux", GOARCH: "amd64", }}, + NumImportedByModules: -1, }, wantDoc: []string{"Baz returns the string "baz"."}, }, { @@ -154,8 +156,9 @@ {Types: []string{"0BSD"}, FilePath: "LICENSE"}, {Types: []string{"UNKNOWN"}, FilePath: "unk/LICENSE.md"}, }, - IsRedistributable: false, - NumImports: 2, + IsRedistributable: false, + NumImports: 2, + NumImportedByModules: -1, }, }, { modulePath: buildConstraintsModulePath, @@ -184,6 +187,7 @@ GOOS: "linux", GOARCH: "amd64", }}, + NumImportedByModules: -1, }, wantDoc: []string{"const CacheLinePadSize = 3"}, dontWantDoc: []string{
diff --git a/internal/worker/refetch_test.go b/internal/worker/refetch_test.go index 36a51af..d51b27d 100644 --- a/internal/worker/refetch_test.go +++ b/internal/worker/refetch_test.go
@@ -118,7 +118,8 @@ Licenses: sample.LicenseMetadata(), }, }, - SymbolHistory: map[string]string{"Bar": "v1.0.0"}, + SymbolHistory: map[string]string{"Bar": "v1.0.0"}, + NumImportedByModules: -1, } got, err := testDB.GetUnitMeta(ctx, pkgBar, internal.UnknownModulePath, version) if err != nil {
diff --git a/static/frontend/unit/_header.tmpl b/static/frontend/unit/_header.tmpl index 641678b..dbabc45 100644 --- a/static/frontend/unit/_header.tmpl +++ b/static/frontend/unit/_header.tmpl
@@ -182,10 +182,22 @@ {{define "detail-item-importedby"}} <span class="go-Main-headerDetailItem" data-test-id="UnitHeader-importedby"> - <a href="{{$.URLPath}}?tab=importedby" aria-label="Imported By: {{.Details.ImportedByCount}}" + <a href="{{$.URLPath}}?tab=importedby" aria-label="Imported By {{.Details.ImportedByModules}}, {{.Details.ImportedByPackages}}" data-gtmc="header link" aria-describedby="importedby-description"> - <span class="go-textSubtle">Imported by: </span>{{.Details.ImportedByCount}} + <span class="go-textSubtle">Imported by: </span>{{.Details.ImportedByModules}}, {{.Details.ImportedByPackages}} </a> + <details class="go-Tooltip js-tooltip" data-gtmc="tooltip"> + <summary class="go-textSubtle"> + <img class="go-Icon" role="button" tabindex="0" src="/static/shared/icon/help_gm_grey_24dp.svg" alt="" aria-label="Toggle imported-by help tooltip" height="24" width="24"> + </summary> + <p aria-live="polite" role="tooltip"> + The first number is the count of modules that contain a package that imports this one. + The module count is a better indicator of popularity than the package count. + A module with 100 importing packages is counted only once instead of 100 times, + reflecting the fact that the choice to use the package was likely made only once, + not 100 times. <br/> + A '?' means that the value hasn't been computed yet.</p> + </details> </span> <div class="screen-reader-only" id="importedby-description" hidden> Opens a new window with list of known importers.
diff --git a/tests/screentest/testcases.ci.txt b/tests/screentest/testcases.ci.txt index 3a90bba..6600d43 100644 --- a/tests/screentest/testcases.ci.txt +++ b/tests/screentest/testcases.ci.txt
@@ -3,7 +3,7 @@ block https://codecov.io/* https://travis-ci.com/* https://travis-ci.org/* https://sourcegraph.com/* {{$ready := "[role='treeitem'][aria-selected='true']"}} -{{$hideElements := "const s=document.createElement('style');s.innerHTML='*{transition:none!important;animation:none!important;caret-color:transparent!important} .js-surveyBanner { display: none !important; }';document.head.appendChild(s);document.querySelector(\"[data-test-id='UnitHeader-importedby']\")?.remove();document.querySelector(\"[data-test-id='meta-link-depsdev']\")?.remove();"}} +{{$hideElements := "const s=document.createElement('style');s.innerHTML='*{transition:none!important;animation:none!important;caret-color:transparent!important} .js-surveyBanner { display: none !important; }';document.head.appendChild(s);document.querySelector(\"[data-test-id='UnitHeader-importedby']\")?.remove();document.querySelector(\"[data-test-id='meta-link-depsdev']\")?.remove();window.dispatchEvent(new Event('resize'));"}} {{$scrollTop := "window.scrollTo({top:0});"}} test vuln
diff --git a/tests/screentest/testdata/testcases.ci/vuln-toolchain-module-540x1080.want.png b/tests/screentest/testdata/testcases.ci/vuln-toolchain-module-540x1080.want.png index a011761..1ed1a2e 100644 --- a/tests/screentest/testdata/testcases.ci/vuln-toolchain-module-540x1080.want.png +++ b/tests/screentest/testdata/testcases.ci/vuln-toolchain-module-540x1080.want.png Binary files differ
diff --git a/tests/screentest/testdata/testcases.ci/vuln-toolchain-module.want.png b/tests/screentest/testdata/testcases.ci/vuln-toolchain-module.want.png index 0c0e206..9ae8965 100644 --- a/tests/screentest/testdata/testcases.ci/vuln-toolchain-module.want.png +++ b/tests/screentest/testdata/testcases.ci/vuln-toolchain-module.want.png Binary files differ
