[WIP] Notifications system #321

andreynering · 2016-11-30T23:29:03Z

WIP: still in an early stage

bkcsoft · 2016-12-01T08:21:41Z

models/models.go

-		new(UpdateTask), new(HookTask),
-		new(Team), new(OrgUser), new(TeamUser), new(TeamRepo),
-		new(Notice), new(EmailAddress))
+		new(User),


Thanks! This has been messing with my OCD for a while 😆

How about reorder them?

@lunny in alphabetical order?

If order doesn't need to match the order in which fields are defined in the structure, then it makes sense to me to sort them alphabetically.

bkcsoft

Few things

bkcsoft · 2016-12-01T08:22:12Z

models/notification.go

+)
+
+const (
+	NotificationStatusUnread = "U"


Why not use iota ?

I prefer string because it is more readable (while querying the database).

So you're throwing away performance for readibility of the DB? 🙂

One char string: 1 byte
Int: 4 bytes, unless we used smallint, it would be 2 bytes in the DB.

I don't think there will be a performance difference at all. But I'm open to change to int if most of you prefer. 😉

I would also prefer iota.

bkcsoft · 2016-12-01T08:23:19Z

models/notification.go

+	RepoID int64 `xorm:"INDEX NOT NULL"`
+
+	Status string `xorm:"VARCHAR(1) INDEX NOT NULL"`
+	Source string `xorm:"VARCHAR(1) INDEX NOT NULL"`


These should be type NotificationStatus string and type NotificationSource string (if string, see above comment)

^

Status NotificationStatus Source NotificationSource

bkcsoft · 2016-12-01T08:24:54Z

models/notification.go

+		now     = time.Now()
+		nowUnix = now.Unix()
+	)
+	n.Updated = now


bkcsoft · 2016-12-01T08:25:18Z

models/notification.go

+	if err := sess.Begin(); err != nil {
+		return err
+	}
+


no new-line IMO

bkcsoft · 2016-12-01T08:27:09Z

models/notification.go

