Check for valid renamed usernames #2077
Conversation
|
LGTM |
tboerger
added
the
lgtm/need 1
|
May be add integration test? |
|
LGTM otherwise @lafriks 's comment. |
tboerger
added
lgtm/done
|
Awsome 👍 |
integrations/user_test.go
Outdated
| htmlDoc := NewHTMLParser(t, resp.Body) | ||
| req = NewRequestWithValues(t, "POST", "/user/settings", map[string]string{ | ||
| "_csrf": htmlDoc.GetCSRF(), | ||
| "name": "%2f*", // not valid |
There was a problem hiding this comment.
Could you add more tests here? The original issue lists a bunch of cases that we should guard against 🤔
tests := []string{ ... }
for test := range tests {
t.Logf("testing %q", test)
req = NewRequest...("POST" ...)
[...]
models.AssertNot...
}( ☝️ Table-driven testing 🙂 https://blog.golang.org/subtests )
There was a problem hiding this comment.
It would be also nice to have valid user name change test
There was a problem hiding this comment.
Added all invalid cases from #2066, and a reserved username test, and a valid username test.
| assert.Contains(t, | ||
| htmlDoc.doc.Find(".ui.negative.message").Text(), | ||
| i18n.Tr("en", "user.newName_reserved"), | ||
| ) |
There was a problem hiding this comment.
now you have 3 identical loops, have them be a common function like so testUpdateUser(t *testing.T, username, email string, code http.StatusCode) 🙂
There was a problem hiding this comment.
@bkcsoft They're not identical. A lot of the setup code (e.g. getting the CSRF token) is shared, but once you make the POST /user/settings request, they have little in common (different status codes, may or may not have to follow a redirect, may or may not check for whether rename succeeded, have to check for different error messages).
The boilerplate setup code is also shared with tests from other files, so IMO the best solution is to add helper functions for things like getting a CSRF token.
|
Please also add test case for username with space so that #2061 can be also closed |
|
@lafriks Done |
|
need @bkcsoft approval. |
modules/auth/user_form.go
Outdated
| @@ -100,7 +100,7 @@ func (f *SignInForm) Validate(ctx *macaron.Context, errs binding.Errors) binding | |||
|
|
|||
| // UpdateProfileForm form for updating profile | |||
| type UpdateProfileForm struct { | |||
| Name string `binding:"OmitEmpty;MaxSize(35)"` | |||
| Name string `binding:"OmitEmpty;AlphaDashDot;MaxSize(35)"` | |||
There was a problem hiding this comment.
I think it is also needs to be Required
|
@bkcsoft need your approval |
Fixes #2066 and #2061.