Skip to content

Conversation

lunny
Copy link
Member

@lunny lunny commented Jun 20, 2025

This PR improved notifications.

  • Improve repository transfer notification to use the queue rather than directly insert database records

image

image

@lunny lunny added this to the 1.25.0 milestone Jun 20, 2025
@lunny lunny added the type/enhancement An improvement of existing functionality label Jun 20, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 20, 2025
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/migrations labels Jun 20, 2025
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 2, 2025

updatedByIndex := schemas.NewIndex("idx_notification_updated_by", schemas.IndexType)
updatedByIndex.AddColumn("updated_by")
indices = append(indices, updatedByIndex)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these indices well-designed and correct?

You can't just add index to every column and expect that "oh it should work".

Does database work really work that way?

Copy link
Member Author

Choose a reason for hiding this comment

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

This highlights a significant design flaw in the current table structure.

In this PR, we introduced a new column, release_id, to support release-type notifications.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 12, 2025
releaseIDIndex := schemas.NewIndex("idx_notification_release_id", schemas.IndexType)
releaseIDIndex.AddColumn("release_id")
indices = append(indices, releaseIDIndex)

Copy link
Contributor

Choose a reason for hiding this comment

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

The database design looks strange.

If I understand correctly, what you need is "a unique key to avoid duplication and help to mark the notification as read". I think it could be resolved by a more flexible approach like:

unique_key = 'commit-{CommitID}'
unique_key = 'release-{ReleaseID}'
unique_key = 'comment-{IssueID}-{CommentID}'

Then we only need one unique index (user_id, unique_key)


And, some legacy indices like status, source seem not able to help to improve querying performance because if user_id is used, then these indices won't be used.


And, the user_id index is redundancy because there is already (user_id, status, updated_unix). If you would take more care about performance, these problems should be handled.

Copy link
Member Author

Choose a reason for hiding this comment

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

The source should also be part of the unique key. Currently, issue and release notifications require a unique key, while commit and repository notifications do not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should use a one column to store this id, otherwise the unique key seems too complicated.

@lunny lunny marked this pull request as draft August 21, 2025 03:59
@lunny lunny modified the milestones: 1.25.0, 1.26.0 Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/frontend modifies/go Pull requests that update Go code modifies/migrations modifies/templates This PR modifies the template files type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants