Remove Library type + GeoIP & iptools dependencies #2955

tfmorris · 2020-02-02T18:14:50Z

Closes #2896. Replaces #2866.

Technical

This removes the Library page type and all associated templates, as well as the inlibrary and geo_ip modules, along with all code the references them.

Testing

Evidence

Stakeholders

tfmorris · 2020-02-02T18:38:05Z

From a deployment perspective, you probably want to make sure that there aren't crontab entries or something similar which are updating the GeoIP database, etc. The only dependency changes are dependencies which are no longer needed, so they can be cleaned up when convenient.

hornc · 2020-02-03T01:45:11Z

openlibrary/plugins/books/readlinks.py

-            self.set_inlibrary = True
-            self.inlibrary = inlibrary.get_library()
-        return self.inlibrary
-

    def get_item_status(self, ekey, iaid, collections, subjects):


@mekarpeles Is this readlinks.get_item_status() method use anywhere in the current lending system? It looks like it should be @deprecated or removed. I believe using subjects to determine lending status is fully deprecated, and the subjects are being cleared (see #2107) -- if this is still used, I don't think it will be giving good results.

hornc · 2020-02-03T01:47:33Z

openlibrary/plugins/books/readlinks.py

                status = 'restricted'
-            elif not self.get_inlibrary():
+            elif True: # not self.get_inlibrary(): - Deprecated


I'd probably use else here, but this whole method looks like it should be deprecated, so I'd rather get confirmation from @mekarpeles before changing anything else.

hornc · 2020-02-03T01:50:57Z

openlibrary/plugins/openlibrary/events.py

@@ -30,5 +24,4 @@ def on_new_version(self, page):
 def setup():
    """Installs handlers for various events.
    """
-    eventer.bind("page.edit.libraries", on_library_edit)
    eventer.bind("page.edit", on_page_edit)


This looks like it could be removed, along with the def on_page_edit(page) -- I wouldn't know if this causes other problems. Is there a specific reason you left it with a pass, or just general caution?

Mainly to leave a vestige of the mechanism in place in case someone wanted to use it again. I'm OK with removing the whole thing if you think that's best.

hornc · 2020-02-03T02:01:54Z

Thanks @tfmorris , I'm still reviewing. There are more changes here that I expected, but everything you have removed looks like stuff I'm happy to see gone!

I think even more of that lending code you have touched should be deprecated, but not necessarily in this PR. I think it means there is little risk of changes to those files causing problems since the lending system and has been updated, and we now have an availability API used to provide this functionality elsewhere. I'm hoping @mekarpeles can confirm and ok these changes too.

The main thing I'm unsure about is the stats aspect. I don't know where those stats were going, and whether they are still being used in any way. Again, @mekarpeles will hopefully have more insight.

On the whole this looks like the right approach to me, and I'd like to see it through after some due diligence and review. Thanks!

Getting rid of the Library class and all those templates feels long overdue. I hope no one disagrees - @cdrini, @jdlrobson , please comment here if you have any reservations about removing these files touched by this PR. I'm definitely in favour of seeing them gone.

hornc · 2020-02-03T02:05:20Z

sorry, just realised that @cclauss is responsible for many of these changes too, thank you! :)

mekarpeles · 2020-02-08T00:48:31Z

Most of this LGTM, core.lending is what drives lending and I'm seeing much in the way of dependencies here.

cclauss

RSLGTM

hornc

This looks good, I'm happy it avoids current lending code and is only removing stuff we want gone.

@tfmorris I was going to merge, but this has conflicts -- are you able to resolve / rebase against master?

Fixes #2896

Remove all remaining code and templates for the libraries page type as well as the inlibrary and geo_ip modules and the code that references them.

tfmorris · 2020-02-10T17:23:01Z

@hornc I've rebased it against the current master.

hornc · 2020-02-10T22:58:16Z

Thanks for your efforts in pushing this forward @tfmorris!

I tested locally in my local env, and also on dev.openlibrary.org. Mainly to check that lending status and borrowing were not affected. I did not notice anything. We should keep an eye out after deploy just in case.

This should unblock a number of the smaller Python 3 PRs currently in flight, @cclauss

relates to #2955

openlibrary/core/models.py

mekarpeles · 2020-03-19T20:23:55Z

openlibrary/plugins/openlibrary/libraries.py

-    return LoanStats().get_active_loans_of_libraries()
-
-
-def on_loan_created_statsdb(loan):


:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(:(

Context: Looks like these methods were being used to populate the Borrows graph on the homepage. The setup method of this file hooks to to events and calls these functions. Today we learned. Mek's got a fix in progress.

Thank you for a more meaningful message. The relevant issue is #3194

tfmorris marked this pull request as ready for review February 2, 2020 18:35

tfmorris requested review from cclauss and hornc February 2, 2020 18:35

tfmorris changed the title ~~WIP - Remove Library type + GeoIP & iptools dependencies~~ Remove Library type + GeoIP & iptools dependencies Feb 2, 2020

hornc self-assigned this Feb 3, 2020

hornc reviewed Feb 3, 2020

View reviewed changes

hornc requested a review from mekarpeles February 3, 2020 02:03

hornc added Theme: Upgrade to Python 3 Type: Refactor/Clean-up Issues related to reorganization/clean-up of data or code (e.g. for maintainability). [managed] labels Feb 3, 2020

cclauss mentioned this pull request Feb 7, 2020

WIP: Fix other Python 3 issues after iptools and statsd #2979

Closed

cclauss approved these changes Feb 8, 2020

View reviewed changes

hornc approved these changes Feb 10, 2020

View reviewed changes

cclauss and others added 12 commits February 10, 2020 12:17

WIP: Remove class Library

03133d3

Fixes #2896

Stop registering Library

110a766

Remove TestLibrary

7f38f69

requirements_common.txt: remove iptools

4143b36

Delete test_iprange.py

72eeaca

Delete iprange.py

3c52d3f

Delete inlibrary.py

c6d56e8

Remove OLGeoIP

a473eff

Update .travis.yml

7ba8a4d

Remove libraries type, inlibrary module, geo_ip

7bb750c

Remove all remaining code and templates for the libraries page type as well as the inlibrary and geo_ip modules and the code that references them.

Fix typo

5a64121

Small reorg to make clearer

7dd26fd

tfmorris force-pushed the remove-class-Library branch from 3c9d1d7 to 7dd26fd Compare February 10, 2020 17:21

hornc merged commit df36ecc into master Feb 10, 2020

hornc added a commit that referenced this pull request Feb 10, 2020

remove unsused get_library() reference errors

f5f710d

relates to #2955

This was referenced Feb 10, 2020

remove unsused get_library() reference errors #3014

Merged

from openlibrary.plugins.openlibrary.libraries import LoanStats #3008

Closed

cclauss deleted the remove-class-Library branch February 11, 2020 03:22

mekarpeles reviewed Feb 22, 2020

View reviewed changes

openlibrary/core/models.py Show resolved Hide resolved

mekarpeles reviewed Mar 19, 2020

View reviewed changes

mekarpeles mentioned this pull request Mar 20, 2020

fixing borrow stat graph regression #3217

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Library type + GeoIP & iptools dependencies #2955

Remove Library type + GeoIP & iptools dependencies #2955

tfmorris commented Feb 2, 2020 •

edited

Loading

tfmorris commented Feb 2, 2020

hornc Feb 3, 2020

hornc Feb 3, 2020

hornc Feb 3, 2020

tfmorris Feb 8, 2020

hornc commented Feb 3, 2020

hornc commented Feb 3, 2020

mekarpeles commented Feb 8, 2020

cclauss left a comment

hornc left a comment

tfmorris commented Feb 10, 2020

hornc commented Feb 10, 2020

mekarpeles Mar 19, 2020 •

edited

Loading

cdrini Mar 19, 2020

tfmorris Mar 19, 2020

		return LoanStats().get_active_loans_of_libraries()


		def on_loan_created_statsdb(loan):

Remove Library type + GeoIP & iptools dependencies #2955

Remove Library type + GeoIP & iptools dependencies #2955

Conversation

tfmorris commented Feb 2, 2020 • edited Loading

Technical

Testing

Evidence

Stakeholders

tfmorris commented Feb 2, 2020

hornc Feb 3, 2020

Choose a reason for hiding this comment

hornc Feb 3, 2020

Choose a reason for hiding this comment

hornc Feb 3, 2020

Choose a reason for hiding this comment

tfmorris Feb 8, 2020

Choose a reason for hiding this comment

hornc commented Feb 3, 2020

hornc commented Feb 3, 2020

mekarpeles commented Feb 8, 2020

cclauss left a comment

Choose a reason for hiding this comment

hornc left a comment

Choose a reason for hiding this comment

tfmorris commented Feb 10, 2020

hornc commented Feb 10, 2020

mekarpeles Mar 19, 2020 • edited Loading

Choose a reason for hiding this comment

cdrini Mar 19, 2020

Choose a reason for hiding this comment

tfmorris Mar 19, 2020

Choose a reason for hiding this comment

tfmorris commented Feb 2, 2020 •

edited

Loading

mekarpeles Mar 19, 2020 •

edited

Loading