From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp2 ([2001:41d0:8:6d80::]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) by ms0.migadu.com with LMTPS id SNSNBTNyfGAAHgEAgWs5BA (envelope-from ) for ; Sun, 18 Apr 2021 19:53:55 +0200 Received: from aspmx1.migadu.com ([2001:41d0:8:6d80::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp2 with LMTPS id aGQeATNyfGBxagAAB5/wlQ (envelope-from ) for ; Sun, 18 Apr 2021 17:53:55 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 7F663252BF for ; Sun, 18 Apr 2021 19:53:54 +0200 (CEST) Received: from localhost ([::1]:33724 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lYBcH-0007K7-L8 for larch@yhetil.org; Sun, 18 Apr 2021 13:53:53 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:54604) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lYBbu-0007Ij-Rh for guix-devel@gnu.org; Sun, 18 Apr 2021 13:53:30 -0400 Received: from mira.cbaines.net ([212.71.252.8]:42588) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lYBbs-00073F-AZ for guix-devel@gnu.org; Sun, 18 Apr 2021 13:53:30 -0400 Received: from localhost (unknown [IPv6:2a02:8010:68c1:0:8ac0:b4c7:f5c8:7caa]) by mira.cbaines.net (Postfix) with ESMTPSA id 8F02B27BC6C; Sun, 18 Apr 2021 18:53:26 +0100 (BST) Received: from capella (localhost [127.0.0.1]) by localhost (OpenSMTPD) with ESMTP id 12ab0f72; Sun, 18 Apr 2021 17:53:25 +0000 (UTC) References: <79d3d2e5c1386b1e162f1ba8380562720131856d.camel@telenet.be> <87tuo7xljp.fsf@cbaines.net> <87lf9ixz5j.fsf@cbaines.net> User-agent: mu4e 1.4.15; emacs 27.1 From: Christopher Baines To: Canan Talayhan Subject: Re: [Outreachy] - Guix Data Service - Set a more informative page title In-reply-to: Date: Sun, 18 Apr 2021 18:53:23 +0100 Message-ID: <87wnsziing.fsf@cbaines.net> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Received-SPF: pass client-ip=212.71.252.8; envelope-from=mail@cbaines.net; helo=mira.cbaines.net X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: guix-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: guix-devel@gnu.org Errors-To: guix-devel-bounces+larch=yhetil.org@gnu.org Sender: "Guix-devel" X-Migadu-Flow: FLOW_IN ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1618768434; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references:list-id:list-help:list-unsubscribe: list-subscribe:list-post; bh=gWmblRjK//hEFdFi6aGKfUppQ08JHyocmOQ7o9mt3j0=; b=C7WGMS/GE+gk9ttDEGrj2a7Bc76Vt7aT98P2oym2OPa7xNFPeKV+oeAhZ83xtRHvhGZ6O+ DEhY+Q4V4Wb2fMbUd8h0RnYvmiow9caxkUOEvuVDT2hT4mUi1p1SdBZwbDU2tgCa5dGgsS G5qnbEb475kJF8iKx/h2Te/jvF5YOhwZTy5kNxH3ts4h0rUqSL41IsfmxDiuyyJHqjn2KJ 5Qt4Vm5Iv8BHXpEUrrYfLCPzaMPDG9QxxzT+yoABluS2A9K6pvG/v/9aaAXuIpEn4Id1oR VZE9bdrms07KL/qCteqvozCewR76w86F4whRiX0eT/Qa/1R19gfyvXW2aYg+Fg== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1618768434; a=rsa-sha256; cv=none; b=Bdjfv5SnNkX7pAstVgkX//uMTZGl3NGod13l/HtG578Pal2cRoM7pkXzAhxTE4EfeU2M/D 1hLk94s6I5rVkUebYS2lNfw/BJt9dHpwLb3fa1Vy6SD3ERMfk7PNT6LVB6d0bhPM22CXsf 41R0IOPrDZulQkmxewt+YWEZIHAFli1O3LD3gf9riIcVTAlJzntgtBPwR6RORL0OjXYFEu WA27oU8FQNKLPu8Yb0fvsFg212RpD4qMHN0/AKkr9cScSwbLy/Gx2JsXg3rya/UhyUJnYu lLoinmb2AwvsMF5ZqulEolQVzbNnQJ0zIHX6jXphm2yu8fyfnuqW7QCuklCUSg== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=none; dmarc=none; spf=pass (aspmx1.migadu.com: domain of guix-devel-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-devel-bounces@gnu.org X-Migadu-Spam-Score: -4.54 Authentication-Results: aspmx1.migadu.com; dkim=none; dmarc=none; spf=pass (aspmx1.migadu.com: domain of guix-devel-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-devel-bounces@gnu.org X-Migadu-Queue-Id: 7F663252BF X-Spam-Score: -4.54 X-Migadu-Scanner: scn0.migadu.com X-TUID: tRzq2Dok/lFW --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Canan Talayhan writes: > I've updated the patch that contains all the suggestions. I think the pat= ch > is ready to merge. > > One thing that I would like to ask you about the package and package-name > in web/repository/controller.scm. > > When I test the URL below I'm getting this error. ( > https://pastebin.ubuntu.com/p/HdKShmKqH7/) > > - ('GET "repository" repository-id "branch" branch-name "package" > package-name) -> > http://localhost:8765/repository/1/branch/master/package/emacs > > What do you think? BTW it's accessible on the official server. > > - https://data.guix.gnu.org/repository/1/branch/master/package/emacs/ Hmm, this could possibly be due to an issue with the small dump of the database. > Could you please review the patch attached? > I'm very excited to make my first FOSS contribution. :) I've had a look though, and I have some more comments: diff --git a/guix-data-service/web/compare/html.scm b/guix-data-service/w= eb/compare/html.scm index 5b5fe0a..170fb12 100644 --- a/guix-data-service/web/compare/html.scm +++ b/guix-data-service/web/compare/html.scm @@ -96,7 +96,12 @@ (unless invalid-query? (query-parameters->string query-parameters))) + (define page-header "Comparing") + (layout + #:title + (string-append page-header " " (string-take base-commit 8) " and " + (string-take target-commit 8)) #:body `(,(header) (div @@ -107,7 +112,7 @@ (@ (class "col-sm-7")) ,@(if invalid-query? `((h1 "Compare")) - `((h1 "Comparing " + `((h1 ,page-header ," " (a (@ (href ,(string-append "/revision/" base-commit= ))) (samp ,(string-take base-commit 8) "=E2=80=A6")) " and " There's a couple of things here. I'd be tempted not to use a variable for "Comparing", it's not really the page header, as that's more complicated, so I think I'd just use the string in both places. Second thing, the (if invalid-query? bit when constructing the h1 element is important. The query parameters being invalid could mean anything from the form just hasn't been filled in, to the value isn't actually a commit hash, but something else, maybe some HTML/JavaScript that is malicious and shouldn't be included in the page. A similar approach probably needs taking for the title. @@ -419,14 +424,18 @@ '(span (@ (class "text-success glyphicon glyphicon-plus pull-left") (style "font-size: 1.5em; padding-right: 0.4em;")))) + (define page-header "Comparing derivations") + (layout + #:title + page-header #:body `(,(header) (div (@ (class "container")) (div (@ (class "row")) - (h1 ,@(let ((base-commit (assq-ref query-parameters 'base_commit)) + (h1 ,(let ((base-commit (assq-ref query-parameters 'base_commit)) Why's the @ being removed here? @@ -435,7 +444,7 @@ " and " (a (@ (href ,(string-append "/revision/" target-com= mit))) (samp ,(string-take target-commit 8) "=E2=80=A6"= ))) - '("Comparing derivations"))))) + `,page-header)))) The quote then immediate unquote here isn't necessary, also, I think this should stick to being a list containing a string, as the other part of the if returns a list. diff --git a/guix-data-service/web/dumps/html.scm b/guix-data-service/web= /dumps/html.scm index 71e69c8..9645f7c 100644 --- a/guix-data-service/web/dumps/html.scm +++ b/guix-data-service/web/dumps/html.scm @@ -21,8 +21,13 @@ #:use-module (guix-data-service web view html) #:export (view-dumps)) +(define page-header "Database dumps") + (define (view-dumps available-dumps) + (layout + #:title + page-header #:body `(,(header) (div @@ -31,7 +36,7 @@ (@ (class "row")) (div (@ (class "col-sm-12")) - (h1 "Database dumps"))) + (h1 ,page-header))) Like the others, I'd probably put page-header inside the view-dumps procedure. Same goes for other places where it's outside. @@ -267,7 +279,7 @@ (@ (class "col-sm-12")) (a (@ (href "/jobs")) (h3 "Jobs")) - (h1 "Queued jobs (" + (h1 ,page-header"(" ,(length jobs-and-events) ")"))) (div I'd suspect the title here would be "Queued jobs(", I'd also put a space between ,page-header the bit after it in the code. @@ -329,8 +341,13 @@ '()))))) jobs-and-events))))))))) + (define (view-job job-id query-parameters log) + (define page-header (string-append "Job " job-id)) + (layout + #:title + page-header #:body `(,(header) (div Most of the procedures are separated by one line, and I wouldn't change that here. @@ -24,7 +24,11 @@ #:export (view-package)) (define* (view-package name package-version-with-branches) + (define page-header "Package") + (layout + #:title + (string-append page-header " " name) #:body `(,(header) (div @@ -33,7 +37,7 @@ (@ (class "row")) (div (@ (class "col-md-12")) - (h1 "Package: " ,name))) + (h1 ,page-header ," " ,name))) ,@(map (match-lambda ((('version . version) I'm not that fussed about the colon, but I'd probably keep it. I'd try to keep the page-header variable meaningful if you're going to use it though. "Package" is named as the page-header, but it's not what the title is, or the h1 element. They're both better as they include the package name, I'd probably just make the page-header the actual string used in both places. @@ -65,7 +69,11 @@ (define* (view-git-repository git-repository-id label url cgit-url-base branches-with-most-recent-commits) + (define page-header (string-append "Repository " (string-drop url 8))) + (layout + #:title + page-header #:body `(,(header) (div This is really nice, it's good that the pages for different repositories won't have the same title. @@ -197,7 +209,11 @@ branch-name package-name versions-by-revision-range) + (define page-header (string-append branch-name " " package-name)) + (layout + #:title + page-header #:body `(,(header) (div I think you might need some more words for this title to make sense though, even just having package-name " on " branch-name would probably help people work out what's going on. @@ -386,6 +402,8 @@ (map first derivations-by-revision-range)))) (layout + #:title + (string-append package-name " Package derivations") #:body `(,(header) (div @@ -636,6 +654,8 @@ (map first outputs-by-revision-range)))) (layout + #:title + (string-append package-name " Package outputs") #:body `(,(header) (div @@ -849,6 +869,8 @@ valid-systems system-test-history) (layout + #:title + (string-append system-test-name " History") #:body `(,(header) (div Given there's something coming first in these titles, I wouldn't capitalise the H in History or P in Package, since it's not at the start of the title. @@ -48,7 +48,12 @@ (define* (view-revision-news commit-hash query-parameters news-entries) + (define page-header "Revision") + (layout + #:title + (string-append "Channel News Entries " page-header " " + (string-take commit-hash 7)) #:body `(,(header) (div @@ -48,7 +48,12 @@ (define* (view-revision-news commit-hash query-parameters news-entries) + (define page-header "Revision") + (layout + #:title + (string-append "Channel News Entries " page-header " " + (string-take commit-hash 7)) #:body `(,(header) (div Same thing here regarding defining a page-header which isn't actually used as the header. I do think it's useful to include both the Channel News Entries bit and the revision bit in the title, but I'd probably put a separator (like a =2D) in between them. Same goes for the other changes in this file. @@ -2314,7 +2386,11 @@ figure { (define (unknown-revision commit-hash job git-repositories-and-branches jobs-and-events) + (define page-header "Uknown revision") + (layout + #:title + page-header #:body `(,(header) (div A letter was lost here. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmB8chNfFIAAAAAALgAo aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh aW5lcy5uZXQACgkQXiijOwuE9Xdjiw//b8pOoJDrk/f37FzuZwLn9DmPTaiYz9WC f1K5kQZoyxXHRlOqHYxAQyZRR+Ov4S6tGhPUUd1NKZ0F1O6TWLpLnaMAA9UYFRFJ 5QyYBkJ9g7u2FdQetGraZLJGkHQHHnuYpC/BJ0Lz/8eIrw4XMreXTOp8QvcJk767 jH2ddd+KLOITrRQgXrGWCbzzu3Q4Wqrb9mUk8zA4mx0Hjt1B6LUhghN3rtmSyGLV fPmjmbb0vP7CG0/hltz0Jf+BuQ3SIFUIB0Bee+oIjqoGoIi5BAWmbKxhMcBCciZ8 o9afm3JKO/wLZTJPOqw6jjBr+banzthQPBsLDDbAsTvhBnyGzJcuKo4pxLHAid78 rIrCkJxBCniTt7pE2eMFfe79q+FqA/CVPcREPPt179xN2ZzjpWoB2uw8J+rhoaea A1miosObNIGsxKmgmgFGCfAUc+kQolQWMs6rKxHxyc5lK9w1gq4uxB+cGzvl8msx HqWyVUc7lcfRDB1kQrwEulXv+FYYgRauRPPO0Jo9WN78cZunwosZ8ZMeyzmFVLKv /l6yLqQ8klrALn3fDhm5hUEea6qvZLWyil7U2lyVRwIT9UzvmMbAfJmys0RB7Itd qwLGL20ZOTQF8kMjseXyZoIqm/jGs/JRPHci4EvEJVlolif0Xu3j0VGE6q/z325w fI2W3SV+FV4= =12aZ -----END PGP SIGNATURE----- --=-=-=--