Skip to content

Update User model comments about permissions #17583

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

Jump to bottom
Merged
merged 8 commits into from
Nov 9, 2021
Merged

Update User model comments about permissions #17583

merged 8 commits into from
Nov 9, 2021

Conversation

wxiaoguang
Copy link
Contributor

Copied from and close #17467

Please help to check whether my understanding and comments are correct.

@wxiaoguang wxiaoguang requested a review from lafriks November 8, 2021 07:12
Gusted
Gusted suggested changes Nov 8, 2021
View reviewed changes
Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

The descriptions are good(if I've read the code correctly) and describes what the fields are, I think the wording need a little bit of change and maybe clarify some things up.

models/user.go Outdated
AllowGitHook bool
AllowImportLocal bool // Allow migrate repository by local path
AllowCreateOrganization bool `xorm:"DEFAULT true"`
ProhibitLogin bool `xorm:"NOT NULL DEFAULT false"`

// true: user is not allowed to log in Web UI. Git SSH access could still be allowed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// true: user is not allowed to log in Web UI. Git SSH access could still be allowed.
// true: user is not allowed to log into the Web UI. Git SSH access could still be allowed.

I think Git SSH access >could< still be allowed. that could could be clarified, as in, which conditions the Git SSH access is allowed.

Copy link
Contributor Author

@wxiaoguang wxiaoguang Nov 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The conditions belong to SSH access module, User model has nothing to do (no knowledge) about it.

If we need to clarify the conditions, related comments should be written in SSH access module.

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont have a problem if its clarified to the SSH access module.

For someone who's trying understand what a field does and it refers to something that isnt described somewhere, then I would personally still not fully understand what the field does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I just added a new comment: "please refer to Git/SSH access related code/documents".

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 8, 2021
Gusted
Gusted approved these changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@Gusted Gusted left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The prohibtedLogin could be done in another PR as this is just for the user struct. LGTM

@GiteaBot 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 Nov 9, 2021
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 9, 2021
@wxiaoguang wxiaoguang merged commit a5b4720 into go-gitea:main Nov 9, 2021
@wxiaoguang wxiaoguang deleted the update-user-comment branch November 9, 2021 10:43
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@wxiaoguang
Update User model comments about permissions (go-gitea#17583)
4cde73d
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny approved these changes

@Gusted Gusted Gusted approved these changes

@lafriks lafriks Awaiting requested review from lafriks

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore.
Projects
None yet
Milestone
No milestone
4 participants
@wxiaoguang @lunny @Gusted @GiteaBot