+	for _, watch := range watches {
+		exists, err := issueNotificationExists(sess, watch.UserID, watch.RepoID)
+		if err != nil {
+			return err


Why not put the errors in a channel and check that one afterwards... what happens now is that if a User is improperly deleted, no-one will get any notifications 😕

Oh, you're right. Ideally we should have foreign keys, so that wouldn't happen. 😄

But I think I'll call this in a goroutine, and just log on any error, instead of break.

bkcsoft · 2016-12-01T08:27:22Z

models/notification.go

+		}
+
+		if err != nil {
+			return err


Same as above

strk · 2016-12-02T08:20:30Z

Does this address gogs/gogs#2929 ?

andreynering · 2016-12-02T09:24:37Z

@strk It will.

strk · 2016-12-02T09:53:19Z

On Fri, Dec 02, 2016 at 01:23:24AM -0800, Andrey Nering wrote: + NotificationStatusUnread = "U" One char string: 1 byte Int: 4 bytes, unless we used smallint, it would be 2 bytes in the DB.

A char is 1 byte but a string of one char may be 2 bytes (if null terminated) or 5 bytes (if having a 4 bytes size header, as it happens for the "varchar" PostgreSQL datatype). So, can you turn it into a char ? Still, I do like readability in the DB

bkcsoft · 2016-12-06T12:23:07Z

Just a thought... Can't this be "extracted" into a Service? Since (as far as I can read) this will block issue-creation response until all watchers have been notified

Setting it up as a Service also have the added bonus of being able to move Mail-sending and such to the background (not sure how this works currently though)

andreynering · 2016-12-06T13:20:58Z

@bkcsoft Yes, that's a good idea

bkcsoft · 2016-12-06T14:11:58Z

@andreynering Preferably use channels 😉

struct NotificationService type {
  queue chan *models.Notification
}

func (ns *NotificationService) Notify(in *models.Notification) {
  ns.queue <- in
}

func(ns *NotificationService) Run() {
  for {
    select {
    case notif<- ns.queue: // Will stall until queue to process
      ns.processNotification(notif)
    }
  }
}

strk · 2016-12-11T10:32:53Z

routers/repo/issue.go

@@ -453,6 +454,8 @@ func NewIssuePost(ctx *context.Context, form auth.CreateIssueForm) {
 		return
 	}

+	notification.Service.NotifyIssue(issue)


Should this be put in background too ?
is: go notification.Service.NotifyIssue(issues)

If I read the code correctly the NotifyIssue writes into a channel, is that operation immediate or blocks until someone reads from the channel ? (lame gopher here)

You have a good point. Channels act like a queue, so I think it will block until it's consumed by the service.

We can use buffered channels [1], ~~but I think we will have a panic if we exceed the buffer size. I will make some tests.~~

[1] https://gobyexample.com/channel-buffering

Edit: buffered channels will do the trick: https://tour.golang.org/concurrency/3

- Replace old IssueUser table with Notifications - Mark notification as read on opening - Changing creation of notifications from router to models

bkcsoft · 2016-12-20T07:07:48Z

This is growing quite huge, and yes I'm aware that it's a large feature, but could you possibly do this in two steps? first one being the bare NotificationService, then in the second one actually start using it, because it's going to be a pain to review all this in one go 😕

andreynering · 2016-12-20T12:03:49Z

@bkcsoft Hmm... I tried to keep the commit history, but yeah, I agree that it's big.

I'll split then, but I'll have to wait the 1.0.0 release, I think.

andreynering · 2016-12-20T22:42:58Z

Splitted. Step 1 at #429

andreynering added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Nov 30, 2016

bkcsoft reviewed Dec 1, 2016

View reviewed changes

bkcsoft suggested changes Dec 1, 2016

View reviewed changes

andreynering mentioned this pull request Dec 1, 2016

Complete notifications system #145

Closed

10 tasks

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

tboerger added this to the 1.1.0 milestone Dec 2, 2016

andreynering force-pushed the notifications branch from 192b3c7 to 72794ec Compare December 6, 2016 23:20

bkcsoft approved these changes Dec 8, 2016

View reviewed changes

andreynering added 10 commits December 10, 2016 14:08

Create notification model/table

0f1b484

Creating notifications on new issue

b19ec55

Use own type (smallint on DB) and iota instead of VARCHAR(1)

75c2752

Lint

c3810c6

Also create notification while commenting on issue

8a3c856

Fix SQLs

679d91a

First version of notification service

f692ade

Notification item on menu

fedf445

Register notification router

a9844ae

First version of page to see notifications

7da3f89

andreynering force-pushed the notifications branch from 72794ec to 7da3f89 Compare December 10, 2016 19:29

strk reviewed Dec 11, 2016

View reviewed changes

andreynering added 3 commits December 19, 2016 09:28

Use buffered channel

7b01639

Notification unread count badge

3e3590d

Refactoring:

deb45be

- Replace old IssueUser table with Notifications - Mark notification as read on opening - Changing creation of notifications from router to models

Do not send notification to poster

b1a6b9a

andreynering closed this Dec 20, 2016

andreynering deleted the notifications branch December 20, 2016 22:16

andreynering mentioned this pull request Dec 20, 2016

Notifications - Step 1 #429

Closed

2 tasks

tboerger added reviewed/invalid and removed type/feature Completely new functionality. Can only be merged if feature freeze is not active. lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. pr/wip This PR is not ready for review labels Dec 21, 2016

tboerger removed this from the 1.1.0 milestone Dec 21, 2016

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

delvh added issue/not-a-bug The reported issue is the intended behavior or the problem is not inside Gitea and removed reviewed/invalid labels Oct 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Notifications system #321

[WIP] Notifications system #321

andreynering commented Nov 30, 2016

bkcsoft Dec 1, 2016

lunny Dec 7, 2016

bkcsoft Dec 8, 2016

strk Dec 11, 2016

bkcsoft left a comment

bkcsoft Dec 1, 2016

andreynering Dec 1, 2016

bkcsoft Dec 2, 2016

andreynering Dec 2, 2016

tboerger Dec 2, 2016

bkcsoft Dec 1, 2016

lunny Dec 1, 2016

bkcsoft Dec 1, 2016

bkcsoft Dec 1, 2016

bkcsoft Dec 1, 2016

andreynering Dec 1, 2016 •

edited

Loading

bkcsoft Dec 1, 2016

strk commented Dec 2, 2016 •

edited

Loading

andreynering commented Dec 2, 2016

strk commented Dec 2, 2016 via email

bkcsoft commented Dec 6, 2016

andreynering commented Dec 6, 2016

bkcsoft commented Dec 6, 2016

strk Dec 11, 2016

andreynering Dec 11, 2016 •

edited

Loading

bkcsoft commented Dec 20, 2016

andreynering commented Dec 20, 2016

andreynering commented Dec 20, 2016

[WIP] Notifications system #321

[WIP] Notifications system #321

Conversation

andreynering commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreynering Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strk commented Dec 2, 2016 • edited Loading

andreynering commented Dec 2, 2016

strk commented Dec 2, 2016 via email

bkcsoft commented Dec 6, 2016

andreynering commented Dec 6, 2016

bkcsoft commented Dec 6, 2016

Choose a reason for hiding this comment

andreynering Dec 11, 2016 • edited Loading

Choose a reason for hiding this comment

bkcsoft commented Dec 20, 2016

andreynering commented Dec 20, 2016

andreynering commented Dec 20, 2016

andreynering Dec 1, 2016 •

edited

Loading

strk commented Dec 2, 2016 •

edited

Loading

andreynering Dec 11, 2016 •

edited

Loading