Changes as per requested from code review

Move the serialize method to the billing_schema
Update variable names
Improve tests
Fix bug in command

app/billing/billing_schemas.py

@@ -1,3 +1,5 @@
+from datetime import datetime
+
 create_or_update_free_sms_fragment_limit_schema = {
     "$schema": "http://json-schema.org/draft-04/schema#",
     "description": "POST annual billing schema",
@@ -8,3 +10,17 @@ create_or_update_free_sms_fragment_limit_schema = {
     },
     "required": ["free_sms_fragment_limit"]
 }
+
+
+def serialize_ft_billing_remove_emails(data):
+    results = []
+    billed_notifications = [x for x in data if x.notification_type != 'email']
+    for notifications in billed_notifications:
+        json_result = {
+            "month": (datetime.strftime(notifications.month, "%B")),
+            "notification_type": notifications.notification_type,
+            "billing_units": int(notifications.billable_units),
+            "rate": float(notifications.rate),
+        }
+        results.append(json_result)
+    return results

app/billing/rest.py

@@ -1,25 +1,30 @@
-from datetime import datetime
 import json
+from datetime import datetime
 
 from flask import Blueprint, jsonify, request
 
+from app.billing.billing_schemas import (
+    create_or_update_free_sms_fragment_limit_schema,
+    serialize_ft_billing_remove_emails
+)
+from app.dao.annual_billing_dao import (
+    dao_get_free_sms_fragment_limit_for_year,
+    dao_get_all_free_sms_fragment_limit,
+    dao_create_or_update_annual_billing_for_year,
+    dao_update_annual_billing_for_future_years
+)
+from app.dao.date_util import get_current_financial_year_start_year
+from app.dao.date_util import get_months_for_financial_year
 from app.dao.fact_billing_dao import fetch_monthly_billing_for_year
 from app.dao.monthly_billing_dao import (
     get_billing_data_for_financial_year,
     get_monthly_billing_by_notification_type
 )
-from app.dao.date_util import get_months_for_financial_year
+from app.errors import InvalidRequest
 from app.errors import register_errors
 from app.models import SMS_TYPE, EMAIL_TYPE, LETTER_TYPE
-from app.utils import convert_utc_to_bst
-from app.dao.annual_billing_dao import (dao_get_free_sms_fragment_limit_for_year,
-                                        dao_get_all_free_sms_fragment_limit,
-                                        dao_create_or_update_annual_billing_for_year,
-                                        dao_update_annual_billing_for_future_years)
-from app.billing.billing_schemas import create_or_update_free_sms_fragment_limit_schema
-from app.errors import InvalidRequest
 from app.schema_validation import validate
