Skip to content

Commit

Permalink
Stop using python money objects, and just use Decimal instead
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathangreen committed Oct 30, 2024
1 parent 987027a commit 8a635f8
Show file tree
Hide file tree
Showing 13 changed files with 88 additions and 78 deletions.
14 changes: 1 addition & 13 deletions poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ module = [
"jwcrypto",
"kombu.*",
"lxml.*",
"money",
"multipledispatch",
"nameparser",
"onelogin.saml2.*",
Expand Down Expand Up @@ -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"
Expand Down
12 changes: 2 additions & 10 deletions src/palace/manager/api/authentication/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down
2 changes: 1 addition & 1 deletion src/palace/manager/api/circulation_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
...
Expand Down
8 changes: 4 additions & 4 deletions src/palace/manager/api/config.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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:
Expand Down
5 changes: 2 additions & 3 deletions src/palace/manager/api/millenium_patron.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion src/palace/manager/api/sip/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
9 changes: 2 additions & 7 deletions src/palace/manager/api/util/patron.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import datetime

import dateutil
from money import Money

from palace.manager.api.circulation_exceptions import (
AuthorizationBlocked,
Expand Down Expand Up @@ -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

Expand Down
25 changes: 13 additions & 12 deletions src/palace/manager/util/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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:
Expand Down
10 changes: 5 additions & 5 deletions tests/manager/api/test_authenticator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
)

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
)
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions tests/manager/api/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions tests/manager/api/test_patron_utility.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
72 changes: 54 additions & 18 deletions tests/manager/util/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from decimal import Decimal

import pytest
from money import Money

from palace.manager.util import (
Bigrams,
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit 8a635f8

Please sign in to comment.