[Notifications Step 6] Per issue/PR watch/unwatch #1410

andreynering · 2017-03-30T00:13:04Z

Users that don't watch the repo can choose watch a single issue
Users that watch the repo can choose unwatch a single issue

pgaskin

Looks good other than some minor fixes, still need to try testing it.

pgaskin · 2017-03-30T00:18:21Z

models/issue_watch.go

+	iw.UpdatedUnix = time.Now().Unix()
+}
+
+func (iw *IssueWatch) BeforeUpdate() {


Needs a documentation comment

Nevermind, you just fixed it

pgaskin · 2017-03-30T00:18:32Z

models/issue_watch.go

+	return
+}
+
+func GetIssueWatchers(issueID int64) ([]*IssueWatch, error) {


Needs a documentation comment

Nevermind, you just fixed it

pgaskin · 2017-03-30T00:20:03Z

models/issue_watch.go

+	iw.CreatedUnix = time.Now().Unix()
+	iw.Updated = time.Now()
+	iw.UpdatedUnix = time.Now().Unix()
+}


IMO, this should store the time in a variable and use that, to improve the accuracy of the times, and to optimize the code a bit.

pgaskin · 2017-03-30T00:21:17Z

options/locale/locale_en-US.ini

@@ -652,6 +652,9 @@ issues.label.filter_sort.reverse_alphabetically = Reverse alphabetically
 issues.num_participants = %d Participants
 issues.attachment.open_tab = `Click to see "%s" in a new tab`
 issues.attachment.download = `Click to download "%s"`
+issues.watch = Watch
+issues.watch_issue = Watch issue
+issues.unwatch_issue = Unwatch issue


Maybe subscribe and unsubscribe like github (because it is slightly shorter and more clear)?

subscribe/unsubscribe also matches the icon better (which should otherwise be an eye)

pgaskin · 2017-03-30T00:23:21Z

templates/repo/issue/view_content/sidebar.tmpl

+							<i class="octicon octicon-mute"></i>
+							{{.i18n.Tr "repo.issues.unwatch_issue"}}
+						{{else}}
+							<i class="octicon octicon-megaphone"></i>


Use octicon-unmute for consistency.

strk

It's great to see this coming. I guess it needs a migration, and would be great to see a unit test for it.

strk · 2017-03-30T10:09:01Z

models/issue_watch.go

@@ -0,0 +1,83 @@
+package models


copyright header please

strk · 2017-03-30T10:10:17Z

models/issue_watch.go

+// BeforeUpdate is invoked from XORM before updating an object of this type.
+func (iw *IssueWatch) BeforeUpdate() {
+	iw.Updated = time.Now()
+	iw.UpdatedUnix = time.Now().Unix()


Same here, call time.Now() a single time please

strk · 2017-03-30T10:11:34Z

routers/repo/issue_watch.go

@@ -0,0 +1,34 @@
+package repo


Copyright header please

andreynering · 2017-03-30T23:24:55Z

@geek1011 and @strk

Updated with your suggestions + added unit tests.

appleboy · 2017-03-31T08:44:08Z

models/issue_watch.go

+
+// GetIssueWatch returns an issue watch by user and issue
+func GetIssueWatch(userID, issueID int64) (iw *IssueWatch, exists bool, err error) {
+	iw, exists, err = getIssueWatch(x, userID, issueID)


return getIssueWatch(x, userID, issueID)

otherwise LGTM

lunny · 2017-03-31T11:16:17Z

models/issue_watch.go

+	iw, exists, err = getIssueWatch(x, userID, issueID)
+	return
+}
+func getIssueWatch(e Engine, userID, issueID int64) (iw *IssueWatch, exists bool, err error) {


need a blank line.

lunny · 2017-03-31T11:16:52Z

models/issue_watch.go

+func GetIssueWatchers(issueID int64) ([]*IssueWatch, error) {
+	return getIssueWatchers(x, issueID)
+}
+func getIssueWatchers(e Engine, issueID int64) (watches []*IssueWatch, err error) {


strk

Great progress, I hope you can fill the final gaps :)

strk · 2017-03-31T11:46:40Z

models/issue_watch_test.go

+	assert.NoError(t, err)
+	_, exists, err = GetIssueWatch(2, 2)
+	assert.Equal(t, true, exists)
+	assert.NoError(t, err)


Can you also check a user having no entry in issue_watch table here ?
Also, ideally both a user who's watching and one who's not watching the repository containing the issue.

strk · 2017-03-31T11:47:10Z

models/issue_watch_test.go

+
+	iws, err := GetIssueWatchers(1)
+	assert.NoError(t, err)
+	assert.Equal(t, 1, len(iws))


Please also check an issue with multiple watchers and check the actual content of the returned slice

lunny · 2017-03-31T11:50:43Z

otherwise LGTM

andreynering · 2017-04-01T13:06:44Z

Updated

strk · 2017-04-01T13:41:18Z

I would still like to see a test with multiple watchers and something showing how repository watchers affect (or not) the issue-watch-list retriving function

andreynering · 2017-04-01T13:48:43Z

@strk The test as is is enough to show that it works as expected.

strk · 2017-04-01T14:20:14Z

@andreynering is it not expected that the GetIssueWatchers function return the information about who are the watchers ? The test is not checking that. Why don't you want to check that ? Also, is it expected that an issue can have more than a single watcher ? I find those to be legit cases to test. I've no problem with this being merged as is (LGTM) but as you are on it it would be nice to get a better test coverage.

andreynering · 2017-04-01T18:13:46Z

@strk Testing the data would makes sense for data we are saving in the database.

But in this case, we would be testing data present on test fixtures. This don't makes sense, we know the data on fixtures is right.

andreynering added 5 commits March 29, 2017 19:55

Create issue_watch table

a0d0de7

Add watch button on issue

b674460

Consider issue_watchers while sending notifications

aa6e949

Add updated_unix column on issue_watch

cb36251

Add octicons to watch/unwatch buttons

e4a33ed

andreynering added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Mar 30, 2017

andreynering added this to the 1.2.0 milestone Mar 30, 2017

Fix lint

caed86f

pgaskin reviewed Mar 30, 2017

View reviewed changes

strk suggested changes Mar 30, 2017

View reviewed changes

andreynering added 4 commits March 30, 2017 19:10

UI and translation improvements

4b284f8

Add copyright headers

18952c4

Use variables for times

a90ffff

Add unit tests for issue_watch

e6781d5

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 30, 2017

appleboy reviewed Mar 31, 2017

View reviewed changes

appleboy approved these changes Mar 31, 2017

View reviewed changes

lunny requested changes Mar 31, 2017

View reviewed changes

strk suggested changes Mar 31, 2017

View reviewed changes

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 31, 2017

andreynering added 2 commits April 1, 2017 09:58

Code style fixes

e5c56fe

Improve tests a little

f6e5ce6

lunny approved these changes Apr 1, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 1, 2017

andreynering merged commit 37a34c1 into go-gitea:master Apr 1, 2017

andreynering deleted the notification/issue-watch branch April 1, 2017 18:12

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Uh oh!

[Notifications Step 6] Per issue/PR watch/unwatch #1410

[Notifications Step 6] Per issue/PR watch/unwatch #1410

Uh oh!

Conversation

andreynering commented Mar 30, 2017

Uh oh!

pgaskin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreynering commented Mar 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lunny commented Mar 31, 2017

Uh oh!

andreynering commented Apr 1, 2017

Uh oh!

strk commented Apr 1, 2017 via email

Uh oh!

andreynering commented Apr 1, 2017

Uh oh!

strk commented Apr 1, 2017

Uh oh!

andreynering commented Apr 1, 2017

Uh oh!

Uh oh!