-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Batch updates for issues #926
Conversation
b5b37a4
to
89c354b
Compare
I don't like having |
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.
Please don't do this 😢
routers/repo/issue.go
Outdated
return | ||
} | ||
// UpdateIssues perform update on collection of issues | ||
func UpdateIssues(ctx *context.Context) { |
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.
Ooh god please no 😱
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.
So do you want
- separate functions for milestones/labels/assignees, each of which can handle both a single or multiple issues?
- keep the pre-existing functions milestones/labels/assignees (which can only handle a single issue), and introduce 3 new milestones/labels/assignees for multiple issues?
Also, what is wrong with what I did? I don't mean to argue, I just want to learn.
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.
Preferably the first one. I'm not read up on the specific functions to give any advice, but I could have a look sometime during the week if you need pointers 🙂
The part that was wrong (in my opinion), was taking 3 small and clean functions, and turning them into one huge, hard to read switch-statement 🙂
08ab657
to
a422be7
Compare
@bkcsoft I have addressed your concerns (combine |
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.
Just two little sintax issues.
public/js/index.js
Outdated
"_csrf": csrf, | ||
"action": action, | ||
"issue_ids": issueIds, | ||
"id": elementId |
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.
Missing indentation of data
object
public/js/index.js
Outdated
@@ -119,13 +125,15 @@ function initCommentForm() { | |||
$(this).removeClass('checked'); | |||
$(this).find('.octicon').removeClass('octicon-check'); | |||
if (hasLabelUpdateAction) { | |||
updateIssueMeta($labelMenu.data('update-url'), "detach", $(this).data('id')); | |||
updateIssuesMeta($labelMenu.data('update-url'), "detach", | |||
$labelMenu.data('issue-id'), $(this).data('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.
I think, or keep function call on a single line, or keep each param in a line, like this:
updateIssuesMeta(
$labelMenu.data('update-url'),
"detach",
$labelMenu.data('issue-id'),
$(this).data('id')
);
Same for similar cases below.
Useful feature. LGTM Ref: gogs/gogs#2049 |
d566013
to
395e9cc
Compare
Rebasing to resolve conflicts. |
395e9cc
to
b168e75
Compare
conflicted. |
b168e75
to
92fa9ea
Compare
Rebased. |
routers/repo/issue.go
Outdated
if len(commaSeparatedIssueIDs) == 0 { | ||
return nil | ||
} | ||
stringIssueIDs := strings.Split(commaSeparatedIssueIDs, ",") |
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.
Why not use x.In().Find()
? It's better performance?
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.
Fixed.
routers/repo/issue.go
Outdated
log.Warn("Unrecognized action: %s", action) | ||
} | ||
|
||
for _, issue := range issues { |
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.
IssueList(issues).LoadRepositories()
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.
Fixed.
otherwise LGTM |
@bkcsoft I've addressed your concerns from a few weeks ago; please re-review when you get the chance 😄 |
b72d61f
to
43af73b
Compare
but seems you changed index.css file and not index.less ? |
Oh, didn't realize there was a |
@lunny Updated to use |
build failed and need @bkcsoft confirmation. |
393b30b
to
230c903
Compare
230c903
to
0d2a991
Compare
LGTM |
#868
Updates the
:reponame/issues/:index/label
:reponame/issues/:index/milestone
:reponame/issues/:index/assignee
endpoints to allow for multiple issues.
Also adds a front-end for opening/closing/assigning/labelling/milestoning multiple issues at once.
I am by no means a front-end expert, so any feedback is welcome 😄