[Issue #3539] Add notifications to notifications backend #3794
Conversation
Co-authored-by: Michael Chouinard <[email protected]>
…es' into mikehgrantsgov/3539-add-notifications-to-notifications-backend
…ifications-backend
…es' into mikehgrantsgov/3539-add-notifications-to-notifications-backend
| # Get user email from LinkExternalUser using select pattern | ||
| stmt = select(LinkExternalUser).where(LinkExternalUser.user_id == user_id) | ||
| user = self.db_session.execute(stmt).scalar_one_or_none() | ||
|
|
||
| if not user or not user.email: |
There was a problem hiding this comment.
Just because we'll likely need the email for other things in the future, and it might get pulled from a few places, what if we add a property to the user table, something like:
@property
def email(self) -> str | None:
if self.login_gov_user is not None: # TODO - would need to add a relationship that fetches this
return self.login_gov_user.email
return None| send_pinpoint_email_raw( | ||
| to_address=user.email, | ||
| subject=subject, | ||
| message=message, | ||
| pinpoint_client=self.pinpoint_client, | ||
| app_id=self.config.app_id, | ||
| ) |
There was a problem hiding this comment.
Probably want a try/except around this - if it fails, we should update the UserNotificationLog notification_sent field.
…backend' of https://github.com/HHS/simpler-grants-gov into mikehgrantsgov/3539-add-notifications-to-notifications-backend
api/src/db/models/user_models.py
Outdated
| linked_external_user: Mapped["LinkExternalUser | None"] = relationship( | ||
| "LinkExternalUser", | ||
| primaryjoin=lambda: LinkExternalUser.user_id == User.user_id, | ||
| uselist=False, | ||
| viewonly=True, | ||
| ) | ||
|
|
||
| @property | ||
| def email(self) -> str | None: | ||
| if self.linked_external_user is not None: | ||
| return self.linked_external_user.email | ||
| return None |
There was a problem hiding this comment.
While at the moment we only support a single linked user type (login.gov), that will change in the future. Could we do something like:
login_gov_external_user: Mapped["LinkExternalUser | None"] = relationship(
"LinkExternalUser",
primaryjoin=lambda: and_(LinkExternalUser.user_id == User.user_id, LinkExternalUser.external_user_type == ExternalUserType.LOGIN_GOV),
uselist=False,
viewonly=True,
)There was a problem hiding this comment.
Makes sense, added this.
api/src/db/models/user_models.py
Outdated
| user: Mapped["User"] = relationship( | ||
| "User", primaryjoin=lambda: LinkExternalUser.user_id == User.user_id | ||
| ) |
There was a problem hiding this comment.
Did this need to change? It should be able to reference the user from the foreign key uneventfully
There was a problem hiding this comment.
This change wasn't needed, but a part of the fiddling I was doing to get the sqlalchemy pieces to work. Updated this too
api/src/db/models/user_models.py
Outdated
| @@ -30,6 +30,22 @@ class User(ApiSchemaTable, TimestampMixin): | |||
| "UserSavedSearch", back_populates="user", uselist=True, cascade="all, delete-orphan" | |||
| ) | |||
|
|
|||
| linked_external_user: Mapped["LinkExternalUser | None"] = relationship( | |||
There was a problem hiding this comment.
Just one last thing, could we call this linked_login_gov_external_user or something with login_gov in it?
There was a problem hiding this comment.
Makes sense, just updated
mikehgrantsgov
deleted the
mikehgrantsgov/3539-add-notifications-to-notifications-backend
branch
Summary
Fixes #3539
Time to review: 15 mins
Changes proposed
Integrate AWS Pinpoint client to
generate_notifications.pytaskSet pinpoint app ID as an environment variable
Context for reviewers
Open question: how do we handle pinpoint app id? Is it going to set as an env var?
Additional information
See attached unit tests