From 9f05374c9a5eec90ae99ba70d938e1c1dd8471f3 Mon Sep 17 00:00:00 2001 From: Dave Verwer Date: Mon, 20 May 2024 17:55:33 +0100 Subject: [PATCH 1/4] =?UTF-8?q?Link=20to=20the=20=E2=80=9Ccurrent=E2=80=9D?= =?UTF-8?q?=20docs=20from=20the=20package=20page.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...API+PackageController+GetRoute+Model.swift | 8 +- .../PackageController+PackageResult.swift | 12 +++ .../PackageController/PackageShow+View.swift | 2 +- ...ackageController+GetRoute+ModelTests.swift | 4 +- Tests/AppTests/PackageResultTests.swift | 84 +++++++++++++++++++ Tests/AppTests/WebpageSnapshotTests.swift | 2 +- 6 files changed, 104 insertions(+), 8 deletions(-) diff --git a/Sources/App/Controllers/API/API+PackageController+GetRoute+Model.swift b/Sources/App/Controllers/API/API+PackageController+GetRoute+Model.swift index 211e5fb17..c79e71686 100644 --- a/Sources/App/Controllers/API/API+PackageController+GetRoute+Model.swift +++ b/Sources/App/Controllers/API/API+PackageController+GetRoute+Model.swift @@ -45,7 +45,7 @@ extension API.PackageController.GetRoute { var isArchived: Bool var hasBinaryTargets: Bool var homepageUrl: String? - var documentationTarget: DocumentationTarget? = nil + var currentDocumentationTarget: DocumentationTarget? = nil var weightedKeywords: [WeightedKeyword] var releaseReferences: [App.Version.Kind: App.Reference] var fundingLinks: [FundingLink] @@ -75,7 +75,7 @@ extension API.PackageController.GetRoute { isArchived: Bool, hasBinaryTargets: Bool = false, homepageUrl: String? = nil, - documentationTarget: DocumentationTarget? = nil, + currentDocumentationTarget: DocumentationTarget? = nil, weightedKeywords: [WeightedKeyword] = [], defaultBranchReference: App.Reference, releaseReference: App.Reference?, @@ -109,7 +109,7 @@ extension API.PackageController.GetRoute { self.isArchived = isArchived self.hasBinaryTargets = hasBinaryTargets self.homepageUrl = homepageUrl - self.documentationTarget = documentationTarget + self.currentDocumentationTarget = currentDocumentationTarget self.weightedKeywords = weightedKeywords self.releaseReferences = { var refs = [App.Version.Kind.defaultBranch: defaultBranchReference] @@ -171,7 +171,7 @@ extension API.PackageController.GetRoute { isArchived: repository.isArchived, hasBinaryTargets: result.defaultBranchVersion.hasBinaryTargets, homepageUrl: repository.homepageUrl, - documentationTarget: result.canonicalDocumentationTarget(), + currentDocumentationTarget: result.currentDocumentationTarget(), weightedKeywords: weightedKeywords, defaultBranchReference: result.defaultBranchVersion.reference, releaseReference: result.releaseVersion?.reference, diff --git a/Sources/App/Controllers/PackageController+PackageResult.swift b/Sources/App/Controllers/PackageController+PackageResult.swift index 509195579..a93fd40d3 100644 --- a/Sources/App/Controllers/PackageController+PackageResult.swift +++ b/Sources/App/Controllers/PackageController+PackageResult.swift @@ -90,4 +90,16 @@ extension PackageController.PackageResult { preReleaseVersion?.model ].canonicalDocumentationTarget() } + + func currentDocumentationTarget() -> DocumentationTarget? { + guard let target = canonicalDocumentationTarget() + else { return nil } + + switch target { + case .external: + return target + case .internal(_, let archive): + return .internal(docVersion: .current(referencing: nil), archive: archive) + } + } } diff --git a/Sources/App/Views/PackageController/PackageShow+View.swift b/Sources/App/Views/PackageController/PackageShow+View.swift index 37f4a4d84..ef6ec338a 100644 --- a/Sources/App/Views/PackageController/PackageShow+View.swift +++ b/Sources/App/Views/PackageController/PackageShow+View.swift @@ -280,7 +280,7 @@ extension PackageShow { ) ) }, - .unwrap(model.documentationTarget) { target in + .unwrap(model.currentDocumentationTarget) { target in .li( .a( .href(SiteURL.relativeURL(owner: model.repositoryOwner, diff --git a/Tests/AppTests/API+PackageController+GetRoute+ModelTests.swift b/Tests/AppTests/API+PackageController+GetRoute+ModelTests.swift index bff7618a4..c9c31bca5 100644 --- a/Tests/AppTests/API+PackageController+GetRoute+ModelTests.swift +++ b/Tests/AppTests/API+PackageController+GetRoute+ModelTests.swift @@ -89,7 +89,7 @@ class API_PackageController_GetRoute_ModelTests: SnapshotTestCase { swift6Readiness: nil)) // validate - XCTAssertEqual(model.documentationTarget, .internal(docVersion: .reference("main"), archive: "archive1")) + XCTAssertEqual(model.currentDocumentationTarget, .internal(docVersion: .reference("main"), archive: "archive1")) } func test_init_external_documentation() async throws { @@ -115,7 +115,7 @@ class API_PackageController_GetRoute_ModelTests: SnapshotTestCase { swift6Readiness: nil)) // validate - XCTAssertEqual(model.documentationTarget, .external(url: "https://example.com/package/documentation")) + XCTAssertEqual(model.currentDocumentationTarget, .external(url: "https://example.com/package/documentation")) } func test_gitHubOwnerUrl() throws { diff --git a/Tests/AppTests/PackageResultTests.swift b/Tests/AppTests/PackageResultTests.swift index 699c1e610..eda608183 100644 --- a/Tests/AppTests/PackageResultTests.swift +++ b/Tests/AppTests/PackageResultTests.swift @@ -285,4 +285,88 @@ class PackageResultTests: AppTestCase { XCTAssertEqual(res.canonicalDocumentationTarget(), nil) } } + + func test_currentDocumentationTarget() async throws { + do { + // Test package with branch docs and stable version docs + let pkg = try savePackage(on: app.db, "1".url) + try await Repository(package: pkg, + defaultBranch: "main", + forks: 42, + license: .mit, + name: "bar1", + owner: "foo", + stars: 17, + summary: "summary").save(on: app.db) + try await App.Version(package: pkg, + docArchives: [.init(name: "archive1", title: "Archive 1")], + latest: .defaultBranch, + reference: .branch("main")).save(on: app.db) + try await App.Version(package: pkg, + // Note the name change on the default branch. The current should point to this archive. + docArchives: [.init(name: "archive2", title: "Archive 2")], + latest: .release, + reference: .tag(1, 2, 3)).save(on: app.db) + } + + do { + // Test package with only branch docs hosted externally. + let pkg = try savePackage(on: app.db, "2".url) + try await Repository(package: pkg, + defaultBranch: "main", + forks: 42, + license: .mit, + name: "bar2", + owner: "foo", + stars: 17, + summary: "summary").save(on: app.db) + try await App.Version(package: pkg, + latest: .defaultBranch, + reference: .branch("main"), + spiManifest: .init(externalLinks: .init(documentation: "https://example.com"))).save(on: app.db) + } + + do { + // Test package with no documentation + let pkg = try savePackage(on: app.db, "3".url) + try await Repository(package: pkg, + defaultBranch: "main", + forks: 42, + license: .mit, + name: "bar3", + owner: "foo", + stars: 17, + summary: "summary").save(on: app.db) + try await App.Version(package: pkg, + latest: .defaultBranch, + reference: .branch("main")).save(on: app.db) + } + + do { + // Testing for internally hosted documentation pointing at the "current". + let res = try await PackageController.PackageResult.query(on: app.db, owner: "foo", repository: "bar1") + let currentTarget = try XCTUnwrap(res.currentDocumentationTarget()) + + // Validaton + XCTAssertEqual(currentTarget, .internal(docVersion: .current(referencing: nil), archive: "archive2")) + } + + do { + // Testing for `.external` case pass-through. + let res = try await PackageController.PackageResult.query(on: app.db, owner: "foo", repository: "bar2") + let currentTarget = try XCTUnwrap(res.currentDocumentationTarget()) + + // Validaton + XCTAssertEqual(currentTarget, .external(url: "https://example.com")) + } + + do { + // Testing for "no documentation" pass-through. + let res = try await PackageController.PackageResult.query(on: app.db, owner: "foo", repository: "bar3") + + // Validaton + XCTAssertNil(res.currentDocumentationTarget()) + } + } + } diff --git a/Tests/AppTests/WebpageSnapshotTests.swift b/Tests/AppTests/WebpageSnapshotTests.swift index 846e6e9b3..e2e5b6da4 100644 --- a/Tests/AppTests/WebpageSnapshotTests.swift +++ b/Tests/AppTests/WebpageSnapshotTests.swift @@ -191,7 +191,7 @@ class WebpageSnapshotTests: SnapshotTestCase { func test_PackageShowView_with_documentation_link() throws { var model = API.PackageController.GetRoute.Model.mock - model.documentationTarget = .internal(docVersion: .reference("main"), archive: "archive") + model.currentDocumentationTarget = .internal(docVersion: .reference("main"), archive: "archive") let page = { PackageShow.View(path: "", model: model, packageSchema: .mock).document() } assertSnapshot(of: page, as: .html) From 4525342827c79e402ca8185ffe949246394aa892 Mon Sep 17 00:00:00 2001 From: Dave Verwer Date: Mon, 20 May 2024 18:13:05 +0100 Subject: [PATCH 2/4] Fixed up other tests. --- .../API+PackageController+GetRoute+ModelTests.swift | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/Tests/AppTests/API+PackageController+GetRoute+ModelTests.swift b/Tests/AppTests/API+PackageController+GetRoute+ModelTests.swift index c9c31bca5..185858b61 100644 --- a/Tests/AppTests/API+PackageController+GetRoute+ModelTests.swift +++ b/Tests/AppTests/API+PackageController+GetRoute+ModelTests.swift @@ -89,18 +89,14 @@ class API_PackageController_GetRoute_ModelTests: SnapshotTestCase { swift6Readiness: nil)) // validate - XCTAssertEqual(model.currentDocumentationTarget, .internal(docVersion: .reference("main"), archive: "archive1")) + XCTAssertEqual(model.currentDocumentationTarget, .internal(docVersion: .current(referencing: nil), archive: "archive1")) } func test_init_external_documentation() async throws { let pkg = try await savePackage(on: app.db, "1".url) try await Repository(package: pkg, name: "bar", owner: "foo").save(on: app.db) let version = try App.Version(package: pkg, latest: .defaultBranch, packageName: nil, reference: .branch("main")) - version.spiManifest = try .init(yml: """ - version: 1 - external_links: - documentation: "https://example.com/package/documentation" - """) + version.spiManifest = .init(externalLinks: .init(documentation: "https://example.com/package/documentation")) try await version.save(on: app.db) let packageResult = try await PackageResult.query(on: app.db, owner: "foo", repository: "bar") From e03d948253f6e6a68473fbf0868b1e704cbabc11 Mon Sep 17 00:00:00 2001 From: Dave Verwer Date: Tue, 21 May 2024 07:04:43 +0100 Subject: [PATCH 3/4] Clarified a comment. --- Sources/App/Views/DocumentationPageProcessor.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Sources/App/Views/DocumentationPageProcessor.swift b/Sources/App/Views/DocumentationPageProcessor.swift index d0b161e15..379b6b9e9 100644 --- a/Sources/App/Views/DocumentationPageProcessor.swift +++ b/Sources/App/Views/DocumentationPageProcessor.swift @@ -83,9 +83,9 @@ struct DocumentationPageProcessor { try document.head()?.append(self.javascriptLinks) if let canonicalUrl = self.canonicalUrl { try document.head()?.append( - // We should not use `url` here as some of the DocC JavaScript lowercases - // both the `og:url` and `twitter:url` properties, if present. It is better - // to have no `og:url` and `twitter:url` properties than incorrect ones. + // We should not use Plot's `url` helper here as some of the DocC JavaScript + // lowercases both the `og:url` and `twitter:url` properties, if present. It's + // better to have no `og:url` and `twitter:url` properties than incorrect ones. Plot.Node.link( .rel(.canonical), .href(canonicalUrl) From d805cd333e33d9e935b2cfccbb0b7b2d78b43d9f Mon Sep 17 00:00:00 2001 From: Dave Verwer Date: Mon, 27 May 2024 13:17:23 +0100 Subject: [PATCH 4/4] Refactored out the block setting `canonicalUrl`. --- .../PackageController+routes.swift | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/Sources/App/Controllers/PackageController+routes.swift b/Sources/App/Controllers/PackageController+routes.swift index eb476959b..875c576e8 100644 --- a/Sources/App/Controllers/PackageController+routes.swift +++ b/Sources/App/Controllers/PackageController+routes.swift @@ -122,19 +122,11 @@ enum PackageController { .init(archive: $0, isCurrent: $0.name == route.archive) } - let canonicalUrl: String? = { - guard let canonicalOwner = documentationMetadata.owner, - let canonicalRepository = documentationMetadata.repository, - let canonicalTarget = documentationMetadata.canonicalTarget - else { return nil } - - return Self.canonicalDocumentationUrl(from: "\(req.url)", - owner: canonicalOwner, - repository: canonicalRepository, - docVersion: route.docVersion, - toTarget: canonicalTarget) - }() - + let canonicalUrl = Self.canonicalDocumentationUrl(from: "\(req.url)", + owner: documentationMetadata.owner, + repository: documentationMetadata.repository, + docVersion: route.docVersion, + toTarget: documentationMetadata.canonicalTarget) // Try and parse the page and add our header, but fall back to the unprocessed page if it fails. guard let body = awsResponse.body, @@ -466,10 +458,12 @@ extension PackageController { extension PackageController { static func canonicalDocumentationUrl(from url: String, - owner: String, - repository: String, + owner: String?, + repository: String?, docVersion: DocVersion, - toTarget target: DocumentationTarget) -> String? { + toTarget target: DocumentationTarget?) -> String? { + guard let owner, let repository, let target else { return nil } + // It's important to use `docVersion.reference` here to make sure we match with true reference urls and not ~ let urlPrefix = "/\(owner)/\(repository)/\(docVersion.reference.pathEncoded)/" if case let .internal(canonicalReference, _) = target,