Add tests for users, fix template NPE, fix user HTTP return codes, show validations #1493
Conversation
googlebot
added
the
cla: yes
|
/assign @whaught |
sethvargo
force-pushed
the
sethvargo/user_tests
branch
from
a6354f2
to
63ee1fe
Compare
| @@ -55,39 +55,41 @@ | |||
| </div> | |||
| {{end}} | |||
|
|
|||
| <nav class="nav nav-tabs navbar-expand-md navbar-light bg-light"> | |||
There was a problem hiding this comment.
This is just indented (?w=1). It can really only fail in tests, but it's good to guard against it anyway in the template.
| @@ -84,7 +83,7 @@ func (c *Controller) HandleUserStats() http.Handler { | |||
| c.h.RenderJSON(w, http.StatusOK, stats) | |||
| return | |||
| default: | |||
| controller.InternalError(w, r, c.h, fmt.Errorf("unknown path %q", pth)) | |||
| controller.BadRequest(w, r, c.h) | |||
sethvargo
force-pushed
the
sethvargo/user_tests
branch
from
63ee1fe
to
b822da1
Compare
sethvargo
force-pushed
the
sethvargo/user_tests
branch
from
b822da1
to
c267762
Compare
| @@ -51,17 +59,42 @@ func (c *Controller) HandleExport() http.Handler { | |||
| return | |||
| } | |||
|
|
|||
| w.Header().Add("Content-Disposition", "") | |||
| w.Header().Add("Content-Type", "text/CSV") | |||
| filename := fmt.Sprintf("%s-users.csv", time.Now().Format(project.RFC3339Squish)) | |||
There was a problem hiding this comment.
Yay nice touch.
I did this to the bulk-upload files in javascript: https://github.com/google/exposure-notifications-verification-server/pull/1469/files
Now I'm wondering if I should flip it with the date first (for string sortability)
There was a problem hiding this comment.
I like the date being first
There was a problem hiding this comment.
I agree! I'm saying can you change where I did it wrong for consistency (or I'll slip that in to my next PR)
| if err := w.Write([]string{ | ||
| m.User.Name, | ||
| m.User.Email, | ||
| m.CreatedAt.Format(project.RFC3339Date), |
There was a problem hiding this comment.
Wondering: why add CreatedAt - presumably when we import these we want a fresh CreatedAt?
There was a problem hiding this comment.
I think we would discard that value on import (we don't consume it). This is for if someone wants to know when someone was added to a realm.
sethvargo
changed the title
sethvargo
changed the title
sethvargo
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sethvargo, whaught The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Still needs tests for stats and import, but I'll do that in another PR since this is already large.
Release Note