Canan Talayhan <canan.t.talayhan@gmail.com> writes:

> I've updated the patch that contains all the suggestions. I think the patch
> 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/web/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) "…"))
                       " 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-commit)))
                           (samp ,(string-take target-commit 8) "…")))
  -                   '("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
-) 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.