--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

@@ -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,13 +104,15 @@ 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
 
         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,18 +189,22 @@ 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
 
         Args:
             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

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')

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:

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())

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"""