-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Fix #2001 and fix issue comments hidden #2016
Conversation
LGTM |
models/issue_comment.go
Outdated
if opts.Since > 0 { | ||
cond = cond.And(builder.Gte{"comment.updated_unix": opts.Since}) | ||
} | ||
if opts.Type > -1 { |
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.
Probably it would be better to use opts.Type != CommentTypeUnknown
in this case
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.
done.
models/issue_comment.go
Outdated
cond = cond.And(builder.Eq{"issue.repo_id": opts.RepoID}) | ||
} | ||
if opts.IssueID > 0 { | ||
cond = cond.And(builder.Eq{"issue.id": opts.IssueID}) |
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.
Could use "comment.issue_id"
instead of "issue.id"
, so that we can avoid joining if opts.RepoID
is 0.
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.
done.
models/issue_comment.go
Outdated
Asc("created_unix") | ||
if since > 0 { | ||
sess.And("updated_unix >= ?", since) | ||
// FindCommentsOptions describes the condtions to Find comments |
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.
condtions -> conditions
models/issue_comment.go
Outdated
sess.And("comment.updated_unix >= ?", since) | ||
} | ||
return comments, sess.Find(&comments) | ||
return comments, e.Join("INNER", "issue", "issue.id = comment.issue_id"). |
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.
The join is only necessary if opts.RepoID > 0
, it would be preferable to only join when we have to.
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.
done.
LGTM |
make L-G-T-M work |
@ethantkoenig