Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'mark all read' option to notifications #3097

Merged
merged 13 commits into from Dec 7, 2017
22 changes: 22 additions & 0 deletions models/fixtures/notification.yml
Expand Up @@ -19,3 +19,25 @@
issue_id: 2
created_unix: 946684800
updated_unix: 946684800

-
id: 3
user_id: 2
repo_id: 1
status: 3 # pinned
source: 1 # issue
updated_by: 1
issue_id: 2
created_unix: 946684800
updated_unix: 946684800

-
id: 4
user_id: 2
repo_id: 1
status: 1 # unread
source: 1 # issue
updated_by: 1
issue_id: 2
created_unix: 946684800
updated_unix: 946684800
11 changes: 11 additions & 0 deletions models/notification.go
Expand Up @@ -311,3 +311,14 @@ func getNotificationByID(notificationID int64) (*Notification, error) {

return notification, nil
}

// 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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please remove commented-out code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch -- thanks

_, err := x.
Where("user_id = ? AND status = ?", user.ID, currentStatus).
Cols("status", "updated_by", "updated_unix").
Update(n)
return err
}
28 changes: 24 additions & 4 deletions models/notification_test.go
Expand Up @@ -31,9 +31,11 @@ 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert.EqualValues(t, 2, notfs[0].ID)
assert.EqualValues(t, user.ID, notfs[0].UserID)
assert.EqualValues(t, 4, notfs[1].ID)
assert.EqualValues(t, user.ID, notfs[1].UserID)
}
}

Expand All @@ -57,12 +59,12 @@ func TestNotification_GetIssue(t *testing.T) {

func TestGetNotificationCount(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())
user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
cnt, err := GetNotificationCount(user, NotificationStatusUnread)
user := AssertExistsAndLoadBean(t, &User{ID: 1}).(*User)
cnt, err := GetNotificationCount(user, NotificationStatusRead)
assert.NoError(t, err)
assert.EqualValues(t, 0, cnt)

cnt, err = GetNotificationCount(user, NotificationStatusRead)
cnt, err = GetNotificationCount(user, NotificationStatusUnread)
assert.NoError(t, err)
assert.EqualValues(t, 1, cnt)
}
Expand All @@ -79,3 +81,21 @@ func TestSetNotificationStatus(t *testing.T) {
assert.Error(t, SetNotificationStatus(1, user, NotificationStatusRead))
assert.Error(t, SetNotificationStatus(NonexistentID, user, NotificationStatusRead))
}

func TestUpdateNotificationStatuses(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())
user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
notfUnread := AssertExistsAndLoadBean(t,
&Notification{UserID: user.ID, Status: NotificationStatusUnread}).(*Notification)
notfRead := AssertExistsAndLoadBean(t,
&Notification{UserID: user.ID, Status: NotificationStatusRead}).(*Notification)
notfPinned := AssertExistsAndLoadBean(t,
&Notification{UserID: user.ID, Status: NotificationStatusPinned}).(*Notification)
assert.NoError(t, UpdateNotificationStatuses(user, NotificationStatusUnread, NotificationStatusRead))
AssertExistsAndLoadBean(t,
&Notification{ID: notfUnread.ID, Status: NotificationStatusRead})
AssertExistsAndLoadBean(t,
&Notification{ID: notfRead.ID, Status: NotificationStatusRead})
AssertExistsAndLoadBean(t,
&Notification{ID: notfPinned.ID, Status: NotificationStatusPinned})
}
1 change: 1 addition & 0 deletions options/locale/locale_en-US.ini
Expand Up @@ -1547,6 +1547,7 @@ no_read = You do not have any read notifications.
pin = Pin notification
mark_as_read = Mark as read
mark_as_unread = Mark as unread
mark_all_as_read = Mark all as read

[gpg]
error.extract_sign = Failed to extract signature
Expand Down
1 change: 1 addition & 0 deletions routers/routes/routes.go
Expand Up @@ -706,6 +706,7 @@ func RegisterRoutes(m *macaron.Macaron) {
m.Group("/notifications", func() {
m.Get("", user.Notifications)
m.Post("/status", user.NotificationStatusPost)
m.Post("/purge", user.NotificationPurgePost)
}, reqSignIn)

m.Group("/api", func() {
Expand Down
12 changes: 12 additions & 0 deletions routers/user/notification.go
Expand Up @@ -112,3 +112,15 @@ func NotificationStatusPost(c *context.Context) {
url := fmt.Sprintf("%s/notifications", setting.AppSubURL)
c.Redirect(url, 303)
}

// NotificationPurgePost is a route for 'purging' the list of notifications - marking all unread as read
func NotificationPurgePost(c *context.Context) {
err := models.UpdateNotificationStatuses(c.User, models.NotificationStatusUnread, models.NotificationStatusRead)
if err != nil {
c.Handle(500, "ErrUpdateNotificationStatuses", err)
return
}

url := fmt.Sprintf("%s/notifications", setting.AppSubURL)
c.Redirect(url, 303)
}
8 changes: 8 additions & 0 deletions templates/user/notification/notification.tmpl
Expand Up @@ -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)}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but there is NotificationUnreadCount check

<form action="{{AppSubUrl}}/notifications/purge" method="POST" style="margin-left: auto;">
{{$.CsrfTokenHtml}}
<button class="ui mini button primary" title='{{$.i18n.Tr "notification.mark_all_as_read"}}'>
<i class="octicon octicon-checklist"></i>
</button>
</form>
{{end}}
</div>
<div class="ui bottom attached active tab segment">
{{if eq (len .Notifications) 0}}
Expand Down