fix(issue): Replace stopwatch toggle with explicit start/stop actions (#34818)

This PR fixes a state de-synchronization bug with the issue stopwatch,
it resolves the issue by replacing the ambiguous `/toggle` endpoint
with two explicit endpoints: `/start` and `/stop`.

- The "Start timer" button now exclusively calls the `/start` endpoint.
- The "Stop timer" button now exclusively calls the `/stop` endpoint.

This ensures the user's intent is clearly communicated to the server,
eliminating the state inconsistency and fixing the bug.

---------

Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

routers/api/v1/repo/issue_stopwatch.go

@@ -4,7 +4,6 @@
 package repo
 
 import (
-	"errors"
 	"net/http"
 
 	issues_model "code.gitea.io/gitea/models/issues"
@@ -49,14 +48,17 @@ func StartIssueStopwatch(ctx *context.APIContext) {
 	//   "409":
 	//     description: Cannot start a stopwatch again if it already exists
 
-	issue, err := prepareIssueStopwatch(ctx, false)
-	if err != nil {
+	issue := prepareIssueForStopwatch(ctx)
+	if ctx.Written() {
 		return
 	}
 
-	if err := issues_model.CreateIssueStopwatch(ctx, ctx.Doer, issue); err != nil {
+	if ok, err := issues_model.CreateIssueStopwatch(ctx, ctx.Doer, issue); err != nil {
 		ctx.APIErrorInternal(err)
 		return
+	} else if !ok {
+		ctx.APIError(http.StatusConflict, "cannot start a stopwatch again if it already exists")
+		return
 	}
 
 	ctx.Status(http.StatusCreated)
@@ -96,18 +98,20 @@ func StopIssueStopwatch(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 	//   "409":
-	//     description:  Cannot stop a non existent stopwatch
+	//     description:  Cannot stop a non-existent stopwatch
 
-	issue, err := prepareIssueStopwatch(ctx, true)
-	if err != nil {
+	issue := prepareIssueForStopwatch(ctx)
+	if ctx.Written() {
 		return
 	}
 
-	if err := issues_model.FinishIssueStopwatch(ctx, ctx.Doer, issue); err != nil {
+	if ok, err := issues_model.FinishIssueStopwatch(ctx, ctx.Doer, issue); err != nil {
 		ctx.APIErrorInternal(err)
 		return
+	} else if !ok {
+		ctx.APIError(http.StatusConflict, "cannot stop a non-existent stopwatch")
+		return
 	}
-
 	ctx.Status(http.StatusCreated)
 }
 
@@ -145,22 +149,25 @@ func DeleteIssueStopwatch(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 	//   "409":
-	//     description:  Cannot cancel a non existent stopwatch
+	//     description:  Cannot cancel a non-existent stopwatch
 
-	issue, err := prepareIssueStopwatch(ctx, true)
-	if err != nil {
+	issue := prepareIssueForStopwatch(ctx)
+	if ctx.Written() {
 		return
 	}
 
-	if err := issues_model.CancelStopwatch(ctx, ctx.Doer, issue); err != nil {
+	if ok, err := issues_model.CancelStopwatch(ctx, ctx.Doer, issue); err != nil {
 		ctx.APIErrorInternal(err)
 		return
+	} else if !ok {
+		ctx.APIError(http.StatusConflict, "cannot cancel a non-existent stopwatch")
+		return
 	}
 
 	ctx.Status(http.StatusNoContent)
 }
 
-func prepareIssueStopwatch(ctx *context.APIContext, shouldExist bool) (*issues_model.Issue, error) {
+func prepareIssueForStopwatch(ctx *context.APIContext) *issues_model.Issue {
 	issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("index"))
 	if err != nil {
 		if issues_model.IsErrIssueNotExist(err) {
@@ -168,32 +175,19 @@ func prepareIssueStopwatch(ctx *context.APIContext, shouldExist bool) (*issues_m
 		} else {
 			ctx.APIErrorInternal(err)
 		}
-
-		return nil, err
+		return nil
 	}
 
 	if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) {
 		ctx.Status(http.StatusForbidden)
-		return nil, errors.New("Unable to write to PRs")
+		return nil
 	}
 
 	if !ctx.Repo.CanUseTimetracker(ctx, issue, ctx.Doer) {
 		ctx.Status(http.StatusForbidden)
-		return nil, errors.New("Cannot use time tracker")
+		return nil
 	}
-
-	if issues_model.StopwatchExists(ctx, ctx.Doer.ID, issue.ID) != shouldExist {
-		if shouldExist {
-			ctx.APIError(http.StatusConflict, "cannot stop/cancel a non existent stopwatch")
-			err = errors.New("cannot stop/cancel a non existent stopwatch")
-		} else {
-			ctx.APIError(http.StatusConflict, "cannot start a stopwatch again if it already exists")
-			err = errors.New("cannot start a stopwatch again if it already exists")
-		}
-		return nil, err
-	}
-
-	return issue, nil
+	return issue
 }
 
 // GetStopwatches get all stopwatches

routers/web/repo/issue_stopwatch.go

@@ -10,33 +10,47 @@ import (
 	"code.gitea.io/gitea/services/context"
 )
 
-// IssueStopwatch creates or stops a stopwatch for the given issue.
-func IssueStopwatch(c *context.Context) {
+// IssueStartStopwatch creates a stopwatch for the given issue.
+func IssueStartStopwatch(c *context.Context) {
 	issue := GetActionIssue(c)
 	if c.Written() {
 		return
 	}
 
-	var showSuccessMessage bool
-
-	if !issues_model.StopwatchExists(c, c.Doer.ID, issue.ID) {
-		showSuccessMessage = true
-	}
-
 	if !c.Repo.CanUseTimetracker(c, issue, c.Doer) {
 		c.NotFound(nil)
 		return
 	}
 
-	if err := issues_model.CreateOrStopIssueStopwatch(c, c.Doer, issue); err != nil {
-		c.ServerError("CreateOrStopIssueStopwatch", err)
+	if ok, err := issues_model.CreateIssueStopwatch(c, c.Doer, issue); err != nil {
+		c.ServerError("CreateIssueStopwatch", err)
+		return
+	} else if !ok {
+		c.Flash.Warning(c.Tr("repo.issues.stopwatch_already_created"))
+	} else {
+		c.Flash.Success(c.Tr("repo.issues.tracker_auto_close"))
+	}
+	c.JSONRedirect("")
+}
+
+// IssueStopStopwatch stops a stopwatch for the given issue.
+func IssueStopStopwatch(c *context.Context) {
+	issue := GetActionIssue(c)
+	if c.Written() {
 		return
 	}
 
-	if showSuccessMessage {
-		c.Flash.Success(c.Tr("repo.issues.tracker_auto_close"))
+	if !c.Repo.CanUseTimetracker(c, issue, c.Doer) {
+		c.NotFound(nil)
+		return
 	}
 
+	if ok, err := issues_model.FinishIssueStopwatch(c, c.Doer, issue); err != nil {
+		c.ServerError("FinishIssueStopwatch", err)
+		return
+	} else if !ok {
+		c.Flash.Warning(c.Tr("repo.issues.stopwatch_already_stopped"))
+	}
 	c.JSONRedirect("")
 }
 
@@ -51,7 +65,7 @@ func CancelStopwatch(c *context.Context) {
 		return
 	}
 
-	if err := issues_model.CancelStopwatch(c, c.Doer, issue); err != nil {
+	if _, err := issues_model.CancelStopwatch(c, c.Doer, issue); err != nil {
 		c.ServerError("CancelStopwatch", err)
 		return
 	}

routers/web/repo/pull.go

@@ -1258,13 +1258,8 @@ func CancelAutoMergePullRequest(ctx *context.Context) {
 }
 
 func stopTimerIfAvailable(ctx *context.Context, user *user_model.User, issue *issues_model.Issue) error {
-	if issues_model.StopwatchExists(ctx, user.ID, issue.ID) {
-		if err := issues_model.CreateOrStopIssueStopwatch(ctx, user, issue); err != nil {
-			return err
-		}
-	}
-
-	return nil
+	_, err := issues_model.FinishIssueStopwatch(ctx, user, issue)
+	return err
 }
 
 func PullsNewRedirect(ctx *context.Context) {

routers/web/web.go

@@ -1253,7 +1253,8 @@ func registerWebRoutes(m *web.Router) {
 					m.Post("/add", web.Bind(forms.AddTimeManuallyForm{}), repo.AddTimeManually)
 					m.Post("/{timeid}/delete", repo.DeleteTime)
 					m.Group("/stopwatch", func() {
-						m.Post("/toggle", repo.IssueStopwatch)
+						m.Post("/start", repo.IssueStartStopwatch)
+						m.Post("/stop", repo.IssueStopStopwatch)
 						m.Post("/cancel", repo.CancelStopwatch)
 					})
 				})