Adds API to get library-specific info on inventory reports. (PP-1210) #1840

tdilauro · 2024-05-12T15:00:46Z

Description

Adds the /admin/reports/generate_inventory_report/:library_short_name GET method endpoint to provide information about the collections that will be included if an inventory report is generated.
Includes API documentation at:
- Swagger: .../apidoc/swagger#/admin.inventory
- ReDoc: .../apidoc/redoc#tag/admin.inventory
Small refactoring in ...api_routes.MockControllerMethod class to make it a little easier to verify calls to controller methods on routes for which content is validated on the way out.

Motivation and Context

It is useful to know which collections will be included in the inventory reports. This is primarily for users of the Palace Manage admin user interface.

[Jira PP-1210]

How Has This Been Tested?

Adds new tests for the new feature.
Tested locally via curl and with an updated Admin UI client.
Added new tests to the test suite.
CI tests for the associated feature branch pass.

Checklist

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

codecov · 2024-05-12T15:08:38Z

Codecov Report

Attention: Patch coverage is 95.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 90.12%. Comparing base (dcb23df) to head (195e2a2).
Report is 4 commits behind head on main.

Files	Patch %	Lines
...elery/tasks/generate_inventory_and_hold_reports.py	83.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1840      +/-   ##
==========================================
+ Coverage   90.10%   90.12%   +0.01%     
==========================================
  Files         299      300       +1     
  Lines       39530    39563      +33     
  Branches     8585     8595      +10     
==========================================
+ Hits        35618    35655      +37     
+ Misses       2590     2586       -4     
  Partials     1322     1322

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

tdilauro · 2024-05-13T01:18:19Z

src/palace/manager/api/admin/controller/report.py

