Skip to content
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.

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

Do not store user projects as organization projects #23353

Merged

Conversation

yp05327
Copy link
Contributor

@yp05327 yp05327 commented Mar 7, 2023

A part of #22865

At first, I think we do not need 3 ProjectTypes, as we can check user type, but it seems that it is not database friendly.

@lunny
Copy link
Member

lunny commented Mar 7, 2023

And should we correct the error history types?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 7, 2023
@lunny lunny added the type/bug label Mar 7, 2023
@codecov-commenter

This comment was marked as off-topic.

@lunny lunny added this to the 1.20.0 milestone Mar 7, 2023
@lunny lunny added the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label Mar 7, 2023
@lunny
Copy link
Member

lunny commented Mar 7, 2023

Or we can merge the two types as one. Because there is no difference between them.

@yp05327
Copy link
Contributor Author

yp05327 commented Mar 7, 2023

Or we can merge the two types as one. Because there is no difference between them.

If we merge the two types as one, when we want to check org/individual project type, we need to call LoadOwner which will call a db request. So maybe three types is better?

(Edited) Is there a better way to check org/individual project type?

@yp05327 yp05327 mentioned this pull request Mar 7, 2023
10 tasks
@delvh
Copy link
Member

delvh commented Mar 7, 2023

We can store the type of project inside the db when creating it?
0 - repo project,
1 - org project,
(2 - user project, if it is ever supported)

As there is no convert this repo project to an org project, there isn't even anything to keep in mind…

@yp05327
Copy link
Contributor Author

yp05327 commented Mar 8, 2023

We already store the type of project inside the db when creating it.

We have three project types in current design:

const (
// TypeIndividual is a type of project board that is owned by an individual
TypeIndividual Type = iota + 1
// TypeRepository is a project that is tied to a repository
TypeRepository
// TypeOrganization is a project that is tied to an organisation
TypeOrganization
)

But when creating projects, we store TypeOrganization to both Individual and Organization Projects. This PR will fix this problem.

But before simply using TypeIndividual when creating individual projects,
As Individual and Organization are both User, why we don't merge TypeIndividual and TypeOrganization into TypeUser

const (
// UserTypeIndividual defines an individual user
UserTypeIndividual UserType = iota // Historic reason to make it starts at 0.
// UserTypeOrganization defines an organization
UserTypeOrganization
)

At first, I think TypeUser is better because there is no difference between them, as lunny said.
But when we want to check whether a project is an Individual or Organization project, we need to LoadOwner, and then we can use project.Owner.IsOrganization() or project.Owner.IsIndividual().
This idea will call a db search, which is unnecessary I think, as if we use 3 project types, we can directly check project.Type==TypeOrganization or project.Type==TypeIndividual

I think this problem is a bit similar to #23038 (comment)
When there are a lot of projects in one page, holding the old design can reduce the calling of LoadOwner

After we decided to use which design, I will add a db migration to fix the error history types.

@lunny
Copy link
Member

lunny commented Mar 8, 2023

I don't against this solution. But we need to consider the migration.

@lunny
Copy link
Member

lunny commented Mar 8, 2023

CI failure is related.

@yp05327
Copy link
Contributor Author

yp05327 commented Mar 9, 2023

I found 2 problems, when using xorm:

  • The order of variables in struct will effect the result

User struct
image

Correct Results:
image
Incorrect Results:
image

  • Cant not find User.Type when use Update

When use Find, it works
image

When use Update, it does't work
image

You can check the code used in 5bcc68c

@lunny
Is this XORM's bugs or my code is bad?

@yp05327 yp05327 changed the title Fix incorrect project type when create new org/user projects Fix incorrect project type when create/view org/user projects Mar 9, 2023
models/migrations/v1_20/v245.go Outdated Show resolved Hide resolved
models/migrations/v1_20/v245.go Outdated Show resolved Hide resolved
models/migrations/v1_20/v245.go Outdated Show resolved Hide resolved
@delvh delvh changed the title Fix incorrect project type when create/view org/user projects Do not store user projects as organization projects Mar 9, 2023
@lunny
Copy link
Member

lunny commented Mar 15, 2023

Since there is a migration in this PR, it will not easy to backport the migration. But other code could be backport.

@yp05327
Copy link
Contributor Author

yp05327 commented Mar 15, 2023

#22235 added new column OwnerID to Project but have no migrations.
So the migration test failed in thie PR.

Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

Does the unknown column error still happen?
If no, please add the wait-merge label

@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 Mar 16, 2023
@yp05327
Copy link
Contributor Author

yp05327 commented Mar 17, 2023

Does the unknown column error still happen?

unknown column is fixed in #23482

@6543 6543 added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 17, 2023
@lunny lunny merged commit 8e45fcb into go-gitea:main Mar 17, 2023
@GiteaBot
Copy link
Contributor

I was unable to create a backport for 1.19, please send one manually. 🍵

@GiteaBot GiteaBot added the backport/manual No power to the bots! Create your backport yourself! label Mar 17, 2023
@wxiaoguang
Copy link
Contributor

There is a DB migration, IMO it's impossible to be backported.

@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 17, 2023
@delvh delvh removed outdated/backport/v1.19 This PR should be backported to Gitea 1.19 backport/manual No power to the bots! Create your backport yourself! labels Mar 17, 2023
lafriks added a commit that referenced this pull request Mar 19, 2023
#23538)

The project type will be changed in
#23353, so the old fix
#23325 will not work as well.

And I also found that there were some problems in the old fix....

---------

Co-authored-by: Lauris BH <[email protected]>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 20, 2023
* giteaofficial/main: (23 commits)
  Display the version of runner in the runner list (go-gitea#23490)
  Add `.patch` to `attachment.ALLOWED_TYPES` (go-gitea#23580)
  Sort Python package descriptors by version to mimic PyPI format (go-gitea#23550)
  Use `project.IconName` instead of repeated unreadable `if-else` chains (go-gitea#23538)
  Match api migration behavior to web behavior (go-gitea#23552)
  Fix dropdown icon misalignment when using fomantic icon (go-gitea#23558)
  Enable color for consistency checks diffs (go-gitea#23563)
  [skip ci] Updated translations via Crowdin
  Fix sticky header in diff view (go-gitea#23554)
  Fix some broken css (go-gitea#23560)
  Fix JS error on compare page (go-gitea#23551)
  Upgrade to npm lockfile v3 and explicitely set it (go-gitea#23561)
  Fix long name ui issues and label ui issue  (go-gitea#23541)
  Remove worker-loader (go-gitea#23548)
  [skip ci] Updated translations via Crowdin
  Return `repository` in npm package metadata endpoint (go-gitea#23539)
  Fix diff detail buttons wrapping, use tippy for review box (go-gitea#23271)
  Do not store user projects as organization projects (go-gitea#23353)
  Imrove scroll behavior to hash issuecomment(scroll position, auto expand if file is folded, and on refreshing) (go-gitea#23513)
  Increase horizontal page padding (go-gitea#23507)
  ...
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants