diff --git a/internal/api/controller/pullreq/comment_create.go b/internal/api/controller/pullreq/comment_create.go index e829504de..5eda6f59b 100644 --- a/internal/api/controller/pullreq/comment_create.go +++ b/internal/api/controller/pullreq/comment_create.go @@ -169,11 +169,14 @@ func (c *Controller) CommentCreate( _, err = c.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error { pr.CommentCount++ + if act.IsBlocking() { + pr.UnresolvedCount++ + } return nil }) if err != nil { // non-critical error - log.Ctx(ctx).Err(err).Msgf("failed to increment pull request comment counter") + log.Ctx(ctx).Err(err).Msgf("failed to increment pull request comment counters") } return act, nil @@ -226,7 +229,10 @@ func (c *Controller) writeActivity(ctx context.Context, pr *types.PullReq, act * // sets the correct Order and SubOrder values and writes the activity to the database. // Even if the writing fails, the updating of the sequence number can succeed. func (c *Controller) writeReplyActivity(ctx context.Context, parent, act *types.PullReqActivity) error { - parentUpd, err := c.activityStore.UpdateReplySeq(ctx, parent) + parentUpd, err := c.activityStore.UpdateOptLock(ctx, parent, func(act *types.PullReqActivity) error { + act.ReplySeq++ + return nil + }) if err != nil { return fmt.Errorf("failed to get pull request activity number: %w", err) } diff --git a/internal/api/controller/pullreq/comment_delete.go b/internal/api/controller/pullreq/comment_delete.go index 1d5584b3a..9e6d2f22e 100644 --- a/internal/api/controller/pullreq/comment_delete.go +++ b/internal/api/controller/pullreq/comment_delete.go @@ -10,7 +10,10 @@ import ( "time" "github.com/harness/gitness/internal/auth" + "github.com/harness/gitness/types" "github.com/harness/gitness/types/enum" + + "github.com/rs/zerolog/log" ) // CommentDelete deletes a pull request comment. @@ -40,13 +43,28 @@ func (c *Controller) CommentDelete( return nil } - now := time.Now().UnixMilli() - act.Deleted = &now + isBlocking := act.IsBlocking() - err = c.activityStore.Update(ctx, act) + _, err = c.activityStore.UpdateOptLock(ctx, act, func(act *types.PullReqActivity) error { + now := time.Now().UnixMilli() + act.Deleted = &now + return nil + }) if err != nil { return fmt.Errorf("failed to mark comment as deleted: %w", err) } + _, err = c.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error { + pr.CommentCount-- + if isBlocking { + pr.UnresolvedCount-- + } + return nil + }) + if err != nil { + // non-critical error + log.Ctx(ctx).Err(err).Msgf("failed to decrement pull request comment counters") + } + return nil } diff --git a/internal/api/controller/pullreq/comment_status.go b/internal/api/controller/pullreq/comment_status.go new file mode 100644 index 000000000..baceb0786 --- /dev/null +++ b/internal/api/controller/pullreq/comment_status.go @@ -0,0 +1,117 @@ +// Copyright 2022 Harness Inc. All rights reserved. +// Use of this source code is governed by the Polyform Free Trial License +// that can be found in the LICENSE.md file for this repository. + +package pullreq + +import ( + "context" + "fmt" + "time" + + "github.com/harness/gitness/internal/api/controller" + "github.com/harness/gitness/internal/api/usererror" + "github.com/harness/gitness/internal/auth" + "github.com/harness/gitness/types" + "github.com/harness/gitness/types/enum" +) + +type CommentStatusInput struct { + Status enum.PullReqCommentStatus `json:"status"` +} + +func (in *CommentStatusInput) Validate() error { + _, ok := in.Status.Sanitize() + if !ok { + return usererror.BadRequest("Invalid value provided for comment status") + } + + return nil +} + +func (in *CommentStatusInput) hasChanges(act *types.PullReqActivity, userID int64) bool { + // clearing resolved + if in.Status == enum.PullReqCommentStatusActive { + return act.Resolved != nil + } + // setting resolved + return act.Resolved == nil || act.ResolvedBy == nil || *act.ResolvedBy != userID +} + +// CommentStatus updates a pull request comment status. +func (c *Controller) CommentStatus( + ctx context.Context, + session *auth.Session, + repoRef string, + prNum int64, + commentID int64, + in *CommentStatusInput, +) (*types.PullReqActivity, error) { + repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoView) + if err != nil { + return nil, fmt.Errorf("failed to acquire access to repo: %w", err) + } + + var act *types.PullReqActivity + + err = controller.TxOptLock(ctx, c.db, func(ctx context.Context) error { + pr, err := c.pullreqStore.FindByNumber(ctx, repo.ID, prNum) + if err != nil { + return fmt.Errorf("failed to find pull request by number: %w", err) + } + + if errValidate := in.Validate(); errValidate != nil { + return errValidate + } + + act, err = c.getCommentCheckChangeStatusAccess(ctx, session, pr, commentID) + if err != nil { + return fmt.Errorf("failed to get comment: %w", err) + } + + if !in.hasChanges(act, session.Principal.ID) { + return nil + } + + now := time.Now().UnixMilli() + act.Edited = now + + act.Resolved = nil + act.ResolvedBy = nil + + if in.Status != enum.PullReqCommentStatusActive { + // In the future if we add more comment resolved statuses + // we'll add the ResolvedReason field and put the reason there. + // For now, the nullable timestamp field/db-column "Resolved" tells the status (active/resolved). + act.Resolved = &now + act.ResolvedBy = &session.Principal.ID + } + + err = c.activityStore.Update(ctx, act) + if err != nil { + return fmt.Errorf("failed to update status of pull request activity: %w", err) + } + + // Here we deliberately use the transaction and counting the unresolved comments, + // rather than optimistic locking and incrementing/decrementing the counter. + // The idea is that if the counter ever goes out of sync, this would be the place where we get it back in sync. + unresolvedCount, err := c.activityStore.CountUnresolved(ctx, pr.ID) + if err != nil { + return fmt.Errorf("failed to count unresolved comments: %w", err) + } + + pr.UnresolvedCount = unresolvedCount + + err = c.pullreqStore.Update(ctx, pr) + if err != nil { + return fmt.Errorf("failed to update pull request's unresolved comment count: %w", err) + } + + return nil + }) + if err != nil { + return nil, err + } + + return act, nil +} diff --git a/internal/api/controller/pullreq/comment_update.go b/internal/api/controller/pullreq/comment_update.go index 9ce5ffd1c..513755704 100644 --- a/internal/api/controller/pullreq/comment_update.go +++ b/internal/api/controller/pullreq/comment_update.go @@ -18,6 +18,11 @@ type CommentUpdateInput struct { Text string `json:"text"` } +func (in *CommentUpdateInput) Validate() error { + // TODO: Check Text length + return nil +} + func (in *CommentUpdateInput) hasChanges(act *types.PullReqActivity) bool { return in.Text != act.Text } @@ -41,6 +46,10 @@ func (c *Controller) CommentUpdate( return nil, fmt.Errorf("failed to find pull request by number: %w", err) } + if errValidate := in.Validate(); errValidate != nil { + return nil, errValidate + } + act, err := c.getCommentCheckEditAccess(ctx, session, pr, commentID) if err != nil { return nil, fmt.Errorf("failed to get comment: %w", err) @@ -50,12 +59,12 @@ func (c *Controller) CommentUpdate( return act, nil } - now := time.Now().UnixMilli() - act.Edited = now - - act.Text = in.Text - - err = c.activityStore.Update(ctx, act) + act, err = c.activityStore.UpdateOptLock(ctx, act, func(act *types.PullReqActivity) error { + now := time.Now().UnixMilli() + act.Edited = now + act.Text = in.Text + return nil + }) if err != nil { return nil, fmt.Errorf("failed to update comment: %w", err) } diff --git a/internal/api/controller/pullreq/controller.go b/internal/api/controller/pullreq/controller.go index 434a5859a..8209c1f82 100644 --- a/internal/api/controller/pullreq/controller.go +++ b/internal/api/controller/pullreq/controller.go @@ -121,7 +121,7 @@ func (c *Controller) getRepoCheckAccess(ctx context.Context, return repo, nil } -func (c *Controller) getCommentCheckEditAccess(ctx context.Context, +func (c *Controller) getCommentCheckModifyAccess(ctx context.Context, session *auth.Session, pr *types.PullReq, commentID int64, ) (*types.PullReqActivity, error) { if commentID <= 0 { @@ -133,7 +133,7 @@ func (c *Controller) getCommentCheckEditAccess(ctx context.Context, return nil, fmt.Errorf("failed to find comment by ID: %w", err) } - if comment == nil || comment.Type != enum.PullReqActivityTypeComment { + if comment == nil { return nil, usererror.ErrNotFound } @@ -145,6 +145,21 @@ func (c *Controller) getCommentCheckEditAccess(ctx context.Context, return nil, usererror.BadRequest("Can't update a comment created by the system.") } + if comment.Type != enum.PullReqActivityTypeComment && comment.Type != enum.PullReqActivityTypeCodeComment { + return nil, usererror.BadRequest("Only comments and code comments can be edited.") + } + + return comment, nil +} + +func (c *Controller) getCommentCheckEditAccess(ctx context.Context, + session *auth.Session, pr *types.PullReq, commentID int64, +) (*types.PullReqActivity, error) { + comment, err := c.getCommentCheckModifyAccess(ctx, session, pr, commentID) + if err != nil { + return nil, err + } + if comment.CreatedBy != session.Principal.ID { return nil, usererror.BadRequest("Only own comments may be updated.") } @@ -152,6 +167,21 @@ func (c *Controller) getCommentCheckEditAccess(ctx context.Context, return comment, nil } +func (c *Controller) getCommentCheckChangeStatusAccess(ctx context.Context, + session *auth.Session, pr *types.PullReq, commentID int64, +) (*types.PullReqActivity, error) { + comment, err := c.getCommentCheckModifyAccess(ctx, session, pr, commentID) + if err != nil { + return nil, err + } + + if comment.SubOrder != 0 { + return nil, usererror.BadRequest("Can't change status of replies.") + } + + return comment, nil +} + func (c *Controller) checkIfAlreadyExists(ctx context.Context, targetRepoID, sourceRepoID int64, targetBranch, sourceBranch string, ) error { diff --git a/internal/api/controller/tx.go b/internal/api/controller/tx.go new file mode 100644 index 000000000..a892b1151 --- /dev/null +++ b/internal/api/controller/tx.go @@ -0,0 +1,36 @@ +// Copyright 2022 Harness Inc. All rights reserved. +// Use of this source code is governed by the Polyform Free Trial License +// that can be found in the LICENSE.md file for this repository. + +package controller + +import ( + "context" + "errors" + "github.com/harness/gitness/internal/store" + "github.com/harness/gitness/internal/store/database/dbtx" + "github.com/jmoiron/sqlx" +) + +type TxOptionRetryCount int + +// TxOptLock runs the provided function inside a database transaction. If optimistic lock error occurs +// during the operation, the function will retry the whole transaction again (to the maximum of 5 times, +// but this can be overridden by providing an additional TxOptionRetryCount option) +func TxOptLock(ctx context.Context, db *sqlx.DB, txFn func(ctx context.Context) error, opts ...interface{}) (err error) { + tries := 5 + for _, opt := range opts { + if n, ok := opt.(TxOptionRetryCount); ok { + tries = int(n) + } + } + + for try := 0; try < tries; try++ { + err = dbtx.New(db).WithTx(ctx, txFn, opts...) + if !errors.Is(err, store.ErrVersionConflict) { + break + } + } + + return +} diff --git a/internal/api/handler/pullreq/comment_status.go b/internal/api/handler/pullreq/comment_status.go new file mode 100644 index 000000000..cc0fb2a2a --- /dev/null +++ b/internal/api/handler/pullreq/comment_status.go @@ -0,0 +1,55 @@ +// Copyright 2022 Harness Inc. All rights reserved. +// Use of this source code is governed by the Polyform Free Trial License +// that can be found in the LICENSE.md file for this repository. + +package pullreq + +import ( + "encoding/json" + "net/http" + + "github.com/harness/gitness/internal/api/controller/pullreq" + "github.com/harness/gitness/internal/api/render" + "github.com/harness/gitness/internal/api/request" +) + +// HandleCommentStatus is an HTTP handler for updating a pull request comment status. +func HandleCommentStatus(pullreqCtrl *pullreq.Controller) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + session, _ := request.AuthSessionFrom(ctx) + + repoRef, err := request.GetRepoRefFromPath(r) + if err != nil { + render.TranslatedUserError(w, err) + return + } + + pullreqNumber, err := request.GetPullReqNumberFromPath(r) + if err != nil { + render.TranslatedUserError(w, err) + return + } + + commentID, err := request.GetPullReqCommentIDPath(r) + if err != nil { + render.TranslatedUserError(w, err) + return + } + + in := new(pullreq.CommentStatusInput) + err = json.NewDecoder(r.Body).Decode(in) + if err != nil { + render.BadRequestf(w, "Invalid Request Body: %s.", err) + return + } + + comment, err := pullreqCtrl.CommentStatus(ctx, session, repoRef, pullreqNumber, commentID, in) + if err != nil { + render.TranslatedUserError(w, err) + return + } + + render.JSON(w, http.StatusOK, comment) + } +} diff --git a/internal/api/openapi/pullreq.go b/internal/api/openapi/pullreq.go index b90b2b2b2..14eff0e04 100644 --- a/internal/api/openapi/pullreq.go +++ b/internal/api/openapi/pullreq.go @@ -71,7 +71,11 @@ type commentUpdatePullReqRequest struct { type commentDeletePullReqRequest struct { pullReqCommentRequest - pullreq.CommentUpdateInput +} + +type commentStatusPullReqRequest struct { + pullReqCommentRequest + pullreq.CommentStatusInput } type reviewerListPullReqRequest struct { @@ -364,6 +368,18 @@ func pullReqOperations(reflector *openapi3.Reflector) { _ = reflector.Spec.AddOperation(http.MethodDelete, "/repos/{repo_ref}/pullreq/{pullreq_number}/comments/{pullreq_comment_id}", commentDeletePullReq) + commentStatusPullReq := openapi3.Operation{} + commentStatusPullReq.WithTags("pullreq") + commentStatusPullReq.WithMapOfAnything(map[string]interface{}{"operationId": "commentStatusPullReq"}) + _ = reflector.SetRequest(&commentStatusPullReq, new(commentStatusPullReqRequest), http.MethodPut) + _ = reflector.SetJSONResponse(&commentStatusPullReq, new(types.PullReqActivity), http.StatusOK) + _ = reflector.SetJSONResponse(&commentStatusPullReq, new(usererror.Error), http.StatusBadRequest) + _ = reflector.SetJSONResponse(&commentStatusPullReq, new(usererror.Error), http.StatusInternalServerError) + _ = reflector.SetJSONResponse(&commentStatusPullReq, new(usererror.Error), http.StatusUnauthorized) + _ = reflector.SetJSONResponse(&commentStatusPullReq, new(usererror.Error), http.StatusForbidden) + _ = reflector.Spec.AddOperation(http.MethodPut, + "/repos/{repo_ref}/pullreq/{pullreq_number}/comments/{pullreq_comment_id}/status", commentStatusPullReq) + reviewerAdd := openapi3.Operation{} reviewerAdd.WithTags("pullreq") reviewerAdd.WithMapOfAnything(map[string]interface{}{"operationId": "reviewerAddPullReq"}) diff --git a/internal/router/api.go b/internal/router/api.go index 37ee10c8a..211723398 100644 --- a/internal/router/api.go +++ b/internal/router/api.go @@ -266,6 +266,7 @@ func SetupPullReq(r chi.Router, pullreqCtrl *pullreq.Controller) { r.Route(fmt.Sprintf("/{%s}", request.PathParamPullReqCommentID), func(r chi.Router) { r.Patch("/", handlerpullreq.HandleCommentUpdate(pullreqCtrl)) r.Delete("/", handlerpullreq.HandleCommentDelete(pullreqCtrl)) + r.Put("/status", handlerpullreq.HandleCommentStatus(pullreqCtrl)) }) }) r.Route("/reviewers", func(r chi.Router) { diff --git a/internal/store/database.go b/internal/store/database.go index 145fb7445..e841550bb 100644 --- a/internal/store/database.go +++ b/internal/store/database.go @@ -256,7 +256,7 @@ type ( Create(ctx context.Context, pullreq *types.PullReq) error // Update the pull request. It will set new values to the Version and Updated fields. - Update(ctx context.Context, repo *types.PullReq) error + Update(ctx context.Context, pr *types.PullReq) error // UpdateOptLock the pull request details using the optimistic locking mechanism. UpdateOptLock(ctx context.Context, pr *types.PullReq, @@ -281,7 +281,7 @@ type ( Find(ctx context.Context, id int64) (*types.PullReqActivity, error) // Create a new pull request activity. Value of the Order field should be fetched with UpdateActivitySeq. - // Value of the SubOrder field (for replies) should be fetched with UpdateReplySeq (non-replies have 0). + // Value of the SubOrder field (for replies) should be the incremented ReplySeq field (non-replies have 0). Create(ctx context.Context, act *types.PullReqActivity) error // CreateWithPayload create a new system activity from the provided payload. @@ -291,13 +291,18 @@ type ( // Update the pull request activity. It will set new values to the Version and Updated fields. Update(ctx context.Context, act *types.PullReqActivity) error - // UpdateReplySeq the pull request activity's reply sequence number. - // It will set new values to the ReplySeq, Version and Updated fields. - UpdateReplySeq(ctx context.Context, act *types.PullReqActivity) (*types.PullReqActivity, error) + // UpdateOptLock updates the pull request activity using the optimistic locking mechanism. + UpdateOptLock(ctx context.Context, + act *types.PullReqActivity, + mutateFn func(act *types.PullReqActivity) error, + ) (*types.PullReqActivity, error) // Count returns number of pull request activities in a pull request. Count(ctx context.Context, prID int64, opts *types.PullReqActivityFilter) (int64, error) + // CountUnresolved returns number of unresolved comments. + CountUnresolved(ctx context.Context, prID int64) (int, error) + // List returns a list of pull request activities in a pull request (a timeline). List(ctx context.Context, prID int64, opts *types.PullReqActivityFilter) ([]*types.PullReqActivity, error) } diff --git a/internal/store/database/migrate/postgres/0016_alter_pullreq_add_unresolved.down.sql b/internal/store/database/migrate/postgres/0016_alter_pullreq_add_unresolved.down.sql new file mode 100644 index 000000000..e59610193 --- /dev/null +++ b/internal/store/database/migrate/postgres/0016_alter_pullreq_add_unresolved.down.sql @@ -0,0 +1 @@ +ALTER TABLE pullreqs DROP COLUMN pullreq_unresolved_count; diff --git a/internal/store/database/migrate/postgres/0016_alter_pullreq_add_unresolved.up.sql b/internal/store/database/migrate/postgres/0016_alter_pullreq_add_unresolved.up.sql new file mode 100644 index 000000000..18449aa10 --- /dev/null +++ b/internal/store/database/migrate/postgres/0016_alter_pullreq_add_unresolved.up.sql @@ -0,0 +1,18 @@ +ALTER TABLE pullreqs ADD COLUMN pullreq_unresolved_count INTEGER NOT NULL DEFAULT 0; + +WITH unresolved_counts AS ( + SELECT + pullreq_activity_pullreq_id AS "unresolved_pullreq_id", + COUNT(*) AS "unresolved_count" + FROM pullreq_activities + WHERE + pullreq_activity_sub_order = 0 AND + pullreq_activity_resolved IS NULL AND + pullreq_activity_deleted IS NULL AND + pullreq_activity_kind <> 'system' + GROUP BY pullreq_activity_pullreq_id +) +UPDATE pullreqs +SET pullreq_unresolved_count = unresolved_counts.unresolved_count +FROM unresolved_counts +WHERE pullreq_id = unresolved_pullreq_id; diff --git a/internal/store/database/migrate/sqlite/0016_alter_pullreq_add_unresolved.down.sql b/internal/store/database/migrate/sqlite/0016_alter_pullreq_add_unresolved.down.sql new file mode 100644 index 000000000..e59610193 --- /dev/null +++ b/internal/store/database/migrate/sqlite/0016_alter_pullreq_add_unresolved.down.sql @@ -0,0 +1 @@ +ALTER TABLE pullreqs DROP COLUMN pullreq_unresolved_count; diff --git a/internal/store/database/migrate/sqlite/0016_alter_pullreq_add_unresolved.up.sql b/internal/store/database/migrate/sqlite/0016_alter_pullreq_add_unresolved.up.sql new file mode 100644 index 000000000..18449aa10 --- /dev/null +++ b/internal/store/database/migrate/sqlite/0016_alter_pullreq_add_unresolved.up.sql @@ -0,0 +1,18 @@ +ALTER TABLE pullreqs ADD COLUMN pullreq_unresolved_count INTEGER NOT NULL DEFAULT 0; + +WITH unresolved_counts AS ( + SELECT + pullreq_activity_pullreq_id AS "unresolved_pullreq_id", + COUNT(*) AS "unresolved_count" + FROM pullreq_activities + WHERE + pullreq_activity_sub_order = 0 AND + pullreq_activity_resolved IS NULL AND + pullreq_activity_deleted IS NULL AND + pullreq_activity_kind <> 'system' + GROUP BY pullreq_activity_pullreq_id +) +UPDATE pullreqs +SET pullreq_unresolved_count = unresolved_counts.unresolved_count +FROM unresolved_counts +WHERE pullreq_id = unresolved_pullreq_id; diff --git a/internal/store/database/pullreq.go b/internal/store/database/pullreq.go index 71fbd9113..b06fbaf93 100644 --- a/internal/store/database/pullreq.go +++ b/internal/store/database/pullreq.go @@ -54,7 +54,8 @@ type pullReq struct { State enum.PullReqState `db:"pullreq_state"` IsDraft bool `db:"pullreq_is_draft"` - CommentCount int `db:"pullreq_comment_count"` + CommentCount int `db:"pullreq_comment_count"` + UnresolvedCount int `db:"pullreq_unresolved_count"` Title string `db:"pullreq_title"` Description string `db:"pullreq_description"` @@ -90,6 +91,7 @@ const ( ,pullreq_state ,pullreq_is_draft ,pullreq_comment_count + ,pullreq_unresolved_count ,pullreq_title ,pullreq_description ,pullreq_source_repo_id @@ -178,6 +180,7 @@ func (s *PullReqStore) Create(ctx context.Context, pr *types.PullReq) error { ,pullreq_state ,pullreq_is_draft ,pullreq_comment_count + ,pullreq_unresolved_count ,pullreq_title ,pullreq_description ,pullreq_source_repo_id @@ -204,6 +207,7 @@ func (s *PullReqStore) Create(ctx context.Context, pr *types.PullReq) error { ,:pullreq_state ,:pullreq_is_draft ,:pullreq_comment_count + ,:pullreq_unresolved_count ,:pullreq_title ,:pullreq_description ,:pullreq_source_repo_id @@ -247,6 +251,7 @@ func (s *PullReqStore) Update(ctx context.Context, pr *types.PullReq) error { ,pullreq_state = :pullreq_state ,pullreq_is_draft = :pullreq_is_draft ,pullreq_comment_count = :pullreq_comment_count + ,pullreq_unresolved_count = :pullreq_unresolved_count ,pullreq_title = :pullreq_title ,pullreq_description = :pullreq_description ,pullreq_activity_seq = :pullreq_activity_seq @@ -288,8 +293,7 @@ func (s *PullReqStore) Update(ctx context.Context, pr *types.PullReq) error { return store.ErrVersionConflict } - pr.Version = dbPR.Version - pr.Updated = dbPR.Updated + *pr = *s.mapPullReq(ctx, dbPR) return nil } @@ -468,6 +472,7 @@ func mapPullReq(pr *pullReq) *types.PullReq { State: pr.State, IsDraft: pr.IsDraft, CommentCount: pr.CommentCount, + UnresolvedCount: pr.UnresolvedCount, Title: pr.Title, Description: pr.Description, SourceRepoID: pr.SourceRepoID, @@ -487,7 +492,8 @@ func mapPullReq(pr *pullReq) *types.PullReq { Author: types.PrincipalInfo{}, Merger: nil, Stats: types.PullReqStats{ - Conversations: pr.CommentCount, + Conversations: pr.CommentCount, + UnresolvedCount: pr.UnresolvedCount, DiffStats: types.DiffStats{ Commits: 0, FilesChanged: 0, @@ -508,6 +514,7 @@ func mapInternalPullReq(pr *types.PullReq) *pullReq { State: pr.State, IsDraft: pr.IsDraft, CommentCount: pr.CommentCount, + UnresolvedCount: pr.UnresolvedCount, Title: pr.Title, Description: pr.Description, SourceRepoID: pr.SourceRepoID, diff --git a/internal/store/database/pullreq_activity.go b/internal/store/database/pullreq_activity.go index 73a1ba030..b25ba7c28 100644 --- a/internal/store/database/pullreq_activity.go +++ b/internal/store/database/pullreq_activity.go @@ -287,20 +287,25 @@ func (s *PullReqActivityStore) Update(ctx context.Context, act *types.PullReqAct return store.ErrVersionConflict } - act.Version = dbAct.Version - act.Updated = dbAct.Updated + *act = *s.mapPullReqActivity(ctx, dbAct) return nil } -// UpdateReplySeq updates the pull request activity's reply sequence. -func (s *PullReqActivityStore) UpdateReplySeq(ctx context.Context, - act *types.PullReqActivity) (*types.PullReqActivity, error) { +// UpdateOptLock updates the pull request using the optimistic locking mechanism. +func (s *PullReqActivityStore) UpdateOptLock(ctx context.Context, + act *types.PullReqActivity, + mutateFn func(act *types.PullReqActivity) error, +) (*types.PullReqActivity, error) { for { dup := *act - dup.ReplySeq++ - err := s.Update(ctx, &dup) + err := mutateFn(&dup) + if err != nil { + return nil, err + } + + err = s.Update(ctx, &dup) if err == nil { return &dup, nil } @@ -414,6 +419,32 @@ func (s *PullReqActivityStore) List(ctx context.Context, prID int64, return result, nil } +func (s *PullReqActivityStore) CountUnresolved(ctx context.Context, prID int64) (int, error) { + stmt := builder. + Select("count(*)"). + From("pullreq_activities"). + Where("pullreq_activity_pullreq_id = ?", prID). + Where("pullreq_activity_sub_order = 0"). + Where("pullreq_activity_resolved IS NULL"). + Where("pullreq_activity_deleted IS NULL"). + Where("pullreq_activity_kind <> ?", enum.PullReqActivityKindSystem) + + sql, args, err := stmt.ToSql() + if err != nil { + return 0, errors.Wrap(err, "Failed to convert query to sql") + } + + db := dbtx.GetAccessor(ctx, s.db) + + var count int + err = db.QueryRowContext(ctx, sql, args...).Scan(&count) + if err != nil { + return 0, processSQLErrorf(err, "Failed executing count unresolved query") + } + + return count, nil +} + func mapPullReqActivity(act *pullReqActivity) *types.PullReqActivity { m := &types.PullReqActivity{ ID: act.ID, diff --git a/types/enum/pullreq.go b/types/enum/pullreq.go index c8cf68348..b6ea17d11 100644 --- a/types/enum/pullreq.go +++ b/types/enum/pullreq.go @@ -111,6 +111,30 @@ var pullReqActivityKinds = sortEnum([]PullReqActivityKind{ PullReqActivityKindChangeComment, }) +// PullReqCommentStatus defines status of a pull request comment. +type PullReqCommentStatus string + +func (PullReqCommentStatus) Enum() []interface{} { return toInterfaceSlice(pullReqCommentStatuses) } + +func (s PullReqCommentStatus) Sanitize() (PullReqCommentStatus, bool) { + return Sanitize(s, GetAllPullReqCommentStatuses) +} + +func GetAllPullReqCommentStatuses() ([]PullReqCommentStatus, PullReqCommentStatus) { + return pullReqCommentStatuses, "" // No default value +} + +// PullReqCommentStatus enumeration. +const ( + PullReqCommentStatusActive PullReqCommentStatus = "active" + PullReqCommentStatusResolved PullReqCommentStatus = "resolved" +) + +var pullReqCommentStatuses = sortEnum([]PullReqCommentStatus{ + PullReqCommentStatusActive, + PullReqCommentStatusResolved, +}) + // PullReqReviewDecision defines state of a pull request review. type PullReqReviewDecision string diff --git a/types/pullreq.go b/types/pullreq.go index 16288d010..f5a90229d 100644 --- a/types/pullreq.go +++ b/types/pullreq.go @@ -22,7 +22,8 @@ type PullReq struct { State enum.PullReqState `json:"state"` IsDraft bool `json:"is_draft"` - CommentCount int `json:"-"` // returned as "conversations" in the Stats + CommentCount int `json:"-"` // returned as "conversations" in the Stats + UnresolvedCount int `json:"-"` // returned as "unresolved_count" in the Stats Title string `json:"title"` Description string `json:"description"` @@ -52,14 +53,15 @@ type PullReq struct { // DiffStats shows total number of commits and modified files. type DiffStats struct { - Commits int `json:"commits"` - FilesChanged int `json:"files_changed"` + Commits int `json:"commits,omitempty"` + FilesChanged int `json:"files_changed,omitempty"` } // PullReqStats shows Diff statistics and number of conversations. type PullReqStats struct { DiffStats - Conversations int `json:"conversations,omitempty"` + Conversations int `json:"conversations,omitempty"` + UnresolvedCount int `json:"unresolved_count,omitempty"` } // PullReqFilter stores pull request query parameters. diff --git a/types/pullreq_activity.go b/types/pullreq_activity.go index b8fac9b7c..a7f3b0d74 100644 --- a/types/pullreq_activity.go +++ b/types/pullreq_activity.go @@ -92,6 +92,11 @@ func (a *PullReqActivity) IsReply() bool { return a.SubOrder > 0 } +// IsBlocking returns true if the pull request activity (comment/code-comment) is blocking the pull request merge. +func (a *PullReqActivity) IsBlocking() bool { + return a.SubOrder == 0 && a.Resolved == nil && a.Deleted == nil && a.Kind != enum.PullReqActivityKindSystem +} + // SetPayload sets the payload and verifies it's of correct type for the activity. func (a *PullReqActivity) SetPayload(payload PullReqActivityPayload) error { if payload == nil {