Add type hints to src/palace/manager/api/admin/ code. by dbernstein · Pull Request #3046 · ThePalaceProject/circulation

dbernstein · 2026-02-12T17:40:32Z

Description

As part of the ongoing effort to add type hints everywhere. This PR adds type hints to everything below src/palace/manager/api/admin/ and tightens up an implementations that require changes due to type hint additions.

Motivation and Context

Long term team-wide effort.

How Has This Been Tested?

Tests pass. Mypy passes.

Checklist

I have updated the documentation accordingly.
All new and existing tests passed.

codecov · 2026-02-13T20:49:47Z

Codecov Report

❌ Patch coverage is 78.50467% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.96%. Comparing base (ebb43bf) to head (d4e43d6).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
src/palace/manager/api/admin/validator.py	76.47%	5 Missing and 3 partials ⚠️
...anager/api/admin/controller/collection_settings.py	30.00%	6 Missing and 1 partial ⚠️
.../manager/api/admin/controller/metadata_services.py	41.66%	6 Missing and 1 partial ⚠️
...nager/api/admin/controller/patron_auth_services.py	36.36%	6 Missing and 1 partial ⚠️
src/palace/manager/api/admin/controller/lanes.py	60.00%	6 Missing ⚠️
...alace/manager/api/admin/controller/custom_lists.py	50.00%	4 Missing ⚠️
...e/manager/api/admin/controller/catalog_services.py	71.42%	2 Missing ⚠️
...manager/api/admin/controller/discovery_services.py	71.42%	2 Missing ⚠️
...pi/admin/password_admin_authentication_provider.py	80.00%	1 Missing and 1 partial ⚠️
src/palace/manager/api/admin/routes.py	98.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3046      +/-   ##
==========================================
- Coverage   93.04%   92.96%   -0.09%     
==========================================
  Files         480      480              
  Lines       43716    43798      +82     
  Branches     6027     6034       +7     
==========================================
+ Hits        40677    40715      +38     
- Misses       1968     2005      +37     
- Partials     1071     1078       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jonathangreen

This looks good! Need to address some of the Any type issues here though before it gets merged.

jonathangreen · 2026-02-19T14:48:27Z

src/palace/manager/api/admin/password_admin_authentication_provider.py

    @staticmethod
