From 0736da3bfa36f281fe141bd9235bd97033775963 Mon Sep 17 00:00:00 2001 From: Nickolas Comeau Date: Tue, 19 Sep 2023 15:47:21 -0400 Subject: [PATCH] `--force` option for proposal changes (#330) * Add --force flag to proposal creation and modify_date * skip check for overlapping proposals if force flag provided * Incorporate a warning for when proposals are being created/modified to overlap with an existing proposal * Use BooleanOptionalAction instead * Test function signature with --force provided * First pass of tests for --force option * Active proposal query used in tests sorts by descending start date * Use active proposal query in --force argument tests * Sort found active proposals and investments by start date if more than one is present * include note about using force flag in error --- bank/account_logic.py | 61 ++++++++++++++------ bank/cli/parsers.py | 16 ++++- tests/_utils.py | 6 +- tests/account_logic/test_ProposalServices.py | 47 +++++++++++++-- tests/cli/parsers/test_ProposalParser.py | 11 ++++ 5 files changed, 115 insertions(+), 26 deletions(-) diff --git a/bank/account_logic.py b/bank/account_logic.py index de346d6a..4dfb969a 100644 --- a/bank/account_logic.py +++ b/bank/account_logic.py @@ -11,6 +11,7 @@ from logging import getLogger from math import ceil from typing import Collection, Iterable, Optional, Union +from warnings import warn from dateutil.relativedelta import relativedelta from prettytable import PrettyTable @@ -39,8 +40,9 @@ def __init__(self, account_name: str) -> None: self._account_name = account.account_name AccountServices.setup_db_account_entry(self._account_name) - def _get_active_proposal_id(self) -> None: - """Return the active proposal ID for the current account + def _get_active_proposal_id(self) -> int: + """Return the active proposal ID for the current account. If there are multiple "active" proposals, + return the ID of the proposal with the most recent start date Raises: MissingProposalError: If no active proposal is found @@ -50,7 +52,8 @@ def _get_active_proposal_id(self) -> None: active_proposal_id_query = select(Proposal.id) \ .where(Proposal.account_id.in_(subquery)) \ - .where(Proposal.is_active) + .where(Proposal.is_active) \ + .order_by(Proposal.start_date.desc()) with DBConnection.session() as session: proposal_id = session.execute(active_proposal_id_query).scalars().first() @@ -101,6 +104,7 @@ def create( self, start: Optional[date] = date.today(), end: Optional[date] = None, + force: Optional[bool] = False, **clusters_sus: int ) -> None: """Create a new proposal for the account @@ -108,6 +112,7 @@ def create( Args: start: The start date of the proposal, default is today end: Date of the proposal expiration, default is 1 year + force: Optionally replace current active proposal with provided values, default is false **clusters_sus: Service units to allocate to each cluster """ @@ -130,9 +135,16 @@ def create( not_(and_(start >= Proposal.end_date, end > Proposal.end_date)) ) ) - - if session.execute(overlapping_proposal_query).scalars().first(): - raise ProposalExistsError('Proposals for a given account cannot overlap.') + overlapping_proposal = session.execute(overlapping_proposal_query).scalars().first() + if overlapping_proposal: + if force: + warn("Creating the proposal despite overlap with an existing proposal") + else: + raise ProposalExistsError(f'By default, proposals for a given account cannot overlap: \n' + f' existing range {overlapping_proposal.start_date} ' + f'- {overlapping_proposal.end_date} \n' + f' provided range {start} - {end} \n' + f'This can be overridden with the `--force` flag') # Create the new proposal and allocations new_proposal = Proposal( @@ -177,7 +189,8 @@ def modify_date( self, proposal_id: Optional[int] = None, start: Optional[date] = None, - end: Optional[date] = None + end: Optional[date] = None, + force: Optional[bool] = False ) -> None: """Overwrite the date of an account proposal @@ -185,10 +198,13 @@ def modify_date( proposal_id: Modify a specific proposal by its inv_id (Defaults to currently active proposal) start: Optionally set a new start date for the proposal end: Optionally set a new end date for the proposal + force: Optionally overwrite dates even if it would cause overlap with another proposal Raises: MissingProposalError: If the proposal ID does not match the account ValueError: If neither a start date nor end date are provided, and if provided start/end dates are not in chronological order with amongst themselves or with the existing DB values. + ProposalExistsError: If the proposal being created would otherwise overlap with an existing proposal, + and the force flag is not provided. """ proposal_id = proposal_id or self._get_active_proposal_id() @@ -218,13 +234,20 @@ def modify_date( .where(Proposal.account_id.in_(overlapping_proposal_sub_1)) \ .where( and_( - not_(and_(start < Proposal.start_date, end <= Proposal.start_date)), - not_(and_(start >= Proposal.end_date, end > Proposal.end_date)) + not_(and_(start < Proposal.start_date, end <= Proposal.start_date)), + not_(and_(start >= Proposal.end_date, end > Proposal.end_date)) ) ) - - if session.execute(overlapping_proposal_query).scalars().first(): - raise ProposalExistsError('Proposals for a given account cannot overlap.') + overlapping_proposal = session.execute(overlapping_proposal_query).scalars().first() + if overlapping_proposal: + if force: + warn("Modifying the proposal dates despite overlap with an existing proposal") + else: + raise ProposalExistsError(f'By default, proposals for a given account cannot overlap: \n' + f' existing range {overlapping_proposal.start_date} ' + f'- {overlapping_proposal.end_date} \n' + f' provided range {start} - {end} \n' + f'This can be overridden with the `--force` flag') # Update the proposal record if start != proposal.start_date: @@ -286,10 +309,11 @@ def subtract_sus(self, proposal_id: Optional[int] = None, **clusters_sus: int) - self._verify_proposal_id(proposal_id) self._verify_cluster_values(**clusters_sus) - query = select(Allocation).join(Proposal).where(Proposal.id == proposal_id) + query = select(Proposal).where(Proposal.id == proposal_id) with DBConnection.session() as session: - allocations = session.execute(query).scalars().all() - for allocation in allocations: + proposal = session.execute(query).scalars().first() + + for allocation in proposal.allocations: allocation.service_units_total -= clusters_sus.get(allocation.cluster_name, 0) session.commit() @@ -629,14 +653,16 @@ def __init__(self, account_name: str) -> None: self._active_proposal_query = select(Proposal) \ .where(Proposal.account_id.in_(subquery)) \ - .where(Proposal.is_active) + .where(Proposal.is_active) \ + .order_by(Proposal.start_date.desc()) self._recent_proposals_query = select(Proposal) \ .where(Proposal.account_id.in_(subquery)) self._active_investment_query = select(Investment) \ .where(Investment.account_id.in_(subquery)) \ - .where(Investment.is_active) + .where(Investment.is_active) \ + .order_by(Investment.start_date.desc()) self._investments_query = select(Investment) \ .where(Investment.account_id.in_(subquery)) @@ -929,6 +955,7 @@ def update_status(self) -> None: # Gather the account's active proposal and investments if they exist proposal = session.execute(self._active_proposal_query).scalars().first() + # TODO: utilize all investments (scalars().all()) investment = session.execute(self._active_investment_query).scalars().first() # Update proposal or investment usage diff --git a/bank/cli/parsers.py b/bank/cli/parsers.py index 38c841f6..9d59d575 100644 --- a/bank/cli/parsers.py +++ b/bank/cli/parsers.py @@ -2,9 +2,8 @@ application's commandline interface. Individual parsers are designed around different services provided by the banking app. """ - import sys -from argparse import ArgumentParser, Namespace +from argparse import ArgumentParser, BooleanOptionalAction, Namespace from datetime import datetime from typing import List, Tuple @@ -190,6 +189,12 @@ def __init__(self, *args, **kwargs) -> None: metavar='date', type=Date, help=f'proposal end date ({safe_date_format}) - defaults to 1 year from today') + create_parser.add_argument( + '--force', + action=BooleanOptionalAction, + help=f"boolean flag for whether or not to set the existing proposal to inactive and substitute a proposal " + f"with the provided values in it's place - default is False" + ) self._add_cluster_args(create_parser) # Proposal deletion @@ -231,6 +236,12 @@ def __init__(self, *args, **kwargs) -> None: metavar='date', type=Date, help=f'set a new proposal end date ({safe_date_format})') + modify_date_parser.add_argument( + '--force', + action=BooleanOptionalAction, + help=f"boolean flag for whether or not to overwrite the existing proposal's dates, even if it " + f"would otherwise cause date range overlap - default is False" + ) @staticmethod def _add_cluster_args(parser: ArgumentParser) -> None: @@ -344,6 +355,7 @@ def __init__(self, *args, **kwargs) -> None: type=Date, help=f'set a new investment end date ({safe_date_format})') + # Advance investment SUs advance_parser = subparsers.add_parser( name='advance', help='forward service units from future investments to a given investment') diff --git a/tests/_utils.py b/tests/_utils.py index ad3fde82..2e58360f 100644 --- a/tests/_utils.py +++ b/tests/_utils.py @@ -26,11 +26,13 @@ active_proposal_query = select(Proposal) \ .where(Proposal.account_id.in_(account_subquery)) \ - .where(Proposal.is_active) + .where(Proposal.is_active) \ + .order_by(Proposal.start_date.desc()) active_investment_query = select(Investment) \ .where(Investment.account_id.in_(account_subquery)) \ - .where(Investment.is_active) + .where(Investment.is_active) \ + .order_by(Investment.start_date.desc()) def add_proposal_to_test_account(proposal: Proposal) -> None: diff --git a/tests/account_logic/test_ProposalServices.py b/tests/account_logic/test_ProposalServices.py index bb46939c..c74be82e 100644 --- a/tests/account_logic/test_ProposalServices.py +++ b/tests/account_logic/test_ProposalServices.py @@ -8,8 +8,8 @@ from bank.account_logic import ProposalServices from bank.exceptions import MissingProposalError, ProposalExistsError, AccountNotFoundError from bank.orm import Account, Allocation, DBConnection, Proposal -from tests._utils import account_proposals_query, DAY_AFTER_TOMORROW, DAY_BEFORE_YESTERDAY, EmptyAccountSetup, \ - ProposalSetup, TODAY, TOMORROW, YESTERDAY +from tests._utils import active_proposal_query, account_proposals_query, DAY_AFTER_TOMORROW, DAY_BEFORE_YESTERDAY, \ + EmptyAccountSetup, ProposalSetup, TODAY, TOMORROW, YESTERDAY joined_tables = join(join(Allocation, Proposal), Account) sus_query = select(Allocation.service_units_total) \ @@ -18,6 +18,7 @@ .where(Allocation.cluster_name == settings.test_cluster) \ .where(Proposal.is_active) + class InitExceptions(EmptyAccountSetup, TestCase): """Tests to ensure proposals report that provided account does not exist""" @@ -246,7 +247,7 @@ def test_error_on_subtract(self) -> None: self.account.subtract_sus(**{settings.test_cluster: 1}) -class PreventOverlappingProposals(EmptyAccountSetup, TestCase): +class OverlappingProposals(EmptyAccountSetup, TestCase): """Tests to ensure proposals cannot overlap in time""" def setUp(self) -> None: @@ -281,10 +282,46 @@ def test_error_on_proposal_creation_default_dates(self): self.account.create() def test_error_on_proposal_modification(self): - """Test existing proposals can not be modified to overlap with other proposals""" + """Test existing proposals can not be modified to overlap with other proposals by default""" self.account.create(start=YESTERDAY, end=TOMORROW, **{settings.test_cluster: 100}) - self.account.create(start=DAY_AFTER_TOMORROW, end=DAY_AFTER_TOMORROW+timedelta(days=2), **{settings.test_cluster: 100}) + self.account.create(start=DAY_AFTER_TOMORROW, + end=DAY_AFTER_TOMORROW+timedelta(days=2), + **{settings.test_cluster: 100}) with self.assertRaises(ProposalExistsError): self.account.modify_date(end=DAY_AFTER_TOMORROW) + + def test_success_on_proposal_forced_create(self): + """Test existing proposals can be created to overlap with other proposals, when force flag is provided""" + + self.account.create(start=YESTERDAY, end=DAY_AFTER_TOMORROW, **{settings.test_cluster: 100}) + first_id = self.account._get_active_proposal_id() + + with self.assertWarns(UserWarning): + self.account.create(start=TODAY, + end=DAY_AFTER_TOMORROW+timedelta(days=2), + force=True, + **{settings.test_cluster: 100}) + second_id = self.account._get_active_proposal_id() + + # The new active proposal ID should be that of the second proposal + self.assertNotEqual(first_id, second_id) + + def test_success_on_proposal_forced_modify_date(self): + """Test existing proposals can be modified to overlap with other proposals, when force flag is provided""" + + self.account.create(start=DAY_BEFORE_YESTERDAY, end=TOMORROW, **{settings.test_cluster: 100}) + first_id = self.account._get_active_proposal_id() + + self.account.create(start=YESTERDAY, + end=DAY_AFTER_TOMORROW+timedelta(days=2), + force=True, + **{settings.test_cluster: 100}) + second_id = self.account._get_active_proposal_id() + self.assertNotEqual(first_id, second_id) + + with self.assertWarns(UserWarning): + self.account.modify_date(proposal_id=first_id, start=TODAY, force=True) + + self.assertEqual(first_id, self.account._get_active_proposal_id()) diff --git a/tests/cli/parsers/test_ProposalParser.py b/tests/cli/parsers/test_ProposalParser.py index 86975de9..245d15e6 100644 --- a/tests/cli/parsers/test_ProposalParser.py +++ b/tests/cli/parsers/test_ProposalParser.py @@ -20,6 +20,12 @@ def test_create_proposal(self) -> None: self.assert_parser_matches_func_signature(ProposalParser(), f'create {TEST_ACCOUNT} --{TEST_CLUSTER} 100') + def test_create_proposal_force(self) -> None: + """Test proposal creation providing the force argument""" + + self.assert_parser_matches_func_signature(ProposalParser(), + f'create {TEST_ACCOUNT} --{TEST_CLUSTER} 100 --force') + def test_missing_account_name_error(self) -> None: """Test a ``SystemExit`` error is raised for a missing ``account`` argument""" @@ -213,6 +219,11 @@ def test_modify_active_proposal(self) -> None: self.assert_parser_matches_func_signature( ProposalParser(), f'modify_date {TEST_ACCOUNT} --start {self.start_date_str} --end {self.end_date_str}') + # Modify the start and end dates, forcing + self.assert_parser_matches_func_signature( + ProposalParser(), + f'modify_date {TEST_ACCOUNT} --start {self.start_date_str} --end {self.end_date_str} --force') + def test_modify_specific_proposal(self) -> None: """Test changing the dates while specifying a proposal ID"""