adding support for GitHub v3 delete comment #2540

jyoo980 · 2020-08-31T22:50:27Z

Before submitting a pull-request to GitBucket I have first:

read the contribution guidelines
rebased my branch over master
verified that project is compiling
verified that tests are passing
squashed my commits as appropriate (keep several commits if it is relevant to understand the PR)
marked as closed using commit message all issue ID that this PR should correct

jyoo980 · 2020-08-31T22:51:47Z

src/main/scala/gitbucket/core/service/HandleCommentService.scala

+    implicit context: Context,
+    s: Session
+  ): Option[(Issue, Int)] = context.loginAccount.flatMap { _ =>
+    val deleteResult = deleteComment(comment.issueId, comment.commentId)


I couldn't find any docs for deleteComment here. It returns an Int - am I safe to assume that the returned value is the number of records that were deleted in the DB?

If that's the case, then I think making sure it is equal to a non-zero number (or in this case, 1) would be a good idea. Otherwise I don't think there's any need for this deleteCommentByApi function to return Option[(Issue, Int)]

I couldn't find any docs for deleteComment here. It returns an Int - am I safe to assume that the returned value is the number of records that were deleted in the DB?

I'm not familiar with these codes, but it seems to return from here eventually.

either (1) the row count for SQL Data Manipulation Language (DML) statements
or (2) 0 for SQL statements that return nothing

If that's the case, then I think making sure it is equal to a non-zero number (or in this case, 1) would be a good idea. Otherwise I don't think there's any need for this deleteCommentByApi function to return Option[(Issue, Int)]

What will happen if there is no issue to delete on GitHub?

jyoo980 · 2020-08-31T22:52:31Z

src/main/scala/gitbucket/core/controller/api/ApiIssueCommentControllerBase.scala

+    (for {
+      commentId <- params("id").toIntOpt
+      comment <- getComment(repository.owner, repository.name, commentId.toString)
+      issue <- getIssue(repository.owner, repository.name, comment.issueId.toString)


I'm thinking of doing a permissions check here using isEditable, is that a valid approach?

Yeah, checking whether the user is allowed to update the comment is necessary as same as the comment update API (#2538).

SIkebe

This implementation allows to delete a comment whatever its action is.
e.g. You can delete an add_label comment or a change_milestone comment.

You can't delete those comments via Web UI in GitBucket. Does GitHub allow that?

SIkebe · 2020-09-01T13:53:07Z

src/main/scala/gitbucket/core/controller/api/ApiIssueCommentControllerBase.scala

   */
+  delete("/api/v3/repos/{owner}/{repo}/issues/comments/:id")(readableUsersOnly { repository =>


Suggested change

delete("/api/v3/repos/{owner}/{repo}/issues/comments/:id")(readableUsersOnly { repository =>

delete("/api/v3/repos/:owner/:repository/issues/comments/:id")(readableUsersOnly { repository =>

Hmm, how did you verify that this api work?

SIkebe · 2020-09-01T13:58:58Z

src/main/scala/gitbucket/core/service/HandleCommentService.scala

+    implicit context: Context,
+    s: Session
+  ): Option[(Issue, Int)] = context.loginAccount.flatMap { _ =>
+    val deleteResult = deleteComment(comment.issueId, comment.commentId)


I couldn't find any docs for deleteComment here. It returns an Int - am I safe to assume that the returned value is the number of records that were deleted in the DB?

I'm not familiar with these codes, but it seems to return from here eventually.

either (1) the row count for SQL Data Manipulation Language (DML) statements
or (2) 0 for SQL statements that return nothing

If that's the case, then I think making sure it is equal to a non-zero number (or in this case, 1) would be a good idea. Otherwise I don't think there's any need for this deleteCommentByApi function to return Option[(Issue, Int)]

What will happen if there is no issue to delete on GitHub?

SIkebe · 2020-09-03T14:03:10Z

Closes #2539

takezoe · 2020-09-05T07:31:24Z

This implementation allows to delete a comment whatever its action is.
e.g. You can delete an add_label comment or a change_milestone comment.

You can't delete those comments via Web UI in GitBucket. Does GitHub allow that?

Perhaps, GitHub doesn't treat these events as comments? Anyway, we must not allow to delete these event comments in GitBucket even via Web API.

takezoe · 2020-09-06T19:08:35Z

src/main/scala/gitbucket/core/controller/api/ApiIssueCommentControllerBase.scala

+        comment <- getComment(repository.owner, repository.name, commentId.toString)
+        issue <- getIssue(repository.owner, repository.name, comment.issueId.toString)
+      } yield {
+        if (isEditable(repository.owner, repository.name, comment.commentedUserName)) {


takezoe

LGTM 👍

takezoe · 2020-09-10T20:44:51Z

@jyoo980 Sorry, other pull requests which I merged before this conflicted with this pull request. Could you resolve the conflicts?

jyoo980 · 2020-09-10T20:45:37Z

@takezoe sure, I can take a look this weekend after my classes

takezoe · 2020-09-12T05:52:40Z

Thanks!

takezoe · 2020-09-12T05:54:50Z

@jyoo980 Could you also add this endpoint to the wiki page?
https://github.com/gitbucket/gitbucket/wiki/API-WebHook

jyoo980 · 2020-09-12T17:01:00Z

@takezoe I've added the endpoint to the wiki page. I think we can close this.

adding support for GitHub v3 delete comment

e4f5a21

jyoo980 commented Aug 31, 2020

View reviewed changes

onukura added the APIv3 GitHub compatible API label Sep 1, 2020

SIkebe reviewed Sep 1, 2020

View reviewed changes

takezoe added this to the 4.35.0 milestone Sep 5, 2020

deleting only comment types, requring auth

af52852

takezoe reviewed Sep 6, 2020

View reviewed changes

takezoe approved these changes Sep 10, 2020

View reviewed changes

resolving merge conflicts

b19dfbd

takezoe merged commit 0d20bc0 into gitbucket:master Sep 12, 2020

jyoo980 deleted the jy/add-delete-comment-support branch September 12, 2020 06:05

onukura mentioned this pull request Sep 17, 2020

Add support for GitHub API v3 delete-comment endpoint #2539

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding support for GitHub v3 delete comment #2540

adding support for GitHub v3 delete comment #2540

jyoo980 commented Aug 31, 2020

jyoo980 Aug 31, 2020

SIkebe Sep 1, 2020

jyoo980 Aug 31, 2020

takezoe Sep 5, 2020 •

edited

SIkebe left a comment •

edited

SIkebe Sep 1, 2020

SIkebe Sep 1, 2020

SIkebe Sep 1, 2020

SIkebe commented Sep 3, 2020

takezoe commented Sep 5, 2020

takezoe Sep 6, 2020

takezoe left a comment

takezoe commented Sep 10, 2020 •

edited

jyoo980 commented Sep 10, 2020

takezoe commented Sep 12, 2020

takezoe commented Sep 12, 2020

jyoo980 commented Sep 12, 2020

		*/
		delete("/api/v3/repos/{owner}/{repo}/issues/comments/:id")(readableUsersOnly { repository =>

	delete("/api/v3/repos/{owner}/{repo}/issues/comments/:id")(readableUsersOnly { repository =>
	delete("/api/v3/repos/:owner/:repository/issues/comments/:id")(readableUsersOnly { repository =>

adding support for GitHub v3 delete comment #2540

adding support for GitHub v3 delete comment #2540

Conversation

jyoo980 commented Aug 31, 2020

Before submitting a pull-request to GitBucket I have first:

jyoo980 Aug 31, 2020

Choose a reason for hiding this comment

SIkebe Sep 1, 2020

Choose a reason for hiding this comment

jyoo980 Aug 31, 2020

Choose a reason for hiding this comment

takezoe Sep 5, 2020 • edited

Choose a reason for hiding this comment

SIkebe left a comment • edited

Choose a reason for hiding this comment

SIkebe Sep 1, 2020

Choose a reason for hiding this comment

SIkebe Sep 1, 2020

Choose a reason for hiding this comment

SIkebe Sep 1, 2020

Choose a reason for hiding this comment

SIkebe commented Sep 3, 2020

takezoe commented Sep 5, 2020

takezoe Sep 6, 2020

Choose a reason for hiding this comment

takezoe left a comment

Choose a reason for hiding this comment

takezoe commented Sep 10, 2020 • edited

jyoo980 commented Sep 10, 2020

takezoe commented Sep 12, 2020

takezoe commented Sep 12, 2020

jyoo980 commented Sep 12, 2020

takezoe Sep 5, 2020 •

edited

SIkebe left a comment •

edited

takezoe commented Sep 10, 2020 •

edited