refactor: clean up tests and minor hygiene

- remove spuriously additional build_aip from archive creation
- start to refactor various Factory test mock classes
- add a flag defer_fs currently only used by tests but in the future
  could support creating the published archive asynchronously as a
  scheduled task (huey / temporal / etc)
- hypothesis tests were generating inconsistent results due to issues
  with our state generation for Codebases, CodebaseRelease, Users, etc.
  switching to get_or_create for now, this may have downstream effects
  but probably shouldn't
- minor logger tuning to remove unnecessary messaging

django/core/settings/defaults.py

@@ -10,6 +10,8 @@
 """
 
 import os
+import warnings
+from elasticsearch.exceptions import ElasticsearchWarning
 from enum import Enum
 from pathlib import Path
 
@@ -216,6 +218,9 @@ def is_test(self):
 
 ROOT_URLCONF = "core.urls"
 
+# tune down elasticsearch complaints
+warnings.simplefilter("ignore", category=ElasticsearchWarning)
+
 # configure elasticsearch 7 wagtail backend
 WAGTAILSEARCH_BACKENDS = {
     "default": {

django/core/settings/test.py

@@ -1,6 +1,7 @@
+from os import path
+
 from .defaults import *
 
-from os import path
 
 DEPLOY_ENVIRONMENT = Environment.TEST
 

django/core/tests/base.py

@@ -4,10 +4,10 @@
 import shlex
 import shutil
 import subprocess
-from datetime import date, timedelta
 
+from datetime import date, timedelta
 from django.conf import settings
-from django.contrib.auth.models import User
+from django.contrib.auth import get_user_model
 from django.test import TestCase
 from django.utils import timezone
 
@@ -17,6 +17,8 @@
 
 logger = logging.getLogger(__name__)
 
+User = get_user_model()
+
 
 class ContentModelFactory(ABC):
     def __init__(self, submitter):
@@ -57,6 +59,9 @@ def create(self, **overrides):
         content.save()
         return content
 
+    def get_or_create(self, **kwargs):
+        return self.model.objects.get_or_create(**kwargs)
+
     def create_unsaved(self, **overrides):
         kwargs = self.get_default_data()
         kwargs.update(overrides)
@@ -95,21 +100,17 @@ def get_default_data(self):
         }
 
 
-def make_user(
-    username="test_user",
-    password="default.testing.password",
-    email="[email protected]",
+def create_test_user(
+    username="test_user", email="[email protected]", **kwargs
 ):
-    factory = UserFactory()
-    return factory.create(username=username, password=password, email=email), factory
+    factory = UserFactory(username=username, email=email, **kwargs)
+    return factory.create(), factory
 
 
 class UserFactory:
     def __init__(self, **defaults):
-        if not defaults.get("password"):
-            defaults["password"] = "test"
         self.id = 0
-        self.password = defaults.get("password")
+        self.password = defaults.get("password", "testing-password")
         self.defaults = {}
         username = defaults.get("username")
         if username:
@@ -119,41 +120,41 @@ def __init__(self, **defaults):
             self.defaults.update({"email": email})
 
     def extract_password(self, overrides):
-        if overrides.get("password"):
+        if "password" in overrides:
             return overrides.pop("password")
         else:
             return self.password
 
     def get_default_data(self):
         defaults = self.defaults.copy()
-        defaults["username"] = defaults.get("username", "submitter{}".format(self.id))
+        defaults["username"] = defaults.get("username", f"submitter{self.id}")
         self.id += 1
         return defaults
 
-    def create(self, **overrides):
-        user = self.create_unsaved(**overrides)
+    def create(self, **kwargs):
+        return self.get_or_create(**kwargs)
+
+    def get_or_create(self, **kwargs):
+        password = self.extract_password(kwargs)
+        default_data = self.get_default_data()
+        if "email" not in kwargs:
+            default_email = f"{default_data['username']}@mailinator.com"
+            kwargs.update(email=default_email)
+        if "username" not in kwargs:
+            kwargs.update(username=default_data["username"])
+        user, created = User.objects.get_or_create(**kwargs)
+        user.set_password(password)
         user.save()
         return user
 
-    def create_unsaved(self, **overrides):
-        password = self.extract_password(overrides)
-        kwargs = self.get_default_data()
-        kwargs.update(overrides)
-        if not kwargs.get("email"):
-            kwargs["email"] = "{}@gmail.com".format(kwargs["username"])
-        user = User(**kwargs)
-        if password:
-            user.set_password(password)
-        return user
-
 
 class BaseModelTestCase(TestCase):
     def setUp(self):
         self.user = self.create_user()
 
-    def create_user(self, username="test_user", password="test", **kwargs):
-        kwargs.setdefault("email", "[email protected]")
-        return User.objects.create_user(username=username, password=password, **kwargs)
+    def create_user(self, **kwargs):
+        user, factory = create_test_user(**kwargs)
+        return user
 
 
 def initialize_test_shared_folders():

django/core/tests/permissions_base.py

@@ -5,6 +5,8 @@
 from rest_framework.test import APITestCase
 from django.contrib.auth.models import AnonymousUser
 
+from core.tests.base import UserFactory
+
 import logging
 
 logger = logging.getLogger(__name__)
@@ -59,6 +61,9 @@ class LoginFailure(Exception):
 
 
 class ApiAccountMixin:
+
+    user_factory = UserFactory()
+
     def create_representative_users(self, submitter, user_factory=None):
         # Inactive users cannot login so are not included
         if not user_factory:
@@ -72,26 +77,28 @@ def create_representative_users(self, submitter, user_factory=None):
     def users_able_to_login(self):
         return [self.regular_user, self.superuser, self.submitter]
 
-    def login(self, user, password):
+    def login(self, user, password=None):
         if not user.is_active:
+            logger.debug("Inactive user, cannot login")
             return user, False
-        logged_in = self.client.login(username=user.username, password=password)
-        if not logged_in:
-            if user.is_active:
-                raise LoginFailure()
-        return user, logged_in
+        if password is None:
+            password = self.user_factory.password
+        authenticated = self.client.login(username=user.username, password=password)
+        if not authenticated:
+            raise LoginFailure(
+                f"Failed to login as {user.username} with password {password}"
+            )
+        return user, authenticated
 
 
 class ResponseStatusCodesMixin:
     def responseErrorMessage(self, response, name):
         user = response.wsgi_request.user
-        user_msg = "<{} is_superuser={} is_active={} is_anonymous={}>".format(
-            user.username, user.is_superuser, user.is_active, user.is_anonymous
-        )
+        user_msg = f"<{user} is_superuser={user.is_superuser} is_active={user.is_active} is_anonymous={user.is_anonymous}>"
         data = getattr(response, "data", None)
-        msg = "Response not {} for user {}".format(name, user_msg)
+        msg = f"Response not {name} for user {user_msg}"
         if data is not None:
-            msg += ". Got {}".format(data)
+            msg += f". Received {data}"
         return msg
 
     def assertResponseOk(self, response):
@@ -230,7 +237,7 @@ def check_list_permissions(self, user, instance):
         self.assertEqual(is_visible_in_list, is_instance_public)
 
     def with_logged_in(self, user, instance, method):
-        self.login(user, password=self.user_factory.password)
+        self.login(user)
         method(user, instance)
         self.client.logout()
 

django/core/tests/test_views.py

@@ -6,8 +6,8 @@
 from django.urls import reverse
 from django.test import TestCase
 
-from core.tests.base import UserFactory
-from core.tests.permissions_base import BaseViewSetTestCase
+from .base import create_test_user
+from .permissions_base import BaseViewSetTestCase
 from core.views import EventViewSet, JobViewSet
 from core.models import Job, Event, SpamModeration, ComsesGroups
 from .base import JobFactory, EventFactory
@@ -20,10 +20,9 @@ class JobViewSetTestCase(BaseViewSetTestCase):
     _view = JobViewSet
 
     def setUp(self):
-        self.user_factory = UserFactory()
-        submitter = self.user_factory.create(username="submitter")
-        self.instance_factory = JobFactory(submitter=submitter)
-        self.create_representative_users(submitter)
+        self.submitter = self.user_factory.create(username="submitter")
+        self.instance_factory = JobFactory(submitter=self.submitter)
+        self.create_representative_users(self.submitter)
         self.instance = self.instance_factory.create()
 
     def test_retrieve(self):
@@ -46,7 +45,6 @@ class EventViewSetTestCase(BaseViewSetTestCase):
     _view = EventViewSet
 
     def setUp(self):
-        self.user_factory = UserFactory()
         submitter = self.user_factory.create(username="submitter")
         self.instance_factory = EventFactory(submitter=submitter)
         self.create_representative_users(submitter)
@@ -72,10 +70,9 @@ class JobPageRenderTestCase(TestCase):
     client_class = APIClient
 
     def setUp(self):
-        user_factory = UserFactory()
-        self.submitter = user_factory.create(username="submitter")
-        job_factory = JobFactory(submitter=self.submitter)
-        self.job = job_factory.create()
+        self.submitter, self.user_factory = create_test_user(username="submitter")
+        self.job_factory = JobFactory(submitter=self.submitter)
+        self.job = self.job_factory.create()
 
     def test_detail(self):
         response = self.client.get(
@@ -92,10 +89,9 @@ class EventPageRenderTestCase(TestCase):
     client_class = APIClient
 
     def setUp(self):
-        user_factory = UserFactory()
-        self.submitter = user_factory.create(username="submitter")
-        event_factory = EventFactory(submitter=self.submitter)
-        self.event = event_factory.create()
+        self.submitter, self.user_factory = create_test_user(username="submitter")
+        self.event_factory = EventFactory(submitter=self.submitter)
+        self.event = self.event_factory.create()
 
     def test_detail(self):
         response = self.client.get(
@@ -116,8 +112,7 @@ class ProfilePageRenderTestCase(TestCase):
     client_class = APIClient
 
     def setUp(self):
-        user_factory = UserFactory()
-        self.submitter = user_factory.create()
+        self.submitter, self.user_factory = create_test_user()
         self.profile = self.submitter.member_profile
         self.profile.personal_url = "https://geocities.com/{}".format(
             self.submitter.username
@@ -138,10 +133,9 @@ def test_list(self):
 class SpamDetectionTestCase(BaseViewSetTestCase):
 
     def setUp(self):
-        self.user_factory = UserFactory()
-        self.submitter = self.user_factory.create(username="submitter")
+        self.submitter, self.user_factory = create_test_user(username="submitter")
         self.moderator = self.user_factory.create(username="moderator")
-        self.moderator.groups.add(ComsesGroups.MODERATOR.get_group())
+        ComsesGroups.MODERATOR.add(self.moderator)
         self.superuser = self.user_factory.create(username="admin", is_superuser=True)
         self.client.login(
             username=self.submitter.username, password=self.user_factory.password

django/library/fs.py

@@ -525,10 +525,12 @@ def get_codemeta_json(self):
         return self.codemeta.to_json()
 
     def build_published_archive(self, force=False):
+        """
+        FIXME: some of this should be moved to an async processing task.
+        """
         self.create_or_update_codemeta(force=force)
         bag = self.get_or_create_sip_bag(self.bagit_info)
         self.validate_bagit(bag)
-        self.build_aip()
         self.build_archive(force=force)
 
     def build_review_archive(self):

django/library/models.py

@@ -989,7 +989,7 @@ def create_release(
         existing_draft = self.releases.filter(status=status).first()
         if existing_draft:
             logger.warning(
-                "Creating a new %s release when one already exists: %s",
+                "Attempting to create a new %s release when one already exists: %s",
                 status,
                 existing_draft.identifier,
             )
@@ -1686,17 +1686,20 @@ def add_contributor(self, contributor: Contributor, role=Role.AUTHOR, index=None
             return existing_release_contributor
 
     @transaction.atomic
-    def publish(self):
+    def publish(self, defer_fs=False):
         self.validate_publishable()
-        self._publish()
+        self._publish(defer_fs)
 
-    def _publish(self):
+    def _publish(self, defer_fs=False):
         if not self.live:
             now = timezone.now()
             self.first_published_at = now
             self.last_published_on = now
             self.status = self.Status.PUBLISHED
-            self.get_fs_api().build_published_archive(force=True)
+            if defer_fs:
+                logger.debug("FIXME: set up async publish archive task")
+            else:
+                self.get_fs_api().build_published_archive(force=True)
             self.save()
             codebase = self.codebase
             codebase.latest_version = self

django/library/tests/base.py

@@ -48,11 +48,13 @@ def data_for_create_request(self, **overrides):
         del serialized["id"]
         return serialized
 
-    def create_published_release(self, codebase=None, **overrides):
+    def create_published_release(self, codebase=None, **kwargs):
         if codebase is None:
-            codebase = self.create(**overrides)
+            codebase, created = self.get_or_create(
+                **kwargs, defaults=self.get_default_data()
+            )
         release = ReleaseSetup.setUpPublishableDraftRelease(codebase)
-        release.publish()
+        release.publish(defer_fs=True)
         return release
 
 
@@ -78,11 +80,7 @@ def create(self, **overrides) -> Contributor:
         contributor, created = Contributor.objects.get_or_create(
             user=user, defaults=default_data
         )
-        if created:
-            return contributor
-        else:
-            logger.warning("Contributor with user already exists: %s", user)
-            return contributor
+        return contributor
 
 
 class ReleaseContributorFactory:
@@ -165,8 +163,8 @@ def setUpPublishableDraftRelease(cls, codebase):
             status=CodebaseRelease.Status.DRAFT,
             initialize=True,
         )
-        draft_release.license = License.objects.create(name="MIT")
-        draft_release.os = "Linux"
+        draft_release.license, created = License.objects.get_or_create(name="MIT")
+        draft_release.os = "Any"
         draft_release.programming_languages.add("Python")
         contributor_factory = ContributorFactory(user=draft_release.submitter)
         release_contributor_factory = ReleaseContributorFactory(draft_release)