-from app.dao.date_util import get_current_financial_year_start_year
+from app.utils import convert_utc_to_bst
 
 billing_blueprint = Blueprint(
     'billing',
@@ -38,7 +43,7 @@ def get_yearly_usage_by_monthly_from_ft_billing(service_id):
     except TypeError:
         return jsonify(result='error', message='No valid year provided'), 400
     results = fetch_monthly_billing_for_year(service_id=service_id, year=year)
-    data = serialize_ft_billing(results)
+    data = serialize_ft_billing_remove_emails(results)
     return jsonify(data)
 
 
@@ -204,17 +209,3 @@ def update_free_sms_fragment_limit_data(service_id, free_sms_fragment_limit, fin
             free_sms_fragment_limit,
             financial_year_start
         )
-
-
-def serialize_ft_billing(data):
-    results = []
-    no_emails = [x for x in data if x.notification_type != 'email']
-    for d in no_emails:
-        j = {
-            "month": (datetime.strftime(d.month, "%B")),
-            "notification_type": d.notification_type,
-            "billing_units": int(d.billable_units),
-            "rate": float(d.rate),
-        }
-        results.append(j)
-    return results

app/commands.py

@@ -599,4 +599,4 @@ def compare_ft_billing_to_monthly_billing(year, service_id=None):
         with current_app.test_request_context(
                 path='/service/{}/billing/ft-monthly-usage?year={}'.format(service_id, year)):
             ft_billing_response = get_yearly_usage_by_monthly_from_ft_billing(service_id)
-        compare_ft_billing_to_monthly_billing(ft_billing_response, monthly_billing_response)
+        compare_monthly_billing_to_ft_billing(ft_billing_response, monthly_billing_response)

tests/app/billing/test_billing.py

@@ -1,3 +1,4 @@
+from calendar import monthrange
 from datetime import datetime, timedelta
 import json
 
@@ -438,8 +439,7 @@ def test_get_yearly_usage_by_monthly_from_ft_billing(client, notify_db_session):
     letter_template = create_template(service=service, template_type="letter")
     for month in range(1, 13):
         mon = str(month).zfill(2)
-        days_in_month = {1: 32, 2: 30, 3: 32, 4: 31, 5: 32, 6: 31, 7: 32, 8: 32, 9: 31, 10: 32, 11: 31, 12: 32}
-        for day in range(1, days_in_month[month]):
+        for day in range(1, monthrange(2016, month)[1] + 1):
             d = str(day).zfill(2)
             create_ft_billing(bst_date='2016-{}-{}'.format(mon, d),
                               service=service,
@@ -489,10 +489,9 @@ def test_compare_ft_billing_to_monthly_billing(client, notify_db_session):
     sms_template = create_template(service=service, template_type="sms")
     email_template = create_template(service=service, template_type="email")
     letter_template = create_template(service=service, template_type="letter")
-    for month in range(4, 9):
+    for month in range(1, 13):
         mon = str(month).zfill(2)
-        days_in_month = {1: 32, 2: 30, 3: 32, 4: 31, 5: 32, 6: 31, 7: 32, 8: 32, 9: 31, 10: 32, 11: 31, 12: 32}
-        for day in range(1, days_in_month[month]):
+        for day in range(1, monthrange(2016, month)[1] + 1):
             d = str(day).zfill(2)
             create_ft_billing(bst_date='2016-{}-{}'.format(mon, d),
                               service=service,
@@ -515,7 +514,6 @@ def test_compare_ft_billing_to_monthly_billing(client, notify_db_session):
                               template=letter_template,
                               notification_type='letter',
                               rate=0.33)
-
         start_date, end_date = get_month_start_and_end_date_in_utc(datetime(2016, int(mon), 1))
         create_monthly_billing_entry(service=service, start_date=start_date,
                                      end_date=end_date,

tests/app/dao/test_ft_billing_dao.py

@@ -1,3 +1,4 @@
+from calendar import monthrange
 from decimal import Decimal
 
 from datetime import datetime, timedelta
@@ -259,37 +260,47 @@ def test_fetch_monthly_billing_for_year_return_financial_year(notify_db_session)
     sms_template = create_template(service=service, template_type="sms")
     email_template = create_template(service=service, template_type="email")
     letter_template = create_template(service=service, template_type="letter")
+
+    for year in (2016, 2017):
         for month in range(1, 13):
             mon = str(month).zfill(2)
-        days_in_month = {1: 32, 2: 30, 3: 32, 4: 31, 5: 32, 6: 31, 7: 32, 8: 32, 9: 31, 10: 32, 11: 31, 12: 32}
-        for day in range(1, days_in_month[month]):
+            for day in range(1, monthrange(year, month)[1] + 1):
                 d = str(day).zfill(2)
-            create_ft_billing(bst_date='2016-{}-{}'.format(mon, d),
+                create_ft_billing(bst_date='{}-{}-{}'.format(year, mon, d),
                                   service=service,
                                   template=sms_template,
                                   notification_type='sms',
                                   rate=0.162)
-            create_ft_billing(bst_date='2016-{}-{}'.format(mon, d),
+                create_ft_billing(bst_date='{}-{}-{}'.format(year, mon, d),
                                   service=service,
                                   template=email_template,
                                   notification_type='email',
                                   rate=0)
-            create_ft_billing(bst_date='2016-{}-{}'.format(mon, d),
+                create_ft_billing(bst_date='{}-{}-{}'.format(year, mon, d),
                                   service=service,
                                   template=letter_template,
                                   notification_type='letter',
                                   rate=0.33)
 
     results = fetch_monthly_billing_for_year(service.id, 2016)
-    # returns 3 rows, per month, returns financial year april to december
+    # returns 3 rows, per month, returns financial year april to end of march
     # Orders by Month
 
-    assert len(results) == 27
+    assert len(results) == 36
     assert str(results[0].month) == "2016-04-01 00:00:00+01:00"
     assert results[0].notification_type == 'email'
+    assert results[0].notifications_sent == 30
+    assert results[0].billable_units == 30
+    assert results[0].rate == Decimal('0')
     assert str(results[1].month) == "2016-04-01 00:00:00+01:00"
     assert results[1].notification_type == 'letter'
+    assert results[1].notifications_sent == 30
+    assert results[1].billable_units == 30
+    assert results[1].rate == Decimal('0.33')
     assert str(results[2].month) == "2016-04-01 00:00:00+01:00"
     assert results[2].notification_type == 'sms'
+    assert results[2].notifications_sent == 30
+    assert results[2].billable_units == 30
+    assert results[2].rate == Decimal('0.162')
     assert str(results[3].month) == "2016-05-01 00:00:00+01:00"
-    assert str(results[26].month) == "2016-12-01 00:00:00+00:00"
+    assert str(results[35].month) == "2017-03-01 00:00:00+00:00"