-    def forgot_password_template(redirect):
+    def forgot_password_template(
+        redirect: str | None | Response | WerkzeugResponse,


I think this should be str | None, we don't want to pass a Response into a jinja template do we?

jonathangreen · 2026-02-19T14:48:47Z

src/palace/manager/api/admin/password_admin_authentication_provider.py

+    def reset_password_template(
+        reset_password_token: str,
+        admin_id: int,
+        redirect: str | None | Response | WerkzeugResponse,


Same as: https://github.com/ThePalaceProject/circulation/pull/3046/changes#r2828319615

jonathangreen · 2026-02-19T14:49:55Z

src/palace/manager/api/admin/routes.py

@@ -1,7 +1,9 @@
+# Decorators from palace.manager.api.routes and core.app_server are untyped.
+# mypy: disallow_untyped_decorators=false


Instead of this, lets add types to these decorators

jonathangreen · 2026-02-19T14:50:30Z

src/palace/manager/api/admin/routes.py

+) -> Callable[..., Any]:
    @wraps(f)
-    def decorated(*args, **kwargs):
+    def decorated(*args: Any, **kwargs: Any) -> Any:


For all of these decorators we should add actual types instead of defaulting to 'Any'.

circulation/src/palace/manager/api/admin/routes.py

Lines 47 to 65 in d4e43d6

def requires_basic_auth[**P, T](func: Callable[P, T]) -> Callable[P, T | ProblemDetail]:

"""Basic auth for stateless system admin only API calls."""

@wraps(func)

def wrapper(*args: P.args, **kwargs: P.kwargs) -> T | ProblemDetail:

auth = request.authorization

if not auth or auth.username is None or auth.password is None:

return INVALID_ADMIN_CREDENTIALS

admin = Admin.authenticate(app.manager._db, auth.username, auth.password)

if not admin:

return INVALID_ADMIN_CREDENTIALS

if not admin.is_system_admin():

return ADMIN_NOT_AUTHORIZED

return func(*args, **kwargs)

return wrapper

provides an example of how these should be typed.

jonathangreen · 2026-02-19T14:51:00Z

src/palace/manager/api/admin/routes.py

 @app.route("/admin/sign_in_with_password", methods=["POST"])
 @returns_problem_detail
-def password_auth():
+def password_auth() -> Any:


For all these routes, we should add types instead of defaulting to Any.

jonathangreen · 2026-02-19T15:01:34Z

src/palace/manager/api/admin/validator.py

@@ -1,4 +1,10 @@
+from __future__ import annotations
+
+# mypy: warn_unreachable=false


We shouldn't add this at the file level. If we need this ignore, lets add it line by line, so that we get warnings when its unnecessary. Ideally though, we wouldn't need this at all. usually this is a sign that something is incorrectly typed.

jonathangreen · 2026-02-19T15:07:33Z

src/palace/manager/api/admin/validator.py

+        self,
+        settings: list[dict[str, Any]],
+        value: str,
+        form: dict[str, Any] | None,


Minor: The form = form or {} on line 46 handles None, but the callers in validate_email (line 72) already do content.get("form") or {}, so None can never actually reach here. The | None is harmless but misleading.

jonathangreen · 2026-02-19T15:08:21Z

src/palace/manager/api/admin/template_styles.py

@@ -1,4 +1,4 @@
-body_style = """
+body_style: str = """


Minor: Adding : str to every module-level string constant adds noise. Mypy already infers these as str. This isn't wrong, just unnecessary.

jonathangreen · 2026-02-19T15:09:49Z

src/palace/manager/api/admin/validator.py

        return [_f for _f in result if _f]

-    def _value(self, field, form):
+    def _value(self, field: dict[str, Any], form: Any) -> Any:


Typing form as Any defeats the purpose. Any means mypy won't catch misuse.

jonathangreen · 2026-02-19T15:09:58Z

src/palace/manager/api/admin/validator.py


-    def _list_of_values(self, fields, form):
-        result = []
+    def _list_of_values(self, fields: list[dict[str, Any]], form: Any) -> list[Any]:


Typing form as Any defeats the purpose. Any means mypy won't catch misuse.

dbernstein force-pushed the add-type-hints-to-api-admin branch from cd03642 to 168c3c1 Compare February 13, 2026 17:16

dbernstein added 2 commits February 13, 2026 12:42

Add type hints to src/palace/manager/api/admin/ code.

1f9a9f3

Fix broken tests.

d4e43d6

dbernstein force-pushed the add-type-hints-to-api-admin branch from 168c3c1 to d4e43d6 Compare February 13, 2026 20:42

dbernstein marked this pull request as ready for review February 17, 2026 01:34

dbernstein requested a review from a team February 17, 2026 01:34

jonathangreen reviewed Feb 19, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Add type hints to src/palace/manager/api/admin/ code.#3046

Add type hints to src/palace/manager/api/admin/ code.#3046
dbernstein wants to merge 2 commits intomainfrom
add-type-hints-to-api-admin

dbernstein commented Feb 12, 2026 •

edited

Loading

Uh oh!

codecov bot commented Feb 13, 2026 •

edited

Loading

Uh oh!

jonathangreen left a comment

Uh oh!

jonathangreen Feb 19, 2026

Uh oh!

jonathangreen Feb 19, 2026

Uh oh!

jonathangreen Feb 19, 2026

Uh oh!

jonathangreen Feb 19, 2026

Uh oh!

jonathangreen Feb 19, 2026

Uh oh!

jonathangreen Feb 19, 2026

Uh oh!

jonathangreen Feb 19, 2026

Uh oh!

jonathangreen Feb 19, 2026

Uh oh!

jonathangreen Feb 19, 2026

Uh oh!

jonathangreen Feb 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -1,7 +1,9 @@
		# Decorators from palace.manager.api.routes and core.app_server are untyped.
		# mypy: disallow_untyped_decorators=false

	def requires_basic_auth[**P, T](func: Callable[P, T]) -> Callable[P, T \| ProblemDetail]:
	"""Basic auth for stateless system admin only API calls."""

	@wraps(func)
	def wrapper(args: P.args, *kwargs: P.kwargs) -> T \| ProblemDetail:
	auth = request.authorization
	if not auth or auth.username is None or auth.password is None:
	return INVALID_ADMIN_CREDENTIALS

	admin = Admin.authenticate(app.manager._db, auth.username, auth.password)

	if not admin:
	return INVALID_ADMIN_CREDENTIALS

	if not admin.is_system_admin():
	return ADMIN_NOT_AUTHORIZED
	return func(args, *kwargs)

	return wrapper

		@@ -1,4 +1,10 @@
		from __future__ import annotations

		# mypy: warn_unreachable=false

Comments

Conversation

dbernstein commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Checklist

Uh oh!

codecov bot commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jonathangreen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dbernstein commented Feb 12, 2026 •

edited

Loading

codecov bot commented Feb 13, 2026 •

edited

Loading