It seems after testing lots of pages this one escaped me since I only tested the working case. Please find the quick fix in the link below. https://pastebin.ubuntu.com/p/s7tWyPHZ8F/ I'm looking forward to making another contribution. Could you please review it as soon as possible? Thanks, Canan Talayhan On Thu, Apr 22, 2021 at 10:47 PM Christopher Baines wrote: > > > Canan Talayhan writes: > > > I've missed it unintentionally. I've not touched the "@" sign this time. :) > > > >>> + (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 " > > > > I think I misunderstood that comment. > >>> 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. > > > > Now, I've fixed it this way. I hope this version is good. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > *(layout #:title (string-append "Comparing " (string-take base-commit > > 8) " and " (string-take target-commit 8)) #:body `(,(header) > > (div (@ (class "container")) (div (@ (class "row")) > > (div (@ (class "col-sm-7")) ,@(if invalid-query? > > `((h1 "Compare")) `((h1 "Comparing " (a > > (@ (href ,(string-append "/revision/" base-commit))) > > (samp ,(string-take base-commit 8) "…")) " and "* > > > > I think your email came through a bit garbled, anyway, I think there's > still an issue with the title here. > > > On Mon, Apr 19, 2021 at 10:16 PM Christopher Baines > > wrote: > > > >> > >> Canan Talayhan writes: > >> > >> > 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? > >> > >> I've had another look, see my comments below. > >> > >> > On Sun, Apr 18, 2021 at 8:53 PM Christopher Baines > >> wrote: > >> >> > >> >> > >> >> Canan Talayhan 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. > >> > >> This stuff above still looks the same to me, although part of my > >> analysis was wrong, as the type of an invalid parameter is a record, so > >> the page just breaks if the parameters are invalid (which I guses is > >> better than what I was describing). > >> > >> Anyway, I think this still needs fixing. > > These are my relevant comments from before, you should be able to see > the error yourself if you go to /compare locally, does this page work > for you?