Merge branch 'backend/refactor/long-method' into 'master'

Reduce complexity and simplify long methods

See merge request !37

backend/secfit/users/views.py

@@ -106,28 +106,20 @@ class OfferList(
     def get_queryset(self):
         result = Offer.objects.none()
 
-        if self.request.user:
-            qs = Offer.objects.filter(
-                Q(owner=self.request.user) | Q(recipient=self.request.user)
-            ).distinct()
-            qp = self.request.query_params
-            u = self.request.user
-
-            s = qp.get("status", None)
-            if s is not None and self.request is not None:
-                qs = qs.filter(status=s)
-                if qp.get("status", None) is None:
-                    qs = Offer.objects.filter(Q(owner=u)).distinct()
-
-            c = qp.get("category", None)
-            if c is not None and qp is not None:
-                if c == "sent":
-                    qs = qs.filter(owner=u)
-                elif c == "received":
-                    qs = qs.filter(recipient=u)
-            return qs
-        else:
-            return result
+        if user := self.request.user:
+            result = Offer.objects.filter(Q(owner=user) | Q(recipient=user)).distinct()
+            params = self.request.query_params
+
+            if status := params.get("status"):
+                result = result.filter(status=status)
+
+            if category := params.get("category"):
+                if category == "sent":
+                    result = result.filter(owner=user)
+                elif category == "received":
+                    result = result.filter(recipient=user)
+
+        return result
 
 
 class OfferDetail(

backend/secfit/workouts/serializers.py

@@ -129,60 +129,74 @@ class WorkoutSerializer(serializers.HyperlinkedModelSerializer):
         Returns:
             Workout: Updated Workout instance
         """
-        exercise_instances_data = validated_data.pop("exercise_instances")
-        exercise_instances = instance.exercise_instances
+        if data := validated_data.pop("exercise_instances"):
+            self.update_workout_exercises(instance, data)
 
-        instance.name = validated_data.get("name", instance.name)
-        instance.notes = validated_data.get("notes", instance.notes)
-        instance.visibility = validated_data.get("visibility", instance.visibility)
-        instance.date = validated_data.get("date", instance.date)
-        instance.save()
+        for attr in ["name", "notes", "visibility", "date"]:
+            if attr in validated_data:
+                setattr(instance, attr, validated_data[attr])
 
-        for exercise_instance, exercise_instance_data in zip(
-            exercise_instances.all(), exercise_instances_data
-        ):
-            exercise_instance.exercise = exercise_instance_data.get(
-                "exercise", exercise_instance.exercise
-            )
-            exercise_instance.number = exercise_instance_data.get(
-                "number", exercise_instance.number
-            )
-            exercise_instance.sets = exercise_instance_data.get(
-                "sets", exercise_instance.sets
-            )
-            exercise_instance.save()
+        if data := validated_data.pop("files"):
+            self.update_workout_files(instance, data)
 
-        if len(exercise_instances_data) > len(exercise_instances.all()):
-            for i in range(len(exercise_instances.all()), len(exercise_instances_data)):
-                exercise_instance_data = exercise_instances_data[i]
-                ExerciseInstance.objects.create(
-                    workout=instance, **exercise_instance_data
-                )
+        instance.save()
+        return instance
 
-        elif len(exercise_instances_data) < len(exercise_instances.all()):
-            for i in range(len(exercise_instances_data), len(exercise_instances.all())):
-                exercise_instances.all()[i].delete()
+    def update_workout_exercises(self, workout, data):
+        """Updates the exercise instances associated with a workout.
 
-        if "files" in validated_data:
-            files_data = validated_data.pop("files")
-            files = instance.files
+        Args:
+            workout (Workout): Current workout object.
+            data (list): A list of validated exercise instance data.
+        """
+        exercises = workout.exercise_instances.all()
 
-            for file, file_data in zip(files.all(), files_data):
-                file.file = file_data.get("file", file.file)
+        for exercise, exercise_data in zip(exercises, data):
+            for attr in ["exercise", "number", "sets"]:
+                if attr in exercise_data:
+                    setattr(exercise, attr, exercise_data[attr])
+            exercise.save()
 
-            if len(files_data) > len(files.all()):
-                for i in range(len(files.all()), len(files_data)):
-                    WorkoutFile.objects.create(
-                        workout=instance,
-                        owner=instance.owner,
-                        file=files_data[i].get("file"),
-                    )
+        num_data = len(data)
+        num_exercises = len(exercises)
+
+        if num_data > num_exercises:
+            for i in range(num_exercises, num_data):
+                ExerciseInstance.objects.create(
+                    workout=workout,
+                    **data[i],
+                )
+        elif num_data < num_exercises:
+            for i in range(num_data, num_exercises):
+                exercises[i].delete()
 
-            elif len(files_data) < len(files.all()):
-                for i in range(len(files_data), len(files.all())):
-                    files.all()[i].delete()
+    def update_workout_files(self, workout, data):
+        """Updates the files associated with a workout.
 
-        return instance
+        Args:
+            workout (Workout): Current workout object.
+            data (list): A list of validated file data.
+        """
+        files = workout.files.all()
+
+        for file_obj, file_data in zip(files, data):
+            if file := file_data.get("file"):
+                file_obj.file = file
+            file_obj.save()
+
+        num_data = len(data)
+        num_files = len(files)
+
+        if num_data > num_files:
+            for i in range(num_files, num_data):
+                WorkoutFile.objects.create(
+                    workout=workout,
+                    owner=workout.owner,
+                    **data[i],
+                )
+        elif num_data < num_files:
+            for i in range(num_data, num_files):
+                files[i].delete()
 
     def get_owner_username(self, obj):
         """Returns the owning user's username.

backend/secfit/workouts/tests/data/exercises.json

+[
+    {
+        "model": "workouts.exercise",
+        "pk": 1,
+        "fields": {
+            "name": "Foo",
+            "description": "Do a foo forever.",
+            "unit": "reps"
+        }
+    },
+    {
+        "model": "workouts.exercise",
+        "pk": 2,
+        "fields": {
+            "name": "Bar",
+            "description": "Try to lift the bar as high as possible.",
+            "unit": "reps"
+        }
+    },
+    {
+        "model": "workouts.exercise",
+        "pk": 3,
+        "fields": {
+            "name": "Baz",
+            "description": "Nobody quite knows what this is all about.",
+            "unit": "reps"
+        }
+    }
+]

backend/secfit/workouts/tests/data/test-file.txt

+Content

backend/secfit/workouts/tests/data/users.json

+[
+    {
+        "model": "users.user",
+        "pk": 1,
+        "fields": {
+            "username": "arya",
+            "email": "arya@stark.io",
+            "password": "pbkdf2_sha256$216000$glNd7zmKsCPk$Et1Wg9R1857TSb3eiaV6p4OZKi46AEwupcix2pAIS+I=",
+            "coach": 1
+        }
+    }
+]

backend/secfit/workouts/tests/data/workouts.json

+[
+    {
+        "model": "workouts.workout",
+        "pk": 1,
+        "fields": {
+            "name": "Test workout",
+            "date": "2021-04-15T18:49:43.00000Z",
+            "notes": "Notes for Test workout",
+            "visibility": "PU",
+            "owner": 1
+        }
+    },
+    {
+        "model": "workouts.exerciseinstance",
+        "pk": 1,
+        "fields": {
+            "workout": 1,
+            "exercise": 1,
+            "sets": 3,
+            "number": 8
+        }
+    },
+    {
+        "model": "workouts.exerciseinstance",
+        "pk": 2,
+        "fields": {
+            "workout": 1,
+            "exercise": 2,
+            "sets": 3,
+            "number": 8
+        }
+    }
+]

backend/secfit/workouts/tests/test_views.py

+import json
+import pathlib
+import unittest
+from dataclasses import dataclass
+from datetime import datetime
+
+from django.contrib.auth import get_user_model
+from django.core.files import File
+from django.core.files.uploadedfile import SimpleUploadedFile
+from django.test import TestCase
+from rest_framework import status
+from rest_framework.reverse import reverse
+from rest_framework.test import APIClient
+from rest_framework.test import APIRequestFactory
+from rest_framework.test import APITestCase
+
+from workouts.models import Workout
+from workouts.serializers import WorkoutSerializer
+
+here = pathlib.Path(__file__).parent.resolve()
+data_dir = here / "data"
+
+
+@dataclass
+class LoginData(dict):
+    username: str
+    password: str
+
+
+test_user = LoginData("arya", "dOnOtCaLlMeMiLaDy")
+
+
+def user_factory(user: LoginData):
+    return get_user_model().objects.get(username=user.username)
+
+
+def exercise_instance_factory(pk, sets=3, number=8):
+    return dict(
+        exercise=reverse("exercise-detail", kwargs=dict(pk=pk)),
+        sets=sets,
+        number=number,
+    )
+
+
+def workout_data_factory(name, *exercise_pks):
+    return dict(
+        name=name,
+        date=datetime.now().isoformat(),
+        notes=f"Notes for {name}",
+        visibility=Workout.PUBLIC,
+        exercise_instances=[exercise_instance_factory(pk) for pk in exercise_pks],
+    )
+
+
+def create_payload(data):
+    """Returns a proper workout payload for a request."""
+    data["exercise_instances"] = json.dumps(data["exercise_instances"])
+    return data
+
+
+class WorkoutListTest(TestCase):
+    """Test case for the WorkoutList view class."""
+
+    fixtures = [
+        data_dir / "users.json",
+        data_dir / "exercises.json",
+    ]
+
+    def setUp(self):
+        self.user = user_factory(test_user)
+        self.client = APIClient()
+
+    @property
+    def url(self):
+        return reverse("workout-list")
+
+    def test_get(self):
+        """Test GET request on the WorkoutList view."""
+        self.client.force_authenticate(self.user)
+        response = self.client.get(self.url)
+        data = response.json()
+        self.assertTrue(status.is_success(response.status_code))
+        self.assertEqual(data["count"], 0)
+        self.assertEqual(data["results"], [])
+
+    def test_post(self):
+        """Test that a new workout is added with a POST request."""
+        self.client.force_authenticate(self.user)
+        workout_name = "Test workout"
+        workout_data = workout_data_factory(workout_name, 1)
+        response = self.client.post(
+            self.url,
+            create_payload(workout_data),
+            format="multipart",
+        )
+        self.assertTrue(status.is_success(response.status_code))
+        response_data = response.json()
+        self.assertEqual(response_data["id"], 1)
+        self.assertEqual(response_data["name"], workout_name)
+        self.assertEqual(len(response_data["exercise_instances"]), 1)
+
+
+class WorkoutDetailTest(APITestCase):
+    """Test case for the workout-detail view."""
+
+    fixtures = [
+        data_dir / "users.json",
+        data_dir / "exercises.json",
+        data_dir / "workouts.json",
+    ]
+
+    def setUp(self):
+        self.user = user_factory(test_user)
+        self.client = APIClient()
+        self.client.force_authenticate(self.user)
+        self.factory = APIRequestFactory()
+        self.context = dict(request=self.factory.get("/"))
+
+    def url(self, pk):
+        return reverse("workout-detail", kwargs=dict(pk=pk))
+
+    def put(self, workout_id, data):
+        """Returns the response from a PUT request on a workout resource."""
+        return self.client.put(
+            self.url(workout_id),
+            create_payload(data),
+            format="multipart",
+        )
+
+    def serialize(self, workout):
+        """Returns serialized data for a workout."""
+        serializer = WorkoutSerializer(instance=workout, context=self.context)
+        return serializer.data
+
+    def test_get_workout(self):
+        """Tests that a workout is retrived."""
+        self.client.force_authenticate(self.user)
+        response = self.client.get(self.url(1))
+        self.assertTrue(status.is_success(response.status_code))
+        response_data = response.json()
+        self.assertEqual(response_data["id"], 1)
+        self.assertEqual(response_data["name"], "Test workout")
+        self.assertEqual(len(response_data["exercise_instances"]), 2)
+
+    def test_update_workout(self):
+        """Tests that a workout is updated."""
+        self.client.force_authenticate(self.user)
+        workout = Workout.objects.get(pk=1)
+        data = self.serialize(workout)
+        data["name"] = "New workout name"
+
+        response = self.put(1, data)
+        response_data = response.json()
+
+        self.assertTrue(status.is_success(response.status_code))
+        self.assertNotEqual(response_data["name"], "Test workout")
+        self.assertEqual(response_data["name"], "New workout name")
+
+    def test_update_workout_add_exercise(self):
+        """Tests that an exercise instance is added to a workout."""
+        self.client.force_authenticate(self.user)
+        workout = Workout.objects.get(pk=1)
+        data = self.serialize(workout)
+        self.assertEqual(len(workout.exercise_instances.all()), 2)
+
+        exercise = exercise_instance_factory(3)
+        data["exercise_instances"].append(exercise)
+
+        response = self.put(1, data)
+        response_data = response.json()
+        self.assertTrue(status.is_success(response.status_code))
+        self.assertEqual(len(response_data["exercise_instances"]), 3)
+
+    def test_update_workout_remove_exercise(self):
+        """Test that an exercise instance is removed from a workout."""
+        self.client.force_authenticate(self.user)
+        workout = Workout.objects.get(pk=1)
+        data = self.serialize(workout)
+        self.assertEqual(len(workout.exercise_instances.all()), 2)
+
+        exercise_instance = data["exercise_instances"].pop()
+        self.assertEqual(exercise_instance.get("id"), 2)
+
+        response = self.put(1, data)
+        response_data = response.json()
+        self.assertTrue(status.is_success(response.status_code))
+        self.assertEqual(len(response_data["exercise_instances"]), 1)
+
+    @unittest.skip("Need to figure out how to include files in the payload.")
+    def test_update_workout_add_files(self):
+        """Tests that a file is added to a workout."""
+        self.client.force_authenticate(self.user)
+        workout = Workout.objects.get(pk=1)
+        data = self.serialize(workout)
+        self.assertEqual(len(data["files"]), 0)
+
+        # FIXME Properly include a file object in the payload
+        # As of now the file object is added to the payload, but is not present
+        # in the returned API response.
+        file_path = data_dir / "test-file.txt"
+        file_obj = SimpleUploadedFile(
+            file_path.name,
+            File(open(file_path, "rb")).read(),
+            content_type="multipart/form-data",
+        )
+        data["files"].append(dict(file=file_obj))
+        self.assertEqual(len(data["files"]), 1)
+
+        response = self.put(1, data)
+        self.assertTrue(status.is_success(response.status_code))
+        self.assertEqual(len(response.data["files"]), len(data["files"]))
+
+    def test_delete_workout(self):
+        """Tests that a workout is deleted."""
+        self.client.force_authenticate(self.user)
+        response = self.client.delete(self.url(1))
+        self.assertTrue(status.is_success(response.status_code))
+        workouts = Workout.objects.all()
+        self.assertListEqual(list(workouts), list())