From f1143562994d311d72c2ca6983576b8ad1325a8d Mon Sep 17 00:00:00 2001 From: a1012112796 <1012112796@qq.com> Date: Mon, 31 Aug 2020 19:26:45 +0800 Subject: [PATCH 1/5] Remove duplicate logic in initListSubmits Using the same logic to handle Choosing reviewers and assignees as choosing label. It's the first step of #10926. Signed-off-by: a1012112796 <1012112796@qq.com> --- routers/repo/issue.go | 14 +-- templates/repo/issue/view_content/pull.tmpl | 2 +- .../repo/issue/view_content/sidebar.tmpl | 8 +- web_src/js/index.js | 88 +++++++------------ 4 files changed, 44 insertions(+), 68 deletions(-) diff --git a/routers/repo/issue.go b/routers/repo/issue.go index dabe0f6b0fe1..4bbc355027b5 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1516,10 +1516,11 @@ func updatePullReviewRequest(ctx *context.Context) { } reviewID := ctx.QueryInt64("id") - event := ctx.Query("is_add") + action := ctx.Query("action") - if event != "add" && event != "remove" { - ctx.ServerError("updatePullReviewRequest", fmt.Errorf("is_add should not be \"%s\"", event)) + // TODO: Not support 'clear' now + if action != "attach" && action != "detach" { + ctx.Status(403) return } @@ -1532,19 +1533,20 @@ func updatePullReviewRequest(ctx *context.Context) { return } - err = isLegalReviewRequest(reviewer, ctx.User, event == "add", issue) + err = isLegalReviewRequest(reviewer, ctx.User, action == "attach", issue) if err != nil { ctx.ServerError("isLegalRequestReview", err) return } - err = issue_service.ReviewRequest(issue, ctx.User, reviewer, event == "add") + err = issue_service.ReviewRequest(issue, ctx.User, reviewer, action == "attach") if err != nil { ctx.ServerError("ReviewRequest", err) return } } else { - ctx.ServerError("updatePullReviewRequest", fmt.Errorf("%d in %d is not Pull Request", issue.ID, issue.Repo.ID)) + ctx.Status(403) + return } } diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 94edc8b12662..b9254adeab1c 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -49,7 +49,7 @@ {{end}} {{if $canChoose }} - + {{svg "octicon-sync" 16}} {{end}} diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl index 71e84de2fa72..65d2cce7cba5 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -12,7 +12,7 @@ {{svg "octicon-gear" 16}} {{end}} -
+
{{.i18n.Tr "repo.issues.new.add_reviewer_title"}}
{{if .Reviewers}}
@@ -44,7 +44,7 @@ {{$canChoose = true}} {{end}} - + {{svg "octicon-check" 16}} {{.GetDisplayName}} @@ -78,7 +78,7 @@ {{end}} {{if $canChoose}} - + {{svg "octicon-sync" 16}} {{end}} @@ -244,7 +244,7 @@ {{svg "octicon-gear" 16}} {{end}} -
+
{{.i18n.Tr "repo.issues.new.add_assignees_title"}}
diff --git a/web_src/js/index.js b/web_src/js/index.js index a8965a437f00..8e3135ef861e 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -157,7 +157,7 @@ function initLabelEdit() { }); } -function updateIssuesMeta(url, action, issueIds, elementId, isAdd) { +function updateIssuesMeta(url, action, issueIds, elementId) { return new Promise(((resolve) => { $.ajax({ type: 'POST', @@ -167,7 +167,6 @@ function updateIssuesMeta(url, action, issueIds, elementId, isAdd) { action, issue_ids: issueIds, id: elementId, - is_add: isAdd }, success: resolve }); @@ -373,21 +372,20 @@ function initCommentForm() { const $list = $(`.ui.${outerSelector}.list`); const $noSelect = $list.find('.no-select'); const $listMenu = $(`.${selector} .menu`); - let hasLabelUpdateAction = $listMenu.data('action') === 'update'; - const labels = {}; + let hasUpdateAction = $listMenu.data('action') === 'update'; + const items = {}; $(`.${selector}`).dropdown('setting', 'onHide', () => { - hasLabelUpdateAction = $listMenu.data('action') === 'update'; // Update the var - if (hasLabelUpdateAction) { + hasUpdateAction = $listMenu.data('action') === 'update'; // Update the var + if (hasUpdateAction) { const promises = []; - Object.keys(labels).forEach((elementId) => { - const label = labels[elementId]; + Object.keys(items).forEach((elementId) => { + const item = items[elementId]; const promise = updateIssuesMeta( - label['update-url'], - label.action, - label['issue-id'], + item['update-url'], + item.action, + item['issue-id'], elementId, - label['is-checked'] ); promises.push(promise); }); @@ -396,66 +394,43 @@ function initCommentForm() { }); $listMenu.find('.item:not(.no-select)').on('click', function () { - // we don't need the action attribute when updating assignees - if (selector === 'select-assignees-modify' || selector === 'select-reviewers-modify') { - // UI magic. We need to do this here, otherwise it would destroy the functionality of - // adding/removing labels - - if ($(this).data('can-change') === 'block') { - return false; - } - - if ($(this).hasClass('checked')) { - $(this).removeClass('checked'); - $(this).find('.octicon-check').addClass('invisible'); - $(this).data('is-checked', 'remove'); - } else { - $(this).addClass('checked'); - $(this).find('.octicon-check').removeClass('invisible'); - $(this).data('is-checked', 'add'); - } - - updateIssuesMeta( - $listMenu.data('update-url'), - '', - $listMenu.data('issue-id'), - $(this).data('id'), - $(this).data('is-checked') - ); - $listMenu.data('action', 'update'); // Update to reload the page when we updated items - return false; - } - + hasUpdateAction = $listMenu.data('action') === 'update'; // Update the var if ($(this).hasClass('checked')) { $(this).removeClass('checked'); $(this).find('.octicon-check').addClass('invisible'); - if (hasLabelUpdateAction) { - if (!($(this).data('id') in labels)) { - labels[$(this).data('id')] = { + if (hasUpdateAction) { + if (!($(this).data('id') in items)) { + items[$(this).data('id')] = { 'update-url': $listMenu.data('update-url'), action: 'detach', 'issue-id': $listMenu.data('issue-id'), }; } else { - delete labels[$(this).data('id')]; + delete items[$(this).data('id')]; } } } else { $(this).addClass('checked'); $(this).find('.octicon-check').removeClass('invisible'); - if (hasLabelUpdateAction) { - if (!($(this).data('id') in labels)) { - labels[$(this).data('id')] = { + if (hasUpdateAction) { + if (!($(this).data('id') in items)) { + items[$(this).data('id')] = { 'update-url': $listMenu.data('update-url'), action: 'attach', 'issue-id': $listMenu.data('issue-id'), }; } else { - delete labels[$(this).data('id')]; + delete items[$(this).data('id')]; } } } + // TODO: Which thing should be done for choosing review requests + // to make choosed items be shown on time here? + if (selector === 'select-reviewers-modify' || selector === 'select-assignees-modify') { + return false; + } + const listIds = []; $(this).parent().find('.item').each(function () { if ($(this).hasClass('checked')) { @@ -474,22 +449,24 @@ function initCommentForm() { return false; }); $listMenu.find('.no-select.item').on('click', function () { - if (hasLabelUpdateAction || selector === 'select-assignees-modify') { + if (hasUpdateAction) { updateIssuesMeta( $listMenu.data('update-url'), 'clear', $listMenu.data('issue-id'), '', - '' ).then(reload); } $(this).parent().find('.item').each(function () { $(this).removeClass('checked'); $(this).find('.octicon').addClass('invisible'); - $(this).data('is-checked', 'remove'); }); + if (selector === 'select-reviewers-modify' || selector === 'select-assignees-modify') { + return false; + } + $list.find('.item').each(function () { $(this).addClass('hide'); }); @@ -521,7 +498,6 @@ function initCommentForm() { '', $menu.data('issue-id'), $(this).data('id'), - $(this).data('is-checked') ).then(reload); } switch (input_id) { @@ -552,7 +528,6 @@ function initCommentForm() { '', $menu.data('issue-id'), $(this).data('id'), - $(this).data('is-checked') ).then(reload); } @@ -672,10 +647,9 @@ function initIssueComments() { event.preventDefault(); updateIssuesMeta( url, - '', + isChecked === 'true' ? 'attach' : 'detach', issueId, id, - isChecked ).then(reload); }); From 1553de676cc1d6afb8b01f28fc68101a6475465a Mon Sep 17 00:00:00 2001 From: a1012112796 <1012112796@qq.com> Date: Mon, 31 Aug 2020 20:23:51 +0800 Subject: [PATCH 2/5] fix choose block --- web_src/js/index.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/web_src/js/index.js b/web_src/js/index.js index 8e3135ef861e..0b4f5776bf2c 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -394,6 +394,11 @@ function initCommentForm() { }); $listMenu.find('.item:not(.no-select)').on('click', function () { + const $canChange = $(this).data('can-change'); + if ($canChange.length > 0) { + return false; + } + hasUpdateAction = $listMenu.data('action') === 'update'; // Update the var if ($(this).hasClass('checked')) { $(this).removeClass('checked'); From 08b284a96f839a7758a07d7c0afedc28a3e263df Mon Sep 17 00:00:00 2001 From: a1012112796 <1012112796@qq.com> Date: Mon, 31 Aug 2020 21:23:46 +0800 Subject: [PATCH 3/5] fix nit --- templates/repo/issue/view_content/sidebar.tmpl | 2 +- web_src/js/index.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl index 65d2cce7cba5..d3ea4802bf2d 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -44,7 +44,7 @@ {{$canChoose = true}} {{end}} - + {{svg "octicon-check" 16}} {{.GetDisplayName}} diff --git a/web_src/js/index.js b/web_src/js/index.js index 0b4f5776bf2c..1f8fd443edd6 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -394,8 +394,8 @@ function initCommentForm() { }); $listMenu.find('.item:not(.no-select)').on('click', function () { - const $canChange = $(this).data('can-change'); - if ($canChange.length > 0) { + const $banChange = $(this).data('ban-change'); + if ($banChange.length > 0) { return false; } From 27a98786e482416edce69d417ef6ea2580d3d737 Mon Sep 17 00:00:00 2001 From: a1012112796 <1012112796@qq.com> Date: Wed, 2 Sep 2020 09:14:08 +0800 Subject: [PATCH 4/5] try fix bug --- templates/repo/issue/view_content/sidebar.tmpl | 10 +++++----- web_src/js/index.js | 6 ++++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl index d3ea4802bf2d..0224d7663511 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -106,15 +106,15 @@
{{end}} -
{{.i18n.Tr "repo.issues.new.clear_labels"}}
+
{{.i18n.Tr "repo.issues.new.clear_labels"}}
{{if or .Labels .OrgLabels}} {{range .Labels}} - {{svg "octicon-check" 16}} {{.Name | RenderEmoji}} + {{svg "octicon-check" 16}} {{.Name | RenderEmoji}} {{if .Description }}
{{.Description | RenderEmoji}}{{end}} {{end}}
{{range .OrgLabels}} - {{svg "octicon-check" 16}} {{.Name | RenderEmoji}} + {{svg "octicon-check" 16}} {{.Name | RenderEmoji}} {{if .Description }}
{{.Description | RenderEmoji}}{{end}} {{end}} {{else}} @@ -250,7 +250,7 @@
-
{{.i18n.Tr "repo.issues.new.clear_assignees"}}
+
{{.i18n.Tr "repo.issues.new.clear_assignees"}}
{{range .Assignees}} {{$AssigneeID := .ID}} @@ -258,7 +258,7 @@ {{if eq .ID $AssigneeID}} checked {{end}} - {{end}}" href="#" data-id="{{.ID}}" data-id-selector="#assignee_{{.ID}}"> + {{end}}" href="#" data-id="{{.ID}}" data-id-selector="#assignee_{{.ID}}" data-ban-change=""> {{$checked := false}} {{range $.Issue.Assignees}} {{if eq .ID $AssigneeID}} diff --git a/web_src/js/index.js b/web_src/js/index.js index 1f8fd443edd6..7b2bf699697a 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -393,7 +393,8 @@ function initCommentForm() { } }); - $listMenu.find('.item:not(.no-select)').on('click', function () { + $listMenu.find('.item:not(.no-select)').on('click', function (e) { + e.preventDefault(); const $banChange = $(this).data('ban-change'); if ($banChange.length > 0) { return false; @@ -453,7 +454,8 @@ function initCommentForm() { $($(this).parent().data('id')).val(listIds.join(',')); return false; }); - $listMenu.find('.no-select.item').on('click', function () { + $listMenu.find('.no-select.item').on('click', function (e) { + e.preventDefault(); if (hasUpdateAction) { updateIssuesMeta( $listMenu.data('update-url'), From 500215bec28f97ae80c3a5693546979156c5ab21 Mon Sep 17 00:00:00 2001 From: a1012112796 <1012112796@qq.com> Date: Wed, 2 Sep 2020 09:48:12 +0800 Subject: [PATCH 5/5] simple code --- templates/repo/issue/view_content/sidebar.tmpl | 12 ++++++------ web_src/js/index.js | 3 +-- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl index 0224d7663511..d02dedfb8119 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -44,7 +44,7 @@ {{$canChoose = true}} {{end}} - + {{svg "octicon-check" 16}} {{.GetDisplayName}} @@ -106,15 +106,15 @@
{{end}} -
{{.i18n.Tr "repo.issues.new.clear_labels"}}
+
{{.i18n.Tr "repo.issues.new.clear_labels"}}
{{if or .Labels .OrgLabels}} {{range .Labels}} - {{svg "octicon-check" 16}} {{.Name | RenderEmoji}} + {{svg "octicon-check" 16}} {{.Name | RenderEmoji}} {{if .Description }}
{{.Description | RenderEmoji}}{{end}} {{end}}
{{range .OrgLabels}} - {{svg "octicon-check" 16}} {{.Name | RenderEmoji}} + {{svg "octicon-check" 16}} {{.Name | RenderEmoji}} {{if .Description }}
{{.Description | RenderEmoji}}{{end}} {{end}} {{else}} @@ -250,7 +250,7 @@
-
{{.i18n.Tr "repo.issues.new.clear_assignees"}}
+
{{.i18n.Tr "repo.issues.new.clear_assignees"}}
{{range .Assignees}} {{$AssigneeID := .ID}} @@ -258,7 +258,7 @@ {{if eq .ID $AssigneeID}} checked {{end}} - {{end}}" href="#" data-id="{{.ID}}" data-id-selector="#assignee_{{.ID}}" data-ban-change=""> + {{end}}" href="#" data-id="{{.ID}}" data-id-selector="#assignee_{{.ID}}"> {{$checked := false}} {{range $.Issue.Assignees}} {{if eq .ID $AssigneeID}} diff --git a/web_src/js/index.js b/web_src/js/index.js index 7b2bf699697a..1c23d0f73597 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -395,8 +395,7 @@ function initCommentForm() { $listMenu.find('.item:not(.no-select)').on('click', function (e) { e.preventDefault(); - const $banChange = $(this).data('ban-change'); - if ($banChange.length > 0) { + if ($(this).hasClass('ban-change')) { return false; }