From 094451d50da32b639898f9db10d2ac245edf52e3 Mon Sep 17 00:00:00 2001 From: Sasha Dresden Date: Thu, 12 Dec 2024 15:12:30 -0500 Subject: [PATCH 1/2] Added tests to make sure all Views use CommitteeOwnedViewMixin and all models use CommitteeOwnedModel, with some exceptions --- .../committee_accounts/test_models.py | 42 ++++++++++- .../fecfiler/committee_accounts/test_views.py | 75 ++++++++++++++++++- django-backend/fecfiler/memo_text/views.py | 3 +- django-backend/fecfiler/reports/views.py | 2 +- django-backend/fecfiler/web_services/views.py | 4 +- 5 files changed, 115 insertions(+), 11 deletions(-) diff --git a/django-backend/fecfiler/committee_accounts/test_models.py b/django-backend/fecfiler/committee_accounts/test_models.py index 9faa4b6ee1..fa7357ac5b 100644 --- a/django-backend/fecfiler/committee_accounts/test_models.py +++ b/django-backend/fecfiler/committee_accounts/test_models.py @@ -1,5 +1,6 @@ +from django.apps import apps from django.test import TestCase -from .models import CommitteeAccount +from .models import CommitteeAccount, CommitteeOwnedModel class CommitteeAccountTestCase(TestCase): @@ -16,9 +17,7 @@ def test_get_contact(self): def test_save_and_delete(self): self.valid_committee_account.save() - committee_account_from_db = CommitteeAccount.objects.get( - committee_id="C87654321" - ) + committee_account_from_db = CommitteeAccount.objects.get(committee_id="C87654321") self.assertIsInstance(committee_account_from_db, CommitteeAccount) self.assertEquals(committee_account_from_db.committee_id, "C87654321") committee_account_from_db.delete() @@ -39,3 +38,38 @@ def test_save_and_delete(self): CommitteeAccount.all_objects.get, committee_id="C87654321", ) + + def test_all_models_include_committee_owned_mixin(self): + ignore_list = [ + "Permission", + "Group", + "ContentType", + "Session", + "CommitteeAccount", + "Membership", + "Form3X", + "Form24", + "Form99", + "Form1M", + "ReportTransaction", + "ScheduleA", + "ScheduleB", + "ScheduleC", + "ScheduleC1", + "ScheduleC2", + "ScheduleD", + "ScheduleE", + "DotFEC", + "UploadSubmission", + "WebPrintSubmission", + "User", + ] + app_models = apps.get_models() + + for model in app_models: + with self.subTest(model=model): + if model.__name__ not in ignore_list: + self.assertTrue( + issubclass(model, CommitteeOwnedModel), + f"{model.__name__} does not include CommitteeOwnedModel mixin.", + ) diff --git a/django-backend/fecfiler/committee_accounts/test_views.py b/django-backend/fecfiler/committee_accounts/test_views.py index bd7780e4d6..8d320629dc 100644 --- a/django-backend/fecfiler/committee_accounts/test_views.py +++ b/django-backend/fecfiler/committee_accounts/test_views.py @@ -1,8 +1,15 @@ +from importlib import import_module +import inspect +import os from uuid import UUID from django.test import RequestFactory, TestCase from fecfiler.committee_accounts.models import Membership -from fecfiler.committee_accounts.views import CommitteeMembershipViewSet, CommitteeViewSet - +from fecfiler.committee_accounts.views import ( + CommitteeMembershipViewSet, + CommitteeOwnedViewMixin, + CommitteeViewSet, +) +from rest_framework.viewsets import ViewSetMixin from fecfiler.user.models import User from unittest.mock import Mock, patch @@ -221,3 +228,67 @@ def test_get_committee_account_data_from_redis(self): self.assertNotEqual(len(was_called_with), 0) self.assertIn("C12345678", was_called_with) self.assertEqual(response.data["name"], "TEST") + + def test_all_viewsets_include_mixin(self): + script_dir = os.path.dirname(os.path.abspath(__file__)) + project_root = os.path.join(script_dir, "../..") + exclude_dirs = ["venv", "__pycache__"] + + exclude_list = [ + "CommitteeViewSet", + "UserViewSet", + "SystemStatusViewSet", + "WebServicesViewSet", + "SummaryViewSet", + "FeedbackViewSet", + ] + + subclasses = find_subclasses(ViewSetMixin, project_root, exclude_dirs) + missing_mixin = [ + subclass + for subclass in subclasses + if not issubclass(subclass, CommitteeOwnedViewMixin) + and subclass.__name__ not in exclude_list + ] + + self.assertEqual( + len(missing_mixin), + 0, + f"The following subclasses of GenericViewSet " + f"do not include CommitteeOwnedViewMixin:" + f"{', '.join([f'{cls.__module__}.{cls.__name__}' for cls in missing_mixin])}", + ) + + +def find_subclasses(base_class, project_path, exclude_dirs=None): + subclasses = [] + exclude_dirs = exclude_dirs or [] + + for root, dirs, files in os.walk(project_path): + dirs[:] = [d for d in dirs if d not in exclude_dirs] + + for file in files: + if ( + file.endswith("views.py") + and not file.startswith("__init__") + and "test_views" not in file + ): + module_path = os.path.join(root, file) + module_name = ( + module_path.replace(project_path, "") + .replace(os.sep, ".") + .lstrip(".") + .replace(".py", "") + ) + try: + module = import_module(module_name) + except (ModuleNotFoundError, ImportError): + print(f"module not found: {module_name}") + continue + + for name, obj in inspect.getmembers(module, inspect.isclass): + if obj.__module__ == module.__name__: + if issubclass(obj, base_class) and obj != base_class: + subclasses.append(obj) + + return subclasses diff --git a/django-backend/fecfiler/memo_text/views.py b/django-backend/fecfiler/memo_text/views.py index d7c22ec563..2397260a85 100644 --- a/django-backend/fecfiler/memo_text/views.py +++ b/django-backend/fecfiler/memo_text/views.py @@ -1,11 +1,10 @@ from .models import MemoText from .serializers import MemoTextSerializer -from fecfiler.committee_accounts.views import CommitteeOwnedViewMixin from fecfiler.reports.views import ReportViewMixin from rest_framework.viewsets import ModelViewSet -class MemoTextViewSet(CommitteeOwnedViewMixin, ReportViewMixin, ModelViewSet): +class MemoTextViewSet(ReportViewMixin, ModelViewSet): def get_queryset(self): memos = super().get_queryset() query_params = self.request.query_params.keys() diff --git a/django-backend/fecfiler/reports/views.py b/django-backend/fecfiler/reports/views.py index eb6db1ecf8..6a83461803 100644 --- a/django-backend/fecfiler/reports/views.py +++ b/django-backend/fecfiler/reports/views.py @@ -183,7 +183,7 @@ def list(self, request, *args, **kwargs): return super().list(request, args, kwargs) -class ReportViewMixin(GenericViewSet): +class ReportViewMixin(CommitteeOwnedViewMixin, GenericViewSet): def get_queryset(self): return filter_by_report(super().get_queryset(), self) diff --git a/django-backend/fecfiler/web_services/views.py b/django-backend/fecfiler/web_services/views.py index cd8b3f7c03..e55b123a03 100644 --- a/django-backend/fecfiler/web_services/views.py +++ b/django-backend/fecfiler/web_services/views.py @@ -17,13 +17,13 @@ from .models import DotFEC, UploadSubmission, WebPrintSubmission from fecfiler.reports.models import Report, FORMS_TO_CALCULATE from celery.result import AsyncResult - +from fecfiler.committee_accounts.views import CommitteeOwnedViewMixin import structlog logger = structlog.get_logger(__name__) -class WebServicesViewSet(viewsets.ViewSet): +class WebServicesViewSet(CommitteeOwnedViewMixin, viewsets.ViewSet): """ A viewset that provides actions to start web service tasks and retrieve thier statuses and results From 3f731265db477a26301b3f54411efa64ee7bebeb Mon Sep 17 00:00:00 2001 From: Sasha Dresden Date: Mon, 16 Dec 2024 16:31:58 -0500 Subject: [PATCH 2/2] Instead of using a file search to find all the ViewSet, made use of the django routers to identify all the ViewSets --- django-backend/fecfiler/cash_on_hand/urls.py | 5 +- .../fecfiler/committee_accounts/test_views.py | 94 +++++++------------ .../fecfiler/committee_accounts/urls.py | 10 +- django-backend/fecfiler/contacts/urls.py | 9 +- django-backend/fecfiler/devops/urls.py | 6 +- django-backend/fecfiler/feedback/urls.py | 5 +- django-backend/fecfiler/memo_text/urls.py | 8 +- django-backend/fecfiler/reports/urls.py | 5 +- django-backend/fecfiler/routers.py | 14 +++ django-backend/fecfiler/transactions/urls.py | 5 +- django-backend/fecfiler/user/urls.py | 5 +- django-backend/fecfiler/web_services/urls.py | 5 +- 12 files changed, 73 insertions(+), 98 deletions(-) create mode 100644 django-backend/fecfiler/routers.py diff --git a/django-backend/fecfiler/cash_on_hand/urls.py b/django-backend/fecfiler/cash_on_hand/urls.py index ca579e4b4f..b6fccb35b9 100644 --- a/django-backend/fecfiler/cash_on_hand/urls.py +++ b/django-backend/fecfiler/cash_on_hand/urls.py @@ -1,9 +1,8 @@ from django.urls import path, include -from rest_framework.routers import DefaultRouter +from fecfiler.routers import register_router from .views import CashOnHandYearlyViewSet -# Create a router and register our viewsets with it. -router = DefaultRouter() +router = register_router() router.register(r"cash_on_hand", CashOnHandYearlyViewSet, basename="cash_on_hand") # The API URLs are now determined automatically by the router. diff --git a/django-backend/fecfiler/committee_accounts/test_views.py b/django-backend/fecfiler/committee_accounts/test_views.py index 8d320629dc..0de4ad7e1c 100644 --- a/django-backend/fecfiler/committee_accounts/test_views.py +++ b/django-backend/fecfiler/committee_accounts/test_views.py @@ -1,6 +1,3 @@ -from importlib import import_module -import inspect -import os from uuid import UUID from django.test import RequestFactory, TestCase from fecfiler.committee_accounts.models import Membership @@ -9,9 +6,12 @@ CommitteeOwnedViewMixin, CommitteeViewSet, ) -from rest_framework.viewsets import ViewSetMixin from fecfiler.user.models import User from unittest.mock import Mock, patch +from fecfiler.routers import get_all_routers +import structlog + +logger = structlog.get_logger(__name__) class CommitteeMemberViewSetTest(TestCase): @@ -229,66 +229,40 @@ def test_get_committee_account_data_from_redis(self): self.assertIn("C12345678", was_called_with) self.assertEqual(response.data["name"], "TEST") - def test_all_viewsets_include_mixin(self): - script_dir = os.path.dirname(os.path.abspath(__file__)) - project_root = os.path.join(script_dir, "../..") - exclude_dirs = ["venv", "__pycache__"] - + def test_viewsets_have_committee_owned_mixin(self): exclude_list = [ "CommitteeViewSet", "UserViewSet", "SystemStatusViewSet", - "WebServicesViewSet", "SummaryViewSet", "FeedbackViewSet", ] - - subclasses = find_subclasses(ViewSetMixin, project_root, exclude_dirs) - missing_mixin = [ - subclass - for subclass in subclasses - if not issubclass(subclass, CommitteeOwnedViewMixin) - and subclass.__name__ not in exclude_list - ] - - self.assertEqual( - len(missing_mixin), - 0, - f"The following subclasses of GenericViewSet " - f"do not include CommitteeOwnedViewMixin:" - f"{', '.join([f'{cls.__module__}.{cls.__name__}' for cls in missing_mixin])}", - ) - - -def find_subclasses(base_class, project_path, exclude_dirs=None): - subclasses = [] - exclude_dirs = exclude_dirs or [] - - for root, dirs, files in os.walk(project_path): - dirs[:] = [d for d in dirs if d not in exclude_dirs] - - for file in files: - if ( - file.endswith("views.py") - and not file.startswith("__init__") - and "test_views" not in file - ): - module_path = os.path.join(root, file) - module_name = ( - module_path.replace(project_path, "") - .replace(os.sep, ".") - .lstrip(".") - .replace(".py", "") - ) - try: - module = import_module(module_name) - except (ModuleNotFoundError, ImportError): - print(f"module not found: {module_name}") - continue - - for name, obj in inspect.getmembers(module, inspect.isclass): - if obj.__module__ == module.__name__: - if issubclass(obj, base_class) and obj != base_class: - subclasses.append(obj) - - return subclasses + routers = get_all_routers() + missing_mixin = [] + + for router in routers: + for prefix, viewset, basename in router.registry: + if ( + not issubclass(viewset, CommitteeOwnedViewMixin) + and viewset.__name__ not in exclude_list + ): + missing_mixin.append( + { + "viewset": viewset.__name__, + "prefix": prefix, + "basename": basename, + } + ) + + if missing_mixin: + error_message = "\n".join( + [ + f"ViewSet '{entry['viewset']}' " + f"does not inherit from CommitteeOwnedViewMixin." + for entry in missing_mixin + ] + ) + self.fail( + f"The following ViewSets are missing CommitteeOwnedViewMixin:\n" + f"{error_message}" + ) diff --git a/django-backend/fecfiler/committee_accounts/urls.py b/django-backend/fecfiler/committee_accounts/urls.py index fefada1e75..081d3d25f2 100644 --- a/django-backend/fecfiler/committee_accounts/urls.py +++ b/django-backend/fecfiler/committee_accounts/urls.py @@ -1,11 +1,11 @@ -from rest_framework.routers import DefaultRouter +from fecfiler.routers import register_router from .views import CommitteeMembershipViewSet, CommitteeViewSet -# Create a router and register our viewsets with it. -router = DefaultRouter() +router = register_router() router.register(r"committees", CommitteeViewSet, basename="committees") -router.register(r"committee-members", CommitteeMembershipViewSet, - basename="committee-members") +router.register( + r"committee-members", CommitteeMembershipViewSet, basename="committee-members" +) # The API URLs are now determined automatically by the router. urlpatterns = [] diff --git a/django-backend/fecfiler/contacts/urls.py b/django-backend/fecfiler/contacts/urls.py index c09d40f8fd..a1da3e1241 100644 --- a/django-backend/fecfiler/contacts/urls.py +++ b/django-backend/fecfiler/contacts/urls.py @@ -1,13 +1,10 @@ from django.urls import path, include -from rest_framework.routers import DefaultRouter +from fecfiler.routers import register_router from .views import ContactViewSet, DeletedContactsViewSet -# Create a router and register our viewsets with it. -router = DefaultRouter() +router = register_router() router.register(r"contacts", ContactViewSet, basename="contacts") -router.register( - r"contacts-deleted", DeletedContactsViewSet, basename="contacts-deleted" -) +router.register(r"contacts-deleted", DeletedContactsViewSet, basename="contacts-deleted") # The API URLs are now determined automatically by the router. urlpatterns = [ diff --git a/django-backend/fecfiler/devops/urls.py b/django-backend/fecfiler/devops/urls.py index 6bfa58d676..64a5221b8f 100644 --- a/django-backend/fecfiler/devops/urls.py +++ b/django-backend/fecfiler/devops/urls.py @@ -1,10 +1,8 @@ from django.urls import include, path -from rest_framework.routers import DefaultRouter +from fecfiler.routers import register_router from fecfiler.devops.views import SystemStatusViewSet - -# Create a router and register our viewsets with it. -router = DefaultRouter() +router = register_router() router.register(r"devops", SystemStatusViewSet, basename="devops") # The API URLs are now determined automatically by the router. diff --git a/django-backend/fecfiler/feedback/urls.py b/django-backend/fecfiler/feedback/urls.py index 37899a4d26..a8ba99af31 100644 --- a/django-backend/fecfiler/feedback/urls.py +++ b/django-backend/fecfiler/feedback/urls.py @@ -1,10 +1,9 @@ from django.urls import include, path -from rest_framework.routers import DefaultRouter +from fecfiler.routers import register_router from .views import FeedbackViewSet -# Create a router and register our viewsets with it. -router = DefaultRouter() +router = register_router() router.register(r"feedback", FeedbackViewSet, basename="feedback") # The API URLs are now determined automatically by the router. diff --git a/django-backend/fecfiler/memo_text/urls.py b/django-backend/fecfiler/memo_text/urls.py index fc156a1683..ab11771202 100644 --- a/django-backend/fecfiler/memo_text/urls.py +++ b/django-backend/fecfiler/memo_text/urls.py @@ -1,11 +1,9 @@ from django.urls import path, include -from rest_framework.routers import DefaultRouter +from fecfiler.routers import register_router from .views import MemoTextViewSet -# Create a router and register our viewsets with it. -router = DefaultRouter() -router.register(r"memo-text", MemoTextViewSet, - basename="memo-text") +router = register_router() +router.register(r"memo-text", MemoTextViewSet, basename="memo-text") # The API URLs are now determined automatically by the router. urlpatterns = [ diff --git a/django-backend/fecfiler/reports/urls.py b/django-backend/fecfiler/reports/urls.py index 7842d58580..3f7e75f984 100644 --- a/django-backend/fecfiler/reports/urls.py +++ b/django-backend/fecfiler/reports/urls.py @@ -1,13 +1,12 @@ from django.urls import path, include from fecfiler.reports.form_99.views import Form99ViewSet -from rest_framework.routers import DefaultRouter +from fecfiler.routers import register_router from .form_3x.views import Form3XViewSet from .form_24.views import Form24ViewSet from .form_1m.views import Form1MViewSet from .views import ReportViewSet -# Create a router and register our viewsets with it. -router = DefaultRouter() +router = register_router() router.register(r"form-3x", Form3XViewSet, basename="form-3x") router.register(r"form-24", Form24ViewSet, basename="form-24") router.register(r"form-99", Form99ViewSet, basename="form-99") diff --git a/django-backend/fecfiler/routers.py b/django-backend/fecfiler/routers.py new file mode 100644 index 0000000000..250711a2d5 --- /dev/null +++ b/django-backend/fecfiler/routers.py @@ -0,0 +1,14 @@ +from rest_framework.routers import DefaultRouter + +# Global list to store all routers +_all_routers = [] + + +def register_router(): + router = DefaultRouter() + _all_routers.append(router) + return router + + +def get_all_routers(): + return _all_routers diff --git a/django-backend/fecfiler/transactions/urls.py b/django-backend/fecfiler/transactions/urls.py index b18d9f32f3..fd49a36b3f 100644 --- a/django-backend/fecfiler/transactions/urls.py +++ b/django-backend/fecfiler/transactions/urls.py @@ -1,9 +1,8 @@ from django.urls import path, include -from rest_framework.routers import DefaultRouter +from fecfiler.routers import register_router from .views import TransactionViewSet -# Create a router and register our viewsets with it. -router = DefaultRouter() +router = register_router() router.register(r"", TransactionViewSet, basename="transactions") # The API URLs are now determined automatically by the router. diff --git a/django-backend/fecfiler/user/urls.py b/django-backend/fecfiler/user/urls.py index 31f20452d9..b98396d642 100644 --- a/django-backend/fecfiler/user/urls.py +++ b/django-backend/fecfiler/user/urls.py @@ -1,9 +1,8 @@ from django.urls import path, include -from rest_framework.routers import DefaultRouter +from fecfiler.routers import register_router from .views import UserViewSet -# Create a router and register our viewsets with it. -router = DefaultRouter() +router = register_router() router.register(r"users", UserViewSet, basename="users") # The API URLs are now determined automatically by the router. diff --git a/django-backend/fecfiler/web_services/urls.py b/django-backend/fecfiler/web_services/urls.py index 28104c81d2..0468b0e6ad 100644 --- a/django-backend/fecfiler/web_services/urls.py +++ b/django-backend/fecfiler/web_services/urls.py @@ -1,10 +1,9 @@ from django.urls import path, include -from rest_framework.routers import DefaultRouter +from fecfiler.routers import register_router from .views import WebServicesViewSet from .summary.views import SummaryViewSet -# Create a router and register our viewsets with it. -router = DefaultRouter() +router = register_router() router.register(r"web-services", WebServicesViewSet, basename="web-services") router.register(r"web-services/summary", SummaryViewSet, basename="summary")