Skip to content

Conversation

vbrown608
Copy link
Collaborator

I tried a couple things but wound up just putting some very simple strings on the backend and leaving the full descriptions on the frontend.

@vbrown608 vbrown608 requested a review from sydneyli January 29, 2019 18:50
Copy link
Contributor

@sydneyli sydneyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a quick glance at the code! Overall looks great :) I haven't tested it out yet-- setting that up right now!

api.go Outdated
StatusCode int `json:"status_code"`
Message string `json:"message"`
Response interface{} `json:"response"`
TemplatePath string `json:"omit"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want json:"-" here?

api.go Outdated
StatusCode: http.StatusInternalServerError,
Message: err.Error(),
TemplatePath: "views/domain.html.tmpl",
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you decide which APIResponses to transform into HTML templates? If this results in an error, what will template.Execute return?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On api.go:86, we only attempt to render HTML if the template is set. Otherwise we respond to HTML requests as usual (with JSON or plain text). Non-200 responses return plain text, so noscript users would see a single line containing response.Message.

STARTTLS: "Support for STARTTLS",
Version: "Secure version of TLS",
Certificate: "Valid certificate",
MTASTS: "Implementation of MTA-STS",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Inbound MTA-STS support"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh woops - I forgot to say that the strings and template text are basically dummy text I added - feel free to directly push edits. If it's just the ones in this review I'm happy to add them, but if you wanna tweak more things feel free :)

// Text descriptions of checks that can be run
var checkNames = map[string]string{
Connectivity: "Server connectivity",
STARTTLS: "Support for STARTTLS",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Support for inbound STARTTLS

Copy link
Contributor

@sydneyli sydneyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last round of review after testing it :)

{{ $r.Description }}: <strong>{{ $r.StatusText }}</strong>
<ul>
{{ range $_, $message := $r.Messages }}
<li>{{ $message }}<li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<li>{{ $message }}<li>
<li>{{ $message }}</li>

<p>{{ .Response.Data.Message }}</p>

<h2>STARTTLS Everywhere Policy List</h2>
{{ $r := index .Response.Data.ExtraResults "policylist" }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the status of this is bad, can we put in a link to /add-domain ?

{{ end }}
{{ if eq .Response.State "added" }}
Your domain is already on the STARTTLS policy list. Thanks for helping to make e-mail more secure.
{{ end }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a lot of these would never actually show up-- we have some short-circuits in api.Queue that don't actually return the result, like here and here

}
return APIResponse{StatusCode: http.StatusOK, Response: domainData}
// domainData.State = Unvalidated
// or queued?
Copy link
Collaborator Author

@vbrown608 vbrown608 Jan 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into this question - resolved in follow-up PR (and I removed the cryptic comment there).

Copy link
Contributor

@sydneyli sydneyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last thing-- otherwise LGTM!

{{ end }}
</ul>
{{ if eq $r.Status 1 }}
<a href="{{ .BaseURL }}/add-domain">Add your email domain the STARTTLS Everywhere Policy List</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also got a "can't evaluate field BaseURL in type *checker.Result" error here. Any idea what might have caused it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, also GitHub isn't showing my other comment here. I think it should be $r.Status 2, not 1 since we ask them to sign up if it's a failure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh with (line 18) changes the value of . in the template, so now the top-level variable is $r. It does a lot more than I thought it did!

@vbrown608
Copy link
Collaborator Author

I pushed a fuller test of the scan template with this (rendering a more realistic scan object). I think at some point it could be nice to add a fn to the checker package that generates typical scan objects. I'm often editing them in lots of places in tests when we change their structure.

@vbrown608 vbrown608 force-pushed the backend-render-html branch from bae82de to dcf6daa Compare January 31, 2019 20:11
@vbrown608 vbrown608 force-pushed the backend-render-html branch from dcf6daa to 3d6ff0f Compare January 31, 2019 20:23
@vbrown608
Copy link
Collaborator Author

Github is such a snitch now

Copy link
Contributor

@sydneyli sydneyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@sydneyli sydneyli merged commit dcb2290 into master Feb 1, 2019
@sydneyli sydneyli deleted the backend-render-html branch February 1, 2019 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants