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

Support multiple projects #27984

Closed

Conversation

colonelpanic8
Copy link

@colonelpanic8 colonelpanic8 commented Nov 10, 2023

Revival of @tyroneyeh 's PR here #25164

Related #12974

Might still need some work

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 10, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 10, 2023
var ips []int64
if err := db.GetEngine(ctx).Table("project_issue").Select("project_id").
Where("issue_id=?", issue.ID).Find(&ips); err != nil {
return nil
Copy link
Member

Choose a reason for hiding this comment

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

err should not be ignored. I know the previous code should still be corrected.

@lunny lunny added the type/enhancement An improvement of existing functionality label Nov 10, 2023
@delvh
Copy link
Member

delvh commented Nov 10, 2023

I'm not sure if this functionality should be merged at all

  1. It is an enormous maintenance overhead as the common assumption <= 1 project per issue needs to be completely discarded, and as such bugs will occur.
    For example, assume someone deletes an issue. Someone might assume there is only a single project per issue and doesn't delete the rest.
  2. The UI needs a complete rewrite
  3. It makes it more complicated for the typical user:
    As the UI is not tailored to support exactly one project anymore, errors are far easier making the overall user experience more painful
  4. At the moment, I think it is a rare enough use case not to warrant all these changes.
    Or tell me, how often do you think this feature is needed compared to how it increases the complexity (both from a maintainer perspective and a user perspective)?

<a class="text muted flex-text-block">
<strong>{{ctx.Locale.Tr "repo.issues.new.projects"}}</strong>
<strong>{{ctx.locale.Tr "repo.issues.new.projects"}}</strong>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change ctx.Locale.Tr to ctx.locale.Tr?

Copy link
Author

Choose a reason for hiding this comment

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

not sure, this is likely something that was done by @tyroneyeh

Copy link
Contributor

Choose a reason for hiding this comment

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

ctx.Locale.Tr is from #27231 changed, ctx.locale.Tr is old version's

<div class="ui select-project list">
<span class="no-select item {{if .Issue.Project}}gt-hidden{{end}}">{{ctx.Locale.Tr "repo.issues.new.no_projects"}}</span>
<div class="ui projects list">
<span class="no-select item {{if .Issue.Projects}}gt-hidden{{end}}">{{.locale.Tr "repo.issues.new.no_projects"}}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

left -= limit
issueIDs = issueIDs[limit:]
for _, projectIssue := range projects {
issue := issueMap[projectIssue.IssueID]
issue.Projects = append(issue.Projects, projectIssue.Project)
Copy link
Contributor

@yp05327 yp05327 Nov 13, 2023

Choose a reason for hiding this comment

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

Maybe we need to check whether projectIssue.Project is already in issue.Projects here.

@colonelpanic8
Copy link
Author

colonelpanic8 commented Nov 13, 2023

I'm not sure if this functionality should be merged at all

  1. It is an enormous maintenance overhead as the common assumption <= 1 project per issue needs to be completely discarded, and as such bugs will occur.
    For example, assume someone deletes an issue. Someone might assume there is only a single project per issue and doesn't delete the rest.

Maybe stop making these assumptions? But I mean where specifically, and why? Seems like a strange assumption to make to me.

  1. The UI needs a complete rewrite

Why? Can you elaborate?

  1. It makes it more complicated for the typical user:
    As the UI is not tailored to support exactly one project anymore, errors are far easier making the overall user experience more painful

How is this not just you saying the same things you've already said?

  1. At the moment, I think it is a rare enough use case not to warrant all these changes.
    Or tell me, how often do you think this feature is needed compared to how it increases the complexity (both from a maintainer perspective and a user perspective)?

Multiple issues have been filed about this:

#12974

#25164

and also a previous PR:

#25164

conceptually restricting issues to a single project is very "worse is better" of you, but I guess I shouldn't expect anything different from the go community.

In any case, I'm not particularly interested in negotiating a bunch to get this merged.

I can probably just run from this on my own self hosted instance, but I thought that maybe others would be interested in keeping the original pr up to date, and maybe eventually someone who is motivated enough or a core contributor could use this as a base/foundation.

I kind of expect that people will not want to merge this, but at least its linked to the original pr and linked to the original issue in case people are interested.

Also, @delvh seems like this is listed as something @lunny (major contributor... who are you?) was at least somewhat interested in (see #14710). Your super dismissive attitude here is honestly kind of offputting.

} else if has {
issue.Project = &p
}
Where("project_issue.issue_id = ?", issue.ID).OrderBy("title").
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need to order the result here?
And maybe it is better to convert the function name into LoadProjects?

Copy link
Author

Choose a reason for hiding this comment

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

Again, this is just a holdover from @tyroneyeh 's original thing. All I did was try to resolve the conflicts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry. I was just reviewing this PR and didn't notice your comment above.
Are you planning to continually improve this PR or just resolve the conflicts?

Copy link
Contributor

Choose a reason for hiding this comment

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

This sorting applies to the sidebar menu list order

@@ -26,7 +26,7 @@ type IndexerData struct {
LabelIDs []int64 `json:"label_ids"`
NoLabel bool `json:"no_label"` // True if LabelIDs is empty
MilestoneID int64 `json:"milestone_id"`
ProjectID int64 `json:"project_id"`
ProjectIDs []int64 `json:"project_id"`
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
ProjectIDs []int64 `json:"project_id"`
ProjectIDs []int64 `json:"project_ids"`

Other places also need to be modified.

Comment on lines +92 to 95
var projectIDs []int64
for _, project := range issue.Projects {
projectIDs = append(projectIDs, project.ID)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a function is better? Just a suggestion. Then we can use it in other places if necessary.

@delvh
Copy link
Member

delvh commented Nov 13, 2023

@colonelpanic8

Maybe stop making these assumptions? But I mean where specifically, and why? Seems like a strange assumption to make to me.

I am not the one making these assumptions. Gitea is a project that is led by PRs from volunteers, many of whom have no in-depth knowledge of the code. These assumptions are coming automatically when you don't know the codebase well. That's what I mean by will be buggy.

The UI needs a complete rewrite

Why? Can you elaborate?

The current UI is set up to support exactly one project, both when assigning a project as well as displaying it (inside the issue, issue list, milestone, pull requests).
If you allow multiple projects, all these places need to be adapted to the new UI.
Additionally, many users rely on that the previous project is unassigned when you change the project, this wouldn't be the case anymore with multiple projects.

In any case, I'm not particularly interested in negotiating a bunch to get this merged.

Understandable

conceptually restricting issues to a single project is very "worse is better" of you, but I guess I shouldn't expect anything different from the go community.

I don't understand what the Go community has to do with this.
This is simply my personal opinion on what is best for the future of Gitea:
I am not categorically against this feature.
At the moment, there are too many loose ends whose behavior isn't clearly defined for me to allow this feature.
As such, it's my duty as a PR reviewer to clear them up before allowing this to be merged.
My "job" (I'm a volunteer after all) is making sure that Gitea doesn't end up bloated with features with inherent design flaws.
As I implied already, I have nothing against well-designed features.
As far as I know, all maintainers generally agree with this approach of only allowing well-designed features.

Your super dismissive attitude here is honestly kind of offputting.

Understandable.
But I think it is similarly understandable, that from my point of view, yours is equally bad. 😁

So, in conclusion, no, I'm not promoting worse is better, but only allow well-designed in the first place

@tyroneyeh
Copy link
Contributor

We should seek official help to implement this feature. Instead of submitting wrong code and causing everyone trouble,

Thanks,

@yp05327
Copy link
Contributor

yp05327 commented Nov 16, 2023

As we added organization/individual level project support in the early of this year,
I think it is necessary to support assign multiple projects, for users who want to manage issues not only in the repository level but also in the organization/individual level.

@delvh
Copy link
Member

delvh commented Nov 17, 2023

Hmm… Makes sense.
The question is: Is the usecase really support arbitrary many projects, or is it at most two projects with one repo and one user-level project?
Depending on that, the implementation will differ drastically.
Anyway, I don't think that's a discussion we should be having in this PR, better use the issue for it.

@colonelpanic8
Copy link
Author

implementation

Why would you limit things in that way. Furthermore, the database design already supports the one to many. Seems silly to insist on limiting things.

@komali2
Copy link

komali2 commented Feb 14, 2024

IMO the UI seems to imply that issues might have multiple projects:

2024-02-14-163604_587x378_scrot

For our co-op, this is a critical feature that would let us do things such as show a "master board" of all issues across all our FOSS projects, something we really like on Github, and a big part of why we're migrating off github is the paywall behind having more than 5 FOSS projects feed automatically into another table. Even if we have to manually add issues per-project, it's still really nice.

So for us, that would be 1 repo level, and at least 1 organization-level project, but honestly possibly more if we can, for various organization reasons. I'm looking through the code now and I don't quite understand why arbitrary vs 1 org and 1 user is significantly different in complication, seems the future proof solution is better, but I defer to those more familiar with the codebase.

For what it's worth, I got 70 folks over here with time, unsure how many are interested in helping with this, but I and some others I were talking today are, so if "it's too complicated" is a major blocker, maybe we can throw our weight into the ring to help alleviate that concern?

@komali2
Copy link

komali2 commented Feb 14, 2024

@delvh you seem to have the right sense of side-effects, that's good to have in a PR reviewer imo, I'm glad to see this project doesn't do "LGTM" and merges. It sounds like the main blocker for this PR right now is insufficient documentation of all the places that this change could effect (and actually handling those places in the code), is that right?

If so I'm happy to start documenting all the places at least in the UI that I suspect might be affected by this change. I'm still getting familiar with gitea (I'm just using in through codeberg right now, which isn't even really gitea lol) so I need to get a local deployment working and get familiar with the codebase, so any assistance connecting what I see in the UI with where that's reflected in code would be really helpful.

@komali2
Copy link

komali2 commented Feb 15, 2024

Compiling UI locations I can see that will probably be affected by this change:

{org}/{repo}/issues/new?project={id}

The "Projects" list per new issue

2024-02-15-140810_472x285_scrot

Also, the "projects" list dropdown on the same page may be affected

2024-02-15-141004_524x595_scrot

Note for above, these issues might be created with a milestone preselected via query parameter:
/{org}/{repo}/issues/new?milestone={milestone_id}

/{org}/{repo}/issues/{id}

The notifications chain for an issue may be affected, as it indicates when an issue was added to a project

2024-02-15-141045_672x450_scrot

Also, that same page has likely the same component for the "projects" list and "projects" dropdown.

{org}/{repo}/projects/{id}

2024-02-15-141232_1114x630_scrot

However, clicking "new issue" seems to redirect to the issue UI already mentioned above. I couldn't find any dropdown style or "live-add" functionality (such as github has) that would be affected by this change. Maybe I missed it?

Obviously, the kanban board view will be affected

{org}/{project}/issues

2024-02-15-142029_447x670_scrot

Potentially the "project" dropdown for a repo under issues tab might be affected?

The above URLs all can swap {org} for {user} and have the same ux, so, if those are handled differently in code, then those locations as well will be affected.

Surprisingly, I couldn't find any other locations in UX that might be affected. What other places am I missing?

@komali2
Copy link

komali2 commented Feb 15, 2024

Also

{org}/{repo}/projects

2024-02-15-201739_631x328_scrot

Deleting a project is also probably effected.

@tyroneyeh
Copy link
Contributor

Try new modifying #30163 again

@yp05327
Copy link
Contributor

yp05327 commented Apr 8, 2024

Try new modifying #30163 again

So we can close this one?

@tyroneyeh
Copy link
Contributor

Try new modifying #30163 again

So we can close this one?

Maybe?

@colonelpanic8

@mkoskl
Copy link

mkoskl commented Sep 19, 2024

Another use-case for allowing multiple projects per issue. I'm tracking a data migration process from multiple (related) data sources via Gitea issue tracker.

Same textual data formatting rules apply to every data source / project. I have one issue for this textual data formatting and I would like to link it to all these projects.

@wxiaoguang
Copy link
Contributor

Close according to #27984 (comment)

For #30381, I think it is almost there if it could follow the new sidebar design: #30381 (comment)

@wxiaoguang wxiaoguang closed this Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants