PP-35 Migrate license goal integrations :mountain: by RishiDiwanTT · Pull Request #1189 · ThePalaceProject/circulation

RishiDiwanTT · 2023-06-09T07:13:48Z

Description

These changes are a refactor of the external integrations system.
The refactor has undergone several refactors itself, every time a unit test would point out a different use case, some changes would have to be made. Due to this there may be some dead code lying around, I will try to clean it up.

Defined pydantic settings for all license goal type APIs.
Migration scripts have been added for the Goal enum and to move the data into IntegrationConfigurations.
The External Integration data has not been deleted, yet.
The Admin CollectionSettings controller defines new methods for integration configurations and uses them appropriately.
All API classes have been modified to utilise IntegrationConfigurations.
All API classes use the modified HasCollectionSelfTests class.
Migrated all collection tests to use IntegrationConfigurations.

⚠️ If you create a collection while in this branch, then downgrade the DB migration to move to another branch. The upgrade will not work again until the new collection has been deleted from the database. This is because the new collection would not have the ConfigurationSettings the migration relies on.

JIRA

Motivation and Context

How Has This Been Tested?

Manually imported an OPDS feed.
Randomly ran collection self tests.
Modified the unit tests to work with integration configurations.
Modified existing collections and created new collections from the Admin UI.

Checklist

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

codecov · 2023-06-09T10:17:54Z

Codecov Report

Patch coverage: 90.90% and project coverage change: -0.06 ⚠️

Comparison is base (920041b) 89.94% compared to head (38451dc) 89.88%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1189      +/-   ##
==========================================
- Coverage   89.94%   89.88%   -0.06%     
==========================================
  Files         195      197       +2     
  Lines       29203    29451     +248     
  Branches     6714     6749      +35     
==========================================
+ Hits        26266    26473     +207     
- Misses       1913     1946      +33     
- Partials     1024     1032       +8

Impacted Files	Coverage Δ
api/admin/controller/patron_auth_services.py	`91.39% <33.33%> (-2.49%)`	⬇️
api/admin/controller/collection_self_tests.py	`97.14% <83.33%> (+2.69%)`	⬆️
core/migration/migrate_external_integration.py	`84.61% <84.61%> (ø)`
core/opds2_import.py	`79.56% <85.18%> (-1.20%)`	⬇️
core/opds_import.py	`89.23% <86.53%> (-0.54%)`	⬇️
api/admin/controller/collection_settings.py	`91.17% <87.50%> (+0.06%)`	⬆️
api/lcp/collection.py	`90.36% <88.88%> (-1.31%)`	⬇️
api/admin/controller/__init__.py	`86.82% <89.79%> (-0.63%)`	⬇️
api/lcp/encrypt.py	`83.22% <90.00%> (+0.10%)`	⬆️
api/lcp/server.py	`92.77% <90.00%> (+1.41%)`	⬆️
... and 18 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

RishiDiwanTT · 2023-06-12T10:31:58Z

api/admin/controller/collection_settings.py

This file contains all the Admin UI functionality for the Collections page

core/migration/migrate_external_integration.py

core/model/__init__.py

api/admin/controller/__init__.py

RishiDiwanTT · 2023-06-13T07:27:00Z

api/admin/controller/__init__.py

            settings = getattr(api, "SETTINGS", [])
            protocol["settings"] = list(settings)
+            if _db and issubclass(api, HasIntegrationConfiguration):
+                protocol["settings"] = api.settings_class().configuration_form(_db)


Both cls.SETTINGS and def settings_class must coexist while some integrations still use external integrations.

api/admin/controller/collection_self_tests.py

api/integration/registry/license_providers.py

RishiDiwanTT · 2023-06-13T08:16:40Z

