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

Remove SharedCollectionAPI and IntegrationClient 🔥 (PP-224) #1298

Merged
merged 4 commits into from
Aug 10, 2023

Conversation

jonathangreen
Copy link
Member

@jonathangreen jonathangreen commented Jul 26, 2023

Description

In this PR we remove the following classes from the CM:

  • SharedCollectionAPI
  • IntegrationClient

This involves a database migration to drop the integration_client table. I did a quick query of the DBs on the Palace production servers to make sure that none of them have any entries in this table.

JIRA: PP-224

Motivation and Context

This work was started in #1276 as part of the work on PP-11. However when working on a unrelated issue, I noticed that I had removed the "client" side of the Shared ODL integration, but I had failed to remove the "server" side.

Since there is no Shared ODL client anymore, this "server" code is effectively dead, and should also be removed.

How Has This Been Tested?

  • Running tests
  • Did some light testing with local dev server and an ODL feed

Checklist

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

@jonathangreen jonathangreen added DB migration This PR contains a DB migration incompatible changes Changes that require a new major version labels Jul 26, 2023
@jonathangreen jonathangreen requested a review from a team July 26, 2023 14:06
@jonathangreen jonathangreen force-pushed the feature/remove-shared-collection branch from ade9cd5 to db59d64 Compare August 7, 2023 14:25
@jonathangreen
Copy link
Member Author

Rebased this, and resolved the conflicts that came in with #1288.

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.03% ⚠️

Comparison is base (0ed3967) 89.87% compared to head (14a2f87) 89.84%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1298      +/-   ##
==========================================
- Coverage   89.87%   89.84%   -0.03%     
==========================================
  Files         210      208       -2     
  Lines       28727    28542     -185     
  Branches     6567     6536      -31     
==========================================
- Hits        25817    25643     -174     
+ Misses       1896     1890       -6     
+ Partials     1014     1009       -5     
Files Changed Coverage Δ
core/model/patron.py 97.95% <ø> (-0.03%) ⬇️
api/odl2.py 92.07% <100.00%> (ø)
core/metadata_layer.py 88.09% <100.00%> (ø)
core/model/collection.py 96.27% <100.00%> (+0.24%) ⬆️
core/model/datasource.py 87.64% <100.00%> (-0.28%) ⬇️
core/model/licensing.py 87.43% <100.00%> (-0.05%) ⬇️

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

@@ -1409,197 +1409,6 @@ def active_licensepool_for(self, work):
return super().active_licensepool_for(work, library=self.library)


class SharedCollectionAnnotator(CirculationManagerAnnotator):
Copy link
Contributor

Choose a reason for hiding this comment

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

You just blew away days of work from the Annotator refactor 🥳

Copy link
Member Author

Choose a reason for hiding this comment

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

@RishiDiwanTT that is great to hear! 🎉

Trying to save us time, and reduce cognitive overhead when trying to reason about the code it the big reason I've been trying to remove bits and pieces that are no longer used or needed.

Copy link
Contributor

@RishiDiwanTT RishiDiwanTT left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@jonathangreen jonathangreen force-pushed the feature/remove-shared-collection branch from db59d64 to 14a2f87 Compare August 10, 2023 12:53
@jonathangreen jonathangreen merged commit 66ebcdf into main Aug 10, 2023
@jonathangreen jonathangreen deleted the feature/remove-shared-collection branch August 10, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DB migration This PR contains a DB migration incompatible changes Changes that require a new major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants