Add 'mark all read' option to notifications #3097

svarlamov · 2017-12-05T17:02:28Z

Signed-off-by: Sasha Varlamov sasha@sashavarlamov.com

Targeting: #3034

Signed-off-by: Sasha Varlamov <sasha@sashavarlamov.com>

svarlamov · 2017-12-05T17:36:02Z

Not sure why CI build is failing... The commands run fine on my local dev box... Any thoughts on what I might be doing wrong?

lafriks · 2017-12-05T17:53:46Z

Try force pushing or squash commits. There is drone bug that it builds wrong commit sometime

lafriks · 2017-12-05T21:31:14Z

templates/user/notification/notification.tmpl

@@ -14,6 +14,14 @@
 			<a href="{{AppSubUrl}}/notifications?q=read" class="{{if eq .Status 2}}active{{end}} item">
 				{{.i18n.Tr "notification.read"}}
 			</a>
+			{{if and (gt (len .Notifications) 0) (eq .Status 1) (.NotificationUnreadCount)}}


Is check for gt (len .Notifications) 0 really needed?

I felt as though it should only show if there are unread notifications -- otherwise, the mark all as read wouldn't do anything as there's nothing to mark

Yes but there is NotificationUnreadCount check

lafriks · 2017-12-05T21:32:37Z

Drone build failure does not seems related to your changes

ethantkoenig · 2017-12-05T22:02:26Z

models/notification_test.go

