Split build details and build list pages

Nemo157 · Joshua Nelson · commit 15b4027f8123 · 2020-12-29T17:00:41.000-05:00
diff --git a/src/web/build_details.rs b/src/web/build_details.rs
@@ -0,0 +1,124 @@
+use crate::{
+    db::Pool,
+    impl_webpage,
+    web::{page::WebPage, MetaData, Nope},
+};
+use chrono::{DateTime, Utc};
+use iron::{IronResult, Request, Response};
+use router::Router;
+use serde::Serialize;
+
+#[derive(Debug, Clone, PartialEq, Eq, Serialize)]
+pub(crate) struct BuildDetails {
+    id: i32,
+    rustc_version: String,
+    docsrs_version: String,
+    build_status: bool,
+    build_time: DateTime<Utc>,
+    output: String,
+}
+
+#[derive(Debug, Clone, PartialEq, Eq, Serialize)]
+struct BuildDetailsPage {
+    metadata: MetaData,
+    build_details: BuildDetails,
+}
+
+impl_webpage! {
+    BuildDetailsPage = "crate/build_details.html",
+}
+
+pub fn build_details_handler(req: &mut Request) -> IronResult<Response> {
+    let router = extension!(req, Router);
+    let name = cexpect!(req, router.find("name"));
+    let version = cexpect!(req, router.find("version"));
+    let id: i32 = ctry!(req, cexpect!(req, router.find("id")).parse());
+
+    let mut conn = extension!(req, Pool).get()?;
+
+    let row = ctry!(
+        req,
+        conn.query_opt(
+            "SELECT
+                builds.rustc_version,
+                builds.cratesfyi_version,
+                builds.build_status,
+                builds.build_time,
+                builds.output
+             FROM builds
+             INNER JOIN releases ON releases.id = builds.rid
+             INNER JOIN crates ON releases.crate_id = crates.id
+             WHERE builds.id = $1 AND crates.name = $2 AND releases.version = $3",
+            &[&id, &name, &version]
+        )
+    );
+
+    let build_details = if let Some(row) = row {
+        BuildDetails {
+            id,
+            rustc_version: row.get("rustc_version"),
+            docsrs_version: row.get("cratesfyi_version"),
+            build_status: row.get("build_status"),
+            build_time: row.get("build_time"),
+            output: row.get("output"),
+        }
+    } else {
+        return Err(Nope::BuildNotFound.into());
+    };
+
+    BuildDetailsPage {
+        metadata: cexpect!(req, MetaData::from_crate(&mut conn, &name, &version)),
+        build_details,
+    }
+    .into_response(req)
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::test::{wrapper, FakeBuild};
+    use kuchiki::traits::TendrilSink;
+
+    #[test]
+    fn build_logs() {
+        wrapper(|env| {
+            env.fake_release()
+                .name("foo")
+                .version("0.1.0")
+                .builds(vec![FakeBuild::default().build_log("A build log")])
+                .create()?;
+
+            let page = kuchiki::parse_html().one(
+                env.frontend()
+                    .get("/crate/foo/0.1.0/builds")
+                    .send()?
+                    .text()?,
+            );
+
+            let node = page.select("ul > li a.release").unwrap().next().unwrap();
+            let attrs = node.attributes.borrow();
+            let url = attrs.get("href").unwrap();
+
+            let page = kuchiki::parse_html().one(env.frontend().get(url).send()?.text()?);
+
+            let log = page.select("pre").unwrap().next().unwrap().text_contents();
+
+            assert!(log.contains("A build log"));
+
+            Ok(())
+        });
+    }
+
+    #[test]
+    fn non_existing_build() {
+        wrapper(|env| {
+            env.fake_release().name("foo").version("0.1.0").create()?;
+
+            let res = env.frontend().get("/crate/foo/0.1.0/builds/42").send()?;
+            assert_eq!(res.status(), 404);
+            // TODO: blocked on https://github.com/rust-lang/docs.rs/issues/55
+            // assert!(res.text()?.contains("no such build"));
+
+            Ok(())
+        });
+    }
+}
diff --git a/src/web/builds.rs b/src/web/builds.rs
@@ -21,14 +21,12 @@ pub(crate) struct Build {
     docsrs_version: String,
     build_status: bool,
     build_time: DateTime<Utc>,
-    output: Option<String>,
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Serialize)]
 struct BuildsPage {
     metadata: MetaData,
     builds: Vec<Build>,
-    build_details: Option<Build>,
     limits: Limits,
 }
 
