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

Create notification table #9360

Merged
merged 1 commit into from
Feb 21, 2025
Merged

Conversation

mtomilov
Copy link
Contributor

@mtomilov mtomilov commented Feb 18, 2025

Refs #9359

This adds notification table to the app models together with its sql migration

Testing

@mtomilov mtomilov force-pushed the create-table-to-store-notifications branch from eecde66 to 855bac0 Compare February 18, 2025 17:45
REPLY = "reply"


class Notification(Base, Timestamps):
Copy link
Contributor Author

@mtomilov mtomilov Feb 18, 2025

Choose a reason for hiding this comment

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

In theory we could add send column on top of what we get from Timestamp but it's probably okay.

"""FK to user.id"""
user: Mapped[User] = relationship(back_populates="notifications", uselist=False)

type: Mapped[NotificationType]
Copy link
Contributor Author

@mtomilov mtomilov Feb 18, 2025

Choose a reason for hiding this comment

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

Bare bones version for now to support mention edits.
We will probably add status and actor_id (triggered by) in later phases.

@mtomilov mtomilov force-pushed the create-table-to-store-notifications branch 3 times, most recently from 33041c1 to a5764a4 Compare February 18, 2025 18:04
@mtomilov mtomilov requested a review from marcospri February 19, 2025 08:48
@mtomilov mtomilov force-pushed the create-table-to-store-notifications branch from a5764a4 to ebfc2fe Compare February 19, 2025 14:45
index=True,
)
"""FK to annotation.id - the annotation that triggered this notification"""
source_annotation = relationship(
Copy link
Contributor Author

@mtomilov mtomilov Feb 19, 2025

Choose a reason for hiding this comment

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

Renamed this after discussion in slack

index=True,
)
"""FK to user.id - the user receiving the notification"""
recipient = relationship("User", uselist=False)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And renamed this

Copy link
Member

@marcospri marcospri left a comment

Choose a reason for hiding this comment

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

Looks good.

We might have to tweak the structure when we make decision about how notifications will work in-app vs the immediate need to support edits, but that's fine.

@mtomilov mtomilov merged commit fb0b54b into main Feb 21, 2025
11 checks passed
@mtomilov mtomilov deleted the create-table-to-store-notifications branch February 21, 2025 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants