-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Show Release Notification in the UI #27058
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
base: main
Are you sure you want to change the base?
Conversation
Hmm, this diverges from the GitHub UI that shows releases only on the frontpage feed (which we already do). I see notifications as "things to do", but a release does not seem to fit here thematically. |
It also sends you an notification
We already sent a Mail, so we could also show it in the UI |
|
Interesting, I never received a release notification on GH so far. Maybe it's something in my notifications config. I guess once we have that PR landed that enables filtering notifications type per repo, this will be okay to have. |
I have a PR to do similar work but not finished and closed. The notification should have more types and users could have filters in future. |
continue | ||
} | ||
|
||
release, err := repo_model.GetReleaseByID(ctx, notification.ReleaseID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's low efficiency. We need to id in ()
to fix the possible performance problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetReleaseByID
does not more than doing a Database query with the ID. It does not load other things if it's that what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can find many examples to a batch loading. Use a for loop to get all release id and then get all releases use id in ()
and then assign release to notification.
I have already open #26865 do do exactly that. Backend and tests are ready, but I need some help on the Frontend. |
I can help in UI. what we need to do. |
Thanks. The other PR has a proof of concept UI that already works, but it doesn't look good. Ideally it should look like GitHub. |
will open a pr for the ui. |
@@ -64,13 +66,15 @@ type Notification struct { | |||
IssueID int64 `xorm:"INDEX NOT NULL"` | |||
CommitID string `xorm:"INDEX"` | |||
CommentID int64 | |||
ReleaseID int64 `xorm:"INDEX"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Migration will be added after the Code is reviewed.
Co-authored-by: delvh <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for me as it is.
While it probably makes sense to fix the performance bottleneck mentioned by @lunny, it's not a blocker for me.
The only blocker for me is the missing migration.
If a repo that you are watching creates a new Release, you get a Mail but it is not shown in the UI. This PR changed this. Now the Release Notification is shown in the UI!
The Migration will be added after the Code is reviewed.