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

Add typehints in openlibrary.accounts module #9821

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
15 changes: 11 additions & 4 deletions openlibrary/accounts/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
from .model import * # noqa: F403
from .model import Account, Link

from typing import TYPE_CHECKING

if TYPE_CHECKING:
from openlibrary.plugins.upstream.models import User


# Unconfirmed functions (I'm not sure that these should be here)
def get_group(name):
Expand All @@ -20,7 +25,7 @@ class RunAs:
and then de-escalates to original user.
"""

def __init__(self, username):
def __init__(self, username: str) -> None:
"""
:param str username: Username e.g. /people/mekBot of user to run action as
"""
Expand All @@ -45,14 +50,16 @@ def __exit__(self, exc_type, exc_val, exc_tb):


# Confirmed functions (these have to be here)
def get_current_user():
def get_current_user() -> "User | None":
"""
Returns the currently logged in user. None if not logged in.
"""
return web.ctx.site.get_user()


def find(username=None, lusername=None, email=None):
def find(
username: str | None = None, lusername: str | None = None, email: str | None = None
) -> Account | None:
"""Finds an account by username, email or lowercase username."""

def query(name, value):
Expand Down Expand Up @@ -99,7 +106,7 @@ def update_account(username, **kargs):
web.ctx.site.update_account(username, **kargs)


def get_link(code):
def get_link(code: str) -> Link | bool:
docs = web.ctx.site.store.values(type="account-link", name="code", value=code)
if docs:
doc = docs[0]
Expand Down
2 changes: 1 addition & 1 deletion openlibrary/core/batch_imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def batch_import(raw_data: bytes) -> BatchResult:
The line numbers errors use 1-based counting because they are line numbers in a file.
"""
user = accounts.get_current_user()
username = user.get_username()
username = user.get_username() if user else None
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see you had to make this change to get mypy to pass, and in terms of fixing the error, I would say this is an acceptable thing to do for now. It does however expose an oversight on the part of the knucklehead who wrote this function.

What probably needs to happen here is a dependency injection-like thing, where the username is passed into batch_import. This current implementation sets up a case where (1) it's harder to unit test batch_import than it should be, and (2) people using the function may end up with a submitter of None without expecting it.

Right now, however, batch_import is only called in one place, and there it is gated by a requirement that the account be in the /usergroup/admin group:

user_key = delegate.context.user and delegate.context.user.key
if user_key not in _get_members_of_group("/usergroup/admin"):
raise Forbidden('Permission Denied.')

