From 5706a2452ef15c4f3b394e15516def16f4d00f1c Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 2 Apr 2025 15:48:09 +0000 Subject: [PATCH] fix(ui): display user-friendly message for range error (#7420) - Instead of displaying 'RangeError: Range' display 'x must be a number between $MIN and $MAX' when the validation fails for a range error check. - Resolves forgejo/forgejo#3510 - Added integration testing. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7420 Reviewed-by: 0ko <0ko@noreply.codeberg.org> Co-authored-by: Gusted Co-committed-by: Gusted --- modules/web/middleware/binding.go | 8 ++ options/locale_next/locale_en-US.json | 1 + services/forms/repo_form.go | 4 +- tests/integration/issue_tracked_time_test.go | 77 ++++++++++++++++++++ 4 files changed, 88 insertions(+), 2 deletions(-) create mode 100644 tests/integration/issue_tracked_time_test.go diff --git a/modules/web/middleware/binding.go b/modules/web/middleware/binding.go index 9083e9b485..123eb29015 100644 --- a/modules/web/middleware/binding.go +++ b/modules/web/middleware/binding.go @@ -79,6 +79,11 @@ func GetInclude(field reflect.StructField) string { return getRuleBody(field, "Include(") } +func GetRange(field reflect.StructField) (string, string) { + min, max, _ := strings.Cut(getRuleBody(field, "Range("), ",") + return min, max +} + // Validate populates the data with validation error (if any). func Validate(errs binding.Errors, data map[string]any, f any, l translation.Locale) binding.Errors { if errs.Len() == 0 { @@ -131,6 +136,9 @@ func Validate(errs binding.Errors, data map[string]any, f any, l translation.Loc data["ErrorMsg"] = trName + l.TrString("form.url_error", errs[0].Message) case binding.ERR_INCLUDE: data["ErrorMsg"] = trName + l.TrString("form.include_error", GetInclude(field)) + case binding.ERR_RANGE: + min, max := GetRange(field) + data["ErrorMsg"] = trName + l.TrString("alert.range_error", l.PrettyNumber(min), l.PrettyNumber(max)) case validation.ErrGlobPattern: data["ErrorMsg"] = trName + l.TrString("form.glob_pattern_error", errs[0].Message) case validation.ErrRegexPattern: diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json index 7f9b81a77b..d8636613cc 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -19,6 +19,7 @@ "themes.names.forgejo-dark": "Forgejo dark", "error.not_found.title": "Page not found", "alert.asset_load_failed": "Failed to load asset files from {path}. Please make sure the asset files can be accessed.", + "alert.range_error": " must be a number between %[1]s and %[2]s.", "settings.adopt": "Adopt", "install.invalid_lfs_path": "Unable to create the LFS root at the specified path: %[1]s", "install.lfs_jwt_secret_failed": "Unable to generate a LFS JWT secret: %[1]s" diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index c39c6a7b36..4a46c9cc5f 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -725,8 +725,8 @@ func (f *DeleteRepoFileForm) Validate(req *http.Request, errs binding.Errors) bi // AddTimeManuallyForm form that adds spent time manually. type AddTimeManuallyForm struct { - Hours int `binding:"Range(0,1000)"` - Minutes int `binding:"Range(0,1000)"` + Hours int `binding:"Range(0,1000)" locale:"repo.issues.add_time_hours"` + Minutes int `binding:"Range(0,1000)" locale:"repo.issues.add_time_minutes"` } // Validate validates the fields diff --git a/tests/integration/issue_tracked_time_test.go b/tests/integration/issue_tracked_time_test.go new file mode 100644 index 0000000000..f2e0df5a84 --- /dev/null +++ b/tests/integration/issue_tracked_time_test.go @@ -0,0 +1,77 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package integration + +import ( + "net/http" + "testing" + + issues_model "forgejo.org/models/issues" + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + forgejo_context "forgejo.org/services/context" + "forgejo.org/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestIssueAddTimeManually(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + session := loginUser(t, user2.Name) + issue2 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) + require.NoError(t, issue2.LoadRepo(t.Context())) + + t.Run("No time", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + session.MakeRequest(t, NewRequestWithValues(t, "POST", issue2.Link()+"/times/add", map[string]string{ + "_csrf": GetCSRF(t, session, issue2.Link()), + }), http.StatusSeeOther) + + flashCookie := session.GetCookie(forgejo_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Contains(t, flashCookie.Value, "error%3DNo%2Btime%2Bwas%2Bentered.") + }) + + t.Run("Invalid hours", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + session.MakeRequest(t, NewRequestWithValues(t, "POST", issue2.Link()+"/times/add", map[string]string{ + "_csrf": GetCSRF(t, session, issue2.Link()), + "hours": "-1", + }), http.StatusSeeOther) + + flashCookie := session.GetCookie(forgejo_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Contains(t, flashCookie.Value, "error%3DHours%2Bmust%2Bbe%2Ba%2Bnumber%2Bbetween%2B0%2Band%2B1%252C000.") + }) + + t.Run("Invalid minutes", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + session.MakeRequest(t, NewRequestWithValues(t, "POST", issue2.Link()+"/times/add", map[string]string{ + "_csrf": GetCSRF(t, session, issue2.Link()), + "minutes": "-1", + }), http.StatusSeeOther) + + flashCookie := session.GetCookie(forgejo_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Contains(t, flashCookie.Value, "error%3DMinutes%2Bmust%2Bbe%2Ba%2Bnumber%2Bbetween%2B0%2Band%2B1%252C000.") + }) + + t.Run("Normal", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + session.MakeRequest(t, NewRequestWithValues(t, "POST", issue2.Link()+"/times/add", map[string]string{ + "_csrf": GetCSRF(t, session, issue2.Link()), + "hours": "3", + "minutes": "14", + }), http.StatusSeeOther) + + unittest.AssertExistsIf(t, true, &issues_model.TrackedTime{IssueID: issue2.ID, Time: 11640, UserID: user2.ID}) + }) +}