Re: GSoC: Adding a web interface similar to the Hydra web interface

[Ricardo Wurmus]

Hi Tatiana,

> I have committed the first HTML template page (with table of specifications
> stored in the database) to web-interface branch. Can you please review it?

That’s great!  Congratulations on your first commit!

I’ll make a couple of extra comments on style and conventions because
this is your first commit.  You aren’t expected to remember all of these
conventions right away.

> commit a4fe6dd0d0c82c84a810d3368dd60fea3aa1b2b0 (HEAD -> web-interface, 
> origin/web-interface)
> Author: TSholokhova <address@hidden>
> Date:   Wed May 23 16:37:23 2018 +0300
>
>     basic html templates

Please remember to make expressive commit messages.  We normally use a
format similar to the ChangeLog convention, which consists of a one-line
summary that is usually a sentence, and a listing of all changes.

In this case it would be something like:

--8<---------------cut here---------------start------------->8---
Add basic HTML templates.

* src/cuirass/templates.scm: New file.
* Makefile.am (dist_pkgmodule_DATA): Add it.
* src/cuirass/http.scm (url-handler): Add handler for “status”
endpoint.
--8<---------------cut here---------------end--------------->8---


> diff --git a/src/cuirass/http.scm b/src/cuirass/http.scm
> index e911b9b..f5e3ac1 100644
> --- a/src/cuirass/http.scm
> +++ b/src/cuirass/http.scm
> @@ -1,3 +1,4 @@
> +
>  ;;;; http.scm -- HTTP API
>  ;;; Copyright © 2016 Mathieu Lirzin <address@hidden>
>  ;;; Copyright © 2017 Mathieu Othacehe <address@hidden>

Please make sure to add a copyright line for yourself to the bottom of
the copyright block.  Also try to avoid pure whitespace changes like the
addition of an empty line at the top of the file.

> @@ -135,6 +139,12 @@ Hydra format."
>       #:body
>       (object->json-string
>        `((error . ,message)))))
> +
> +  (define (respond-html body)
> +    (respond '((content-type . (text/html)))
> +             #:body (lambda (port)
> +                      (sxml->xml body port)
> +                      )))

Please don’t leave closing parentheses on a line by itself; they feel
lonely and prefer to be on the previous line.

> @@ -223,6 +233,11 @@ Hydra format."
>                                        ,@params
>                                        (order status+submission-time)))))
>             (respond-json-with-error 500 "Parameter not defined!"))))
> +    (("status")
> +     (respond-html (templatize
> +                 "Status"
> +                 (specifications-table
> +                  (with-critical-section db-channel (db) 
> (db-get-specifications db))))))

Here and in other places you used tabs, but we’re using spaces for
indentation in all source files.  Please configure your editor to use
spaces instead of tabs.

I feel that the “url-handler” procedure is already very large, so it may
be a good idea to break out the handler to a separate procedure instead
of having the details inline.  This is not a problem yet, but it may be
good to keep in mind in case you need to grow this handler in the
future.  As long as it stays this small it’s fine to keep it there.

> diff --git a/src/cuirass/templates.scm b/src/cuirass/templates.scm
> new file mode 100644
> index 0000000..ff63469
> --- /dev/null
> +++ b/src/cuirass/templates.scm
> @@ -0,0 +1,32 @@
> +(define-module (cuirass templates)
> +  #:export (templatize
> +            specifications-table))
> +
> +

Please add the usual copyright header to the top of this module.

> +(define (templatize title body)
> +  `(html
> +    ,(head title)
> +    (body ,body)))

Please also add a docstring to every toplevel definition to explain what
the procedure is supposed to do.  “templatize” is quite a fancy name for
what I’d call “html-page” :)

> +
> +(define (head title)
> +  `(head
> +    (meta (@ (charset "utf-8")))
> +    (title ,title)))

This could become part of “templatize” instead.  It’s generally good to
have small independent procedures, but in this case I don’t see us ever
using “head” without “templatize”.

> +(define (specifications-table specs)
> +  `(table
> +    (@ (class "table-fill"))
> +    (thead
> +     (tr
> +      (th (@ (class "text-left")) Name)
> +      (th (@ (class "text-left")) Branch)))
> +    (tbody
> +     (@ (class "table-fill"))
> +     ,@(map
> +        (lambda (spec)
> +          `(tr
> +            (td ,(assq-ref spec #:name))
> +            (td ,(assq-ref spec #:branch))))
> +        specs))))

Please also add a docstring to this procedure.  What is the result of
this procedure if “specs” is empty?  Should that case be covered to
communicate this to the viewer?

> I am a bit confused about the database structure. As far as I understand,
> there are project_name (project) and branch_name (jobset) properties, but
> project_name is a primary key, so a project can't have several branches?

I share your confusion.  Maybe Ludovic or Mathieu can shed some more
light on this.

It is true that “repo_name” (not project_name) in the Specifications
table is the primary key and it is used as the only foreign key on other
tables.  On the other hand, “repo_name” is really just an arbitrary
name.  You could have “Guix (master branch)” as the value for repo_name
with “branch” as “master”, and you could have another specification with
the repo_name “Guix (next)” with “branch” as “core-updates”.

I feel it would be nicer to have a composite primary key consisting of
both the repo_name and the branch, but since the “branch” is an optional
field maybe that’s not so easy.

But I don’t really know if there are other reasons for doing it this
way.

> I'm working on static file serving but I don't know how to set the path of
> the static file directory. Now I just wrote my absolute path to the
> style.css file. So, I have two questions. 1. Where should I place the
> static files? 2. How can I execute getcwd function (as you do it in
> rcas-web/rcas/config.scm.in)? I tried to add something like
>
> (define-public %current-directory
> `(,(getcwd)))

Do you really need the current directory, or could you use %datadir
instead?  Note that the value of %datadir is provided at configuration
time, i.e. when the “configure” script is run.  “configure” generates
“config.scm” from “config.scm.in” by substituting all placeholders
(strings wrapped in address@hidden@”) with the configured values.

If you want to avoid the need to reconfigure and install cuirass every
time, you could respond to variables set in the “pre-inst-env” script.
For example, you can see the following in src/cuirass/database.scm:

--8<---------------cut here---------------start------------->8---
(define %package-schema-file
  ;; Define to the database schema file of this package.
  (make-parameter (string-append (or (getenv "CUIRASS_DATADIR")
                                     (string-append %datadir "/" %package))
                                 "/schema.sql")))
--8<---------------cut here---------------end--------------->8---

CUIRASS_DATADIR is specified by “pre-inst-env”, so when Cuirass is run
this way, it looks up the schema.sql file from the source directory.  If
Cuirass has been installed, however, and is not run with the
“pre-inst-env” script it looks up the file in the %datadir.

Does this help?

--
Ricardo