So let's just leave this change for now, as user.get_username() won't return None the way the function is currently called`, and it will be a future task to fix this up and write some tests.

batch_name = f"batch-{generate_hash(raw_data)}"
errors: list[BatchImportError] = []
raw_import_records: list[dict] = []
Expand Down
2 changes: 1 addition & 1 deletion openlibrary/core/bookshelves.py
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,7 @@ def get_recently_logged_books(
return cls.fetch(logged_books) if fetch else logged_books

@classmethod
def get_users_read_status_of_work(cls, username: str, work_id: str) -> str | None:
def get_users_read_status_of_work(cls, username: str, work_id: str) -> int | None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch here:

openlibrary=# \d bookshelves_books
                         Table "public.bookshelves_books"
    Column    |            Type             |              Modifiers
--------------+-----------------------------+--------------------------------------
 username     | text                        | not null
 work_id      | integer                     | not null
 bookshelf_id | integer                     | not null
 edition_id   | integer                     |
 private      | boolean                     |
 updated      | timestamp without time zone | default timezone('utc'::text, now())
 created      | timestamp without time zone | default timezone('utc'::text, now())

"""A user can mark a book as (1) want to read, (2) currently reading,
or (3) already read. Each of these states is mutually
exclusive. Returns the user's read state of this work, if one
Expand Down
2 changes: 1 addition & 1 deletion openlibrary/plugins/upstream/borrow.py
Original file line number Diff line number Diff line change
Expand Up @@ -746,7 +746,7 @@ def is_users_turn_to_borrow(user, edition) -> bool:
def is_admin() -> bool:
"""Returns True if the current user is in admin usergroup."""
user = accounts.get_current_user()
return user and user.key in [
return user is not None and user.key in [
m.key for m in web.ctx.site.get('/usergroup/admin').members
]

Expand Down
42 changes: 25 additions & 17 deletions openlibrary/plugins/upstream/mybooks.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import json
import web
from web.template import TemplateResult

from typing import Final, Literal, cast
from typing import Final, Literal, cast, TYPE_CHECKING

from infogami import config
from infogami.utils import delegate
Expand All @@ -26,22 +27,25 @@
from openlibrary.core.follows import PubSub
from openlibrary.core.yearly_reading_goals import YearlyReadingGoals

if TYPE_CHECKING:
from openlibrary.core.lists.model import List
from openlibrary.plugins.upstream.models import Work

RESULTS_PER_PAGE: Final = 25


class avatar(delegate.page):
path = "/people/([^/]+)/avatar"

def GET(self, username):
def GET(self, username: str):
url = User.get_avatar_url(username)
raise web.seeother(url)


class mybooks_home(delegate.page):
path = "/people/([^/]+)/books"

def GET(self, username):
def GET(self, username: str) -> TemplateResult:
"""Renders the template for the my books overview page

The other way to get to this page is /account/books which is
Expand Down Expand Up @@ -398,7 +402,7 @@ def GET(self, username, key='want-to-read'):


@public
def get_patrons_work_read_status(username, work_key):
def get_patrons_work_read_status(username: str, work_key: str) -> int | None:
if not username:
return None
work_id = extract_numeric_id_from_olid(work_key)
Expand Down Expand Up @@ -426,7 +430,7 @@ class MyBooksTemplate:
"imports",
}

def __init__(self, username, key):
def __init__(self, username: str, key: str) -> None:
"""The following is data required by every My Books sub-template (e.g. sidebar)"""
self.username = username
self.user = web.ctx.site.get('/people/%s' % self.username)
Expand Down Expand Up @@ -455,7 +459,7 @@ def __init__(self, username, key):
else {}
)

self.reading_goals = []
self.reading_goals: list = []
self.selected_year = None

if self.me and self.is_my_page or self.is_public:
Expand All @@ -467,9 +471,9 @@ def __init__(self, username, key):
self.sponsorships = get_sponsored_editions(self.user)
self.counts['sponsorships'] = len(self.sponsorships)

self.component_times = {}
self.component_times: dict = {}

def render_sidebar(self):
def render_sidebar(self) -> TemplateResult:
return render['account/sidebar'](
self.username,
self.key,
Expand All @@ -480,7 +484,9 @@ def render_sidebar(self):
self.component_times,
)

def render(self, template, header_title, page=None):
def render(
self, template: TemplateResult, header_title: str, page: "List | None" = None
) -> TemplateResult:
"""
Gather the data necessary to render the My Books template, and then
render the template.
Expand All @@ -506,7 +512,7 @@ def __init__(self, user=None):
self.user = user or accounts.get_current_user()

@property
def lists(self):
def lists(self) -> list:
return self.user.get_lists()

@property
Expand All @@ -525,7 +531,7 @@ def get_sidebar_counts(self):
return counts

@property
def reading_log_counts(self):
def reading_log_counts(self) -> dict[str, int]:
counts = (
Bookshelves.count_total_books_logged_by_user_per_shelf(
self.user.get_username()
Expand Down Expand Up @@ -612,11 +618,11 @@ def add_read_statuses(username, works):
class PatronBooknotes:
"""Manages the patron's book notes and observations"""

def __init__(self, user):
def __init__(self, user: User) -> None:
self.user = user
self.username = user.key.split('/')[-1]

def get_notes(self, limit=RESULTS_PER_PAGE, page=1):
def get_notes(self, limit: int = RESULTS_PER_PAGE, page: int = 1) -> list:
notes = Booknotes.get_notes_grouped_by_work(
self.username, limit=limit, page=page
)
Expand All @@ -633,7 +639,7 @@ def get_notes(self, limit=RESULTS_PER_PAGE, page=1):
}
return notes

def get_observations(self, limit=RESULTS_PER_PAGE, page=1):
def get_observations(self, limit: int = RESULTS_PER_PAGE, page: int = 1) -> list:
observations = Observations.get_observations_grouped_by_work(
self.username, limit=limit, page=page
)
Expand All @@ -648,10 +654,12 @@ def get_observations(self, limit=RESULTS_PER_PAGE, page=1):
entry['observations'] = convert_observation_ids(ids)
return observations

def _get_work(self, work_key):
def _get_work(self, work_key: str) -> "Work | None":
return web.ctx.site.get(work_key)

def _get_work_details(self, work):
def _get_work_details(
self, work: "Work"
) -> dict[str, list[str] | str | int | None]:
author_keys = [a.author.key for a in work.get('authors', [])]

return {
Expand All @@ -665,7 +673,7 @@ def _get_work_details(self, work):
}

@classmethod
def get_counts(cls, username):
def get_counts(cls, username: str) -> dict[str, int]:
return {
'notes': Booknotes.count_works_with_notes_by_user(username),
'observations': Observations.count_distinct_observations(username),
Expand Down
Loading