-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Added dependencies for issues (#2196) #2531
Changes from 107 commits
c5992f5
021a7e0
55f6cf9
30fe97a
751bbee
913584a
53ec08f
14aa1d2
0bbc695
a651cc3
f188031
58e7c2b
5a9f273
ba734d6
5e4b6fc
5302eca
2fe66c2
a2b652e
762266f
6b3a51a
2dca487
d1df4ec
dd24b9f
827291e
c0bc5e8
993a3c0
3beecd2
4e9e1f8
e364cfc
c4ba8d6
9e9d8b9
80437f8
a54f930
d65fe40
f38b3b4
f1f0607
6109fcf
85cdd94
a57bcf1
fc35252
33c5033
c81041f
db4f578
03769da
6eea6c6
d00eb86
78dd6c1
1fded52
3566e9e
9af3aae
f61ba0f
d0baadc
9dd0cb1
fae4466
8bcb624
25427ae
66433a9
01f9e63
13f0e17
cdd322f
e98f92d
822c75c
f02fd33
7e93434
172b1f0
8354f2b
69769ec
1608248
46f099c
e6aeeac
08763cf
32c5605
ed4a47d
200f88d
9714024
2c8103f
1f0bac0
d29ca0c
70589e6
1064acc
c176353
866beeb
5e20447
2058f4e
5dc9c22
f48d6cd
8931ced
df638db
4855f83
108edf9
2a249c4
c534699
57a4f86
fdb106d
a2a6fed
436ea16
9eedb3f
b7ed097
661bd7c
632cca5
12c0093
96d6db4
0016a3e
cd47077
8735bc2
37dc550
f4cb1c7
8bc0947
86be828
eb295f4
171715b
4d15f28
1f8ef92
0fcc3b2
d5b1c33
5f187df
c44e13b
a5b696a
cf1b573
f71f2d4
4277014
31eff2f
30ae70a
dd5cfbf
fdabcf1
992e168
60834bc
18fe48e
647296c
cd734a3
f837429
0ce2170
9ee04eb
cdb5d65
9e29f71
16b1636
be51a47
0ea51f8
e322fa3
9584d3f
1b63df7
7566292
421c9fa
bd3cf60
b58363c
bcaea09
e842472
64e61d6
637895b
77cf3db
32db1b3
ec0e693
5e0a72c
7d1df8b
b4f406c
d2ab9e2
9313332
e4a6426
138a80b
264f895
0c6f56d
250138f
91e0b53
5e5139f
5852b7c
e1f2a8e
20ed1e9
4d28a82
4f94a01
2950021
c3a5878
7a57b6d
497628e
2edfbcd
a25e1dd
f49dd29
3a24b65
b589726
c3818a9
7b26e2c
3820fc4
2f136fc
29dbc17
6f999a0
7528664
be76631
8d2015f
ee77151
82129b8
b00e209
414d1d0
9e11e77
d0b7307
17b2d43
6af8ef7
2e096a5
1a86536
81b51d0
b258bc0
6a5e138
c0cd0c3
022e9d2
3cd497f
16533e9
0f8a212
a3a544e
8266510
edf661d
9185958
f0406c1
395c062
c877186
288cd40
d29f582
83b8831
617d8cd
6733682
236ad85
fbd78f4
ad353f0
5d8383e
8c1f627
15aa9b1
0da2e34
e18627c
39392de
87f41c0
c52e970
c212a82
990985a
05aaee2
ec9797d
0879a36
425e765
6514da4
12602c2
c760e6a
393013a
2d95ab1
10308a0
d89a136
8a6ba47
f01fdf6
e00e5dd
cf6d426
49c471f
a283309
b6e428d
c3a6210
b8aa1d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,10 @@ const ( | |
CommentTypeAddTimeManual | ||
// Cancel a stopwatch for time tracking | ||
CommentTypeCancelTracking | ||
// Dependency added | ||
CommentTypeAddDependency | ||
//Dependency removed | ||
CommentTypeRemoveDependency | ||
) | ||
|
||
// CommentTag defines comment tag type | ||
|
@@ -75,23 +79,25 @@ const ( | |
|
||
// Comment represents a comment in commit and issue page. | ||
type Comment struct { | ||
ID int64 `xorm:"pk autoincr"` | ||
Type CommentType | ||
PosterID int64 `xorm:"INDEX"` | ||
Poster *User `xorm:"-"` | ||
IssueID int64 `xorm:"INDEX"` | ||
LabelID int64 | ||
Label *Label `xorm:"-"` | ||
OldMilestoneID int64 | ||
MilestoneID int64 | ||
OldMilestone *Milestone `xorm:"-"` | ||
Milestone *Milestone `xorm:"-"` | ||
OldAssigneeID int64 | ||
AssigneeID int64 | ||
Assignee *User `xorm:"-"` | ||
OldAssignee *User `xorm:"-"` | ||
OldTitle string | ||
NewTitle string | ||
ID int64 `xorm:"pk autoincr"` | ||
Type CommentType | ||
PosterID int64 `xorm:"INDEX"` | ||
Poster *User `xorm:"-"` | ||
IssueID int64 `xorm:"INDEX"` | ||
LabelID int64 | ||
Label *Label `xorm:"-"` | ||
OldMilestoneID int64 | ||
MilestoneID int64 | ||
OldMilestone *Milestone `xorm:"-"` | ||
Milestone *Milestone `xorm:"-"` | ||
OldAssigneeID int64 | ||
AssigneeID int64 | ||
Assignee *User `xorm:"-"` | ||
OldAssignee *User `xorm:"-"` | ||
OldTitle string | ||
NewTitle string | ||
DependentIssueID int64 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need this too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes i do? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is 2 places with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup. Thats why i need it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I saw that i was wrong with removing I use this to get the Dependent issue details displayed in the comments. But to know which issue i need to get informations about, I also need the ID of the Dependent Issue. And that's where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've probably started the featue and then forget about it, which is why there was this function (and the corresponding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This means you can only have 1 dependency right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One per comment. (You can ofc have more than dependency per issue) The ID here is used to get issue details (title, index) later when displaying the comment to be able to link to it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does Comments track dependencies? 😕 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They don't, the ID is only used to display informations in a comment (title, index, link) when someone adds or removes a dependency. |
||
DependentIssue *Issue `xorm:"-"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To load issue Dependency Details in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But usage of it? I don't see reason to have this field. You never use it. You only set it and save to db, but never read. Or did I overlooked something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is used in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, loaded, but for what? Where you use it? You load it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finally :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, see my latest comment... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kolaente yes, that added html is the reason I was looking for. I looked at it yesterday and have some suggestions for it too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what suggestions? |
||
|
||
CommitID int64 | ||
Line int64 | ||
|
@@ -269,6 +275,18 @@ func (c *Comment) LoadAssignees() error { | |
return nil | ||
} | ||
|
||
// LoadDepIssueDetails loads Dependent Issue Details | ||
func (c *Comment) LoadDepIssueDetails() error { | ||
var err error | ||
if c.DependentIssueID > 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
c.DependentIssue, err = getIssueByID(x, c.DependentIssueID) | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
// MailParticipants sends new comment emails to repository watchers | ||
// and mentioned people. | ||
func (c *Comment) MailParticipants(e Engine, opType ActionType, issue *Issue) (err error) { | ||
|
@@ -297,24 +315,40 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err | |
if opts.Label != nil { | ||
LabelID = opts.Label.ID | ||
} | ||
|
||
var depID int64 | ||
if opts.DependentIssue != nil { | ||
depID = opts.DependentIssue.ID | ||
} | ||
|
||
comment := &Comment{ | ||
Type: opts.Type, | ||
PosterID: opts.Doer.ID, | ||
Poster: opts.Doer, | ||
IssueID: opts.Issue.ID, | ||
LabelID: LabelID, | ||
OldMilestoneID: opts.OldMilestoneID, | ||
MilestoneID: opts.MilestoneID, | ||
OldAssigneeID: opts.OldAssigneeID, | ||
AssigneeID: opts.AssigneeID, | ||
CommitID: opts.CommitID, | ||
CommitSHA: opts.CommitSHA, | ||
Line: opts.LineNum, | ||
Content: opts.Content, | ||
OldTitle: opts.OldTitle, | ||
NewTitle: opts.NewTitle, | ||
} | ||
if _, err = e.Insert(comment); err != nil { | ||
Type: opts.Type, | ||
PosterID: opts.Doer.ID, | ||
Poster: opts.Doer, | ||
IssueID: opts.Issue.ID, | ||
LabelID: LabelID, | ||
OldMilestoneID: opts.OldMilestoneID, | ||
MilestoneID: opts.MilestoneID, | ||
OldAssigneeID: opts.OldAssigneeID, | ||
AssigneeID: opts.AssigneeID, | ||
CommitID: opts.CommitID, | ||
CommitSHA: opts.CommitSHA, | ||
Line: opts.LineNum, | ||
Content: opts.Content, | ||
OldTitle: opts.OldTitle, | ||
NewTitle: opts.NewTitle, | ||
DependentIssue: opts.DependentIssue, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you need to set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, you're right, Removed it. |
||
DependentIssueID: depID, | ||
} | ||
|
||
//fmt.Println(comment) | ||
|
||
// TODO: WHY ISNT THIS INSERTED?????? | ||
// It seems to be inserted, but isnt. (Doesn't return an error, raw pasting | ||
// the sql query in a database console does work). But after the function | ||
// is called, there is no entry in the database. At least for type 12 and 13. | ||
_, err = e.Insert(comment) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
|
@@ -486,13 +520,31 @@ func createDeleteBranchComment(e *xorm.Session, doer *User, repo *Repository, is | |
}) | ||
} | ||
|
||
// Creates issue dependency comment | ||
func createIssueDependencyComment(e *xorm.Session, doer *User, issue *Issue, dependentIssue *Issue, add bool) (*Comment, error) { | ||
cType := CommentTypeAddDependency | ||
if !add { | ||
cType = CommentTypeRemoveDependency | ||
} | ||
|
||
return createComment(e, &CreateCommentOptions{ | ||
Type: cType, | ||
Doer: doer, | ||
Repo: issue.Repo, | ||
Issue: issue, | ||
DependentIssue: dependentIssue, | ||
Content: dependentIssue.Title, | ||
}) | ||
} | ||
|
||
// CreateCommentOptions defines options for creating comment | ||
type CreateCommentOptions struct { | ||
Type CommentType | ||
Doer *User | ||
Repo *Repository | ||
Issue *Issue | ||
Label *Label | ||
Type CommentType | ||
Doer *User | ||
Repo *Repository | ||
Issue *Issue | ||
Label *Label | ||
DependentIssue *Issue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is not dependent issue ID enough? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think it doesn't really make a difference, as it is a pointer... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO you should always use minimum and be exact of what you need. You know, that only DependentIssue.ID will be used here, but not everyone will read the code to learn what it rly needs (and should not). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I've changed that. When I first started this pr, I used only the IDs everywhere, but I was told to use structs instead, which is why I used that here. |
||
|
||
OldMilestoneID int64 | ||
MilestoneID int64 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,159 @@ | ||
// Copyright 2017 The Gitea Authors. All rights reserved. | ||
// Use of this source code is governed by a MIT-style | ||
// license that can be found in the LICENSE file. | ||
|
||
package models | ||
|
||
import ( | ||
"time" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. import order There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FIxed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kolaente still a missing one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Github didn't got it... |
||
) | ||
|
||
// IssueDependency is connection request for receiving issue notification. | ||
type IssueDependency struct { | ||
ID int64 `xorm:"pk autoincr"` | ||
UserID int64 `xorm:"UNIQUE(watch) NOT NULL"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This unnecessary. issue notifications are tracked elsewhere, use a JOIN if necessary There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, you're right, it probably is there because I copied it in the beginning 😀 , my fault... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed it. |
||
IssueID int64 `xorm:"UNIQUE(watch) NOT NULL"` | ||
DependencyID int64 `xorm:"UNIQUE(watch) NOT NULL"` | ||
Created time.Time `xorm:"-"` | ||
CreatedUnix int64 `xorm:"INDEX created"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i've just used it, think it works. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use util.TimeStamp instead of int64 and time.Time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can find many example on other object. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, maybe you didn't push success? I cannot find the change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did push however github didn't got it... |
||
Updated time.Time `xorm:"-"` | ||
UpdatedUnix int64 `xorm:"updated"` | ||
} | ||
|
||
// DependencyType Defines Dependency Type Constants | ||
type DependencyType int | ||
|
||
// Define Dependency Types | ||
const ( | ||
DependencyTypeBlockedBy DependencyType = iota | ||
DependencyTypeBlocking | ||
) | ||
|
||
// CreateIssueDependency creates a new dependency for an issue | ||
func CreateIssueDependency(user *User, issue, dep *Issue) (exists bool, err error) { | ||
sess := x.NewSession() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. defer sess.Close() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And you have to if err := sess.Begin(); err != nil {
return err
} |
||
|
||
// Check if it aleready exists | ||
exists, err = issueDepExists(x, issue.ID, dep.ID) | ||
if err != nil { | ||
return exists, err | ||
} | ||
|
||
// If it not exists, create it, otherwise show an error message | ||
if !exists { | ||
newIssueDependency := &IssueDependency{ | ||
UserID: user.ID, | ||
IssueID: issue.ID, | ||
DependencyID: dep.ID, | ||
} | ||
|
||
if _, err := x.Insert(newIssueDependency); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. x -> sess |
||
return exists, err | ||
} | ||
|
||
// Add comment referencing the new dependency | ||
_, err = createIssueDependencyComment(sess, user, issue, dep, true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please replace this and the if _, err = createIssueDependencyComment(sess, user, issue, dep, true); err != nil {
return exists, err
} |
||
|
||
if err != nil { | ||
return exists, err | ||
} | ||
|
||
// Create a new comment for the dependent issue | ||
_, err = createIssueDependencyComment(sess, user, dep, issue, true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comment above |
||
|
||
if err != nil { | ||
return exists, err | ||
} | ||
} | ||
return exists, nil | ||
} | ||
|
||
// RemoveIssueDependency removes a dependency from an issue | ||
func RemoveIssueDependency(user *User, issue *Issue, dep *Issue, depType DependencyType) (err error) { | ||
sess := x.NewSession() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. defer sess.Close() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need |
||
|
||
// Check if it exists | ||
exists, err := issueDepExists(x, issue.ID, dep.ID) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You do check where you switch issue for dep, but later you are deleting in given order. What it should do? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It checks if the dependency already exists as you send it to the backend, but it also checks the other way around to prevent to issues blocking each other. This would result in none of them could be closed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please replace this with: var exists bool
if exists, err := issueDepExists(x, issue.ID, dep.ID); err != nil {
return err
} |
||
if err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like the fact, that
And you don't need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems logic, thanks! Took me some time to understand, a really good solution. :) |
||
return err | ||
} | ||
|
||
// If it exists, remove it, otherwise show an error message | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment seems to be out-of-date; I don't see an error message anywhere There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, I've changed it. |
||
if exists { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain that? I don't understand why this would be necessary, because I still need to do all of that stuff when the dependency exists. Sure, I could add the piece of code you described, but I still need IMHO the way I did it is ok, please tell me if I misunderstood you. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's about unnecessary nested code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But how is it unnecessary nested? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I'll try to explain with "my" English) You should follow the "break early" rule and also keep standard/expected flow without indentation (not nested). Function name is If you know, you will not do anything else in some condition you should immidiately end processing. What if there will be more exceptions in future? Then you will have to create another nested block inside (and still not talking aboud returning different exceptions)...
Another thing is, who will know later why that block is inside "if" and if he/she should add another code after it (between end of block and commit) or inside that block? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I know about that priciple, but this would mean having two times if !exists {
return sess.Commit()
}
// Actual code which is currently inside of if exists {}
// ...
return sess.Commit() I like the way I did it more, that way we have one unnecessary There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is better to have maybe more verbose but clear code. Also question is, if you rly need to commit something instead of returning error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I think I finally understood how it works. Thanks for taking the time to explain :) I've fixed it. I also saw that we don't need to return |
||
|
||
var issueDepToDelete IssueDependency | ||
|
||
switch depType { | ||
case DependencyTypeBlockedBy: | ||
issueDepToDelete = IssueDependency{IssueID: issue.ID, DependencyID: dep.ID} | ||
case DependencyTypeBlocking: | ||
issueDepToDelete = IssueDependency{IssueID: dep.ID, DependencyID: issue.ID} | ||
default: | ||
return | ||
} | ||
|
||
_, err := x.Delete(&issueDepToDelete) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please minimize this like shown above |
||
if err != nil { | ||
return err | ||
} | ||
|
||
// Add comment referencing the removed dependency | ||
_, err = createIssueDependencyComment(sess, user, issue, dep, false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please minimize this like shown above |
||
|
||
if err != nil { | ||
return err | ||
} | ||
|
||
// Create a new comment for the dependent issue | ||
_, err = createIssueDependencyComment(sess, user, dep, issue, false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please minimize this like shown above |
||
|
||
if err != nil { | ||
return err | ||
} | ||
} | ||
return nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. return sess.Commit() |
||
} | ||
|
||
// Check if the dependency already exists | ||
func issueDepExists(e Engine, issueID int64, depID int64) (exists bool, err error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like names There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean by this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, I try to find all dependencies, no matter of direction. To be more clear, an example: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am ok with that as long as I was thinking about it in more general way. Like using this function for checking (from any place of code) any dependency between two issues. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, cool. I think to be more general, it would need to return a struct with both issues, but i dont see a usecase for this (yet). |
||
|
||
deps := new(IssueDependency) | ||
exists, err = e.Where("(issue_id = ? AND dependency_id = ?) OR (issue_id = ? AND dependency_id = ?)", issueID, depID, depID, issueID).Get(deps) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
if err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need this if clause There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you're right. |
||
return exists, err | ||
} | ||
|
||
return exists, nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could simply replace this with |
||
} | ||
|
||
// IssueDependencyIssue custom type for mysql join | ||
type IssueDependencyIssue struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you don't need this at all. Everything you need is in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, i need it to check if there aren't any dependencies left. To do this, i join There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is completely unnecessary since you're already tracking parent<->child in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But i need this to join issue dependencies with issues to check if there are any open dependent issues, the info if the issue is open or closed is saved with the issue, not the dependency. I have an issue with dependencies. To know if any of these dependent issues is still open, I need to join Or am I completly wrong? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need a join table? xorm has support for doing JOINs while only working on a single table 🤔 /cc @lunny There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something I did in another project a while back: contribs := make([]*Contribution, 0, pageSize)
engine.Table("contribution").
Select("contribution.*, compo.*").
Join("INNER", "compo", "compo.id = contribution.compo_id").
Where("contribution.name=?", name).
Limit(pageSize, (page-1)*pageSize).
Find(&contribs) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oookay, i didn't know that, I put it just like on xorm's official documentation: http://gobook.io/read/github.com/go-xorm/manual-en-US/chapter-05/5.join.html But the thing you have does indeed look a lot cleaner, I will change it to something like this. Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
IssueDependency `xorm:"extends"` | ||
Issue `xorm:"extends"` | ||
} | ||
|
||
// TableName returns table name for mysql join | ||
func (IssueDependencyIssue) TableName() string { | ||
return "issue_dependency" | ||
} | ||
|
||
// IssueNoDependenciesLeft checks if issue can be closed | ||
func IssueNoDependenciesLeft(issue *Issue) bool { | ||
|
||
var issueDeps []IssueDependencyIssue | ||
|
||
err := x.Join("INNER", "issue", "issue.id = issue_dependency.issue_id").Where("issue_id = ?", issue.ID).Find(&issueDeps) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can do simple check if there is record in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But i need all informations of the issue, thats why I made the join. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are just returning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, i need to know if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can add it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, i see how you mean this... Isn't this basically the same what you said in your newer review? (the one you linked to) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's same (that's why I linked it). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, done. |
||
|
||
if err != nil { | ||
return false | ||
} | ||
|
||
for _, issueDep := range issueDeps { | ||
if !issueDep.IsClosed { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can include this in |
||
return false | ||
} | ||
} | ||
|
||
return true | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
// Copyright 2017 The Gitea Authors. All rights reserved. | ||
// Use of this source code is governed by a MIT-style | ||
// license that can be found in the LICENSE file. | ||
|
||
package migrations | ||
|
||
import ( | ||
"fmt" | ||
|
||
"code.gitea.io/gitea/models" | ||
|
||
"github.com/go-xorm/xorm" | ||
) | ||
|
||
func addIssueDependencyTables(x *xorm.Engine) (err error) { | ||
|
||
err = x.Sync(new(models.IssueDependency)) | ||
|
||
if err != nil { | ||
return fmt.Errorf("Error creating issue_dependency_table column definition: %v", err) | ||
} | ||
|
||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to add a else here to notify the UI to say it cannot be closed since it has some dependencies not closed. @kolaente @bkcsoft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it shows an error message if it cannot be closed because there are open dependencies 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kolaente Yes, I thinks that's expected? Shouldn't that?