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

Conversation

7 participants
@svarlamov
Contributor

svarlamov commented Dec 5, 2017

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

Targeting: #3034

svarlamov added some commits Dec 5, 2017

Add 'mark all read' option to notifications
Signed-off-by: Sasha Varlamov <sasha@sashavarlamov.com>
Fix exported comment
Signed-off-by: Sasha Varlamov <sasha@sashavarlamov.com>
Format method comments
Signed-off-by: Sasha Varlamov <sasha@sashavarlamov.com>
@svarlamov

This comment has been minimized.

Contributor

svarlamov commented Dec 5, 2017

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?

@tboerger tboerger added the lgtm/need 2 label Dec 5, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 5, 2017

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

@lafriks lafriks added the kind/feature label Dec 5, 2017

@lafriks lafriks added this to the 1.4.0 milestone Dec 5, 2017

@@ -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)}}

This comment has been minimized.

@lafriks

lafriks Dec 5, 2017

Member

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

This comment has been minimized.

@svarlamov

svarlamov Dec 6, 2017

Contributor

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

This comment has been minimized.

@lafriks

lafriks Dec 6, 2017

Member

Yes but there is NotificationUnreadCount check

@lafriks

This comment has been minimized.

Member

lafriks commented Dec 5, 2017

Drone build failure does not seems related to your changes

@@ -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) {

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 5, 2017

Member

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

@@ -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 {

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 5, 2017

Member

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.

This comment has been minimized.

@svarlamov

svarlamov Dec 6, 2017

Contributor

Sure, will refactor accordingly

@lunny

This comment has been minimized.

Member

lunny commented Dec 6, 2017

drone restarted

@codecov-io

This comment has been minimized.

codecov-io commented Dec 6, 2017

Codecov Report

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

Impacted file tree graph

@@            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.

svarlamov added some commits Dec 5, 2017

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

Format method comments

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

Tests for reactions (#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 (#3099)

Refactor notifications swap function
// 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()

This comment has been minimized.

@lunny

lunny Dec 6, 2017

Member

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

@svarlamov

This comment has been minimized.

Contributor

svarlamov commented Dec 6, 2017

@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...

@lunny

lunny approved these changes Dec 6, 2017

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

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 6, 2017

Member

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

svarlamov added some commits Dec 6, 2017

@sapk

sapk approved these changes Dec 6, 2017

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

@lunny

This comment has been minimized.

Member

lunny commented Dec 7, 2017

LGTM

@lunny

This comment has been minimized.

Member

lunny commented Dec 7, 2017

make L-G-T-M work

@tboerger tboerger added lgtm/need 1 and removed lgtm/need 2 labels Dec 7, 2017

@svarlamov

This comment has been minimized.

Contributor

svarlamov commented Dec 7, 2017

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

@ethantkoenig

one nit

// 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()

This comment has been minimized.

@ethantkoenig

ethantkoenig Dec 7, 2017

Member

nit: please remove commented-out code

This comment has been minimized.

@svarlamov

svarlamov Dec 7, 2017

Contributor

Good catch -- thanks

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Dec 7, 2017

LGTM

1 similar comment
@lafriks

This comment has been minimized.

Member

lafriks commented Dec 7, 2017

LGTM

@tboerger tboerger added lgtm/done and removed lgtm/need 1 labels Dec 7, 2017

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

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment