Remove old columns from collection table #1498
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 tasks
2a439b9 to
eafb469
Compare
Base automatically changed from
feature/remove-external-integration-collection
to
main
November 17, 2023 13:58
jonathangreen
added a commit
that referenced
this pull request
Nov 17, 2023
…1494) This PR changes the data model for our collections in a number of ways with the goal of removing all reliance on external integrations from our collections. All the database columns and tables "removed" by this PR have just been renamed on the Python side, but left in place for now. Once this goes into a release #1498 can be merged to remove the now unused tables and columns. The major changes: The external_integration_id column has been removed. Collections no longer have a name column. Previously both integrations and collections had names, this meant that they could get out of sync. Now collection names are driven by the name of their integration. A migration handles making sure that the existing integrations that have gotten out of sync are updated to have their collections name. The collections_libraries table has been removed. Collections used to be able to be related to libraries in two ways: through the collections_libraries table, and via their integration_library_configuration. This situation was confusing, and meant that these could get out of sync. The collection table no longer uses its external_account_id column. This was duplicated between the table and the settings before, it now lives only in the settings. As with the others the column is still present, but is renamed and unused and will be dropped once this is rolled out everywhere. The settings column get a gin index, to make it easier to query the jsonb column. Some of the properties on the collection class have been moved out to the specific license provider API that used them. These were on the collection model because previously some providers didn't have their own API classes. There are still some of these that could be moved into a more suitable location, but that can happen in a follow up PR. The collection controller and collection self test controller have been updated to use the integration settings controller base classes. This lets us keep all the logic for updating these settings isolated there. The biggest consequence of this change is that we now pass around the integration ID rather then the collection ID when referencing collections and collection self tests. This is transparent to the admin UI, since it just sends back the IDs given to it when it queries for the collection settings. This makes the collection controllers act like the other controllers that modify integrations, rather then using different IDs in the collection controller. The collection self test controller removes the special case that used to exist for OPDS importer collections, since they now have their own API class. This PR removes the self test functionality for the OPDS 1.x importer class, since we don't have any OPDS 1.x collections in our production CMs. Here is a ticket to add self tests for the collection classes missing them (OPDS 2, OPDS 2 + ODL), but this is the situation we've been in for awhile, so no impact from this PR. This PR will make it easier to add these self tests by just implementing the self test interface in the api classes. There is also a DB migration to apply these database changes and an associated migration test. All the major files touched by this PR are now fully type hinted, and added to the strict mypy group in pyproject.toml.
b63457a to
1eca9fc
Compare
e8c6e32 to
119215b
Compare
119215b to
3265bb0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1498 +/- ##
=======================================
Coverage 90.19% 90.19%
=======================================
Files 233 233
Lines 28220 28216 -4
Branches 6485 6485
=======================================
- Hits 25452 25449 -3
Misses 1823 1823
+ Partials 945 944 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is now ready for review, since #1494 is in the v12.1.0 release.
Motivation and Context
Clean up leftover columns, once #1494 has gone into a release, and we know that we are not rolling it back.
How Has This Been Tested?
Checklist