Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add type hints for account validity handler #8620

Merged
merged 7 commits into from
Oct 26, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/8620.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add type hints to the account validity handler.
1 change: 1 addition & 0 deletions mypy.ini
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ files =
synapse/federation,
synapse/handlers/_base.py,
synapse/handlers/account_data.py,
synapse/handlers/account_validity.py,
synapse/handlers/appservice.py,
synapse/handlers/auth.py,
synapse/handlers/cas_handler.py,
Expand Down
25 changes: 19 additions & 6 deletions synapse/handlers/account_validity.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,22 @@
import logging
from email.mime.multipart import MIMEMultipart
from email.mime.text import MIMEText
from typing import List
from typing import TYPE_CHECKING, List

from synapse.api.errors import StoreError
from synapse.logging.context import make_deferred_yieldable
from synapse.metrics.background_process_metrics import wrap_as_background_process
from synapse.types import UserID
from synapse.util import stringutils

if TYPE_CHECKING:
from synapse.app.homeserver import HomeServer

logger = logging.getLogger(__name__)


class AccountValidityHandler:
def __init__(self, hs):
def __init__(self, hs: "HomeServer"):
self.hs = hs
self.config = hs.config
self.store = self.hs.get_datastore()
Expand Down Expand Up @@ -67,7 +70,7 @@ def __init__(self, hs):
self.clock.looping_call(self._send_renewal_emails, 30 * 60 * 1000)

@wrap_as_background_process("send_renewals")
async def _send_renewal_emails(self):
async def _send_renewal_emails(self) -> None:
"""Gets the list of users whose account is expiring in the amount of time
configured in the ``renew_at`` parameter from the ``account_validity``
configuration, and sends renewal emails to all of these users as long as they
Expand All @@ -81,11 +84,21 @@ async def _send_renewal_emails(self):
user_id=user["user_id"], expiration_ts=user["expiration_ts_ms"]
)

async def send_renewal_email_to_user(self, user_id: str):
async def send_renewal_email_to_user(self, user_id: str) -> None:
"""
Send a renewal email for a specific user.

Note that if the user is not set to renew at some point in the future then
no email will be sent.

Args:
user_id: The user ID to send a renewal email for.
"""
expiration_ts = await self.store.get_expiration_ts_for_user(user_id)
await self._send_renewal_email(user_id, expiration_ts)
if expiration_ts is not None:
await self._send_renewal_email(user_id, expiration_ts)
Copy link
Member Author

Choose a reason for hiding this comment

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

Or perhaps this should just raise an error instead of silently ignoring it?

Copy link
Member

Choose a reason for hiding this comment

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

Yeahhh, it's probably poor form to return a 200 OK for a request that will never actually send the email.

At the same time, I could see raising an error potentially breaking someone's script that just loops over their entire userbase.

Perhaps we can raise an error and put out a notice in the release notes just in case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to make the changelog entry into a bugfix about this instead. Hopefully that's enough of a notice?


async def _send_renewal_email(self, user_id: str, expiration_ts: int):
async def _send_renewal_email(self, user_id: str, expiration_ts: int) -> None:
"""Sends out a renewal email to every email address attached to the given user
with a unique link allowing them to renew their account.

Expand Down
4 changes: 2 additions & 2 deletions synapse/handlers/profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ async def get_profile_from_cache(self, user_id: str) -> JsonDict:
profile = await self.store.get_from_remote_profile_cache(user_id)
return profile or {}

async def get_displayname(self, target_user: UserID) -> str:
async def get_displayname(self, target_user: UserID) -> Optional[str]:
if self.hs.is_mine(target_user):
try:
displayname = await self.store.get_profile_displayname(
Expand Down Expand Up @@ -211,7 +211,7 @@ async def set_displayname(

await self._update_join_states(requester, target_user)

async def get_avatar_url(self, target_user: UserID) -> str:
async def get_avatar_url(self, target_user: UserID) -> Optional[str]:
if self.hs.is_mine(target_user):
try:
avatar_url = await self.store.get_profile_avatar_url(
Expand Down
4 changes: 2 additions & 2 deletions synapse/storage/databases/main/profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@ async def get_profileinfo(self, user_localpart: str) -> ProfileInfo:
avatar_url=profile["avatar_url"], display_name=profile["displayname"]
)

async def get_profile_displayname(self, user_localpart: str) -> str:
async def get_profile_displayname(self, user_localpart: str) -> Optional[str]:
return await self.db_pool.simple_select_one_onecol(
table="profiles",
keyvalues={"user_id": user_localpart},
retcol="displayname",
desc="get_profile_displayname",
)

async def get_profile_avatar_url(self, user_localpart: str) -> str:
async def get_profile_avatar_url(self, user_localpart: str) -> Optional[str]:
return await self.db_pool.simple_select_one_onecol(
table="profiles",
keyvalues={"user_id": user_localpart},
Expand Down
4 changes: 2 additions & 2 deletions synapse/storage/databases/main/registration.py
Original file line number Diff line number Diff line change
Expand Up @@ -236,13 +236,13 @@ async def get_renewal_token_for_user(self, user_id: str) -> str:
desc="get_renewal_token_for_user",
)

async def get_users_expiring_soon(self) -> List[Dict[str, int]]:
async def get_users_expiring_soon(self) -> List[Dict[str, Any]]:
"""Selects users whose account will expire in the [now, now + renew_at] time
window (see configuration for account_validity for information on what renew_at
refers to).

Returns:
A list of dictionaries mapping user ID to expiration time (in milliseconds).
A list of dictionaries, each with a user ID and expiration time (in milliseconds).
Copy link
Member Author

@clokep clokep Oct 21, 2020

Choose a reason for hiding this comment

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

This comment seemed entirely wrong, it becomes a list of dictionaries like [{"user_id": "@foo:matrix.org", "expiration_time": 1234}].

Although converting this to be a flat dictionary like {"@foo:matrix.org": 1234} would probably make more sense.

"""

def select_users_txn(txn, now_ms, renew_at):
Expand Down