Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 22 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f5cd8a9
remove SnakeCase
yp05327 Feb 10, 2023
1195121
fix repo create
yp05327 Feb 10, 2023
49ea88b
fix org create
yp05327 Feb 10, 2023
6dd02cc
Revert "fix org create"
yp05327 Feb 10, 2023
add2d3c
Revert "fix repo create"
yp05327 Feb 10, 2023
8e4d3c3
Revert "remove SnakeCase"
yp05327 Feb 10, 2023
2404441
rewrite AssignForm
yp05327 Feb 10, 2023
7c3043a
fix org create
yp05327 Feb 10, 2023
9cd9ceb
fix repo create
yp05327 Feb 10, 2023
da3b9ac
fix org name in org create
yp05327 Feb 10, 2023
d36aa52
Merge branch 'main' into fix-no-same-naming-convention-between-templa…
yp05327 Mar 30, 2023
02f156e
fix AdminCreateUserForm
yp05327 Mar 30, 2023
aa1584f
fix
yp05327 Mar 30, 2023
bf1af92
fix visibility in new user page
yp05327 Mar 30, 2023
a64ba42
fix AdminEditUserForm
yp05327 Mar 30, 2023
ad6a191
fix AuthenticationForm
yp05327 Mar 31, 2023
a205529
fix name conflict
yp05327 Mar 31, 2023
d0524a5
fix
yp05327 Mar 31, 2023
4df9ec0
Merge branch 'main' into fix-no-same-naming-convention-between-templa…
yp05327 Apr 6, 2023
6a2262d
fix
yp05327 Apr 6, 2023
d268c40
fix UpdateOrgSettingForm
yp05327 Apr 6, 2023
7311821
Merge branch 'main' into fix-no-same-naming-convention-between-templa…
6543 Apr 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/web/middleware/binding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@wxiaoguang wxiaoguang Feb 10, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@yp05327 yp05327 Feb 10, 2023

Choose a reason for hiding this comment

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

@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}}

Copy link
Contributor

@wxiaoguang wxiaoguang Feb 10, 2023

Choose a reason for hiding this comment

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

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

Maybe the Technical Oversight Committee could help.

Copy link
Member

@wolfogre wolfogre Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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}}

Copy link
Contributor

@zeripath zeripath Feb 10, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@zeripath zeripath Feb 10, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@yp05327 yp05327 Mar 30, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@yp05327 yp05327 Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}