api/lcp/collection.py

        lcp_server = LCPServer(
-            configuration_storage,
-            configuration_factory,
+            self.configuration,


Notice that we pass in a callable.

RishiDiwanTT · 2023-06-13T08:18:01Z

api/lcp/factory.py

        lcp_server = LCPServer(
-            configuration_storage,
-            configuration_factory,
+            integration_association.configuration,


This is a Callable.

RishiDiwanTT · 2023-06-13T08:19:48Z

api/lcp/server.py

        self,
-        configuration_storage,
-        configuration_factory,
+        get_configuration,


Rather than a configuration factory and storage we use a get_configuration Callable to ensure the configuration values we pull at runtime are always pulled from the database and not an in-memory object.

api/odl2.py

api/selftest.py

core/configuration/ignored_identifier.py

core/connection_config.py

RishiDiwanTT · 2023-06-13T09:15:56Z

core/integration/settings.py

+    NUMBER = "number"
+
+    @classmethod
+    def options_from_enum(cls, enum_: Type[Enum]) -> Dict[Enum | str, str]:


The return type is actually Dict[str, str] but due to type invariance of the ConfigurationFormItem.options attribute we must match the typing.

RishiDiwanTT · 2023-06-13T09:15:57Z

core/integration/settings.py

+    NUMBER = "number"
+
+    @classmethod
+    def options_from_enum(cls, enum_: Type[Enum]) -> Dict[Enum | str, str]:


The return type is actually Dict[str, str] but due to type invariance of the ConfigurationFormItem.options attribute we must match the typing.

RishiDiwanTT · 2023-06-13T09:18:29Z

core/model/collection.py

+                IntegrationConfiguration.settings[
+                    Collection.DATA_SOURCE_NAME_SETTING
+                ].astext
+                == data_source


JSONB filter

core/model/collection.py

jonathangreen

Wow, this looks like it was a lot of work! Thanks for this, PR is looking good. I added some comments with a few things, as well as the change we talked about to move away from using the dict lookups directly.

core/model/__init__.py

api/admin/controller/__init__.py

api/admin/controller/collection_self_tests.py

core/overdrive.py

alembic/versions/20230531_0af587ff8595_migrate_license_integrations_to_.py

core/model/collection.py

api/integration/registry/license_providers.py

… the API implementations Added a BaseCirculationAPIProtocol for mypy reasons

jonathangreen · 2023-06-21T13:29:53Z

I think the code here is looking good @RishiDiwanTT. Before merging, I wanted to make sure that we did a smoke test of the migration with an existing database, just to make sure it looked okay.

I tried with the database from minotaur, and I'm seeing an error:

INFO  [alembic.runtime.migration] Running upgrade a9ed3f76d649 -> b883671b7bc5, Add the license type goal
INFO  [alembic.runtime.migration] Running upgrade b883671b7bc5 -> 0af587ff8595, Migrate license integrations to configuration settings
Traceback (most recent call last):
  File "/Users/jgreen/.pyenv/versions/3.9.16/envs/circulation/lib/python3.9/site-packages/sqlalchemy/engine/base.py", line 1905, in _execute_context
    self.dialect.do_execute(
  File "/Users/jgreen/.pyenv/versions/3.9.16/envs/circulation/lib/python3.9/site-packages/sqlalchemy/engine/default.py", line 736, in do_execute
    cursor.execute(statement, parameters)
psycopg2.errors.UndefinedColumn: column "status" of relation "integration_configurations" does not exist

I think that may be an issue that came in during the rebase. Can you take a look? If you want a copy of the database, ping me on slack and I can send it to you.

jonathangreen

Looks good Rishi!

I added one small comment, that maybe should be resolved before merging, but otherwise this looks great.

jonathangreen · 2023-06-21T15:13:41Z

alembic/versions/20230525_0a1c9c3f5dd2_revert_pr_980.py

            postgresql.ENUM("RED", "GREEN", name="status"),
            autoincrement=False,
            nullable=False,
+            server_default=sa.text("'GREEN'::status"),


Not sure we should add this line to an existing migration.

This was required because NULL is not allowed in the column, so rollbacks were failing.

RishiDiwanTT added DB migration This PR contains a DB migration incompatible changes Changes that require a new major version labels Jun 9, 2023

RishiDiwanTT requested a review from a team June 9, 2023 07:13

RishiDiwanTT self-assigned this Jun 9, 2023

RishiDiwanTT changed the title ~~Migrate license goal integrations ⛰️~~ PP-35 Migrate license goal integrations ⛰️ Jun 9, 2023

RishiDiwanTT force-pushed the feature/migrate-license-goal-integrations branch from de63c98 to e6b4a53 Compare June 12, 2023 08:22

RishiDiwanTT commented Jun 12, 2023

View reviewed changes

api/admin/controller/collection_settings.py

Copy link

Contributor Author

RishiDiwanTT Jun 12, 2023

Choose a reason for hiding this comment

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

This file contains all the Admin UI functionality for the Collections page