Skip to content

Batch delete issue and improve tippy opts #25253

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Jun 19, 2023
Merged

Batch delete issue and improve tippy opts #25253

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
58015ba
fix
wxiaoguang Jun 14, 2023
cb30446
fix
wxiaoguang Jun 14, 2023
2768846
fix button style
wxiaoguang Jun 14, 2023
868d059
destructure all the things
silverwind Jun 14, 2023
b1ddb8c
move config destructure to top
silverwind Jun 14, 2023
1f4463a
Revert "destructure all the things"
wxiaoguang Jun 15, 2023
3d825d1
add comments
wxiaoguang Jun 15, 2023
8960441
confirm_delete_selected = Confirm to delete all selected items?
wxiaoguang Jun 15, 2023
d00df2c
Merge branch 'main' into batch-delete-issue
wxiaoguang Jun 16, 2023
c45e233
Merge branch 'main' into batch-delete-issue
wxiaoguang Jun 16, 2023
627d58b
Merge branch 'main' into batch-delete-issue
GiteaBot Jun 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions modules/context/base.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ func (b *Base) JSONRedirect(redirect string) {
b.JSON(http.StatusOK, map[string]any{"redirect": redirect})
}

func (b *Base) JSONOK() {
b.JSON(http.StatusOK, map[string]any{"ok": true}) // this is only a dummy response, frontend seldom uses it
}

// RemoteAddr returns the client machine ip address
func (b *Base) RemoteAddr() string {
return b.Req.RemoteAddr
Expand Down
1 change: 1 addition & 0 deletions options/locale/locale_en-US.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,6 +1412,7 @@ issues.filter_sort.fewestforks = Fewest forks
issues.keyword_search_unavailable = Currently searching by keyword is not available. Please contact your site administrator.
issues.action_open = Open
issues.action_close = Close
issues.action_delete_confirm = Confirm to delete all issues?
Copy link
Member

@silverwind silverwind Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Would prefer something more generic:

delete_all_confirm = Delete all items?

And likely move it to top-level translation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 8960441

But I still do not think it is a generic string.

And the "generic" problem is: without careful maintenance, it will become more and more messy. For example, why rerun_all = Re-run all jobsappears in the global scope? It isn't generic indeed.

Copy link
Member

Choose a reason for hiding this comment

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

If a translation could reasonably appear in more than 1 scope, it should be global, but I guess the one you posted is likely to be single-use.

Overall, I think we should just get rid of scopes altogether in the translation, removing all duplicates in the process. Those scope have no benefit except encourage duplication.

issues.action_label = Label
issues.action_milestone = Milestone
issues.action_milestone_no_select = No milestone
Expand Down
18 changes: 15 additions & 3 deletions routers/web/repo/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2705,6 +2705,20 @@ func ListIssues(ctx *context.Context) {
ctx.JSON(http.StatusOK, convert.ToAPIIssueList(ctx, issues))
}

func BatchDeleteIssues(ctx *context.Context) {
issues := getActionIssues(ctx)
if ctx.Written() {
return
}
for _, issue := range issues {
if err := issue_service.DeleteIssue(ctx, ctx.Doer, ctx.Repo.GitRepo, issue); err != nil {
ctx.ServerError("DeleteIssue", err)
return
}
}
ctx.JSONOK()
}

// UpdateIssueStatus change issue's status
func UpdateIssueStatus(ctx *context.Context) {
issues := getActionIssues(ctx)
Expand Down Expand Up @@ -2740,9 +2754,7 @@ func UpdateIssueStatus(ctx *context.Context) {
}
}
}
ctx.JSON(http.StatusOK, map[string]interface{}{
"ok": true,
})
ctx.JSONOK()
}

