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 circulation api post processor (PP-501) #1470

Merged
merged 4 commits into from
Oct 20, 2023

Conversation

jonathangreen
Copy link
Member

Description

As a follow up to #1442, this removes the CirculationFulfillmentPostProcessor class. All the logic has been moved into the OPDSAPI and OPDS2API classes.

Motivation and Context

CirculationFulfillmentPostProcessor were only used to work around OPDS importers not having an API class. Now that we have a proper API class there, there is no reason to keep them around.

How Has This Been Tested?

Running tests.

Checklist

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

@jonathangreen jonathangreen force-pushed the feature/opds-importer-api branch from 507c8e7 to 078ae8c Compare October 18, 2023 15:59
Base automatically changed from feature/opds-importer-api to main October 18, 2023 16:25
@jonathangreen jonathangreen force-pushed the feature/remove-circulation-api-post-processor branch from 45eb58c to 3e31953 Compare October 18, 2023 17:02
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (1b835b0) 90.40% compared to head (034d11e) 90.40%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1470      +/-   ##
==========================================
- Coverage   90.40%   90.40%   -0.01%     
==========================================
  Files         230      231       +1     
  Lines       29693    29771      +78     
  Branches     6870     6880      +10     
==========================================
+ Hits        26845    26914      +69     
- Misses       1818     1823       +5     
- Partials     1030     1034       +4     
Flag Coverage Δ
Api 73.82% <29.16%> (-0.22%) ⬇️
Core 61.24% <95.83%> (+0.11%) ⬆️
migration 28.98% <22.22%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
core/opds2_import.py 82.87% <100.00%> (+2.00%) ⬆️
core/saml/wayfless.py 100.00% <100.00%> (ø)
core/opds_import.py 89.61% <90.32%> (+<0.01%) ⬆️

... and 2 files with indirect coverage changes

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

@jonathangreen jonathangreen requested a review from a team October 18, 2023 18:15
@jonathangreen
Copy link
Member Author

Since this one is in the same part of the codebase as #1442, it might be easiest for you to review it @tdilauro. If you have some time to look at it.

Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

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

This looks good!

There are a couple of places where it looks like there's a conflict with #1472, but they should be straightforward to resolve.

Comment on lines 638 to 640
return self.settings_class().construct(
**self.integration_configuration().settings_dict
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might need to resolve some changes from #1472 here when you merge/rebase main.

pytest.raises(
ProblemError, MockAxis360API, axis360.db.session, axis360.collection
)
pytest.raises(ProblemError, Axis360Settings, **settings)
Copy link
Contributor

Choose a reason for hiding this comment

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

#1472 changes here, as well, I think.

@jonathangreen jonathangreen force-pushed the feature/remove-circulation-api-post-processor branch from 21a3b3e to 034d11e Compare October 20, 2023 12:12
@jonathangreen jonathangreen merged commit 71ef82a into main Oct 20, 2023
30 checks passed
@jonathangreen jonathangreen deleted the feature/remove-circulation-api-post-processor branch October 20, 2023 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants