From 8a635f8f575a565067730cfc768bafe109a13562 Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Wed, 30 Oct 2024 15:33:56 -0300 Subject: [PATCH] Stop using python money objects, and just use Decimal instead --- poetry.lock | 14 +--- pyproject.toml | 2 - src/palace/manager/api/authentication/base.py | 12 +--- .../manager/api/circulation_exceptions.py | 2 +- src/palace/manager/api/config.py | 8 +-- src/palace/manager/api/millenium_patron.py | 5 +- src/palace/manager/api/sip/__init__.py | 2 +- src/palace/manager/api/util/patron.py | 9 +-- src/palace/manager/util/__init__.py | 25 +++---- tests/manager/api/test_authenticator.py | 10 +-- tests/manager/api/test_config.py | 4 +- tests/manager/api/test_patron_utility.py | 1 + tests/manager/util/test_util.py | 72 ++++++++++++++----- 13 files changed, 88 insertions(+), 78 deletions(-) diff --git a/poetry.lock b/poetry.lock index 82c1a75d69..5346321860 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1537,8 +1537,6 @@ files = [ {file = "frozendict-2.4.4-cp39-cp39-musllinux_1_1_x86_64.whl", hash = "sha256:d13b4310db337f4d2103867c5a05090b22bc4d50ca842093779ef541ea9c9eea"}, {file = "frozendict-2.4.4-cp39-cp39-win_amd64.whl", hash = "sha256:b3b967d5065872e27b06f785a80c0ed0a45d1f7c9b85223da05358e734d858ca"}, {file = "frozendict-2.4.4-cp39-cp39-win_arm64.whl", hash = "sha256:4ae8d05c8d0b6134bfb6bfb369d5fa0c4df21eabb5ca7f645af95fdc6689678e"}, - {file = "frozendict-2.4.4-py311-none-any.whl", hash = "sha256:705efca8d74d3facbb6ace80ab3afdd28eb8a237bfb4063ed89996b024bc443d"}, - {file = "frozendict-2.4.4-py312-none-any.whl", hash = "sha256:d9647563e76adb05b7cde2172403123380871360a114f546b4ae1704510801e5"}, {file = "frozendict-2.4.4.tar.gz", hash = "sha256:3f7c031b26e4ee6a3f786ceb5e3abf1181c4ade92dce1f847da26ea2c96008c7"}, ] @@ -2549,16 +2547,6 @@ files = [ {file = "MarkupSafe-2.1.5.tar.gz", hash = "sha256:d283d37a890ba4c1ae73ffadf8046435c76e7bc2247bbb63c00bd1a709c6544b"}, ] -[[package]] -name = "money" -version = "1.3.0" -description = "Python Money Class" -optional = false -python-versions = "*" -files = [ - {file = "money-1.3.0.tar.gz", hash = "sha256:ac049caf19df9502f7a481d5959fc51b038baaa68c5dcef2070ffbb785729f7d"}, -] - [[package]] name = "msgpack" version = "1.0.8" @@ -5186,4 +5174,4 @@ lxml = ">=3.8" [metadata] lock-version = "2.0" python-versions = ">=3.10,<4" -content-hash = "1dab2a8585f34a1b06a4fb3ea843c1d4cff705e5ff3205faa9aae5483cfdc7b8" +content-hash = "45c2dcd455f64e81499d53304bfcbba49313dbd0b426ca9b2d85323dae6c9ae8" diff --git a/pyproject.toml b/pyproject.toml index 67cfd16567..e50615c601 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -181,7 +181,6 @@ module = [ "jwcrypto", "kombu.*", "lxml.*", - "money", "multipledispatch", "nameparser", "onelogin.saml2.*", @@ -236,7 +235,6 @@ jsonschema = "^4.22.0" jwcrypto = "^1.4.2" levenshtein = "^0.26" lxml = {extras = ["html-clean"], version = "^5.2.1"} -money = "1.3.0" multipledispatch = "^1.0" nameparser = "^1.1" # nameparser is for author name manipulations opensearch-dsl = "~1.0" diff --git a/src/palace/manager/api/authentication/base.py b/src/palace/manager/api/authentication/base.py index 824abf8572..07aa2b0aff 100644 --- a/src/palace/manager/api/authentication/base.py +++ b/src/palace/manager/api/authentication/base.py @@ -3,7 +3,6 @@ from abc import ABC, abstractmethod from typing import TypeVar -from money import Money from sqlalchemy.orm import Session from werkzeug.datastructures import Authorization @@ -222,10 +221,8 @@ def __init__( :param external_type: A string classifying the patron according to some library-specific scheme. - :param fines: A Money object representing the amount the - patron owes in fines. Note that only the value portion of the - Money object will be stored in the database; the currency portion - will be ignored. (e.g. "20 USD" will become 20) + :param fines: A Decimal object representing the amount the + patron owes in fines. :param block_reason: A string indicating why the patron is blocked from borrowing items. (Even if this is set to None, it @@ -324,11 +321,6 @@ def fines(self): @fines.setter def fines(self, value): - """When setting patron fines, only store the numeric portion of - a Money object. - """ - if isinstance(value, Money): - value = value.amount self._fines = value def apply(self, patron: Patron): diff --git a/src/palace/manager/api/circulation_exceptions.py b/src/palace/manager/api/circulation_exceptions.py index 1bc693ca59..e43a7f89a6 100644 --- a/src/palace/manager/api/circulation_exceptions.py +++ b/src/palace/manager/api/circulation_exceptions.py @@ -160,7 +160,7 @@ def __init__( parsed_fines = None if fines: try: - parsed_fines = MoneyUtility.parse(fines).amount + parsed_fines = MoneyUtility.parse(fines) except ValueError: # If the fines are not in a valid format, we'll just leave them as None. ... diff --git a/src/palace/manager/api/config.py b/src/palace/manager/api/config.py index c7a44bec81..25217caa9c 100644 --- a/src/palace/manager/api/config.py +++ b/src/palace/manager/api/config.py @@ -1,10 +1,10 @@ from collections.abc import Iterable +from decimal import Decimal from Crypto.Cipher import PKCS1_OAEP from Crypto.Cipher.PKCS1_OAEP import PKCS1OAEP_Cipher from Crypto.PublicKey import RSA from flask_babel import lazy_gettext as _ -from money import Money from palace.manager.core.config import CannotLoadConfiguration # noqa: autoflake from palace.manager.core.config import IntegrationException # noqa: autoflake @@ -93,10 +93,10 @@ def tiny_collection_languages(cls, library: Library) -> list[str]: return library.settings.tiny_collection_languages @classmethod - def max_outstanding_fines(cls, library: Library) -> Money | None: - if library.settings.max_outstanding_fines is None: + def max_outstanding_fines(cls, library: Library) -> Decimal | None: + if (max_fines := library.settings.max_outstanding_fines) is None: return None - return MoneyUtility.parse(library.settings.max_outstanding_fines) + return MoneyUtility.parse(max_fines) @classmethod def estimate_language_collections_for_library(cls, library: Library) -> None: diff --git a/src/palace/manager/api/millenium_patron.py b/src/palace/manager/api/millenium_patron.py index 126391630f..718ad3d09b 100644 --- a/src/palace/manager/api/millenium_patron.py +++ b/src/palace/manager/api/millenium_patron.py @@ -6,7 +6,6 @@ import dateutil from flask_babel import lazy_gettext as _ from lxml import etree -from money import Money from pydantic import field_validator from palace.manager.api.authentication.base import PatronData @@ -475,9 +474,9 @@ def patron_dump_to_patrondata(self, current_identifier, content): fines = MoneyUtility.parse(v) except ValueError: self.log.warning( - 'Malformed fine amount for patron: "%s". Treating as no fines.' + f"Malformed fine amount ({v}) for patron. Treating as no fines." ) - fines = Money("0", "USD") + fines = MoneyUtility.parse(0) elif k == self.BLOCK_FIELD: block_reason = self._patron_block_reason(self.block_types, v) elif k == self.EXPIRATION_FIELD: diff --git a/src/palace/manager/api/sip/__init__.py b/src/palace/manager/api/sip/__init__.py index a0f17d08da..ef9c994373 100644 --- a/src/palace/manager/api/sip/__init__.py +++ b/src/palace/manager/api/sip/__init__.py @@ -464,7 +464,7 @@ def info_to_patrondata_block_reason( # patron has excessive fines, we can use that as the reason # they're blocked. if "fee_limit" in info: - fee_limit = MoneyUtility.parse(info["fee_limit"]).amount + fee_limit = MoneyUtility.parse(info["fee_limit"]) if fee_limit and patrondata.fines > fee_limit: block_reason = PatronData.EXCESSIVE_FINES diff --git a/src/palace/manager/api/util/patron.py b/src/palace/manager/api/util/patron.py index 58d7bff874..1a622f7574 100644 --- a/src/palace/manager/api/util/patron.py +++ b/src/palace/manager/api/util/patron.py @@ -1,7 +1,6 @@ import datetime import dateutil -from money import Money from palace.manager.api.circulation_exceptions import ( AuthorizationBlocked, @@ -102,13 +101,9 @@ def has_excess_fines(cls, patron): if not patron.fines: return False - if isinstance(patron.fines, Money): - patron_fines = patron.fines - else: - patron_fines = MoneyUtility.parse(patron.fines) - actual_fines = patron_fines.amount + actual_fines = MoneyUtility.parse(patron.fines) max_fines = Configuration.max_outstanding_fines(patron.library) - if max_fines is not None and actual_fines > max_fines.amount: + if max_fines is not None and actual_fines > max_fines: return True return False diff --git a/src/palace/manager/util/__init__.py b/src/palace/manager/util/__init__.py index d9d34c0b14..a4bb22c1ab 100644 --- a/src/palace/manager/util/__init__.py +++ b/src/palace/manager/util/__init__.py @@ -6,10 +6,10 @@ import string from collections import Counter from collections.abc import Generator, Iterable, Sequence +from decimal import Decimal, InvalidOperation from typing import Any, TypeVar import sqlalchemy -from money import Money import palace.manager.sqlalchemy.flask_sqlalchemy_session @@ -462,25 +462,26 @@ def process_data(cls, data, bigrams): class MoneyUtility: - DEFAULT_CURRENCY = "USD" - @classmethod - def parse(cls, amount: str | float | int | None) -> Money: - """Attempt to turn a string into a Money object.""" - currency = cls.DEFAULT_CURRENCY + def parse(cls, amount: str | float | int | None) -> Decimal: + """Attempt to turn a string into a Decimal object representing money.""" if not amount: amount = "0" amount = str(amount) if amount[0] == "$": - currency = "USD" amount = amount[1:] - # `Money` does not properly handle commas in the amount, so we strip - # them out of US dollar amounts, since it won't change the "value." - if currency == "USD": - amount = "".join(amount.split(",")) - return Money(amount, currency) + # We assume we are working with currency amounts where ',' is used as a possible + # thousands seperator, so we can safely strip them. + amount = "".join(amount.split(",")) + try: + return Decimal(amount).quantize(Decimal("1.00")) + except InvalidOperation: + raise ValueError( + "amount value could not be converted to " + "Decimal(): '{}'".format(amount) + ) from None def is_session(value: object) -> bool: diff --git a/tests/manager/api/test_authenticator.py b/tests/manager/api/test_authenticator.py index 13b02e36cb..87d90f3648 100644 --- a/tests/manager/api/test_authenticator.py +++ b/tests/manager/api/test_authenticator.py @@ -19,7 +19,6 @@ from _pytest._code import ExceptionInfo from flask import url_for from freezegun import freeze_time -from money import Money from sqlalchemy.orm import Session from werkzeug.datastructures import Authorization @@ -72,6 +71,7 @@ ) from palace.manager.sqlalchemy.model.library import Library, LibraryLogo from palace.manager.sqlalchemy.model.patron import Patron +from palace.manager.util import MoneyUtility from palace.manager.util.authentication_for_opds import AuthenticationForOPDSDocument from palace.manager.util.datetime_helpers import utc_now from palace.manager.util.http import RemoteIntegrationException @@ -173,7 +173,7 @@ def patron_data() -> PatronData: personal_name="4", email_address="5", authorization_expires=utc_now(), - fines=Money(6, "USD"), + fines=MoneyUtility.parse(6), block_reason=PatronData.NO_VALUE, ) @@ -225,7 +225,7 @@ def test_to_dict(self, patron_data: PatronData): authorization_expires=patron_data.authorization_expires.strftime( "%Y-%m-%d" ), - fines="6", + fines="6.00", block_reason=None, ) assert data == expect @@ -388,7 +388,7 @@ def test_apply_on_incomplete_information(self, db: DatabaseTransactionFixture): patron.authorization_identifier = "1234" patron.username = "user" patron.last_external_sync = now - patron.fines = Money(10, "USD") + patron.fines = MoneyUtility.parse(10) authenticated_by_username = PatronData( authorization_identifier="user", complete=False ) @@ -2434,7 +2434,7 @@ def test_authentication_creates_missing_patron( patrondata = PatronData( permanent_id=db.fresh_str(), authorization_identifier=db.fresh_str(), - fines=Money(1, "USD"), + fines=MoneyUtility.parse(1), ) provider = mock_basic( diff --git a/tests/manager/api/test_config.py b/tests/manager/api/test_config.py index 398e23605a..3fde0134da 100644 --- a/tests/manager/api/test_config.py +++ b/tests/manager/api/test_config.py @@ -114,10 +114,10 @@ def test_max_outstanding_fines( settings.max_outstanding_fines = 0 max_fines = m(library) assert max_fines is not None - assert 0 == max_fines.amount + assert 0 == max_fines # A more lenient approach. settings.max_outstanding_fines = 100.0 max_fines = m(library) assert max_fines is not None - assert 100 == max_fines.amount + assert 100 == max_fines diff --git a/tests/manager/api/test_patron_utility.py b/tests/manager/api/test_patron_utility.py index 1211a40cb3..d45f43e3fc 100644 --- a/tests/manager/api/test_patron_utility.py +++ b/tests/manager/api/test_patron_utility.py @@ -133,6 +133,7 @@ def test_has_excess_fines( # If you accrue excessive fines you lose borrowing privileges. # Verify that all these tests work no matter what data type has been stored in # patron.fines. + patron_fines: str | int | float | Decimal | None for patron_fines in ("1", "0.75", 1, 1.0, Decimal(1), MoneyUtility.parse("1")): patron.fines = patron_fines diff --git a/tests/manager/util/test_util.py b/tests/manager/util/test_util.py index 5b219f4e04..a7357b194b 100644 --- a/tests/manager/util/test_util.py +++ b/tests/manager/util/test_util.py @@ -6,7 +6,6 @@ from decimal import Decimal import pytest -from money import Money from palace.manager.util import ( Bigrams, @@ -375,32 +374,69 @@ def test_median(self): class TestMoneyUtility: @pytest.mark.parametrize( - "expected_amount, input_amount, money_amount, money_currency", + "expected_amount, expected_string, input_amount", [ - [Decimal("0"), None, "0", "USD"], - [Decimal("4.00"), "4", "4.00", "USD"], - [Decimal("-4.00"), "-4", "-4.00", "USD"], - [Decimal("4.40"), "4.40", "4.40", "USD"], - [Decimal("4.40"), "$4.40", "4.40", "USD"], - [Decimal("4.4"), 4.4, "4.40", "USD"], - [Decimal("4"), 4, "4", "USD"], - [Decimal("0.4"), 0.4, ".4", "USD"], - [Decimal("0.4"), ".4", ".4", "USD"], - [Decimal("4444.40"), "4,444.40", "4444.40", "USD"], + [ + Decimal("0"), + "0.00", + None, + ], + [ + Decimal("4.00"), + "4.00", + "4", + ], + [ + Decimal("-4.00"), + "-4.00", + "-4", + ], + [ + Decimal("4.40"), + "4.40", + "4.40", + ], + [ + Decimal("4.40"), + "4.40", + "$4.40", + ], + [ + Decimal("4.4"), + "4.40", + 4.4, + ], + [ + Decimal("4"), + "4.00", + 4, + ], + [ + Decimal("0.4"), + "0.40", + 0.4, + ], + [ + Decimal("0.4"), + "0.40", + ".4", + ], + [ + Decimal("4444.40"), + "4444.40", + "4,444.40", + ], ], ) def test_parse( self, expected_amount: Decimal, + expected_string: str, input_amount: str | float | int | None, - money_amount: str, - money_currency: str, ): parsed = MoneyUtility.parse(input_amount) - money = Money(money_amount, money_currency) - - assert money == parsed - assert expected_amount == parsed.amount + assert expected_amount == parsed + assert expected_string == str(parsed) @pytest.mark.parametrize( "bad_value",