@@ -31,7 +31,7 @@ func TestNotificationsForUser(t *testing.T) {
 	statuses := []NotificationStatus{NotificationStatusRead, NotificationStatusUnread}
 	notfs, err := NotificationsForUser(user, statuses, 1, 10)
 	assert.NoError(t, err)
-	if assert.Len(t, notfs, 1) {
+	if assert.Len(t, notfs, 2) {


Please update this test accordingly (i.e. add asserts for notfs[1])

ethantkoenig · 2017-12-05T22:08:56Z

models/notification.go

@@ -311,3 +311,14 @@ func getNotificationByID(notificationID int64) (*Notification, error) {

 	return notification, nil
 }
+
+// SwapNotificationStatuses swaps the statuses of all of a user's notifications that are of the currentStatus type
+func SwapNotificationStatuses(user *User, currentStatus NotificationStatus, desiredStatus NotificationStatus) error {


Maybe UpdateNotificationStatuses is a better name? My first thought when I saw the name was that it replaced all occurrences of status A with status B, and replaced all occurrences of status B with status A.

Sure, will refactor accordingly

lunny · 2017-12-06T03:15:57Z

drone restarted

codecov-io · 2017-12-06T03:32:31Z

Codecov Report

Merging #3097 into master will increase coverage by 0.03%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #3097      +/-   ##
==========================================
+ Coverage   34.03%   34.07%   +0.03%     
==========================================
  Files         273      273              
  Lines       40002    40018      +16     
==========================================
+ Hits        13615    13635      +20     
  Misses      24449    24449              
+ Partials     1938     1934       -4

Impacted Files	Coverage Δ
routers/user/notification.go	`45.78% <0%> (-0.89%)`	⬇️
models/notification.go	`76.68% <100%> (+0.87%)`	⬆️
routers/routes/routes.go	`87.05% <100%> (+0.02%)`	⬆️
modules/process/manager.go	`76.81% <0%> (-4.35%)`	⬇️
models/repo.go	`38.34% <0%> (+0.18%)`	⬆️
models/repo_list.go	`67.18% <0%> (+1.56%)`	⬆️
models/repo_indexer.go	`49% <0%> (+3.46%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ed7f18...332544d. Read the comment docs.

Signed-off-by: Sasha Varlamov <sasha@sashavarlamov.com> Format method comments Signed-off-by: Sasha Varlamov <sasha@sashavarlamov.com> Tests for reactions (go-gitea#3083) * Unit tests for reactions * Fix import order Signed-off-by: Lauris Bukšis-Haberkorns <lauris@nix.lv> Fix reaction possition when there is attachments (go-gitea#3099) Refactor notifications swap function

lunny · 2017-12-06T03:53:10Z

models/notification.go

+// UpdateNotificationStatuses updates the statuses of all of a user's notifications that are of the currentStatus type to the desiredStatus
+func UpdateNotificationStatuses(user *User, currentStatus NotificationStatus, desiredStatus NotificationStatus) error {
+	n := &Notification{Status: desiredStatus, UpdatedBy: user.ID}
+	n.BeforeUpdate()


BeforeUpdate will be invoked by xorm automatically, it's unnecessary to call it manually.

svarlamov · 2017-12-06T04:16:23Z

@lunny - Fixed per your change request on the BeforeUpdate call... My issue before was that I still needed to explicitly specify cols, so in one shot I also called it manually, but now I get the process...

ethantkoenig · 2017-12-06T05:35:06Z

routers/user/notification.go

+func NotificationPurgePost(c *context.Context) {
+	err := models.UpdateNotificationStatuses(c.User, models.NotificationStatusUnread, models.NotificationStatusRead)
+	if err != nil {
+		c.Handle(500, "ErrPurgeNotificationsForUser", err)


nit: Please update error message to "UpdateNotificationStatuses", for consistency with existing handlers.

sapk

Except unneeded gt (len .Notifications) 0 found by @lafriks. LGTM

lunny · 2017-12-07T01:08:33Z

LGTM

lunny · 2017-12-07T01:53:10Z

make L-G-T-M work

svarlamov · 2017-12-07T03:46:05Z

Dropped gt (len .Notifications) 0 per suggestions from @lafriks @lunny @sapk

ethantkoenig

one nit

ethantkoenig · 2017-12-07T05:00:59Z

models/notification.go

+// UpdateNotificationStatuses updates the statuses of all of a user's notifications that are of the currentStatus type to the desiredStatus
+func UpdateNotificationStatuses(user *User, currentStatus NotificationStatus, desiredStatus NotificationStatus) error {
+	n := &Notification{Status: desiredStatus, UpdatedBy: user.ID}
+	// n.BeforeUpdate()


nit: please remove commented-out code

Good catch -- thanks

ethantkoenig · 2017-12-07T05:19:07Z

LGTM

lafriks · 2017-12-07T05:22:20Z

LGTM

svarlamov added 3 commits December 6, 2017 00:57

Add 'mark all read' option to notifications

444cab6

Signed-off-by: Sasha Varlamov <sasha@sashavarlamov.com>

Fix exported comment

a6ca631

Signed-off-by: Sasha Varlamov <sasha@sashavarlamov.com>

Format method comments

76fe7b4

Signed-off-by: Sasha Varlamov <sasha@sashavarlamov.com>

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

lafriks added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Dec 5, 2017

lafriks added this to the 1.4.0 milestone Dec 5, 2017

Merge branch 'master' into master

3346171

lafriks reviewed Dec 5, 2017

View reviewed changes

ethantkoenig suggested changes Dec 5, 2017

View reviewed changes

Merge branch 'master' into master

09763c7

svarlamov added 2 commits December 6, 2017 11:46

Merge branch 'master' of https://github.com/svarlamov/gitea

e2c9ef4

lunny requested changes Dec 6, 2017

View reviewed changes

Accept change to drop beforeupdate call

ae09d6a

lunny approved these changes Dec 6, 2017

View reviewed changes

ethantkoenig reviewed Dec 6, 2017

View reviewed changes

svarlamov added 2 commits December 6, 2017 22:45

Update purge notifications error message for consistency

4af53c0

Merge branch 'master' into master

ac7ec11

sapk approved these changes Dec 6, 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 Dec 7, 2017

Drop unnecessary check for mark all as read button

70d0dee

ethantkoenig approved these changes Dec 7, 2017

View reviewed changes

Remove debugging comment

8ccaa43

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 Dec 7, 2017

Merge branch 'master' into master

332544d

lunny merged commit 7ec6cdd into go-gitea:master Dec 7, 2017

lunny mentioned this pull request Dec 7, 2017

No way to mark all notifications as 'read' #3034

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'mark all read' option to notifications #3097

Add 'mark all read' option to notifications #3097

svarlamov commented Dec 5, 2017

svarlamov commented Dec 5, 2017

lafriks commented Dec 5, 2017

lafriks Dec 5, 2017

svarlamov Dec 6, 2017

lafriks Dec 6, 2017

lafriks commented Dec 5, 2017

ethantkoenig Dec 5, 2017

ethantkoenig Dec 5, 2017

svarlamov Dec 6, 2017

lunny commented Dec 6, 2017

codecov-io commented Dec 6, 2017 •

edited

Loading

lunny Dec 6, 2017

svarlamov commented Dec 6, 2017

ethantkoenig Dec 6, 2017

sapk left a comment

lunny commented Dec 7, 2017

lunny commented Dec 7, 2017

svarlamov commented Dec 7, 2017

ethantkoenig left a comment

ethantkoenig Dec 7, 2017

svarlamov Dec 7, 2017

ethantkoenig commented Dec 7, 2017

lafriks commented Dec 7, 2017

Add 'mark all read' option to notifications #3097

Add 'mark all read' option to notifications #3097

Conversation

svarlamov commented Dec 5, 2017

svarlamov commented Dec 5, 2017

lafriks commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Dec 6, 2017

codecov-io commented Dec 6, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

svarlamov commented Dec 6, 2017

Choose a reason for hiding this comment

sapk left a comment

Choose a reason for hiding this comment

lunny commented Dec 7, 2017

lunny commented Dec 7, 2017

svarlamov commented Dec 7, 2017

ethantkoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig commented Dec 7, 2017

lafriks commented Dec 7, 2017

codecov-io commented Dec 6, 2017 •

edited

Loading