// NewComment create a comment for issue
Expand Down
1 change: 1 addition & 0 deletions routers/web/web.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1029,6 +1029,7 @@ func registerRoutes(m *web.Route) {
m.Post("/request_review", reqRepoIssuesOrPullsReader, repo.UpdatePullReviewRequest)
m.Post("/dismiss_review", reqRepoAdmin, web.Bind(forms.DismissReviewForm{}), repo.DismissReview)
m.Post("/status", reqRepoIssuesOrPullsWriter, repo.UpdateIssueStatus)
m.Post("/delete", reqRepoAdmin, repo.BatchDeleteIssues)
m.Post("/resolve_conversation", reqRepoIssuesOrPullsReader, repo.UpdateResolveConversation)
m.Post("/attachments", repo.UploadIssueAttachment)
m.Post("/attachments/remove", repo.DeleteAttachment)
Expand Down
4 changes: 3 additions & 1 deletion templates/devtest/fetch-action.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
It might be renamed to "link-fetch-action" to match the "form-fetch-action".
</div>
<div>
<button class="link-action" data-url="fetch-action-test?k=1">test</button>
<button class="link-action" data-url="fetch-action-test?k=1">test action</button>
<button class="link-action" data-url="fetch-action-test?k=1" data-modal-confirm="confirm?">test with confirm</button>
<button class="ui red button link-action" data-url="fetch-action-test?k=1" data-modal-confirm="confirm?">test with risky confirm</button>
</div>
</div>
<div>
Expand Down
10 changes: 8 additions & 2 deletions templates/repo/issue/list.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,9 +281,15 @@
{{if not .Repository.IsArchived}}
<!-- Action Button -->
{{if .IsShowClosed}}
<button class="ui green active basic button issue-action gt-ml-auto" data-action="open" data-url="{{$.RepoLink}}/issues/status">{{.locale.Tr "repo.issues.action_open"}}</button>
<button class="ui green basic button issue-action gt-ml-auto" data-action="open" data-url="{{$.RepoLink}}/issues/status">{{.locale.Tr "repo.issues.action_open"}}</button>
{{else}}
<button class="ui red active basic button issue-action gt-ml-auto" data-action="close" data-url="{{$.RepoLink}}/issues/status">{{.locale.Tr "repo.issues.action_close"}}</button>
<button class="ui red basic button issue-action gt-ml-auto" data-action="close" data-url="{{$.RepoLink}}/issues/status">{{.locale.Tr "repo.issues.action_close"}}</button>
{{end}}
{{if $.IsRepoAdmin}}
<button class="ui red button issue-action gt-ml-auto"
data-action="delete" data-url="{{$.RepoLink}}/issues/delete"
data-action-delete-confirm="{{.locale.Tr "repo.issues.action_delete_confirm"}}"
>{{.locale.Tr "repo.issues.delete"}}</button>
{{end}}
<!-- Labels -->
<div class="ui {{if not .Labels}}disabled{{end}} dropdown jump item">
Expand Down
34 changes: 9 additions & 25 deletions web_src/js/features/common-global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import {svg} from '../svg.js';
import {hideElem, showElem, toggleElem} from '../utils/dom.js';
import {htmlEscape} from 'escape-goat';
import {createTippy} from '../modules/tippy.js';
import {confirmModal} from './comp/ConfirmModal.js';

const {appUrl, csrfToken, i18n} = window.config;
const {appUrl, csrfToken} = window.config;

export function initGlobalFormDirtyLeaveConfirm() {
// Warn users that try to leave a page after entering data into a form.
Expand Down Expand Up @@ -250,7 +251,7 @@ export function initGlobalDropzone() {
}
}

function linkAction(e) {
async function linkAction(e) {
e.preventDefault();

// A "link-action" can post AJAX request to its "data-url"
Expand All @@ -277,33 +278,16 @@ function linkAction(e) {
});
};

const modalConfirmHtml = htmlEscape($this.attr('data-modal-confirm') || '');
if (!modalConfirmHtml) {
const modalConfirmContent = htmlEscape($this.attr('data-modal-confirm') || '');
if (!modalConfirmContent) {
doRequest();
return;
}

const okButtonColor = $this.hasClass('red') || $this.hasClass('yellow') || $this.hasClass('orange') || $this.hasClass('negative') ? 'orange' : 'green';

const $modal = $(`
<div class="ui g-modal-confirm modal">
<div class="content">${modalConfirmHtml}</div>
<div class="actions">
<button class="ui basic cancel button">${svg('octicon-x')} ${i18n.modal_cancel}</button>
<button class="ui ${okButtonColor} ok button">${svg('octicon-check')} ${i18n.modal_confirm}</button>
</div>
</div>
`);

$modal.appendTo(document.body);
$modal.modal({
onApprove() {
doRequest();
},
onHidden() {
$modal.remove();
},
}).modal('show');
const isRisky = $this.hasClass('red') || $this.hasClass('yellow') || $this.hasClass('orange') || $this.hasClass('negative');
if (await confirmModal({content: modalConfirmContent, buttonColor: isRisky ? 'orange' : 'green'})) {
doRequest();
}
}

export function initGlobalLinkActions() {
Expand Down
30 changes: 30 additions & 0 deletions web_src/js/features/comp/ConfirmModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import $ from 'jquery';
import {svg} from '../../svg.js';
import {htmlEscape} from 'escape-goat';

const {i18n} = window.config;

export async function confirmModal(opts = {content: '', buttonColor: 'green'}) {
return new Promise((resolve) => {
const $modal = $(`
<div class="ui g-modal-confirm modal">
<div class="content">${htmlEscape(opts.content)}</div>
<div class="actions">
<button class="ui basic cancel button">${svg('octicon-x')} ${i18n.modal_cancel}</button>
<button class="ui ${opts.buttonColor || 'green'} ok button">${svg('octicon-check')} ${i18n.modal_confirm}</button>
</div>
</div>
`);

$modal.appendTo(document.body);
$modal.modal({
onApprove() {
resolve(true);
},
onHidden() {
$modal.remove();
resolve(false);
},
}).modal('show');
});
}
28 changes: 23 additions & 5 deletions web_src/js/features/repo-issue-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {updateIssuesMeta} from './repo-issue.js';
import {toggleElem} from '../utils/dom.js';
import {htmlEscape} from 'escape-goat';
import {Sortable} from 'sortablejs';
import {confirmModal} from './comp/ConfirmModal.js';

function initRepoIssueListCheckboxes() {
const $issueSelectAll = $('.issue-checkbox-all');
Expand Down Expand Up @@ -36,19 +37,36 @@ function initRepoIssueListCheckboxes() {

$('.issue-action').on('click', async function (e) {
e.preventDefault();

const url = this.getAttribute('data-url');
let action = this.getAttribute('data-action');
let elementId = this.getAttribute('data-element-id');
const url = this.getAttribute('data-url');
const issueIDs = $('.issue-checkbox:checked').map((_, el) => {
return el.getAttribute('data-issue-id');
}).get().join(',');
if (elementId === '0' && url.slice(-9) === '/assignee') {
let issueIDs = [];
for (const el of document.querySelectorAll('.issue-checkbox:checked')) {
issueIDs.push(el.getAttribute('data-issue-id'));
}
issueIDs = issueIDs.join(',');
if (!issueIDs) return;

// for assignee
if (elementId === '0' && url.endsWith('/assignee')) {
elementId = '';
action = 'clear';
}

// for toggle
if (action === 'toggle' && e.altKey) {
action = 'toggle-alt';
}

// for delete
if (action === 'delete') {
const confirmText = e.target.getAttribute('data-action-delete-confirm');
if (!await confirmModal({content: confirmText, buttonColor: 'orange'})) {
return;
}
}

updateIssuesMeta(
url,
action,
Expand Down
16 changes: 6 additions & 10 deletions web_src/js/modules/tippy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,7 @@ import tippy from 'tippy.js';

const visibleInstances = new Set();

export function createTippy(target, opts = {}) {
const {role, content, onHide: optsOnHide, onDestroy: optsOnDestroy, onShow: optOnShow} = opts;
delete opts.onHide;
delete opts.onDestroy;
delete opts.onShow;

export function createTippy(target, {onHide, onShow, onDestroy, role, content, ...other} = {}) {
Copy link
Contributor Author

@wxiaoguang wxiaoguang Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Well, this is wrong now (that's what I said why it is really unclear).

Because the role is exacted, other doesn't contain role. Then how do you use ...other to override the default role ....

I reverted the code, the old code looks good to me.

Copy link
Member

Choose a reason for hiding this comment

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

The only change that was needed would have been role: role || 'menu'. I still want to do my method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think current style is the best, the reasons are:

  1. export function createTippy(target, opts = {}) is shorter and clearer for readers, it's easy to catch the function declaration.
  2. I remember that you mentioned that you don't want to read a line with more than 100 chars, using destructing in the function declaration makes the line longer and longer.
  3. If you destructure "role", this time you can patch the code by role: role || 'menu', next time if another variable is destructed, developers would forget the ...other overriding again. It is quite fragile and unclear. For example, I have mentioned that the code would be more complex for using "role" later., the changed code still contains bugs, so it is a complex and counterintuitive problem.

Copy link
Member

Choose a reason for hiding this comment

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

Well I guess we can keep it.

const instance = tippy(target, {
appendTo: document.body,
animation: false,
Expand All @@ -18,11 +13,11 @@ export function createTippy(target, opts = {}) {
maxWidth: 500, // increase over default 350px
onHide: (instance) => {
visibleInstances.delete(instance);
return optsOnHide?.(instance);
return onHide?.(instance);
},
onDestroy: (instance) => {
visibleInstances.delete(instance);
return optsOnDestroy?.(instance);
return onDestroy?.(instance);
},
onShow: (instance) => {
// hide other tooltip instances so only one tooltip shows at a time
Expand All @@ -32,12 +27,13 @@ export function createTippy(target, opts = {}) {
}
}
visibleInstances.add(instance);
return optOnShow?.(instance);
return onShow?.(instance);
},
arrow: `<svg width="16" height="7"><path d="m0 7 8-7 8 7Z" class="tippy-svg-arrow-outer"/><path d="m0 8 8-7 8 7Z" class="tippy-svg-arrow-inner"/></svg>`,
role: 'menu', // HTML role attribute, only tooltips should use "tooltip"
theme: role || 'menu', // CSS theme, we support either "tooltip" or "menu"
...opts,
content,
...other,
});

// for popups where content refers to a DOM element, we use the 'tippy-target' class
Expand Down