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

[Tatiana Sholokhova <tanja201396@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3141 bytes --]

Hello,

Thank you for your reviews!

I've just fixed codestyle issues and replaced HTML5 preamble with XHTML.


respond-static-file: We should not second-guess the VFS layer.  Checking
> for ".." gives an illusion of security when in fact random things could be
> mounted and also the VFS could have funny syntax for who knows what on the
> filesystem.  Let's rather have a static list of permissible names and allow
> those (whitelist).  That's the intention of the check anyway, right?
>

I adopted the static file serving procedure from code shared by Ricardo.

https://github.com/BIMSBbioinfo/rcas-web/blob/22a114a0f281845117ed0ab105267f132fc525e4/rcas/web/render.scm#L68

I am considering the following possible implementation of a whitelist. We
can create association list with allowed file names and their mime types (to
replace file-mime-type list). What do you think about it?

Also, in light of an ever-changing backing store (cuirass continusly
> evaluates things), the way you are doing pagination is not the correct way
> to do it because the data set will scroll underneath you and you will miss
> items (or see duplicate items) as an user.  Also, it's slow and the DBMS
> can't reuse anything because you are cutting it off and offseting it over
> and over again and the transaction isolation level is too low for the DBMS
> to be able to do anything about it[1].


I understand how to fix the pagination on the database level. However, it
is not completely clear to me how to incorporate the solution in the
web-request handling. Now we have the only one parameter of the request
which is related to pagination. It is the page number. Should we add more
parameters to the request in order to request evaluations with specific
timestamps and IDs? Or there is some other way of doing that? I have
checked the Hydra pagination request structure. It uses the same form of
the request path for pagination buttons that we have now:
"?page=<page-id>".

It is good practise to make small commits, one for every set of
> logically connected changes.  This makes the review simpler and it makes
> it easier to merge some parts while leaving others for later.


Okay, I will follow this commit strategy.

Best regards,
Tatiana



2018-06-13 0:52 GMT+03:00 Ricardo Wurmus <rekado@elephly.net>:

>
> Hi Tatiana,
>
> I have only little to add to Danny’s comments.
>
> > db-get-builds looks fine and we could merge this change to master.
> > But you have other changes in the same commit, so we can't directly
> > cherry-pick it.  (not so bad, but somewhat cumbersome now)
>
> It is good practise to make small commits, one for every set of
> logically connected changes.  This makes the review simpler and it makes
> it easier to merge some parts while leaving others for later.
>
> As you work on the changes that Danny’s comments imply, please take the
> opportunity to group related changes and commit only those together.  It
> is fine and desirable to have many independent small commits.
>
> Thanks again for your excellent work!
>
> --
> Ricardo
>
>

[-- Attachment #2: Type: text/html, Size: 6926 bytes --]