Skip to content

Commit 0e629c5

Browse files
jskong1124wxiaoguang
jskong1124
and
wxiaoguang
authored
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 <[email protected]> Co-authored-by: wxiaoguang <[email protected]>
1 parent 63fb253 commit 0e629c5

File tree

13 files changed

+161
-203
lines changed

13 files changed

+161
-203
lines changed

‎models/issues/stopwatch.go‎

Lines changed: 55 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ package issues
55

66
import (
77
"context"
8-
"fmt"
98
"time"
109

1110
"code.gitea.io/gitea/models/db"
@@ -15,20 +14,6 @@ import (
1514
"code.gitea.io/gitea/modules/util"
1615
)
1716

18-
// ErrIssueStopwatchNotExist represents an error that stopwatch is not exist
19-
type ErrIssueStopwatchNotExist struct {
20-
UserID int64
21-
IssueID int64
22-
}
23-
24-
func (err ErrIssueStopwatchNotExist) Error() string {
25-
return fmt.Sprintf("issue stopwatch doesn't exist[uid: %d, issue_id: %d", err.UserID, err.IssueID)
26-
}
27-
28-
func (err ErrIssueStopwatchNotExist) Unwrap() error {
29-
return util.ErrNotExist
30-
}
31-
3217
// Stopwatch represents a stopwatch for time tracking.
3318
type Stopwatch struct {
3419
ID int64 `xorm:"pk autoincr"`
@@ -55,13 +40,11 @@ func getStopwatch(ctx context.Context, userID, issueID int64) (sw *Stopwatch, ex
5540
return sw, exists, err
5641
}
5742

58-
// UserIDCount is a simple coalition of UserID and Count
5943
type UserStopwatch struct {
6044
UserID int64
6145
StopWatches []*Stopwatch
6246
}
6347

64-
// GetUIDsAndNotificationCounts between the two provided times
6548
func GetUIDsAndStopwatch(ctx context.Context) ([]*UserStopwatch, error) {
6649
sws := []*Stopwatch{}
6750
if err := db.GetEngine(ctx).Where("issue_id != 0").Find(&sws); err != nil {
@@ -87,7 +70,7 @@ func GetUIDsAndStopwatch(ctx context.Context) ([]*UserStopwatch, error) {
8770
return res, nil
8871
}
8972

90-
// GetUserStopwatches return list of all stopwatches of a user
73+
// GetUserStopwatches return list of the user's all stopwatches
9174
func GetUserStopwatches(ctx context.Context, userID int64, listOptions db.ListOptions) ([]*Stopwatch, error) {
9275
sws := make([]*Stopwatch, 0, 8)
9376
sess := db.GetEngine(ctx).Where("stopwatch.user_id = ?", userID)
@@ -102,7 +85,7 @@ func GetUserStopwatches(ctx context.Context, userID int64, listOptions db.ListOp
10285
return sws, nil
10386
}
10487

105-
// CountUserStopwatches return count of all stopwatches of a user
88+
// CountUserStopwatches return count of the user's all stopwatches
10689
func CountUserStopwatches(ctx context.Context, userID int64) (int64, error) {
10790
return db.GetEngine(ctx).Where("user_id = ?", userID).Count(&Stopwatch{})
10891
}
@@ -136,43 +119,21 @@ func HasUserStopwatch(ctx context.Context, userID int64) (exists bool, sw *Stopw
136119
return exists, sw, issue, err
137120
}
138121

139-
// FinishIssueStopwatchIfPossible if stopwatch exist then finish it otherwise ignore
140-
func FinishIssueStopwatchIfPossible(ctx context.Context, user *user_model.User, issue *Issue) error {
141-
_, exists, err := getStopwatch(ctx, user.ID, issue.ID)
142-
if err != nil {
143-
return err
144-
}
145-
if !exists {
146-
return nil
147-
}
148-
return FinishIssueStopwatch(ctx, user, issue)
149-
}
150-
151-
// CreateOrStopIssueStopwatch create an issue stopwatch if it's not exist, otherwise finish it
152-
func CreateOrStopIssueStopwatch(ctx context.Context, user *user_model.User, issue *Issue) error {
153-
_, exists, err := getStopwatch(ctx, user.ID, issue.ID)
154-
if err != nil {
155-
return err
156-
}
157-
if exists {
158-
return FinishIssueStopwatch(ctx, user, issue)
159-
}
160-
return CreateIssueStopwatch(ctx, user, issue)
161-
}
162-
163-
// FinishIssueStopwatch if stopwatch exist then finish it otherwise return an error
164-
func FinishIssueStopwatch(ctx context.Context, user *user_model.User, issue *Issue) error {
122+
// FinishIssueStopwatch if stopwatch exists, then finish it.
123+
func FinishIssueStopwatch(ctx context.Context, user *user_model.User, issue *Issue) (ok bool, err error) {
165124
sw, exists, err := getStopwatch(ctx, user.ID, issue.ID)
166125
if err != nil {
167-
return err
126+
return false, err
127+
} else if !exists {
128+
return false, nil
168129
}
169-
if !exists {
170-
return ErrIssueStopwatchNotExist{
171-
UserID: user.ID,
172-
IssueID: issue.ID,
173-
}
130+
if err = finishIssueStopwatch(ctx, user, issue, sw); err != nil {
131+
return false, err
174132
}
133+
return true, nil
134+
}
175135

136+
func finishIssueStopwatch(ctx context.Context, user *user_model.User, issue *Issue, sw *Stopwatch) error {
176137
// Create tracked time out of the time difference between start date and actual date
177138
timediff := time.Now().Unix() - int64(sw.CreatedUnix)
178139

@@ -184,14 +145,12 @@ func FinishIssueStopwatch(ctx context.Context, user *user_model.User, issue *Iss
184145
Time: timediff,
185146
}
186147

187-
if err := db.Insert(ctx, tt); err != nil {
148+
if err := issue.LoadRepo(ctx); err != nil {
188149
return err
189150
}
190-
191-
if err := issue.LoadRepo(ctx); err != nil {
151+
if err := db.Insert(ctx, tt); err != nil {
192152
return err
193153
}
194-
195154
if _, err := CreateComment(ctx, &CreateCommentOptions{
196155
Doer: user,
197156
Issue: issue,
@@ -202,90 +161,74 @@ func FinishIssueStopwatch(ctx context.Context, user *user_model.User, issue *Iss
202161
}); err != nil {
203162
return err
204163
}
205-
_, err = db.DeleteByBean(ctx, sw)
164+
_, err := db.DeleteByBean(ctx, sw)
206165
return err
207166
}
208167

209-
// CreateIssueStopwatch creates a stopwatch if not exist, otherwise return an error
210-
func CreateIssueStopwatch(ctx context.Context, user *user_model.User, issue *Issue) error {
211-
if err := issue.LoadRepo(ctx); err != nil {
212-
return err
213-
}
214-
215-
// if another stopwatch is running: stop it
216-
exists, _, otherIssue, err := HasUserStopwatch(ctx, user.ID)
217-
if err != nil {
218-
return err
219-
}
220-
if exists {
221-
if err := FinishIssueStopwatch(ctx, user, otherIssue); err != nil {
222-
return err
168+
// CreateIssueStopwatch creates a stopwatch if the issue doesn't have the user's stopwatch.
169+
// It also stops any other stopwatch that might be running for the user.
170+
func CreateIssueStopwatch(ctx context.Context, user *user_model.User, issue *Issue) (ok bool, err error) {
171+
{ // if another issue's stopwatch is running: stop it; if this issue has a stopwatch: return an error.
172+
exists, otherStopWatch, otherIssue, err := HasUserStopwatch(ctx, user.ID)
173+
if err != nil {
174+
return false, err
175+
}
176+
if exists {
177+
if otherStopWatch.IssueID == issue.ID {
178+
// don't allow starting stopwatch for the same issue
179+
return false, nil
180+
}
181+
// stop the other issue's stopwatch
182+
if err = finishIssueStopwatch(ctx, user, otherIssue, otherStopWatch); err != nil {
183+
return false, err
184+
}
223185
}
224186
}
225187

226-
// Create stopwatch
227-
sw := &Stopwatch{
228-
UserID: user.ID,
229-
IssueID: issue.ID,
188+
if err = issue.LoadRepo(ctx); err != nil {
189+
return false, err
230190
}
231-
232-
if err := db.Insert(ctx, sw); err != nil {
233-
return err
191+
if err = db.Insert(ctx, &Stopwatch{UserID: user.ID, IssueID: issue.ID}); err != nil {
192+
return false, err
234193
}
235-
236-
if err := issue.LoadRepo(ctx); err != nil {
237-
return err
238-
}
239-
240-
if _, err := CreateComment(ctx, &CreateCommentOptions{
194+
if _, err = CreateComment(ctx, &CreateCommentOptions{
241195
Doer: user,
242196
Issue: issue,
243197
Repo: issue.Repo,
244198
Type: CommentTypeStartTracking,
245199
}); err != nil {
246-
return err
200+
return false, err
247201
}
248-
249-
return nil
202+
return true, nil
250203
}
251204

252205
// CancelStopwatch removes the given stopwatch and logs it into issue's timeline.
253-
func CancelStopwatch(ctx context.Context, user *user_model.User, issue *Issue) error {
254-
ctx, committer, err := db.TxContext(ctx)
255-
if err != nil {
256-
return err
257-
}
258-
defer committer.Close()
259-
if err := cancelStopwatch(ctx, user, issue); err != nil {
260-
return err
261-
}
262-
return committer.Commit()
263-
}
264-
265-
func cancelStopwatch(ctx context.Context, user *user_model.User, issue *Issue) error {
266-
e := db.GetEngine(ctx)
267-
sw, exists, err := getStopwatch(ctx, user.ID, issue.ID)
268-
if err != nil {
269-
return err
270-
}
271-
272-
if exists {
273-
if _, err := e.Delete(sw); err != nil {
206+
func CancelStopwatch(ctx context.Context, user *user_model.User, issue *Issue) (ok bool, err error) {
207+
err = db.WithTx(ctx, func(ctx context.Context) error {
208+
e := db.GetEngine(ctx)
209+
sw, exists, err := getStopwatch(ctx, user.ID, issue.ID)
210+
if err != nil {
274211
return err
212+
} else if !exists {
213+
return nil
275214
}
276215

277-
if err := issue.LoadRepo(ctx); err != nil {
216+
if err = issue.LoadRepo(ctx); err != nil {
278217
return err
279218
}
280-
281-
if _, err := CreateComment(ctx, &CreateCommentOptions{
219+
if _, err = e.Delete(sw); err != nil {
220+
return err
221+
}
222+
if _, err = CreateComment(ctx, &CreateCommentOptions{
282223
Doer: user,
283224
Issue: issue,
284225
Repo: issue.Repo,
285226
Type: CommentTypeCancelTracking,
286227
}); err != nil {
287228
return err
288229
}
289-
}
290-
return nil
230+
ok = true
231+
return nil
232+
})
233+
return ok, err
291234
}

‎models/issues/stopwatch_test.go‎

Lines changed: 35 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -10,34 +10,29 @@ import (
1010
issues_model "code.gitea.io/gitea/models/issues"
1111
"code.gitea.io/gitea/models/unittest"
1212
user_model "code.gitea.io/gitea/models/user"
13-
"code.gitea.io/gitea/modules/timeutil"
1413

1514
"github.com/stretchr/testify/assert"
1615
)
1716

1817
func TestCancelStopwatch(t *testing.T) {
1918
assert.NoError(t, unittest.PrepareTestDatabase())
2019

21-
user1, err := user_model.GetUserByID(db.DefaultContext, 1)
22-
assert.NoError(t, err)
23-
24-
issue1, err := issues_model.GetIssueByID(db.DefaultContext, 1)
25-
assert.NoError(t, err)
26-
issue2, err := issues_model.GetIssueByID(db.DefaultContext, 2)
27-
assert.NoError(t, err)
20+
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
21+
issue1 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 1})
2822

29-
err = issues_model.CancelStopwatch(db.DefaultContext, user1, issue1)
23+
ok, err := issues_model.CancelStopwatch(db.DefaultContext, user1, issue1)
3024
assert.NoError(t, err)
25+
assert.True(t, ok)
3126
unittest.AssertNotExistsBean(t, &issues_model.Stopwatch{UserID: user1.ID, IssueID: issue1.ID})
27+
unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{Type: issues_model.CommentTypeCancelTracking, PosterID: user1.ID, IssueID: issue1.ID})
3228

33-
_ = unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{Type: issues_model.CommentTypeCancelTracking, PosterID: user1.ID, IssueID: issue1.ID})
34-
35-
assert.NoError(t, issues_model.CancelStopwatch(db.DefaultContext, user1, issue2))
29+
ok, err = issues_model.CancelStopwatch(db.DefaultContext, user1, issue1)
30+
assert.NoError(t, err)
31+
assert.False(t, ok)
3632
}
3733

3834
func TestStopwatchExists(t *testing.T) {
3935
assert.NoError(t, unittest.PrepareTestDatabase())
40-
4136
assert.True(t, issues_model.StopwatchExists(db.DefaultContext, 1, 1))
4237
assert.False(t, issues_model.StopwatchExists(db.DefaultContext, 1, 2))
4338
}
@@ -58,21 +53,35 @@ func TestHasUserStopwatch(t *testing.T) {
5853
func TestCreateOrStopIssueStopwatch(t *testing.T) {
5954
assert.NoError(t, unittest.PrepareTestDatabase())
6055

61-
user2, err := user_model.GetUserByID(db.DefaultContext, 2)
56+
user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
57+
issue1 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 1})
58+
issue3 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 3})
59+
60+
// create a new stopwatch
61+
ok, err := issues_model.CreateIssueStopwatch(db.DefaultContext, user4, issue1)
6262
assert.NoError(t, err)
63-
org3, err := user_model.GetUserByID(db.DefaultContext, 3)
63+
assert.True(t, ok)
64+
unittest.AssertExistsAndLoadBean(t, &issues_model.Stopwatch{UserID: user4.ID, IssueID: issue1.ID})
65+
// should not create a second stopwatch for the same issue
66+
ok, err = issues_model.CreateIssueStopwatch(db.DefaultContext, user4, issue1)
6467
assert.NoError(t, err)
65-
66-
issue1, err := issues_model.GetIssueByID(db.DefaultContext, 1)
68+
assert.False(t, ok)
69+
// on a different issue, it will finish the existing stopwatch and create a new one
70+
ok, err = issues_model.CreateIssueStopwatch(db.DefaultContext, user4, issue3)
6771
assert.NoError(t, err)
68-
issue2, err := issues_model.GetIssueByID(db.DefaultContext, 2)
72+
assert.True(t, ok)
73+
unittest.AssertNotExistsBean(t, &issues_model.Stopwatch{UserID: user4.ID, IssueID: issue1.ID})
74+
unittest.AssertExistsAndLoadBean(t, &issues_model.Stopwatch{UserID: user4.ID, IssueID: issue3.ID})
75+
76+
// user2 already has a stopwatch in test fixture
77+
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
78+
issue2 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
79+
ok, err = issues_model.FinishIssueStopwatch(db.DefaultContext, user2, issue2)
6980
assert.NoError(t, err)
70-
71-
assert.NoError(t, issues_model.CreateOrStopIssueStopwatch(db.DefaultContext, org3, issue1))
72-
sw := unittest.AssertExistsAndLoadBean(t, &issues_model.Stopwatch{UserID: 3, IssueID: 1})
73-
assert.LessOrEqual(t, sw.CreatedUnix, timeutil.TimeStampNow())
74-
75-
assert.NoError(t, issues_model.CreateOrStopIssueStopwatch(db.DefaultContext, user2, issue2))
76-
unittest.AssertNotExistsBean(t, &issues_model.Stopwatch{UserID: 2, IssueID: 2})
77-
unittest.AssertExistsAndLoadBean(t, &issues_model.TrackedTime{UserID: 2, IssueID: 2})
81+
assert.True(t, ok)
82+
unittest.AssertNotExistsBean(t, &issues_model.Stopwatch{UserID: user2.ID, IssueID: issue2.ID})
83+
unittest.AssertExistsAndLoadBean(t, &issues_model.TrackedTime{UserID: user2.ID, IssueID: issue2.ID})
84+
ok, err = issues_model.FinishIssueStopwatch(db.DefaultContext, user2, issue2)
85+
assert.NoError(t, err)
86+
assert.False(t, ok)
7887
}

‎options/locale/locale_en-US.ini‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1726,6 +1726,8 @@ issues.remove_time_estimate_at = removed time estimate %s
17261726
issues.time_estimate_invalid = Time estimate format is invalid
17271727
issues.start_tracking_history = started working %s
17281728
issues.tracker_auto_close = Timer will be stopped automatically when this issue gets closed
1729+
issues.stopwatch_already_stopped = The timer for this issue is already stopped
1730+
issues.stopwatch_already_created = The timer for this issue already exists
17291731
issues.tracking_already_started = `You have already started time tracking on <a href="%s">another issue</a>!`
17301732
issues.stop_tracking = Stop Timer
17311733
issues.stop_tracking_history = worked for <b>%[1]s</b> %[2]s

0 commit comments

Comments
 (0)