Re: [Outreachy] - Guix Data Service - Set a more informative page title

[Canan Talayhan]

Thanks for your quick response.

>Why's the @ being removed here?
It interprets like an HTML code when I use the page-header like
`,page-header, so I removed it. According to your comment, I reverted
to the original version.

" 'GET repository..." which includes package/package-name in the URL
has not the best titles since I couldn't test them because of the
error that I've mentioned.
I'm open to suggestions.

Could you please re-review the patch that contains all the
modifications you've mentioned in the previous message?

On Sun, Apr 18, 2021 at 8:53 PM Christopher Baines <mail@cbaines.net> wrote:
>
>
> 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.

0001-Set-a-more-informative-page-title-for-any-page.patch
Description: Text Data