data[fieldName] = val.Field(i).Interface()
Expand Down
4 changes: 3 additions & 1 deletion routers/web/org/org.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ const (
// Create render the page for create organization
func Create(ctx *context.Context) {
ctx.Data["Title"] = ctx.Tr("new_org")
ctx.Data["DefaultOrgVisibilityMode"] = setting.Service.DefaultOrgVisibilityMode
ctx.Data["Visibility"] = setting.Service.DefaultOrgVisibilityMode
ctx.Data["RepoAdminChangeTeamAccess"] = true

if !ctx.Doer.CanCreateOrganization() {
ctx.ServerError("Not allowed", errors.New(ctx.Tr("org.form.create_org_not_allowed")))
return
Expand Down
17 changes: 9 additions & 8 deletions routers/web/repo/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,28 +131,29 @@ func Create(ctx *context.Context) {
ctx.Data["Title"] = ctx.Tr("new_repo")

// Give default value for template to render.
ctx.Data["Gitignores"] = repo_module.Gitignores
ctx.Data["DefaultGitignores"] = repo_module.Gitignores
ctx.Data["LabelTemplates"] = repo_module.LabelTemplates
ctx.Data["Licenses"] = repo_module.Licenses
ctx.Data["Readmes"] = repo_module.Readmes
ctx.Data["readme"] = "Default"
ctx.Data["private"] = getRepoPrivate(ctx)
ctx.Data["Readme"] = "Default"
ctx.Data["Private"] = getRepoPrivate(ctx)
ctx.Data["IsForcedPrivate"] = setting.Repository.ForcePrivate
ctx.Data["default_branch"] = setting.Repository.DefaultBranch
ctx.Data["DefaultBranch"] = setting.Repository.DefaultBranch
ctx.Data["TrustModel"] = "Default"

ctxUser := checkContextUser(ctx, ctx.FormInt64("org"))
if ctx.Written() {
return
}
ctx.Data["ContextUser"] = ctxUser

ctx.Data["repo_template_name"] = ctx.Tr("repo.template_select")
ctx.Data["RepoTemplateName"] = ctx.Tr("repo.template_select")
templateID := ctx.FormInt64("template_id")
if templateID > 0 {
templateRepo, err := repo_model.GetRepositoryByID(ctx, templateID)
if err == nil && access_model.CheckRepoUnitUser(ctx, templateRepo, ctxUser, unit.TypeCode) {
ctx.Data["repo_template"] = templateID
ctx.Data["repo_template_name"] = templateRepo.Name
ctx.Data["RepoTemplate"] = templateID
ctx.Data["RepoTemplateName"] = templateRepo.Name
}
}

Expand Down Expand Up @@ -199,7 +200,7 @@ func CreatePost(ctx *context.Context) {
form := web.GetForm(ctx).(*forms.CreateRepoForm)
ctx.Data["Title"] = ctx.Tr("new_repo")

ctx.Data["Gitignores"] = repo_module.Gitignores
ctx.Data["DefaultGitignores"] = repo_module.Gitignores
ctx.Data["LabelTemplates"] = repo_module.LabelTemplates
ctx.Data["Licenses"] = repo_module.Licenses
ctx.Data["Readmes"] = repo_module.Readmes
Expand Down
10 changes: 5 additions & 5 deletions templates/org/create.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,23 +11,23 @@
{{template "base/alert" .}}
<div class="inline required field {{if .Err_OrgName}}error{{end}}">
<label for="org_name">{{.locale.Tr "org.org_name_holder"}}</label>
<input id="org_name" name="org_name" value="{{.org_name}}" autofocus required>
<input id="org_name" name="org_name" value="{{.OrgName}}" autofocus required>
<span class="help">{{.locale.Tr "org.org_name_helper"}}</span>
</div>

<div class="inline field {{if .Err_OrgVisibility}}error{{end}}">
<span class="inline required field"><label for="visibility">{{.locale.Tr "org.settings.visibility"}}</label></span>
<div class="inline-grouped-list">
<div class="ui radio checkbox">
<input class="hidden enable-system-radio" tabindex="0" name="visibility" type="radio" value="0" {{if .DefaultOrgVisibilityMode.IsPublic}}checked{{end}}/>
<input class="hidden enable-system-radio" tabindex="0" name="visibility" type="radio" value="0" {{if .Visibility.IsPublic}}checked{{end}}/>
<label>{{.locale.Tr "org.settings.visibility.public"}}</label>
</div>
<div class="ui radio checkbox">
<input class="hidden enable-system-radio" tabindex="0" name="visibility" type="radio" value="1" {{if .DefaultOrgVisibilityMode.IsLimited}}checked{{end}}/>
<input class="hidden enable-system-radio" tabindex="0" name="visibility" type="radio" value="1" {{if .Visibility.IsLimited}}checked{{end}}/>
<label>{{.locale.Tr "org.settings.visibility.limited"}}</label>
</div>
<div class="ui radio checkbox">
<input class="hidden enable-system-radio" tabindex="0" name="visibility" type="radio" value="2" {{if .DefaultOrgVisibilityMode.IsPrivate}}checked{{end}}/>
<input class="hidden enable-system-radio" tabindex="0" name="visibility" type="radio" value="2" {{if .Visibility.IsPrivate}}checked{{end}}/>
<label>{{.locale.Tr "org.settings.visibility.private"}}</label>
</div>
</div>
Expand All @@ -37,7 +37,7 @@
<label>{{.locale.Tr "org.settings.permission"}}</label>
<div class="inline-grouped-list">
<div class="ui checkbox">
<input class="hidden" type="checkbox" name="repo_admin_change_team_access" checked/>
<input class="hidden" type="checkbox" name="repo_admin_change_team_access" {{if .RepoAdminChangeTeamAccess}}checked{{end}}/>
<label>{{.locale.Tr "org.settings.repoadminchangeteam"}}</label>
</div>
</div>
Expand Down
40 changes: 20 additions & 20 deletions templates/repo/create.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@

<div class="inline required field {{if .Err_RepoName}}error{{end}}">
<label for="repo_name">{{.locale.Tr "repo.repo_name"}}</label>
<input id="repo_name" name="repo_name" value="{{.repo_name}}" autofocus required>
<input id="repo_name" name="repo_name" value="{{.RepoName}}" autofocus required>
<span class="help">{{.locale.Tr "repo.repo_name_helper"}}</span>
</div>
<div class="inline field">
Expand All @@ -53,21 +53,21 @@
<input name="private" type="checkbox" checked readonly>
<label>{{.locale.Tr "repo.visibility_helper_forced" | Safe}}</label>
{{else}}
<input name="private" type="checkbox" {{if .private}}checked{{end}}>
<input name="private" type="checkbox" {{if .Private}}checked{{end}}>
<label>{{.locale.Tr "repo.visibility_helper" | Safe}}</label>
{{end}}
</div>
<span class="help">{{.locale.Tr "repo.visibility_description"}}</span>
</div>
<div class="inline field {{if .Err_Description}}error{{end}}">
<label for="description">{{.locale.Tr "repo.repo_desc"}}</label>
<textarea id="description" name="description" placeholder="{{.locale.Tr "repo.repo_desc_helper"}}">{{.description}}</textarea>
<textarea id="description" name="description" placeholder="{{.locale.Tr "repo.repo_desc_helper"}}">{{.Description}}</textarea>
</div>
<div class="inline field">
<label>{{.locale.Tr "repo.template"}}</label>
<div id="repo_template_search" class="ui search normal selection dropdown">
<input type="hidden" id="repo_template" name="repo_template" value="{{.repo_template}}">
<div class="default text">{{.repo_template_name}}</div>
<input type="hidden" id="repo_template" name="repo_template" value="{{.RepoTemplate}}">
<div class="default text">{{.RepoTemplateName}}</div>
<div class="menu">
</div>
</div>
Expand All @@ -77,33 +77,33 @@
<div class="inline field">
<label>{{.locale.Tr "repo.template.items"}}</label>
<div class="ui checkbox">
<input class="hidden" name="git_content" type="checkbox" tabindex="0" {{if .git_content}}checked{{end}}>
<input class="hidden" name="git_content" type="checkbox" tabindex="0" {{if .GitContent}}checked{{end}}>
<label>{{.locale.Tr "repo.template.git_content"}}</label>
</div>
<div class="ui checkbox{{if not .SignedUser.CanEditGitHook}} tooltip{{end}}"{{if not .SignedUser.CanEditGitHook}} data-content="{{.locale.Tr "repo.template.git_hooks_tooltip"}}"{{end}}>
<input class="hidden" name="git_hooks" type="checkbox" tabindex="0" {{if .git_hooks}}checked{{end}}>
<input class="hidden" name="git_hooks" type="checkbox" tabindex="0" {{if .GitHooks}}checked{{end}}>
<label>{{.locale.Tr "repo.template.git_hooks"}}</label>
</div>
</div>
<div class="inline field">
<label></label>
<div class="ui checkbox">
<input class="hidden" name="webhooks" type="checkbox" tabindex="0" {{if .webhooks}}checked{{end}}>
<input class="hidden" name="webhooks" type="checkbox" tabindex="0" {{if .Webhooks}}checked{{end}}>
<label>{{.locale.Tr "repo.template.webhooks"}}</label>
</div>
<div class="ui checkbox">
<input class="hidden" name="topics" type="checkbox" tabindex="0" {{if .topics}}checked{{end}}>
<input class="hidden" name="topics" type="checkbox" tabindex="0" {{if .Topics}}checked{{end}}>
<label>{{.locale.Tr "repo.template.topics"}}</label>
</div>
</div>
<div class="inline field">
<label></label>
<div class="ui checkbox">
<input class="hidden" name="avatar" type="checkbox" tabindex="0" {{if .avatar}}checked{{end}}>
<input class="hidden" name="avatar" type="checkbox" tabindex="0" {{if .Avatar}}checked{{end}}>
<label>{{.locale.Tr "repo.template.avatar"}}</label>
</div>
<div class="ui checkbox">
<input class="hidden" name="labels" type="checkbox" tabindex="0" {{if .labels}}checked{{end}}>
<input class="hidden" name="labels" type="checkbox" tabindex="0" {{if .Labels}}checked{{end}}>
<label>{{.locale.Tr "repo.template.issue_labels"}}</label>
</div>
</div>
Expand All @@ -113,7 +113,7 @@
<div class="inline field">
<label>{{.locale.Tr "repo.issue_labels"}}</label>
<div class="ui search normal selection dropdown">
<input type="hidden" name="issue_labels" value="{{.issueLabels}}">
<input type="hidden" name="issue_labels" value="{{.IssueLabels}}">
<div class="default text">{{.locale.Tr "repo.issue_labels_helper"}}</div>
<div class="menu">
<div class="item" data-value="">{{.locale.Tr "repo.issue_labels_helper"}}</div>
Expand All @@ -129,10 +129,10 @@
<div class="inline field">
<label>.gitignore</label>
<div class="ui multiple search normal selection dropdown">
<input type="hidden" name="gitignores" value="{{.gitignores}}">
<input type="hidden" name="gitignores" value="{{.Gitignores}}">
<div class="default text">{{.locale.Tr "repo.repo_gitignore_helper"}}</div>
<div class="menu">
{{range .Gitignores}}
{{range .DefaultGitignores}}
<div class="item" data-value="{{.}}">{{.}}</div>
{{end}}
</div>
Expand All @@ -142,7 +142,7 @@
<div class="inline field">
<label>{{.locale.Tr "repo.license"}}</label>
<div class="ui search selection dropdown">
<input type="hidden" name="license" value="{{.license}}">
<input type="hidden" name="license" value="{{.License}}">
<div class="default text">{{.locale.Tr "repo.license_helper"}}</div>
<div class="menu">
<div class="item" data-value="">{{.locale.Tr "repo.license_helper"}}</div>
Expand All @@ -157,7 +157,7 @@
<div class="inline field">
<label>{{.locale.Tr "repo.readme"}}</label>
<div class="ui selection dropdown">
<input type="hidden" name="readme" value="{{.readme}}">
<input type="hidden" name="readme" value="{{.Readme}}">
<div class="default text">{{.locale.Tr "repo.readme_helper"}}</div>
<div class="menu">
{{range .Readmes}}
Expand All @@ -169,19 +169,19 @@
</div>
<div class="inline field">
<div class="ui checkbox" id="auto-init">
<input class="hidden" name="auto_init" type="checkbox" tabindex="0" {{if .auto_init}}checked{{end}}>
<input class="hidden" name="auto_init" type="checkbox" tabindex="0" {{if .AutoInit}}checked{{end}}>
<label>{{.locale.Tr "repo.auto_init"}}</label>
</div>
</div>
<div class="inline field">
<label for="default_branch">{{.locale.Tr "repo.default_branch"}}</label>
<input id="default_branch" name="default_branch" value="{{.default_branch}}" placeholder="{{.default_branch}}">
<input id="default_branch" name="default_branch" value="{{.DefaultBranch}}" placeholder="{{.DefaultBranch}}">
<span class="help">{{.locale.Tr "repo.default_branch_helper"}}</span>
</div>
<div class="inline field">
<label>{{.locale.Tr "repo.settings.trust_model"}}</label>
<div class="ui selection owner dropdown">
<input type="hidden" id="trust_model" name="trust_model" value="default" required>
<input type="hidden" id="trust_model" name="trust_model" value="{{.TrustModel}}" required>
<div class="default text">{{.locale.Tr "repo.settings.trust_model"}}</div>
{{svg "octicon-triangle-down" 14 "dropdown icon"}}
<div class="menu">
Expand All @@ -204,7 +204,7 @@
<div class="inline field">
<label>{{.locale.Tr "repo.template"}}</label>
<div class="ui checkbox">
<input class="hidden" name="template" type="checkbox" tabindex="0">
<input class="hidden" name="template" type="checkbox" tabindex="0" {{if .Template}}checked{{end}}>
<label>{{.locale.Tr "repo.template_helper"}}</label>
</div>
</div>
Expand Down