Merge pull request #3071 from alphagov/add-flake8-bugbear

Do extra code style checks with flake8-bugbear

.flake8

@@ -1,7 +0,0 @@
-[flake8]
-# Rule definitions: http://flake8.pycqa.org/en/latest/user/error-codes.html
-# W503: line break before binary operator
-exclude = venv*,__pycache__,node_modules,cache,migrations,build
-ignore = W503
-max-complexity = 14
-max-line-length = 120

app/dao/users_dao.py

@@ -23,12 +23,12 @@ def create_secret_code():
     return ''.join(map(str, [SystemRandom().randrange(10) for i in range(5)]))
 
 
-def save_user_attribute(usr, update_dict={}):
-    db.session.query(User).filter_by(id=usr.id).update(update_dict)
+def save_user_attribute(usr, update_dict=None):
+    db.session.query(User).filter_by(id=usr.id).update(update_dict or {})
     db.session.commit()
 
 
-def save_model_user(user, update_dict={}, password=None, validated_email_access=False):
+def save_model_user(user, update_dict=None, password=None, validated_email_access=False):
     if password:
         user.password = password
         user.password_changed_at = datetime.utcnow()
@@ -36,7 +36,7 @@ def save_model_user(user, update_dict={}, password=None, validated_email_access=
         user.email_access_validated_at = datetime.utcnow()
     if update_dict:
         _remove_values_for_keys_if_present(update_dict, ['id', 'password_changed_at'])
-        db.session.query(User).filter_by(id=user.id).update(update_dict)
+        db.session.query(User).filter_by(id=user.id).update(update_dict or {})
     else:
         db.session.add(user)
     db.session.commit()

app/schema_validation/__init__.py

@@ -104,8 +104,9 @@ def __format_message(e):
             error_path = e.path.popleft()
             # no need to catch IndexError exception explicity as
             # error_path is None if e.path has no items
-        finally:
-            return error_path
+        except Exception:
+            pass
+        return error_path
 
     def get_error_message(e):
         # e.cause is an exception (such as InvalidPhoneError). if it's not present it was a standard jsonschema error

app/service/rest.py

@@ -543,7 +543,7 @@ def get_detailed_services(start_date, end_date, only_active=False, include_from_
                                                            include_from_test_key=include_from_test_key,
                                                            )
     results = []
-    for service_id, rows in itertools.groupby(stats, lambda x: x.service_id):
+    for _service_id, rows in itertools.groupby(stats, lambda x: x.service_id):
         rows = list(rows)
         s = statistics.format_statistics(rows)
         results.append({

app/v2/errors.py

@@ -34,18 +34,18 @@ class RateLimitError(InvalidRequest):
 class BadRequestError(InvalidRequest):
     message = "An error occurred"
 
-    def __init__(self, fields=[], message=None, status_code=400):
+    def __init__(self, fields=None, message=None, status_code=400):
         self.status_code = status_code
-        self.fields = fields
+        self.fields = fields or []
         self.message = message if message else self.message
 
 
 class ValidationError(InvalidRequest):
     message = "Your notification has failed validation"
 
-    def __init__(self, fields=[], message=None, status_code=400):
+    def __init__(self, fields=None, message=None, status_code=400):
         self.status_code = status_code
-        self.fields = fields
+        self.fields = fields or []
         self.message = message if message else self.message
 
 

gunicorn_config.py

@@ -20,7 +20,7 @@ def on_starting(server):
 
 def worker_abort(worker):
     worker.log.info("worker received ABORT {}".format(worker.pid))
-    for threadId, stack in sys._current_frames().items():
+    for _threadId, stack in sys._current_frames().items():
         worker.log.error(''.join(traceback.format_stack(stack)))
 
 

requirements_for_test.txt

@@ -1,5 +1,6 @@
 -r requirements.txt
 flake8==3.8.4
+flake8-bugbear==20.11.1
 moto==1.3.16
 pytest==6.1.2
 pytest-env==0.6.2

setup.cfg

@@ -3,5 +3,7 @@ xfail_strict=true
 
 
 [flake8]
+exclude = venv*,__pycache__,node_modules,cache,migrations,build
 # W504 line break after binary operator
-extend_ignore=W504
+extend_ignore=B306, W504
+select=B901

tests/app/authentication/test_authentication.py

@@ -479,7 +479,7 @@ def test_should_cache_service_and_api_key_lookups(mocker, client, sample_api_key
         wraps=dao_fetch_service_by_id,
     )
 
-    for i in range(5):
+    for _ in range(5):
         token = __create_token(sample_api_key.service_id)
         client.get('/notifications', headers={
             'Authorization': f'Bearer {token}'

tests/app/aws/test_s3.py

@@ -13,11 +13,11 @@ from app.aws.s3 import (
 from tests.app.conftest import datetime_in_past
 
 
-def single_s3_object_stub(key='foo', last_modified=datetime.utcnow()):
+def single_s3_object_stub(key='foo', last_modified=None):
     return {
         'ETag': '"d41d8cd98f00b204e9800998ecf8427e"',
         'Key': key,
-        'LastModified': last_modified
+        'LastModified': last_modified or datetime.utcnow(),
     }
 
 

tests/app/celery/test_reporting_tasks.py

@@ -212,7 +212,7 @@ def test_create_nightly_billing_for_day_different_sent_by(
     records = FactBilling.query.order_by('rate_multiplier').all()
 
     assert len(records) == 2
-    for i, record in enumerate(records):
+    for _, record in enumerate(records):
         assert record.bst_date == datetime.date(yesterday)
         assert record.rate == Decimal(1.33)
         assert record.billable_units == 1
@@ -226,7 +226,7 @@ def test_create_nightly_billing_for_day_different_letter_postage(
     yesterday = datetime.now() - timedelta(days=1)
     mocker.patch('app.dao.fact_billing_dao.get_rate', side_effect=mocker_get_rate)
 
-    for i in range(2):
+    for _ in range(2):
         create_notification(
             created_at=yesterday,
             template=sample_letter_template,

tests/app/celery/test_tasks.py

@@ -1475,7 +1475,7 @@ def test_send_inbound_sms_to_service_does_not_sent_request_when_inbound_api_does
     mocked = mocker.patch("requests.request")
     send_inbound_sms_to_service(inbound_sms.id, inbound_sms.service_id)
 
-    mocked.call_count == 0
+    assert mocked.call_count == 0
 
 
 def test_send_inbound_sms_to_service_retries_if_request_returns_500(notify_api, sample_service, mocker):
@@ -1525,7 +1525,7 @@ def test_send_inbound_sms_to_service_does_not_retries_if_request_returns_404(not
                           status_code=404)
         send_inbound_sms_to_service(inbound_sms.id, inbound_sms.service_id)
 
-    mocked.call_count == 0
+    assert mocked.call_count == 0
 
 
 def test_process_incomplete_job_sms(mocker, sample_template):

tests/app/dao/notification_dao/test_notification_dao.py

@@ -511,7 +511,7 @@ def test_get_notification_for_job(sample_notification):
 
 
 def test_get_all_notifications_for_job(sample_job):
-    for i in range(0, 5):
+    for _ in range(0, 5):
         try:
             create_notification(template=sample_job.template, job=sample_job)
         except IntegrityError:
@@ -546,7 +546,7 @@ def test_dao_get_notification_count_for_job_id(notify_db_session):
     service = create_service()
     template = create_template(service)
     job = create_job(template, notification_count=3)
-    for i in range(3):
+    for _ in range(3):
         create_notification(job=job)
 
     create_notification(template)

tests/app/dao/test_invited_user_dao.py

@@ -138,13 +138,13 @@ def test_should_not_delete_invitations_less_than_two_days_old(
     assert InvitedUser.query.first().email_address == "valid@2.com"
 
 
-def make_invitation(user, service, age=timedelta(hours=0), email_address="test@test.com"):
+def make_invitation(user, service, age=None, email_address="test@test.com"):
     verify_code = InvitedUser(
         email_address=email_address,
         from_user=user,
         service=service,
         status='pending',
-        created_at=datetime.utcnow() - age,
+        created_at=datetime.utcnow() - (age or timedelta(hours=0)),
         permissions='manage_settings',
         folder_permissions=[str(uuid.uuid4())]
     )

tests/app/dao/test_service_email_reply_to_dao.py

@@ -73,7 +73,7 @@ def test_add_reply_to_email_address_for_service_creates_another_email_for_servic
         elif x.email_address == 'second@address.com':
             assert not x.is_default
         else:
-            assert False
+            raise AssertionError()
 
 
 def test_add_reply_to_email_address_new_reply_to_is_default_existing_reply_to_is_not(notify_db_session):
@@ -89,7 +89,7 @@ def test_add_reply_to_email_address_new_reply_to_is_default_existing_reply_to_is
         elif x.email_address == 'second@address.com':
             assert x.is_default
         else:
-            assert False
+            raise AssertionError()
 
 
 def test_add_reply_to_email_address_can_add_a_third_reply_to_address(sample_service):
@@ -112,7 +112,7 @@ def test_add_reply_to_email_address_can_add_a_third_reply_to_address(sample_serv
         elif x.email_address == 'third@address.com':
             assert not x.is_default
         else:
-            assert False
+            raise AssertionError()
 
 
 def test_add_reply_to_email_address_ensures_first_reply_to_is_default(sample_service):
@@ -161,7 +161,7 @@ def test_update_reply_to_email_address_set_updated_to_default(sample_service):
         elif x.email_address == 'first@address.com':
             assert not x.is_default
         else:
-            assert False
+            raise AssertionError()
 
 
 def test_update_reply_to_email_address_raises_exception_if_single_reply_to_and_setting_default_to_false(sample_service):

tests/app/dao/test_services_dao.py

@@ -512,6 +512,8 @@ def test_removing_all_permission_returns_service_with_no_permissions(notify_db_s
     dao_remove_service_permission(service_id=service.id, permission=EMAIL_TYPE)
     dao_remove_service_permission(service_id=service.id, permission=LETTER_TYPE)
     dao_remove_service_permission(service_id=service.id, permission=INTERNATIONAL_SMS_TYPE)
+    dao_remove_service_permission(service_id=service.id, permission=UPLOAD_LETTERS)
+    dao_remove_service_permission(service_id=service.id, permission=INTERNATIONAL_LETTERS)
 
     service = dao_fetch_service_by_id(service.id)
     assert len(service.permissions) == 0
@@ -1089,22 +1091,22 @@ def test_dao_find_services_sending_to_tv_numbers(notify_db_session, fake_uuid):
 
     for service in services:
         template = create_template(service)
-        for x in range(0, 5):
+        for _ in range(0, 5):
             create_notification(template, normalised_to=tv_number, status="permanent-failure")
 
     service_6 = create_service(service_name="Service 6")  # notifications too old are excluded
     with freeze_time("2019-11-30 15:00:00.000000"):
         template_6 = create_template(service_6)
-        for x in range(0, 5):
+        for _ in range(0, 5):
             create_notification(template_6, normalised_to=tv_number, status="permanent-failure")
 
     service_2 = create_service(service_name="Service 2")  # below threshold is excluded
     template_2 = create_template(service_2)
     create_notification(template_2, normalised_to=tv_number, status="permanent-failure")
-    for x in range(0, 5):
+    for _ in range(0, 5):
         # test key type is excluded
         create_notification(template_2, normalised_to=tv_number, status="permanent-failure", key_type='test')
-    for x in range(0, 5):
+    for _ in range(0, 5):
         # normal numbers are not counted by the query
         create_notification(template_2, normalised_to=normal_number, status="delivered")
         create_notification(template_2, normalised_to=normal_number_resembling_tv_number, status="delivered")
@@ -1126,7 +1128,7 @@ def test_dao_find_services_with_high_failure_rates(notify_db_session, fake_uuid)
 
     for service in services:
         template = create_template(service)
-        for x in range(0, 3):
+        for _ in range(0, 3):
             create_notification(template, status="permanent-failure")
             create_notification(template, status="delivered")
             create_notification(template, status="sending")
@@ -1135,12 +1137,12 @@ def test_dao_find_services_with_high_failure_rates(notify_db_session, fake_uuid)
     service_6 = create_service(service_name="Service 6")
     with freeze_time("2019-11-30 15:00:00.000000"):
         template_6 = create_template(service_6)
-        for x in range(0, 4):
+        for _ in range(0, 4):
             create_notification(template_6, status="permanent-failure")  # notifications too old are excluded
 
     service_2 = create_service(service_name="Service 2")
     template_2 = create_template(service_2)
-    for x in range(0, 4):
+    for _ in range(0, 4):
         create_notification(template_2, status="permanent-failure", key_type='test')  # test key type is excluded
     create_notification(template_2, status="permanent-failure")  # below threshold is excluded
 

tests/app/dao/test_uploads_dao.py

@@ -320,28 +320,28 @@ def test_get_uploaded_letters_by_print_date(sample_template):
     letter_template = create_uploaded_template(sample_template.service)
 
     # Letters for the previous day’s run
-    for i in range(3):
+    for _ in range(3):
         create_uploaded_letter(
             letter_template, sample_template.service, status='delivered',
             created_at=datetime.utcnow().replace(day=1, hour=17, minute=29, second=59)
         )
 
     # Letters from yesterday that rolled into today’s run
-    for i in range(30):
+    for _ in range(30):
         create_uploaded_letter(
             letter_template, sample_template.service, status='delivered',
             created_at=datetime.utcnow().replace(day=1, hour=17, minute=30, second=0)
         )
 
     # Letters that just made today’s run
-    for i in range(30):
+    for _ in range(30):
         create_uploaded_letter(
             letter_template, sample_template.service, status='delivered',
             created_at=datetime.utcnow().replace(hour=17, minute=29, second=59)
         )
 
     # Letters that just missed today’s run
-    for i in range(3):
+    for _ in range(3):
         create_uploaded_letter(
             letter_template, sample_template.service, status='delivered',
             created_at=datetime.utcnow().replace(hour=17, minute=30, second=0)

tests/app/dao/test_users_dao.py

@@ -124,12 +124,12 @@ def test_should_not_delete_verification_codes_less_than_one_day_old(sample_user)
     assert VerifyCode.query.one()._code == "12345"
 
 
-def make_verify_code(user, age=timedelta(hours=0), expiry_age=timedelta(0), code="12335", code_used=False):
+def make_verify_code(user, age=None, expiry_age=None, code="12335", code_used=False):
     verify_code = VerifyCode(
         code_type='sms',
         _code=code,
-        created_at=datetime.utcnow() - age,
-        expiry_datetime=datetime.utcnow() - expiry_age,
+        created_at=datetime.utcnow() - (age or timedelta(hours=0)),
+        expiry_datetime=datetime.utcnow() - (expiry_age or timedelta(0)),
         user=user,
         code_used=code_used
     )

tests/app/db.py

@@ -107,7 +107,7 @@ def create_service(
         service_id=None,
         restricted=False,
         count_as_live=True,
-        service_permissions=[EMAIL_TYPE, SMS_TYPE],
+        service_permissions=None,
         research_mode=False,
         active=True,
         email_from=None,
@@ -136,7 +136,12 @@ def create_service(
             go_live_at=go_live_at,
             crown=crown
         )
-        dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions)
+        dao_create_service(
+            service,
+            service.created_by,
+            service_id,
+            service_permissions=service_permissions,
+        )
 
         service.active = active
         service.research_mode = research_mode
@@ -666,12 +671,12 @@ def create_invited_org_user(organisation, invited_by, email_address='invite@exam
     return invited_org_user
 
 
-def create_daily_sorted_letter(billing_day=date(2018, 1, 18),
+def create_daily_sorted_letter(billing_day=None,
                                file_name="Notify-20180118123.rs.txt",
                                unsorted_count=0,
                                sorted_count=0):
     daily_sorted_letter = DailySortedLetter(
-        billing_day=billing_day,
+        billing_day=billing_day or date(2018, 1, 18),
         file_name=file_name,
         unsorted_count=unsorted_count,
         sorted_count=sorted_count
@@ -1007,22 +1012,22 @@ def create_service_contact_list(
 def create_broadcast_message(
     template,
     created_by=None,
-    personalisation={},
+    personalisation=None,
     status=BroadcastStatusType.DRAFT,
     starts_at=None,
     finishes_at=None,
-    areas={},
+    areas=None,
 ):
     broadcast_message = BroadcastMessage(
         service_id=template.service_id,
         template_id=template.id,
         template_version=template.version,
-        personalisation=personalisation,
+        personalisation=personalisation or {},
         status=status,
         starts_at=starts_at,
         finishes_at=finishes_at,
         created_by_id=created_by.id if created_by else template.created_by_id,
-        areas=areas,
+        areas=areas or {},
     )
     db.session.add(broadcast_message)
     db.session.commit()

tests/app/job/test_rest.py

@@ -536,7 +536,7 @@ def test_create_job_returns_400_if_archived_template(client, sample_template, mo
 
 
 def _setup_jobs(template, number_of_jobs=5):
-    for i in range(number_of_jobs):
+    for _ in range(number_of_jobs):
         create_job(template=template)
 
 

tests/app/letter_branding/test_letter_branding_rest.py

@@ -21,7 +21,7 @@ def test_get_all_letter_brands(client, notify_db_session):
         elif brand['id'] == str(test_branding.id):
             assert test_branding.serialize() == brand
         else:
-            assert False
+            raise AssertionError()
 
 
 def test_get_letter_branding_by_id(client, notify_db_session):

tests/app/notifications/test_process_notification.py

@@ -185,7 +185,7 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key):
     assert NotificationHistory.query.count() == 0
     persisted_notification = Notification.query.all()[0]
     assert persisted_notification.id == n_id
-    persisted_notification.job_id == sample_job.id
+    assert persisted_notification.job_id == sample_job.id
     assert persisted_notification.job_row_number == 10
     assert persisted_notification.created_at == created_at
 

tests/app/notifications/test_receive_notification.py

@@ -385,7 +385,7 @@ def test_returns_ok_to_firetext_if_mismatched_sms_sender(notify_db_session, clie
 
     assert not InboundSms.query.all()
     assert result['status'] == 'ok'
-    mocked.call_count == 0
+    assert mocked.call_count == 0
 
 
 @pytest.mark.parametrize(

tests/app/notifications/test_validators.py

@@ -118,7 +118,7 @@ def test_should_set_cache_value_as_value_from_database_if_cache_not_set(
 ):
     serialised_service = SerialisedService.from_id(sample_service.id)
     with freeze_time("2016-01-01 12:00:00.000000"):
-        for x in range(5):
+        for _ in range(5):
             create_notification(sample_template)
         mocker.patch('app.notifications.validators.redis_store.get', return_value=None)
         mocker.patch('app.notifications.validators.redis_store.set')
@@ -149,7 +149,7 @@ def test_check_service_message_limit_over_message_limit_fails(key_type, sample_s
         template = create_template(sample_service)
         serialised_service = SerialisedService.from_id(sample_service.id)
 
-        for x in range(5):
+        for _ in range(5):
             create_notification(template)
         with pytest.raises(TooManyRequestsError) as e:
             check_service_over_daily_message_limit(key_type, serialised_service)

tests/app/service/test_rest.py

@@ -2149,7 +2149,7 @@ def test_search_for_notification_by_to_field_return_multiple_matches(client, sam
 def test_search_for_notification_by_to_field_returns_next_link_if_more_than_50(
     client, sample_template
 ):
-    for i in range(51):
+    for _ in range(51):
         create_notification(sample_template, to_field='+447700900855', normalised_to='447700900855')
 
     response = client.get(

tests/app/upload/test_rest.py

@@ -212,7 +212,7 @@ def test_get_uploaded_letters_by_print_date(admin_request, sample_template):
 def test_get_uploaded_letters_by_print_date_paginates(admin_request, sample_template):
     letter_template = create_precompiled_template(sample_template.service)
 
-    for i in range(101):
+    for _ in range(101):
         create_uploaded_letter(
             letter_template, sample_template.service, status='delivered',
             created_at=datetime.utcnow() - timedelta(minutes=1)

tests/app/user/test_rest_verify.py

@@ -250,7 +250,7 @@ def test_send_sms_code_returns_404_for_bad_input_data(client):
 
 
 def test_send_sms_code_returns_204_when_too_many_codes_already_created(client, sample_user):
-    for i in range(10):
+    for _ in range(10):
         verify_code = VerifyCode(
             code_type='sms',
             _code=12345,

tests/app/v2/notifications/test_post_letter_notifications.py

@@ -463,14 +463,24 @@ def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded(
     assert not persist_mock.called
 
 
-@pytest.mark.parametrize('service_args', [
-    {'service_permissions': [EMAIL_TYPE, SMS_TYPE]},
-    {'restricted': True}
+@pytest.mark.parametrize('service_args, expected_status, expected_message', [
+    (
+        {'service_permissions': [EMAIL_TYPE, SMS_TYPE]},
+        400,
+        'Service is not allowed to send letters',
+    ),
+    (
+        {'restricted': True},
+        403,
+        'Cannot send letters when service is in trial mode',
+    )
 ])
 def test_post_letter_notification_returns_403_if_not_allowed_to_send_notification(
     client,
     notify_db_session,
-    service_args
+    service_args,
+    expected_status,
+    expected_message,
 ):
     service = create_service(**service_args)
     template = create_template(service, template_type=LETTER_TYPE)
@@ -480,10 +490,10 @@ def test_post_letter_notification_returns_403_if_not_allowed_to_send_notificatio
         'personalisation': test_address
     }
 
-    error_json = letter_request(client, data, service_id=service.id, _expected_status=400)
-    assert error_json['status_code'] == 400
+    error_json = letter_request(client, data, service_id=service.id, _expected_status=expected_status)
+    assert error_json['status_code'] == expected_status
     assert error_json['errors'] == [
-        {'error': 'BadRequestError', 'message': 'Service is not allowed to send letters'}
+        {'error': 'BadRequestError', 'message': expected_message}
     ]
 
 

tests/app/v2/notifications/test_post_notifications.py

@@ -223,7 +223,7 @@ def test_should_cache_template_lookups_in_memory(mocker, client, sample_template
         'template_id': str(sample_template.id),
     }
 
-    for i in range(5):
+    for _ in range(5):
         auth_header = create_authorization_header(service_id=sample_template.service_id)
         client.post(
             path='/v2/notifications/sms',

tests/app/v2/templates/test_get_templates.py

@@ -76,7 +76,7 @@ def test_get_correct_num_templates_for_valid_type_returns_200(client, sample_ser
     num_templates = 3
 
     templates = []
-    for i in range(num_templates):
+    for _ in range(num_templates):
         templates.append(create_template(sample_service, template_type=tmp_type))
 
     for other_type in TEMPLATE_TYPES: