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

Notifications - Step 1 #429

Closed
wants to merge 3 commits into from
Closed

Notifications - Step 1 #429

wants to merge 3 commits into from

Conversation

andreynering
Copy link
Contributor

@andreynering andreynering commented Dec 20, 2016

First step to resolve #145

This deprecates #321

I plan 2 steps:

  • Create database table and create/update notifications (this one)
  • Actually have UI for notifications

@andreynering andreynering added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Dec 20, 2016
@andreynering andreynering added this to the 1.1.0 milestone Dec 20, 2016
NotificationStatusRead
)

const (
Copy link
Member

Choose a reason for hiding this comment

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

Just put the constants in a single block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's necessary for iota and different types, as discussed below.

UserID int64 `xorm:"INDEX NOT NULL"`
RepoID int64 `xorm:"INDEX NOT NULL"`

Status NotificationStatus `xorm:"SMALLINT INDEX NOT NULL"`
Copy link
Member

Choose a reason for hiding this comment

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

Really a separate type for simple integers?

Copy link
Member

Choose a reason for hiding this comment

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

I really like this. :)

Copy link
Member

Choose a reason for hiding this comment

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

If the type got additional functions it makes sense to me, but not for the current status

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 also prefer enum, and others too, since I was asked to use this in the previous PR.

@tboerger
Copy link
Member

Generally not bad, but you should select more collections from the db instead of firing lots of small queries

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 21, 2016
@andreynering
Copy link
Contributor Author

you should select more collections from the db instead of firing lots of small queries

Updated. No more queries inside the loop.

Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

Except that then LGTM

if err := sess.Begin(); err != nil {
return err
}
defer sess.Close()
Copy link
Member

Choose a reason for hiding this comment

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

put sess.Close() before sess.Begin()

@andreynering andreynering mentioned this pull request Dec 28, 2016
2 tasks
@andreynering
Copy link
Contributor Author

Deprecated by #523

@tboerger 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. labels Dec 29, 2016
@tboerger tboerger removed this from the 1.1.0 milestone Dec 29, 2016
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
@delvh 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/not-a-bug The reported issue is the intended behavior or the problem is not inside Gitea
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Complete notifications system
4 participants