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

models/issues/stopwatch.go

@@ -5,7 +5,6 @@ package issues
 
 import (
 	"context"
-	"fmt"
 	"time"
 
 	"code.gitea.io/gitea/models/db"
@@ -15,20 +14,6 @@ import (
 	"code.gitea.io/gitea/modules/util"
 )
 
-// ErrIssueStopwatchNotExist represents an error that stopwatch is not exist
-type ErrIssueStopwatchNotExist struct {
-	UserID  int64
-	IssueID int64
-}
-
-func (err ErrIssueStopwatchNotExist) Error() string {
-	return fmt.Sprintf("issue stopwatch doesn't exist[uid: %d, issue_id: %d", err.UserID, err.IssueID)
-}
-
-func (err ErrIssueStopwatchNotExist) Unwrap() error {
-	return util.ErrNotExist
-}
-
 // Stopwatch represents a stopwatch for time tracking.
 type Stopwatch struct {
 	ID          int64              `xorm:"pk autoincr"`
@@ -55,13 +40,11 @@ func getStopwatch(ctx context.Context, userID, issueID int64) (sw *Stopwatch, ex
 	return sw, exists, err
 }
 
-// UserIDCount is a simple coalition of UserID and Count
 type UserStopwatch struct {
 	UserID      int64
 	StopWatches []*Stopwatch
 }
 
-// GetUIDsAndNotificationCounts between the two provided times
 func GetUIDsAndStopwatch(ctx context.Context) ([]*UserStopwatch, error) {
 	sws := []*Stopwatch{}
 	if err := db.GetEngine(ctx).Where("issue_id != 0").Find(&sws); err != nil {
@@ -87,7 +70,7 @@ func GetUIDsAndStopwatch(ctx context.Context) ([]*UserStopwatch, error) {
 	return res, nil
 }
 
-// GetUserStopwatches return list of all stopwatches of a user
+// GetUserStopwatches return list of the user's all stopwatches
 func GetUserStopwatches(ctx context.Context, userID int64, listOptions db.ListOptions) ([]*Stopwatch, error) {
 	sws := make([]*Stopwatch, 0, 8)
 	sess := db.GetEngine(ctx).Where("stopwatch.user_id = ?", userID)
@@ -102,7 +85,7 @@ func GetUserStopwatches(ctx context.Context, userID int64, listOptions db.ListOp
 	return sws, nil
 }
 
-// CountUserStopwatches return count of all stopwatches of a user
+// CountUserStopwatches return count of the user's all stopwatches
 func CountUserStopwatches(ctx context.Context, userID int64) (int64, error) {
 	return db.GetEngine(ctx).Where("user_id = ?", userID).Count(&Stopwatch{})
 }
@@ -136,43 +119,21 @@ func HasUserStopwatch(ctx context.Context, userID int64) (exists bool, sw *Stopw
 	return exists, sw, issue, err
 }
 
-// FinishIssueStopwatchIfPossible if stopwatch exist then finish it otherwise ignore
-func FinishIssueStopwatchIfPossible(ctx context.Context, user *user_model.User, issue *Issue) error {
-	_, exists, err := getStopwatch(ctx, user.ID, issue.ID)
-	if err != nil {
-		return err
-	}
-	if !exists {
-		return nil
-	}
-	return FinishIssueStopwatch(ctx, user, issue)
-}
-
-// CreateOrStopIssueStopwatch create an issue stopwatch if it's not exist, otherwise finish it
-func CreateOrStopIssueStopwatch(ctx context.Context, user *user_model.User, issue *Issue) error {
-	_, exists, err := getStopwatch(ctx, user.ID, issue.ID)
-	if err != nil {
-		return err
-	}
-	if exists {
-		return FinishIssueStopwatch(ctx, user, issue)
-	}
-	return CreateIssueStopwatch(ctx, user, issue)
-}
-
-// FinishIssueStopwatch if stopwatch exist then finish it otherwise return an error
-func FinishIssueStopwatch(ctx context.Context, user *user_model.User, issue *Issue) error {
+// FinishIssueStopwatch if stopwatch exists, then finish it.
+func FinishIssueStopwatch(ctx context.Context, user *user_model.User, issue *Issue) (ok bool, err error) {
 	sw, exists, err := getStopwatch(ctx, user.ID, issue.ID)
 	if err != nil {
-		return err
+		return false, err
+	} else if !exists {
+		return false, nil
 	}
-	if !exists {
-		return ErrIssueStopwatchNotExist{
-			UserID:  user.ID,
-			IssueID: issue.ID,
-		}
+	if err = finishIssueStopwatch(ctx, user, issue, sw); err != nil {
+		return false, err
 	}
+	return true, nil
+}
 
+func finishIssueStopwatch(ctx context.Context, user *user_model.User, issue *Issue, sw *Stopwatch) error {
 	// Create tracked time out of the time difference between start date and actual date
 	timediff := time.Now().Unix() - int64(sw.CreatedUnix)
 
@@ -184,14 +145,12 @@ func FinishIssueStopwatch(ctx context.Context, user *user_model.User, issue *Iss
 		Time:    timediff,
 	}
 
-	if err := db.Insert(ctx, tt); err != nil {
+	if err := issue.LoadRepo(ctx); err != nil {
 		return err
 	}
-
-	if err := issue.LoadRepo(ctx); err != nil {
+	if err := db.Insert(ctx, tt); err != nil {
 		return err
 	}
-
 	if _, err := CreateComment(ctx, &CreateCommentOptions{
 		Doer:    user,
 		Issue:   issue,
@@ -202,90 +161,74 @@ func FinishIssueStopwatch(ctx context.Context, user *user_model.User, issue *Iss
 	}); err != nil {
 		return err
 	}
-	_, err = db.DeleteByBean(ctx, sw)
+	_, err := db.DeleteByBean(ctx, sw)
 	return err
 }
 
-// CreateIssueStopwatch creates a stopwatch if not exist, otherwise return an error
-func CreateIssueStopwatch(ctx context.Context, user *user_model.User, issue *Issue) error {
-	if err := issue.LoadRepo(ctx); err != nil {
-		return err
-	}
-
-	// if another stopwatch is running: stop it
-	exists, _, otherIssue, err := HasUserStopwatch(ctx, user.ID)
-	if err != nil {
-		return err
-	}
-	if exists {
-		if err := FinishIssueStopwatch(ctx, user, otherIssue); err != nil {
-			return err
+// CreateIssueStopwatch creates a stopwatch if the issue doesn't have the user's stopwatch.
+// It also stops any other stopwatch that might be running for the user.
+func CreateIssueStopwatch(ctx context.Context, user *user_model.User, issue *Issue) (ok bool, err error) {
+	{ // if another issue's stopwatch is running: stop it; if this issue has a stopwatch: return an error.
+		exists, otherStopWatch, otherIssue, err := HasUserStopwatch(ctx, user.ID)
+		if err != nil {
+			return false, err
+		}
+		if exists {
+			if otherStopWatch.IssueID == issue.ID {
+				// don't allow starting stopwatch for the same issue
+				return false, nil
+			}
+			// stop the other issue's stopwatch
+			if err = finishIssueStopwatch(ctx, user, otherIssue, otherStopWatch); err != nil {
+				return false, err
+			}
 		}
 	}
 
-	// Create stopwatch
-	sw := &Stopwatch{
-		UserID:  user.ID,
-		IssueID: issue.ID,
+	if err = issue.LoadRepo(ctx); err != nil {
+		return false, err
 	}
-
-	if err := db.Insert(ctx, sw); err != nil {
-		return err
+	if err = db.Insert(ctx, &Stopwatch{UserID: user.ID, IssueID: issue.ID}); err != nil {
+		return false, err
 	}
-
-	if err := issue.LoadRepo(ctx); err != nil {
-		return err
-	}
-
-	if _, err := CreateComment(ctx, &CreateCommentOptions{
+	if _, err = CreateComment(ctx, &CreateCommentOptions{
 		Doer:  user,
 		Issue: issue,
 		Repo:  issue.Repo,
 		Type:  CommentTypeStartTracking,
 	}); err != nil {
-		return err
+		return false, err
 	}
-
-	return nil
+	return true, nil
 }
 
 // CancelStopwatch removes the given stopwatch and logs it into issue's timeline.
-func CancelStopwatch(ctx context.Context, user *user_model.User, issue *Issue) error {
-	ctx, committer, err := db.TxContext(ctx)
-	if err != nil {
-		return err
-	}
-	defer committer.Close()
-	if err := cancelStopwatch(ctx, user, issue); err != nil {
-		return err
-	}
-	return committer.Commit()
-}
-
-func cancelStopwatch(ctx context.Context, user *user_model.User, issue *Issue) error {
-	e := db.GetEngine(ctx)
-	sw, exists, err := getStopwatch(ctx, user.ID, issue.ID)
-	if err != nil {
-		return err
-	}
-
-	if exists {
-		if _, err := e.Delete(sw); err != nil {
+func CancelStopwatch(ctx context.Context, user *user_model.User, issue *Issue) (ok bool, err error) {
+	err = db.WithTx(ctx, func(ctx context.Context) error {
+		e := db.GetEngine(ctx)
+		sw, exists, err := getStopwatch(ctx, user.ID, issue.ID)
+		if err != nil {
 			return err
+		} else if !exists {
+			return nil
 		}
 
-		if err := issue.LoadRepo(ctx); err != nil {
+		if err = issue.LoadRepo(ctx); err != nil {
 			return err
 		}
-
-		if _, err := CreateComment(ctx, &CreateCommentOptions{
+		if _, err = e.Delete(sw); err != nil {
+			return err
+		}
+		if _, err = CreateComment(ctx, &CreateCommentOptions{
 			Doer:  user,
 			Issue: issue,
 			Repo:  issue.Repo,
 			Type:  CommentTypeCancelTracking,
 		}); err != nil {
 			return err
 		}
-	}
-	return nil
+		ok = true
+		return nil
+	})
+	return ok, err
 }

models/issues/stopwatch_test.go

@@ -10,34 +10,29 @@ import (
 	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
-	"code.gitea.io/gitea/modules/timeutil"
 
 	"github.com/stretchr/testify/assert"
 )
 
 func TestCancelStopwatch(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 
-	user1, err := user_model.GetUserByID(db.DefaultContext, 1)
-	assert.NoError(t, err)
-
-	issue1, err := issues_model.GetIssueByID(db.DefaultContext, 1)
-	assert.NoError(t, err)
-	issue2, err := issues_model.GetIssueByID(db.DefaultContext, 2)
-	assert.NoError(t, err)
+	user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+	issue1 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 1})
 
-	err = issues_model.CancelStopwatch(db.DefaultContext, user1, issue1)
+	ok, err := issues_model.CancelStopwatch(db.DefaultContext, user1, issue1)
 	assert.NoError(t, err)
+	assert.True(t, ok)
 	unittest.AssertNotExistsBean(t, &issues_model.Stopwatch{UserID: user1.ID, IssueID: issue1.ID})
+	unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{Type: issues_model.CommentTypeCancelTracking, PosterID: user1.ID, IssueID: issue1.ID})
 
-	_ = unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{Type: issues_model.CommentTypeCancelTracking, PosterID: user1.ID, IssueID: issue1.ID})
-
-	assert.NoError(t, issues_model.CancelStopwatch(db.DefaultContext, user1, issue2))
+	ok, err = issues_model.CancelStopwatch(db.DefaultContext, user1, issue1)
+	assert.NoError(t, err)
+	assert.False(t, ok)
 }
 
 func TestStopwatchExists(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
-
 	assert.True(t, issues_model.StopwatchExists(db.DefaultContext, 1, 1))
 	assert.False(t, issues_model.StopwatchExists(db.DefaultContext, 1, 2))
 }
@@ -58,21 +53,35 @@ func TestHasUserStopwatch(t *testing.T) {
 func TestCreateOrStopIssueStopwatch(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 
-	user2, err := user_model.GetUserByID(db.DefaultContext, 2)
+	user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
+	issue1 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 1})
+	issue3 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 3})
+
+	// create a new stopwatch
+	ok, err := issues_model.CreateIssueStopwatch(db.DefaultContext, user4, issue1)
 	assert.NoError(t, err)
-	org3, err := user_model.GetUserByID(db.DefaultContext, 3)
+	assert.True(t, ok)
+	unittest.AssertExistsAndLoadBean(t, &issues_model.Stopwatch{UserID: user4.ID, IssueID: issue1.ID})
+	// should not create a second stopwatch for the same issue
+	ok, err = issues_model.CreateIssueStopwatch(db.DefaultContext, user4, issue1)
 	assert.NoError(t, err)
-
-	issue1, err := issues_model.GetIssueByID(db.DefaultContext, 1)
+	assert.False(t, ok)
+	// on a different issue, it will finish the existing stopwatch and create a new one
+	ok, err = issues_model.CreateIssueStopwatch(db.DefaultContext, user4, issue3)
 	assert.NoError(t, err)
-	issue2, err := issues_model.GetIssueByID(db.DefaultContext, 2)
+	assert.True(t, ok)
+	unittest.AssertNotExistsBean(t, &issues_model.Stopwatch{UserID: user4.ID, IssueID: issue1.ID})
+	unittest.AssertExistsAndLoadBean(t, &issues_model.Stopwatch{UserID: user4.ID, IssueID: issue3.ID})
+
+	// user2 already has a stopwatch in test fixture
+	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	issue2 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
+	ok, err = issues_model.FinishIssueStopwatch(db.DefaultContext, user2, issue2)
 	assert.NoError(t, err)
-
-	assert.NoError(t, issues_model.CreateOrStopIssueStopwatch(db.DefaultContext, org3, issue1))
-	sw := unittest.AssertExistsAndLoadBean(t, &issues_model.Stopwatch{UserID: 3, IssueID: 1})
-	assert.LessOrEqual(t, sw.CreatedUnix, timeutil.TimeStampNow())
-
-	assert.NoError(t, issues_model.CreateOrStopIssueStopwatch(db.DefaultContext, user2, issue2))
-	unittest.AssertNotExistsBean(t, &issues_model.Stopwatch{UserID: 2, IssueID: 2})
-	unittest.AssertExistsAndLoadBean(t, &issues_model.TrackedTime{UserID: 2, IssueID: 2})
+	assert.True(t, ok)
+	unittest.AssertNotExistsBean(t, &issues_model.Stopwatch{UserID: user2.ID, IssueID: issue2.ID})
+	unittest.AssertExistsAndLoadBean(t, &issues_model.TrackedTime{UserID: user2.ID, IssueID: issue2.ID})
+	ok, err = issues_model.FinishIssueStopwatch(db.DefaultContext, user2, issue2)
+	assert.NoError(t, err)
+	assert.False(t, ok)
 }

options/locale/locale_en-US.ini

@@ -1726,6 +1726,8 @@ issues.remove_time_estimate_at = removed time estimate %s
 issues.time_estimate_invalid = Time estimate format is invalid
 issues.start_tracking_history = started working %s
 issues.tracker_auto_close = Timer will be stopped automatically when this issue gets closed
+issues.stopwatch_already_stopped = The timer for this issue is already stopped
+issues.stopwatch_already_created = The timer for this issue already exists
 issues.tracking_already_started = `You have already started time tracking on <a href="%s">another issue</a>!`
 issues.stop_tracking = Stop Timer
 issues.stop_tracking_history = worked for <b>%[1]s</b> %[2]s