@@ -40,7 +38,6 @@ pub fn build_list_handler(req: &mut Request) -> IronResult<Response> {
     let router = extension!(req, Router);
     let name = cexpect!(req, router.find("name"));
     let version = cexpect!(req, router.find("version"));
-    let req_build_id: i32 = router.find("id").unwrap_or("0").parse().unwrap_or(0);
 
     let mut conn = extension!(req, Pool).get()?;
     let limits = ctry!(req, Limits::for_crate(&mut conn, name));
@@ -57,8 +54,7 @@ pub fn build_list_handler(req: &mut Request) -> IronResult<Response> {
                 builds.rustc_version,
                 builds.cratesfyi_version,
                 builds.build_status,
-                builds.build_time,
-                builds.output
+                builds.build_time
              FROM builds
              INNER JOIN releases ON releases.id = builds.rid
              INNER JOIN crates ON releases.crate_id = crates.id
@@ -68,36 +64,18 @@ pub fn build_list_handler(req: &mut Request) -> IronResult<Response> {
         )
     );
 
-    let mut build_details = None;
-    // FIXME: getting builds.output may cause performance issues when release have tons of builds
-    let mut builds = query
+    let builds: Vec<_> = query
         .into_iter()
-        .map(|row| {
-            let id: i32 = row.get("id");
-
-            let build = Build {
-                id,
-                rustc_version: row.get("rustc_version"),
-                docsrs_version: row.get("cratesfyi_version"),
-                build_status: row.get("build_status"),
-                build_time: row.get("build_time"),
-                output: row.get("output"),
-            };
-
-            if id == req_build_id {
-                build_details = Some(build.clone());
-            }
-
-            build
+        .map(|row| Build {
+            id: row.get("id"),
+            rustc_version: row.get("rustc_version"),
+            docsrs_version: row.get("cratesfyi_version"),
+            build_status: row.get("build_status"),
+            build_time: row.get("build_time"),
         })
-        .collect::<Vec<Build>>();
+        .collect();
 
     if req.url.path().join("/").ends_with(".json") {
-        // Remove build output from build list for json output
-        for build in builds.iter_mut() {
-            build.output = None;
-        }
-
         let mut resp = Response::with((status::Ok, serde_json::to_string(&builds).unwrap()));
         resp.headers.set(ContentType::json());
         resp.headers.set(Expires(HttpDate(time::now())));
@@ -113,7 +91,6 @@ pub fn build_list_handler(req: &mut Request) -> IronResult<Response> {
         BuildsPage {
             metadata: cexpect!(req, MetaData::from_crate(&mut conn, &name, &version)),
             builds,
-            build_details,
             limits,
         }
         .into_response(req)
@@ -299,34 +276,4 @@ mod tests {
             Ok(())
         });
     }
-
-    #[test]
-    fn build_logs() {
-        wrapper(|env| {
-            env.fake_release()
-                .name("foo")
-                .version("0.1.0")
-                .builds(vec![FakeBuild::default().build_log("A build log")])
-                .create()?;
-
-            let page = kuchiki::parse_html().one(
-                env.frontend()
-                    .get("/crate/foo/0.1.0/builds")
-                    .send()?
-                    .text()?,
-            );
-
-            let node = page.select("ul > li a.release").unwrap().next().unwrap();
-            let attrs = node.attributes.borrow();
-            let url = attrs.get("href").unwrap();
-
-            let page = kuchiki::parse_html().one(env.frontend().get(url).send()?.text()?);
-
-            let log = page.select("pre").unwrap().next().unwrap().text_contents();
-
-            assert!(log.contains("A build log"));
-
-            Ok(())
-        });
-    }
 }
diff --git a/src/web/error.rs b/src/web/error.rs
@@ -9,6 +9,7 @@ use std::{error::Error, fmt};
 #[derive(Debug, Copy, Clone)]
 pub enum Nope {
     ResourceNotFound,
+    BuildNotFound,
     CrateNotFound,
     VersionNotFound,
     NoResults,
@@ -19,6 +20,7 @@ impl fmt::Display for Nope {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         f.write_str(match *self {
             Nope::ResourceNotFound => "Requested resource not found",
+            Nope::BuildNotFound => "Requested build not found",
             Nope::CrateNotFound => "Requested crate not found",
             Nope::VersionNotFound => "Requested crate does not have specified version",
             Nope::NoResults => "Search yielded no results",
@@ -35,6 +37,7 @@ impl From<Nope> for IronError {
 
         let status = match err {
             Nope::ResourceNotFound
+            | Nope::BuildNotFound
             | Nope::CrateNotFound
             | Nope::VersionNotFound
             | Nope::NoResults => status::NotFound,
@@ -59,6 +62,13 @@ impl Handler for Nope {
                 .into_response(req)
             }
 
+            Nope::BuildNotFound => ErrorPage {
+                title: "The requested build does not exist",
+                message: Some("no such build".into()),
+                status: Status::NotFound,
+            }
+            .into_response(req),
+
             Nope::CrateNotFound => {
                 // user tried to navigate to a crate that doesn't exist
                 // TODO: Display the attempted crate and a link to a search for said crate
diff --git a/src/web/mod.rs b/src/web/mod.rs
@@ -77,6 +77,7 @@ macro_rules! extension {
     }};
 }
 
+mod build_details;
 mod builds;
 mod crate_details;
 mod error;
diff --git a/src/web/routes.rs b/src/web/routes.rs
@@ -89,7 +89,7 @@ pub(super) fn build_routes() -> Routes {
     );
     routes.internal_page(
         "/crate/:name/:version/builds/:id",
-        super::builds::build_list_handler,
+        super::build_details::build_details_handler,
     );
     routes.internal_page(
         "/crate/:name/:version/features",
diff --git a/templates/crate/build_details.html b/templates/crate/build_details.html
@@ -0,0 +1,42 @@
+{%- extends "base.html" -%}
+{%- import "header/package_navigation.html" as navigation -%}
+
+{%- block title -%}
+    {{ macros::doc_title(name=metadata.name, version=metadata.version) }}
+{%- endblock title -%}
+
+{%- block topbar -%}
+  {%- set latest_version = "" -%}
+  {%- set latest_path = "" -%}
+  {%- set target = "" -%}
+  {%- set inner_path = metadata.target_name ~ "/index.html" -%}
+  {%- set is_latest_version = true -%}
+  {%- set is_prerelease = false -%}
+  {%- include "rustdoc/topbar.html" -%}
+{%- endblock topbar -%}
+
+{%- block header -%}
+    {{ navigation::package_navigation(metadata=metadata, active_tab="builds") }}
+{%- endblock header -%}
+
+{%- block body -%}
+    <div class="container">
+        <div class="recent-releases-container">
+            <div class="release">
+                <strong>Build #{{ build_details.id }} {{ build_details.build_time | date(format="%+") }}</strong>
+            </div>
+
+            {%- filter dedent -%}
+                <pre>
+                    # rustc version
+                    {{ build_details.rustc_version }}
+                    # docs.rs version
+                    {{ build_details.docsrs_version }}
+
+                    # build log
+                    {{ build_details.output }}
+                </pre>
+            {%- endfilter -%}
+        </div>
+    </div>
+{%- endblock body -%}
diff --git a/templates/crate/builds.html b/templates/crate/builds.html
@@ -22,26 +22,6 @@
 {%- block body -%}
     <div class="container">
         <div class="recent-releases-container">
-            {# If there is a build log then show it #}
-            {# TODO: When viewing a build log, show a back button or a hide button #}
-            {%- if build_details -%}
-                <div class="release">
-                    <strong>Build #{{ build_details.id }} {{ build_details.build_time | date(format="%+") }}</strong>
-                </div>
-
-                {%- filter dedent -%}
-                    <pre>
-                        # rustc version
-                        {{ build_details.rustc_version }}
-                        # docs.rs version
-                        {{ build_details.docsrs_version }}
-
-                        # build log
-                        {{ build_details.output }}
-                    </pre>
-                {%- endfilter -%}
-            {%- endif -%}
-
             <div class="release">
                 <strong>Builds</strong>
             </div>

Original file line number	Diff line number	Diff line change
`@@ -77,6 +77,7 @@ macro_rules! extension {`
`77`	`77`	`}};`
`78`	`78`	`}`
`79`	`79`
	`80`	`+mod build_details;`
`80`	`81`	`mod builds;`
`81`	`82`	`mod crate_details;`
`82`	`83`	`mod error;`