refactor: [ACI-956] little code cleanup (#163)

* refactor: [ACI-956] little code cleanup

- remove unused imports, variables
- add missing docstrings
- add & fix tests

* fix: [ACI-963] remove migration conflict

---------

Co-authored-by: Andrii <[email protected]>

credentials/apps/badges/admin.py

@@ -33,6 +33,10 @@
 
 
 class BadgeRequirementInline(admin.TabularInline):
+    """
+    Badge template requirement inline setup.
+    """
+
     model = BadgeRequirement
     show_change_link = True
     extra = 0
@@ -60,12 +64,19 @@ def rules(self, obj):
 
 
 class BadgePenaltyInline(admin.TabularInline):
+    """
+    Badge template penalty inline setup.
+    """
+
     model = BadgePenalty
     show_change_link = True
     extra = 0
     form = BadgePenaltyForm
 
     def formfield_for_manytomany(self, db_field, request, **kwargs):
+        """
+        Filter requirements by parent badge template.
+        """
         if db_field.name == "requirements":
             template_id = request.resolver_match.kwargs.get("object_id")
             if template_id:
@@ -74,6 +85,10 @@ def formfield_for_manytomany(self, db_field, request, **kwargs):
 
 
 class FulfillmentInline(admin.TabularInline):
+    """
+    Badge template fulfillment inline setup.
+    """
+
     model = Fulfillment
     extra = 0
     readonly_fields = [
@@ -82,6 +97,10 @@ class FulfillmentInline(admin.TabularInline):
 
 
 class DataRuleInline(admin.TabularInline):
+    """
+    Data rule inline setup.
+    """
+
     model = DataRule
     extra = 0
     form = DataRuleForm

credentials/apps/badges/admin_forms.py

@@ -65,11 +65,17 @@ def _ensure_organization_exists(self, api_client):
 
 
 class BadgePenaltyForm(forms.ModelForm):
+    """
+    Form for BadgePenalty model.
+    """
     class Meta:
         model = BadgePenalty
         fields = "__all__"
 
     def clean(self):
+        """
+        Ensure that all penalties belong to the same template.
+        """
         cleaned_data = super().clean()
         requirements = cleaned_data.get("requirements")
 
@@ -81,20 +87,33 @@ def clean(self):
 
 
 class DataRuleFormSet(forms.BaseInlineFormSet):
+    """
+    Formset for DataRule model.
+    """
     def get_form_kwargs(self, index):
+        """
+        Pass parent instance to the form.
+        """
+
         kwargs = super().get_form_kwargs(index)
         kwargs["parent_instance"] = self.instance
         return kwargs
 
 
 class DataRuleForm(forms.ModelForm):
+    """
+    Form for DataRule model.
+    """
     class Meta:
         model = DataRule
         fields = "__all__"
 
     data_path = forms.ChoiceField()
 
     def __init__(self, *args, parent_instance=None, **kwargs):
+        """
+        Load data paths based on the parent instance event type.
+        """
         self.parent_instance = parent_instance
         super().__init__(*args, **kwargs)
 

credentials/apps/badges/models.py

@@ -219,6 +219,10 @@ def reset(self, username: str):
         return bool(deleted)
 
     def is_fulfilled(self, username: str) -> bool:
+        """
+        Checks if the requirement is fulfilled for the user.
+        """
+
         return self.fulfillments.filter(progress__username=username, progress__template=self.template).exists()
 
     def apply_rules(self, data: dict) -> bool:
@@ -291,6 +295,7 @@ def apply(self, data: dict) -> bool:
         and `self.value` is "30", then calling `apply({"user": {"age": 30}})` will return True
         because the age matches the specified value.
         """
+
         comparison_func = getattr(operator, self.operator, None)
         if comparison_func:
             data_value = str(keypath(data, self.data_path))
@@ -357,9 +362,14 @@ def apply_rules(self, data: dict) -> bool:
         """
         Evaluates payload rules.
         """
+
         return all(rule.apply(data) for rule in self.rules.all())
 
     def reset_requirements(self, username: str):
+        """
+        Resets all related requirements for the user.
+        """
+
         for requirement in self.requirements.all():
             requirement.reset(username)
 
@@ -419,23 +429,20 @@ def for_user(cls, *, username, template_id):
         """
         Service shortcut.
         """
+
         progress, __ = cls.objects.get_or_create(username=username, template_id=template_id)
         return progress
 
     @property
     def ratio(self) -> float:
         """
         Calculates badge template progress ratio.
-
-        FIXME: simplify
         """
 
         requirements = BadgeRequirement.objects.filter(template=self.template)
-
         group_ids = requirements.filter(group__isnull=False).values_list("group", flat=True).distinct()
 
         requirements_count = requirements.filter(group__isnull=True).count() + group_ids.count()
-
         fulfilled_requirements_count = Fulfillment.objects.filter(
             progress=self,
             requirement__template=self.template,
@@ -444,15 +451,24 @@ def ratio(self) -> float:
 
         for group_id in group_ids:
             group_requirements = requirements.filter(group=group_id)
-            group_fulfillment_count = Fulfillment.objects.filter(requirement__in=group_requirements).count()
-            fulfilled_requirements_count += 1 if group_fulfillment_count > 0 else 0
+            group_fulfilled_requirements_count = Fulfillment.objects.filter(
+                progress=self,
+                requirement__in=group_requirements,
+            ).count()
+
+            if group_fulfilled_requirements_count > 0:
+                fulfilled_requirements_count += 1
 
-        if fulfilled_requirements_count == 0:
+        if fulfilled_requirements_count == 0 or requirements_count == 0:
             return 0.00
         return round(fulfilled_requirements_count / requirements_count, 2)
 
     @property
     def completed(self):
+        """
+        Checks if the badge template is completed.
+        """
+
         return self.ratio == 1.00
 
     def validate(self):
@@ -556,4 +572,5 @@ def propagated(self):
         """
         Checks if this user credential already has issued (external) Credly badge.
         """
+
         return self.external_uuid and (self.state in self.ISSUING_STATES)

credentials/apps/badges/tests/test_admin_forms.py

@@ -0,0 +1,70 @@
+import uuid
+
+from django import forms
+from django.contrib.sites.models import Site
+from django.test import TestCase
+from django.utils.translation import gettext as _
+
+from credentials.apps.badges.admin_forms import BadgePenaltyForm
+from credentials.apps.badges.models import BadgeRequirement, BadgeTemplate
+
+
+COURSE_PASSING_EVENT = "org.openedx.learning.course.passing.status.updated.v1"
+
+
+class BadgePenaltyFormTestCase(TestCase):
+    def setUp(self):
+        self.site = Site.objects.create(domain="test_domain", name="test_name")
+        self.badge_template1 = BadgeTemplate.objects.create(
+            uuid=uuid.uuid4(), name="test_template", state="draft", site=self.site
+        )
+        self.badge_template2 = BadgeTemplate.objects.create(
+            uuid=uuid.uuid4(), name="test_template", state="draft", site=self.site
+        )
+        self.requirement1 = BadgeRequirement.objects.create(
+            template=self.badge_template1,
+            event_type=COURSE_PASSING_EVENT,
+            description="Test course passing award description 1",
+        )
+        self.requirement2 = BadgeRequirement.objects.create(
+            template=self.badge_template2,
+            event_type=COURSE_PASSING_EVENT,
+            description="Test course passing award description 2",
+        )
+        self.requirement3 = BadgeRequirement.objects.create(
+            template=self.badge_template2,
+            event_type=COURSE_PASSING_EVENT,
+            description="Test course passing award description 3",
+        )
+
+    def test_clean_requirements_same_template(self):
+        form = BadgePenaltyForm()
+        form.cleaned_data = {
+            "template": self.badge_template2,
+            "requirements": [
+                self.requirement2,
+                self.requirement3,
+            ],
+        }
+        self.assertEqual(form.clean(), {
+            "template": self.badge_template2,
+            "requirements": [
+                self.requirement2,
+                self.requirement3,
+            ],
+        })
+
+    def test_clean_requirements_different_template(self):
+        form = BadgePenaltyForm()
+        form.cleaned_data = {
+            "template": self.badge_template1,
+            "requirements": [
+                self.requirement2,
+                self.requirement1,
+            ],
+        }
+
+        with self.assertRaises(forms.ValidationError) as cm:
+            form.clean()
+
+        self.assertEqual(str(cm.exception), "['All requirements must belong to the same template.']")