+                IntegrationConfiguration.goal == Goals.LICENSE_GOAL,
+                not_(
+                    IntegrationConfiguration.settings_dict.contains(
+                        {"include_in_inventory_report": False}


I got most of this select from the code for generating the reports, but I'm not sure where the "include_in_inventory_report" setting is coming from. I do see it here in OPDS collection settings, but I don't see it getting set to false on other collection types that probably shouldn't be included in inventory reports. I looked around a bit, but didn't see it set to either true or false. Here's the query I used:

SELECT * FROM integration_configurations ic WHERE ic.goal = 'LICENSE_GOAL' AND ( ic.settings @> '{"include_in_inventory_report": true}' OR ic.settings @> '{"include_in_inventory_report": false}' )

So getting this info in a pure SQL query is hard, mostly due to our use of inheritance to set the include_in_inventory_report setting.

Some background on how this is set:

You are correct this setting is getting set in the OPDS collection settings:

circulation/src/palace/manager/core/opds_import.py

Lines 131 to 141 in b1ab61a

include_in_inventory_report: bool = FormField(

True,

form=ConfigurationFormItem(

label=_("Include in inventory report?"),

type=ConfigurationFormItemType.SELECT,

options={

"true": "(Default) Yes",

"false": "No",

},

),

)

We don't store the default values for our serialized settings in the database. This is done in the dict() method for the Pydantic settings class:

circulation/src/palace/manager/integration/settings.py

Lines 337 to 341 in 530f25b

def dict(self, *args: Any, **kwargs: Any) -> dict[str, Any]:

"""Override the dict method to remove the default values"""

if "exclude_defaults" not in kwargs:

kwargs["exclude_defaults"] = True

I thought this was better called out in the comments, but I can't find it right now, so we should probably add a better comment adding these details. But we're not storing the defaults when we serialize to the database, so it is easier to update the defaults in the future.

When we are interacting with the settings via the Pydantic class (how I was envisioning we would mostly interact with them, rather then querying, this is transparent, since the defaults get re-filled when the values are loaded from the database).

So the only value that ever gets saved out to the DB is include_in_inventory_report == False. So that is why the code that finds the integrations when generating the reports is structured the way it is.

First it finds any license integrations where include_in_inventory_report == False is not in the settings via a query.

circulation/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py

Lines 66 to 79 in 530f25b

# resolve integrations

integrations = session.scalars(

select(IntegrationConfiguration)

.join(IntegrationLibraryConfiguration)

.where(

IntegrationLibraryConfiguration.library_id == self.library_id,

IntegrationConfiguration.goal == Goals.LICENSE_GOAL,

not_(

IntegrationConfiguration.settings_dict.contains(

{"include_in_inventory_report": False}

)

),

)

).all()

Then it filters down this list, to only those integrations that actually contain this setting:

circulation/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py

Lines 80 to 86 in 530f25b

registry = LicenseProvidersRegistry()

integration_ids: list[int] = []

for integration in integrations:

settings = registry[integration.protocol].settings_load(integration)

if not isinstance(settings, OPDSImporterSettings):

continue

integration_ids.append(integration.id)

Since this code is missing that last filtering step, it will return collections without the setting, which I don't think is what we want here.

Perhaps since this query is so complicated, it should get broken out to a utility function that both the report generation and the controller can call.

jonathangreen

I think the query in inventory_report_info is going to return unrelated collections, so some additional filtering is needed. I added some explanation in a comment. After that change this PR looks good to me.

jonathangreen · 2024-05-13T12:41:47Z

src/palace/manager/api/admin/controller/report.py

+                IntegrationConfiguration.goal == Goals.LICENSE_GOAL,
+                not_(
+                    IntegrationConfiguration.settings_dict.contains(
+                        {"include_in_inventory_report": False}


So getting this info in a pure SQL query is hard, mostly due to our use of inheritance to set the include_in_inventory_report setting.

Some background on how this is set:

You are correct this setting is getting set in the OPDS collection settings:

circulation/src/palace/manager/core/opds_import.py

Lines 131 to 141 in b1ab61a

include_in_inventory_report: bool = FormField(

True,

form=ConfigurationFormItem(

label=_("Include in inventory report?"),

type=ConfigurationFormItemType.SELECT,

options={

"true": "(Default) Yes",

"false": "No",

},

),

)

We don't store the default values for our serialized settings in the database. This is done in the dict() method for the Pydantic settings class:

circulation/src/palace/manager/integration/settings.py

Lines 337 to 341 in 530f25b

def dict(self, *args: Any, **kwargs: Any) -> dict[str, Any]:

"""Override the dict method to remove the default values"""

if "exclude_defaults" not in kwargs:

kwargs["exclude_defaults"] = True

I thought this was better called out in the comments, but I can't find it right now, so we should probably add a better comment adding these details. But we're not storing the defaults when we serialize to the database, so it is easier to update the defaults in the future.

When we are interacting with the settings via the Pydantic class (how I was envisioning we would mostly interact with them, rather then querying, this is transparent, since the defaults get re-filled when the values are loaded from the database).

So the only value that ever gets saved out to the DB is include_in_inventory_report == False. So that is why the code that finds the integrations when generating the reports is structured the way it is.

First it finds any license integrations where include_in_inventory_report == False is not in the settings via a query.

circulation/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py

Lines 66 to 79 in 530f25b

# resolve integrations

integrations = session.scalars(

select(IntegrationConfiguration)

.join(IntegrationLibraryConfiguration)

.where(

IntegrationLibraryConfiguration.library_id == self.library_id,

IntegrationConfiguration.goal == Goals.LICENSE_GOAL,

not_(

IntegrationConfiguration.settings_dict.contains(

{"include_in_inventory_report": False}

)

),

)

).all()

Then it filters down this list, to only those integrations that actually contain this setting:

circulation/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py

Lines 80 to 86 in 530f25b

registry = LicenseProvidersRegistry()

integration_ids: list[int] = []

for integration in integrations:

settings = registry[integration.protocol].settings_load(integration)

if not isinstance(settings, OPDSImporterSettings):

continue

integration_ids.append(integration.id)

jonathangreen · 2024-05-13T12:47:22Z

src/palace/manager/api/admin/controller/report.py

+                IntegrationConfiguration.goal == Goals.LICENSE_GOAL,
+                not_(
+                    IntegrationConfiguration.settings_dict.contains(
+                        {"include_in_inventory_report": False}


Since this code is missing that last filtering step, it will return collections without the setting, which I don't think is what we want here.

Perhaps since this query is so complicated, it should get broken out to a utility function that both the report generation and the controller can call.

tdilauro · 2024-05-14T00:42:08Z

@jonathangreen I think this is ready for another look, when you have a chance.

jonathangreen

Looks good to me! The new route name seems like a much better fit.

Made one comment, but its super minor.

jonathangreen · 2024-05-14T12:07:06Z

src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py

+) -> list[IntegrationConfiguration]:
+    """Return a list of collections to report for the given library."""
+    if session is None:
+        session = Session.object_session(library)


Minor: Since session is always supplied, and the fallback isn't tested, maybe we just make session required.

Adds API to get library-specific info on inventory reports.

16b181c

tdilauro requested a review from a team May 12, 2024 15:00

tdilauro commented May 13, 2024

View reviewed changes

jonathangreen reviewed May 13, 2024

View reviewed changes

tdilauro added 3 commits May 13, 2024 16:08

Move report generation to same endpoint URL as report info.

182b162

Report info/generation share collection selection function.

e81a048

Appease mypy.

f36b52f

jonathangreen approved these changes May 14, 2024

View reviewed changes

Review feedback.

195e2a2

tdilauro merged commit 6a37f4c into main May 14, 2024
20 checks passed

tdilauro deleted the feature/inventory-report-info branch May 14, 2024 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds API to get library-specific info on inventory reports. (PP-1210) #1840

Adds API to get library-specific info on inventory reports. (PP-1210) #1840

tdilauro commented May 12, 2024 •

edited by jira bot

Loading

codecov bot commented May 12, 2024 •

edited

Loading

tdilauro May 13, 2024

jonathangreen May 13, 2024

jonathangreen May 13, 2024

jonathangreen left a comment

jonathangreen May 13, 2024

jonathangreen May 13, 2024

tdilauro commented May 14, 2024

jonathangreen left a comment

jonathangreen May 14, 2024

	include_in_inventory_report: bool = FormField(
	True,
	form=ConfigurationFormItem(
	label=_("Include in inventory report?"),
	type=ConfigurationFormItemType.SELECT,
	options={
	"true": "(Default) Yes",
	"false": "No",
	},
	),
	)

	def dict(self, args: Any, *kwargs: Any) -> dict[str, Any]:
	"""Override the dict method to remove the default values"""

	if "exclude_defaults" not in kwargs:
	kwargs["exclude_defaults"] = True

	# resolve integrations
	integrations = session.scalars(
	select(IntegrationConfiguration)
	.join(IntegrationLibraryConfiguration)
	.where(
	IntegrationLibraryConfiguration.library_id == self.library_id,
	IntegrationConfiguration.goal == Goals.LICENSE_GOAL,
	not_(
	IntegrationConfiguration.settings_dict.contains(
	{"include_in_inventory_report": False}
	)
	),
	)
	).all()

	registry = LicenseProvidersRegistry()
	integration_ids: list[int] = []
	for integration in integrations:
	settings = registry[integration.protocol].settings_load(integration)
	if not isinstance(settings, OPDSImporterSettings):
	continue
	integration_ids.append(integration.id)

Adds API to get library-specific info on inventory reports. (PP-1210) #1840

Adds API to get library-specific info on inventory reports. (PP-1210) #1840

Conversation

tdilauro commented May 12, 2024 • edited by jira bot Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented May 12, 2024 • edited Loading

Codecov Report

tdilauro May 13, 2024

Choose a reason for hiding this comment

jonathangreen May 13, 2024

Choose a reason for hiding this comment

jonathangreen May 13, 2024

Choose a reason for hiding this comment

jonathangreen left a comment

Choose a reason for hiding this comment

jonathangreen May 13, 2024

Choose a reason for hiding this comment

jonathangreen May 13, 2024

Choose a reason for hiding this comment

tdilauro commented May 14, 2024

jonathangreen left a comment

Choose a reason for hiding this comment

jonathangreen May 14, 2024

Choose a reason for hiding this comment

tdilauro commented May 12, 2024 •

edited by jira bot

Loading

codecov bot commented May 12, 2024 •

edited

Loading