(Preview) Fix no same naming convention between template and form #22846

yp05327 · 2023-02-10T02:43:27Z

(This PR is still on working)

The variable's names in form data will be changed to snakecase in func AssignForm if form tag is not defined.
And this makes contributors write various code which follows different naming convention of variables.
Then low readability code caused the bug I mentioned in #22833
(I personally think so)

So I made a change to AssignForm:
The variable's names in form data will be changed into field's name.
Then we just let the variable's names in context data as same as the field's name which is simply defined in each form struct.
(easy to check whether the variable's name is currect, otherwise the vaule of therm will be changed when RenderWithErr or some unknow bugs will be occured?)

This is a big change, because we need to check all functions uses AssignForm.
So I made this preview PR, if it is worth to spend time on this work, I will continue to work on this.

wxiaoguang · 2023-02-10T03:32:53Z

modules/web/middleware/binding.go

@@ -42,7 +42,7 @@ func AssignForm(form interface{}, data map[string]interface{}) {
 		if fieldName == "-" {
 			continue
 		} else if len(fieldName) == 0 {
-			fieldName = util.ToSnakeCase(field.Name)
+			fieldName = field.Name


Maybe the snake_case is done by purpose:

snake_case: for auto filled form, matching the form element's name

PascalCase: for non-form variables

?

If the form-variables and non-form-variables are mixed together, it would be very difficult to understand what a variable is used for, and may cause uncontrollable overwriting?

@wxiaoguang
Thanks for your review.
You are right, it is difficult to distinguish and may cause uncontrollable overwriting.

How about the following changes.

For non-form variables:

// in func ctx.Data["HelloData"] // in template {{.HelloData}}

For auto filled form:

// in func ctx.Data["Form"] = forms.FormStruct { Variable: vaule, // .... } // in template {{.Form.Variable}}

I am not sure and I haven't thought about it carefully.

Maybe the Technical Oversight Committee could help.

Personally, I like the idea {{.Form.Variable}}, it's more readable and go-style. And there will be a clear way to check if it's an error page to repost with entered form: {{if .Form}} /* keep the values in .Form */ {{end}}

I see the point of the snake_case but using the {{.Form}} would prevent us from having to do that.

So there is one potential issue with the .Form object if you're using go code.

I'm not certain if there's any code that does this - but I could imagine some code that doesn't have the type of the Form object but would have been able to update the values in the ctx.Data["some_field"] which by convention was a string.

In some pages, there are several forms in one page. Need to consider a new solution to solve this problem.

In some pages, there are several forms in one page. Need to consider a new solution to solve this problem.

a64ba42 is an example to fix this problem.

This reverts commit 49ea88b.

This reverts commit 1195121.

This reverts commit f5cd8a9.

wolfogre · 2023-02-10T12:50:47Z

Since this is a big decision, I think I should call the TOC.

@lunny @techknowlogick @6543 @zeripath @jolheiser

modules/web/middleware/binding.go

zeripath · 2023-02-10T19:56:29Z

This is a cautious 👍 from me. The snake_case form mapping is weird and feels fragile.

My caution is because when you put stuff in ctx.Data it becomes an interface{} and whilst that field values were also interface{} the number of likely types was very small.

I think there are a few places where we reuse the same or similar templates with different forms but I'm not sure if we have go code that would adjust/use the snake_cases fields. If we do that - then the .Form variant could be quite difficult to do.

As an aside... in order to make migration safer and easier I would recommend leaving in the current auto snake_case mapping but also just stick the Form in {{.Form}} and leave a note to remove it soon.

lunny · 2023-02-19T08:18:17Z

Since this is a big decision, I think I should call the TOC.

@lunny @techknowlogick @6543 @zeripath @jolheiser

An advantage of the new Form in Golang template is maybe it could reduce the potential risk to have the same key in the response context Data map. But once we decide it, I think there are many templates that need to be changed.

delvh

Well… Everything seems to work.
Although I haven't understood yet how the other contents are auto-injected, when all you do is swap out the maps you use…

yp05327 · 2023-02-19T11:01:58Z

@delvh
This pr dosen't work well now.
As @zeripath said here #22846 (comment), there are some potential problems need to be fixed.
I'm working on #22865, and after finished this, I will work on this PR.

…te-and-form

codecov-commenter · 2023-03-30T02:46:20Z

Codecov Report

Merging #22846 (02f156e) into main (f521e88) will decrease coverage by 0.15%.
The diff coverage is 29.09%.

❗ Current head 02f156e differs from pull request most recent head 7311821. Consider uploading reports for the commit 7311821 to get more accurate results

@@            Coverage Diff             @@
##             main   #22846      +/-   ##
==========================================
- Coverage   47.14%   47.00%   -0.15%     
==========================================
  Files        1149     1158       +9     
  Lines      151446   153166    +1720     
==========================================
+ Hits        71397    71989     +592     
- Misses      71611    72676    +1065     
- Partials     8438     8501      +63

Impacted Files	Coverage Δ
cmd/dump.go	`0.66% <0.00%> (-0.01%)`	⬇️
cmd/mailer.go	`0.00% <0.00%> (ø)`
cmd/manager.go	`0.00% <0.00%> (ø)`
cmd/manager_logging.go	`0.00% <0.00%> (ø)`
cmd/migrate_storage.go	`5.76% <0.00%> (-0.12%)`	⬇️
cmd/restore_repo.go	`0.00% <0.00%> (ø)`
cmd/web.go	`0.00% <0.00%> (ø)`
models/actions/run.go	`1.64% <0.00%> (-0.08%)`	⬇️
models/actions/runner.go	`1.44% <ø> (ø)`
models/packages/package.go	`45.45% <0.00%> (-1.13%)`	⬇️
... and 66 more

... and 70 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

…te-and-form

wxiaoguang · 2023-04-20T03:41:00Z

I have some clearer ideas about this problem now.

The root problem is that it's very difficult to re-fill the forms with correct values, the case is:

Open a "GET" page, fill with default model values.
Do "POST", if error occurs, fill the form with posted values.

There is no clear / general approach for various cases/pages. I am pretty sure because I have made various different web frameworks by Java/PHP/Go, the "form-post-refill" problem has no clear solution.

And even worse, there are a lot of misuses in old form-binding code (aka m.Post("", web.Bind())), some mechanisms are just broken.

Then, how to make the UI/UX friendly to end users if something wrong happens? I think the answer is AJAX.

Open a "GET" page, fill with default model values.
Do "POST" with AJAX, if error occurs, just display the error message, the user is still on the current page with all values.

And the AJAX Form can handle more cases, eg: internal server error, broken network, etc.

yp05327 · 2023-04-21T01:24:48Z

If we use AJAX, the change will be much bigger.
But maybe it is a good change as we can combine the codes of handling POST requests from frontend and API request easily.
I found that sometimes we changed the logic of frontend but forgot to apply the changes to API, (e.g. #23776)
and different logics between frontend and API (e.g. #23934).

wxiaoguang · 2023-04-21T01:33:07Z

IMO it's not that big, the most work is rewriting the XxxPost handlers, and these handlers could be simplified a lot because they do not need to handle various errors & re-rendering , just responding the JSON is enough.

yp05327 · 2023-04-21T05:15:03Z

Is it possible to reuse API code?
Or just let all frontend Post request to be an internal api call/direct api call in the backend?

For example:
Client: Post /xxx/xxx/xxx
↓
internal request: Post /api/v1/xxx/xxx/xxx

Or
Client: Post /api/v1/xxx/xxx/xxx

wxiaoguang · 2023-04-21T06:31:19Z

Is it possible to reuse API code?

No, the API handlers and Web handlers shouldn't be mixed together.

API handlers focuses on stability, they do not change too much.

Web handlers focuses on flexibility, and are only used by Web UI (so no swagger document)

Client: Post /xxx/xxx/xxx -> internal request: Post /api/v1/xxx/xxx/xxx OR Client: Post /api/v1/xxx/xxx/xxx

Neither. The AJAX forms could just post to the original web page "post" handlers. This could be a simple change: from <form action> to JS form submit.

wxiaoguang · 2023-06-14T15:18:09Z

Now there is a framework level support for making AJAX requests.

Here is a sample for how to do it: Use fetch to send requests to create issues/comments #25258

Only 2 steps:

Add "form-fetch-action" to the form
Use "JSONRedirect" and "JSONError" in backend code

That's all.

I think this issue can be closed. Move more forms to the AJAX world.

yp05327 added 3 commits February 10, 2023 11:07

remove SnakeCase

f5cd8a9

fix repo create

1195121

fix org create

49ea88b

wxiaoguang reviewed Feb 10, 2023

View reviewed changes

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 10, 2023

wolfogre mentioned this pull request Feb 10, 2023

Holding the values in form when the post request is failed with error #22833

Open

yp05327 added 7 commits February 10, 2023 19:25

Revert "fix org create"

6dd02cc

This reverts commit 49ea88b.

Revert "fix repo create"

add2d3c

This reverts commit 1195121.

Revert "remove SnakeCase"

8e4d3c3

This reverts commit f5cd8a9.

rewrite AssignForm

2404441

fix org create

7c3043a

fix repo create

9cd9ceb

fix org name in org create

da3b9ac

zeripath reviewed Feb 10, 2023

View reviewed changes

modules/web/middleware/binding.go Show resolved Hide resolved

delvh approved these changes Feb 19, 2023

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 19, 2023

yp05327 marked this pull request as draft March 3, 2023 06:31

yp05327 added 3 commits March 30, 2023 10:31

Merge branch 'main' into fix-no-same-naming-convention-between-templa…

d36aa52

…te-and-form

fix AdminCreateUserForm

02f156e

fix

aa1584f

yp05327 added 4 commits March 30, 2023 04:58

fix visibility in new user page

bf1af92

fix AdminEditUserForm

a64ba42

fix AuthenticationForm

ad6a191

fix name conflict

a205529

yp05327 and others added 5 commits March 31, 2023 07:27

fix

d0524a5

Merge branch 'main' into fix-no-same-naming-convention-between-templa…

4df9ec0

…te-and-form

fix

6a2262d

fix UpdateOrgSettingForm

d268c40

Merge branch 'main' into fix-no-same-naming-convention-between-templa…

7311821

…te-and-form

wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Jun 14, 2023

yp05327 closed this Jun 15, 2023

yp05327 removed the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Jun 15, 2023

go-gitea locked as resolved and limited conversation to collaborators Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Preview) Fix no same naming convention between template and form #22846

(Preview) Fix no same naming convention between template and form #22846

yp05327 commented Feb 10, 2023

wxiaoguang Feb 10, 2023

yp05327 Feb 10, 2023

wxiaoguang Feb 10, 2023

wolfogre Feb 10, 2023 •

edited

Loading

zeripath Feb 10, 2023

zeripath Feb 10, 2023

yp05327 Mar 30, 2023

yp05327 Mar 30, 2023 •

edited

Loading

wolfogre commented Feb 10, 2023

zeripath commented Feb 10, 2023

lunny commented Feb 19, 2023

delvh left a comment

yp05327 commented Feb 19, 2023

codecov-commenter commented Mar 30, 2023 •

edited

Loading

wxiaoguang commented Apr 20, 2023 •

edited

Loading

yp05327 commented Apr 21, 2023

wxiaoguang commented Apr 21, 2023

yp05327 commented Apr 21, 2023

wxiaoguang commented Apr 21, 2023 •

edited

Loading

wxiaoguang commented Jun 14, 2023 •

edited

Loading

(Preview) Fix no same naming convention between template and form #22846

(Preview) Fix no same naming convention between template and form #22846

Conversation

yp05327 commented Feb 10, 2023

wxiaoguang Feb 10, 2023

Choose a reason for hiding this comment

yp05327 Feb 10, 2023

Choose a reason for hiding this comment

wxiaoguang Feb 10, 2023

Choose a reason for hiding this comment

wolfogre Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

zeripath Feb 10, 2023

Choose a reason for hiding this comment

zeripath Feb 10, 2023

Choose a reason for hiding this comment

yp05327 Mar 30, 2023

Choose a reason for hiding this comment

yp05327 Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

wolfogre commented Feb 10, 2023

zeripath commented Feb 10, 2023

lunny commented Feb 19, 2023

delvh left a comment

Choose a reason for hiding this comment

yp05327 commented Feb 19, 2023

codecov-commenter commented Mar 30, 2023 • edited Loading

Codecov Report

wxiaoguang commented Apr 20, 2023 • edited Loading

yp05327 commented Apr 21, 2023

wxiaoguang commented Apr 21, 2023

yp05327 commented Apr 21, 2023

wxiaoguang commented Apr 21, 2023 • edited Loading

wxiaoguang commented Jun 14, 2023 • edited Loading

wolfogre Feb 10, 2023 •

edited

Loading

yp05327 Mar 30, 2023 •

edited

Loading

codecov-commenter commented Mar 30, 2023 •

edited

Loading

wxiaoguang commented Apr 20, 2023 •

edited

Loading

wxiaoguang commented Apr 21, 2023 •

edited

Loading

wxiaoguang commented Jun 14, 2023 •

edited

Loading