From 5e0ae1557786454b089e63761d75577a4066569d Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 31 May 2024 13:00:29 +0300 Subject: [PATCH] refactor: [ACI-997] refactor for quality checks pass --- credentials/apps/badges/admin.py | 27 +++---- credentials/apps/badges/admin_forms.py | 21 ++--- credentials/apps/badges/apps.py | 12 ++- credentials/apps/badges/credly/api_client.py | 2 +- credentials/apps/badges/credly/exceptions.py | 4 - credentials/apps/badges/exceptions.py | 6 -- credentials/apps/badges/issuers.py | 2 +- .../sync_organization_badge_templates.py | 10 ++- ...rename_group_fulfillment_blend_and_more.py | 64 +++++++++++++++ credentials/apps/badges/models.py | 26 +++++-- credentials/apps/badges/processing/generic.py | 5 +- credentials/apps/badges/signals/handlers.py | 6 +- credentials/apps/badges/signals/signals.py | 4 +- .../apps/badges/tests/test_admin_forms.py | 27 ++----- .../apps/badges/tests/test_api_client.py | 77 +++++-------------- credentials/apps/badges/tests/test_issuers.py | 10 +-- credentials/apps/badges/tests/test_models.py | 61 ++++++--------- .../apps/badges/tests/test_services.py | 49 ++++++------ credentials/apps/badges/tests/test_signals.py | 4 +- credentials/apps/badges/tests/test_utils.py | 14 ++-- .../apps/badges/tests/test_webhooks.py | 10 +-- credentials/apps/badges/toggles.py | 1 + credentials/apps/badges/utils.py | 16 ++-- 23 files changed, 232 insertions(+), 226 deletions(-) create mode 100644 credentials/apps/badges/migrations/0022_rename_group_fulfillment_blend_and_more.py diff --git a/credentials/apps/badges/admin.py b/credentials/apps/badges/admin.py index fee35b5141..bdf43756b5 100644 --- a/credentials/apps/badges/admin.py +++ b/credentials/apps/badges/admin.py @@ -47,10 +47,10 @@ class BadgeRequirementInline(admin.TabularInline): "event_type", "rules", "description", - "group", + "blend", ) readonly_fields = ("rules",) - ordering = ("group",) + ordering = ("blend",) form = BadgeRequirementForm formset = BadgeRequirementFormSet @@ -194,7 +194,7 @@ def get_fields(self, request, obj=None): return fields def get_readonly_fields(self, request, obj=None): - readonly_fields = super().get_readonly_fields(request, obj) + readonly_fields = list(super().get_readonly_fields(request, obj)) if not obj: return readonly_fields @@ -247,7 +247,7 @@ class CredlyBadgeTemplateAdmin(admin.ModelAdmin): "description": _( """ WARNING: avoid configuration updates on activated badges. - Active badge templates are continuously processed and learners may already have partial progress on them. + Active badge templates are continuously processed and learners may already have progress on them. Any changes in badge template requirements (including data rules) will affect learners' experience! """ ), @@ -318,10 +318,10 @@ def image(self, obj): image.short_description = _("icon") - def save_model(self, request, obj, form, change): # pylint: disable=unused-argument + def save_model(self, request, obj, form, change): pass - def save_formset(self, request, form, formset, change): # pylint: disable=unused-argument + def save_formset(self, request, form, formset, change): """ Check if template is active and has requirements. """ @@ -331,7 +331,7 @@ def save_formset(self, request, form, formset, change): # pylint: disable=unuse messages.set_level(request, messages.ERROR) messages.error(request, _("Active badge template must have at least one requirement.")) return HttpResponseRedirect(request.path) - form.instance.save() + return form.instance.save() class DataRulePenaltyInline(admin.TabularInline): @@ -368,14 +368,14 @@ class BadgeRequirementAdmin(admin.ModelAdmin): "template", "event_type", "template_link", - "group", + "blend", ] fields = [ "template_link", "event_type", "description", - "group", + "blend", ] def has_add_permission(self, request): @@ -455,15 +455,6 @@ def formfield_for_manytomany(self, db_field, request, **kwargs): kwargs["queryset"] = BadgeRequirement.objects.filter(template_id=template_id) return super().formfield_for_manytomany(db_field, request, **kwargs) - def template_link(self, instance): - """ - Interactive link to parent (badge template). - """ - url = reverse("admin:badges_credlybadgetemplate_change", args=[instance.template.pk]) - return format_html('{}', url, instance.template) - - template_link.short_description = _("badge template") - def response_change(self, request, obj): if "_save" in request.POST: return HttpResponseRedirect(reverse("admin:badges_credlybadgetemplate_change", args=[obj.template.pk])) diff --git a/credentials/apps/badges/admin_forms.py b/credentials/apps/badges/admin_forms.py index a27d1b891f..89b70be1d0 100644 --- a/credentials/apps/badges/admin_forms.py +++ b/credentials/apps/badges/admin_forms.py @@ -50,7 +50,7 @@ def clean(self): api_key = settings.BADGES_CONFIG["credly"]["ORGANIZATIONS"][str(uuid)] credly_api_client = CredlyAPIClient(uuid, api_key) - self._ensure_organization_exists(credly_api_client) + self.ensure_organization_exists(credly_api_client) return cleaned_data @@ -64,7 +64,7 @@ def save(self, commit=True): return instance - def _ensure_organization_exists(self, api_client): + def ensure_organization_exists(self, api_client): """ Try to fetch organization data by the configured Credly Organization ID. """ @@ -93,7 +93,7 @@ def clean(self): requirements = cleaned_data.get("requirements") if requirements and not all( - [requirement.template.id == cleaned_data.get("template").id for requirement in requirements] + requirement.template.id == cleaned_data.get("template").id for requirement in requirements ): raise forms.ValidationError(_("All requirements must belong to the same template.")) return cleaned_data @@ -143,7 +143,8 @@ def clean(self): return cleaned_data -class DataRuleFormSet(ParentMixin, forms.BaseInlineFormSet): ... +class DataRuleFormSet(ParentMixin, forms.BaseInlineFormSet): + pass class DataRuleForm(DataRuleExtensionsMixin, forms.ModelForm): @@ -158,7 +159,8 @@ class Meta: data_path = forms.ChoiceField() -class BadgeRequirementFormSet(ParentMixin, forms.BaseInlineFormSet): ... +class BadgeRequirementFormSet(ParentMixin, forms.BaseInlineFormSet): + pass class BadgeRequirementForm(forms.ModelForm): @@ -166,17 +168,18 @@ class Meta: model = BadgeRequirement fields = "__all__" - group = forms.ChoiceField() + blend = forms.ChoiceField() def __init__(self, *args, parent_instance=None, **kwargs): self.template = parent_instance super().__init__(*args, **kwargs) - self.fields["group"].choices = Choices(*[(chr(i), chr(i)) for i in range(65, 91)]) - self.fields["group"].initial = chr(65 + self.template.requirements.count()) + self.fields["blend"].choices = Choices(*[(chr(i), chr(i)) for i in range(65, 91)]) + self.fields["blend"].initial = chr(65 + self.template.requirements.count()) -class PenaltyDataRuleFormSet(ParentMixin, forms.BaseInlineFormSet): ... +class PenaltyDataRuleFormSet(ParentMixin, forms.BaseInlineFormSet): + pass class PenaltyDataRuleForm(DataRuleExtensionsMixin, forms.ModelForm): diff --git a/credentials/apps/badges/apps.py b/credentials/apps/badges/apps.py index 18c9da60eb..1426149e9f 100644 --- a/credentials/apps/badges/apps.py +++ b/credentials/apps/badges/apps.py @@ -1,6 +1,6 @@ from django.apps import AppConfig -from .toggles import check_badges_enabled +from credentials.apps.badges.toggles import check_badges_enabled class BadgesAppConfig(AppConfig): @@ -8,6 +8,8 @@ class BadgesAppConfig(AppConfig): Extended application config with additional Badges-specific logic. """ + plugin_label = "badges" + @property def verbose_name(self): return f"Badges: {self.plugin_label}" @@ -29,9 +31,11 @@ def ready(self): Performs initial registrations for checks, signals, etc. """ - from . import signals # pylint: disable=unused-import,import-outside-toplevel - from .checks import badges_checks # pylint: disable=unused-import,import-outside-toplevel - from .signals.handlers import listen_to_badging_events + from credentials.apps.badges import signals # pylint: disable=unused-import,import-outside-toplevel + from credentials.apps.badges.checks import ( # pylint: disable=unused-import,import-outside-toplevel + badges_checks, + ) + from credentials.apps.badges.signals.handlers import listen_to_badging_events # pylint: import-outside-toplevel listen_to_badging_events() diff --git a/credentials/apps/badges/credly/api_client.py b/credentials/apps/badges/credly/api_client.py index 23f805de01..793f4c85ea 100644 --- a/credentials/apps/badges/credly/api_client.py +++ b/credentials/apps/badges/credly/api_client.py @@ -70,7 +70,7 @@ def perform_request(self, method, url_suffix, data=None): """ url = urljoin(self.base_api_url, url_suffix) logger.debug(f"Credly API: {method.upper()} {url}") - response = requests.request(method.upper(), url, headers=self._get_headers(), json=data) + response = requests.request(method.upper(), url, headers=self._get_headers(), json=data, timeout=10) self._raise_for_error(response) return response.json() diff --git a/credentials/apps/badges/credly/exceptions.py b/credentials/apps/badges/credly/exceptions.py index 6752803f69..7fbb2a4374 100644 --- a/credentials/apps/badges/credly/exceptions.py +++ b/credentials/apps/badges/credly/exceptions.py @@ -10,12 +10,8 @@ class CredlyError(BadgesError): Credly backend generic error. """ - pass - class CredlyAPIError(CredlyError): """ Credly API errors. """ - - pass diff --git a/credentials/apps/badges/exceptions.py b/credentials/apps/badges/exceptions.py index c5950e24f7..fea8e2632b 100644 --- a/credentials/apps/badges/exceptions.py +++ b/credentials/apps/badges/exceptions.py @@ -8,20 +8,14 @@ class BadgesError(Exception): Badges generic exception. """ - pass - class BadgesProcessingError(BadgesError): """ Exception raised for errors that occur during badge processing. """ - pass - class StopEventProcessing(BadgesProcessingError): """ Exception raised to stop processing an event due to a specific condition. """ - - pass diff --git a/credentials/apps/badges/issuers.py b/credentials/apps/badges/issuers.py index d11f74ed0d..fe46de619c 100644 --- a/credentials/apps/badges/issuers.py +++ b/credentials/apps/badges/issuers.py @@ -36,7 +36,7 @@ def issue_credential( attributes=None, date_override=None, request=None, - lms_user_id=None, # pylint: disable=unused-argument + lms_user_id=None, ): """ Issue a credential to the user. diff --git a/credentials/apps/badges/management/commands/sync_organization_badge_templates.py b/credentials/apps/badges/management/commands/sync_organization_badge_templates.py index 29da77e488..b55e968abb 100644 --- a/credentials/apps/badges/management/commands/sync_organization_badge_templates.py +++ b/credentials/apps/badges/management/commands/sync_organization_badge_templates.py @@ -21,8 +21,11 @@ def handle(self, *args, **options): Sync badge templates for a specific organization or all organizations. Usage: - ./manage.py sync_organization_badge_templates --site_id 1 - ./manage.py sync_organization_badge_templates --site_id 1 --organization_id c117c179-81b1-4f7e-a3a1-e6ae30568c13 + site_id=1 + org_id=c117c179-81b1-4f7e-a3a1-e6ae30568c13 + + ./manage.py sync_organization_badge_templates --site_id $site_id + ./manage.py sync_organization_badge_templates --site_id $site_id --organization_id $org_id """ DEFAULT_SITE_ID = 1 organizations_to_sync = [] @@ -40,7 +43,8 @@ def handle(self, *args, **options): else: organizations_to_sync = CredlyOrganization.get_all_organization_ids() logger.info( - f"Organization ID wasn't provided: syncing badge templates for all organizations - {organizations_to_sync}" + "Organization ID wasn't provided: syncing badge templates for all organizations - " + f"{organizations_to_sync}", ) for organization_id in organizations_to_sync: diff --git a/credentials/apps/badges/migrations/0022_rename_group_fulfillment_blend_and_more.py b/credentials/apps/badges/migrations/0022_rename_group_fulfillment_blend_and_more.py new file mode 100644 index 0000000000..fc193b3192 --- /dev/null +++ b/credentials/apps/badges/migrations/0022_rename_group_fulfillment_blend_and_more.py @@ -0,0 +1,64 @@ +# Generated by Django 4.1 on 2024-05-31 13:46 + +from django.db import migrations, models +import model_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ("badges", "0021_alter_credlyorganization_api_key"), + ] + + operations = [ + migrations.RenameField( + model_name="fulfillment", + old_name="group", + new_name="blend", + ), + migrations.RemoveField( + model_name="badgerequirement", + name="group", + ), + migrations.AddField( + model_name="badgerequirement", + name="blend", + field=models.CharField( + blank=True, + help_text="Optional. Group requirements together using the same Group ID for interchangeable (OR processing logic).", + max_length=255, + null=True, + ), + ), + migrations.AlterField( + model_name="badgetemplate", + name="state", + field=model_utils.fields.StatusField( + choices=[("draft", "draft"), ("active", "active"), ("archived", "archived")], + default="draft", + help_text="Credly badge template state (auto-managed).", + max_length=100, + no_check_for_status=True, + ), + ), + migrations.AlterField( + model_name="credlybadge", + name="state", + field=model_utils.fields.StatusField( + choices=[ + ("created", "created"), + ("no_response", "no_response"), + ("error", "error"), + ("pending", "pending"), + ("accepted", "accepted"), + ("rejected", "rejected"), + ("revoked", "revoked"), + ("expired", "expired"), + ], + default="created", + help_text="Credly badge issuing state", + max_length=100, + no_check_for_status=True, + ), + ), + ] diff --git a/credentials/apps/badges/models.py b/credentials/apps/badges/models.py index 0f8b21aeab..b2f028de64 100644 --- a/credentials/apps/badges/models.py +++ b/credentials/apps/badges/models.py @@ -36,7 +36,9 @@ class CredlyOrganization(TimeStampedModel): uuid = models.UUIDField(unique=True, help_text=_("Put your Credly Organization ID here.")) api_key = models.CharField( - max_length=255, help_text=_("Credly API shared secret for Credly Organization."), blank=True + max_length=255, + help_text=_("Credly API shared secret for Credly Organization."), + blank=True, ) name = models.CharField( max_length=255, @@ -104,7 +106,7 @@ def save(self, *args, **kwargs): @property def groups(self): - return self.requirements.values_list("group", flat=True).distinct() + return self.requirements.values_list("blend", flat=True).distinct() @classmethod def by_uuid(cls, template_uuid): @@ -173,17 +175,19 @@ class BadgeRequirement(models.Model): max_length=255, choices=EVENT_TYPES, help_text=_( - 'Public signal type. Available events are configured in "BADGES_CONFIG" setting. The crucial aspect for event to carry UserData in its payload.' + 'Public signal type. Available events are configured in "BADGES_CONFIG" setting. ' + "The crucial aspect for event to carry UserData in its payload." ), ) description = models.TextField(null=True, blank=True, help_text=_("Provide more details if needed.")) - group = models.CharField( + blend = models.CharField( max_length=255, null=True, blank=True, help_text=_( - "Optional. Put requirements into the same arbitrary Group ID to make them interchangeable (OR processing logic applies)." + "Optional. Group requirements together using the same Group ID for interchangeable (OR processing logic)." ), + verbose_name=_("group"), ) def __str__(self): @@ -200,7 +204,7 @@ def fulfill(self, username: str): """ template_id = self.template.id progress = BadgeProgress.for_user(username=username, template_id=template_id) - fulfillment, created = Fulfillment.objects.get_or_create(progress=progress, requirement=self, group=self.group) + fulfillment, created = Fulfillment.objects.get_or_create(progress=progress, requirement=self, blend=self.blend) if created: notify_requirement_fulfilled( @@ -248,7 +252,7 @@ def is_group_fulfilled(cls, *, group: str, template: BadgeTemplate, username: st """ progress = BadgeProgress.for_user(username=username, template_id=template.id) - requirements = cls.objects.filter(template=template, group=group) + requirements = cls.objects.filter(template=template, blend=group) fulfilled_requirements = requirements.filter(fulfillments__progress=progress).count() return fulfilled_requirements > 0 @@ -532,7 +536,13 @@ class Fulfillment(models.Model): null=True, related_name="fulfillments", ) - group = models.CharField(max_length=255, null=True, blank=True, help_text=_("Group ID for the requirement.")) + blend = models.CharField( + max_length=255, + null=True, + blank=True, + help_text=_("Group ID for the requirement."), + verbose_name=_("group"), + ) class CredlyBadge(UserCredential): diff --git a/credentials/apps/badges/processing/generic.py b/credentials/apps/badges/processing/generic.py index 8f54808c16..3ac0839841 100644 --- a/credentials/apps/badges/processing/generic.py +++ b/credentials/apps/badges/processing/generic.py @@ -65,7 +65,10 @@ def identify_user(*, event_type, event_payload): user_data = get_user_data(event_payload) if not user_data: - message = f"User data cannot be found (got: {user_data}): {event_payload}. Does event {event_type} include user data at all?" + message = ( + f"User data cannot be found (got: {user_data}): {event_payload}. " + f"Does event {event_type} include user data at all?" + ) raise BadgesProcessingError(message) user, __ = get_or_create_user_from_event_data(user_data) diff --git a/credentials/apps/badges/signals/handlers.py b/credentials/apps/badges/signals/handlers.py index 27e1ab5fc2..2044e08494 100644 --- a/credentials/apps/badges/signals/handlers.py +++ b/credentials/apps/badges/signals/handlers.py @@ -36,7 +36,7 @@ def listen_to_badging_events(): signal.connect(handle_badging_event, dispatch_uid=event_type) -def handle_badging_event(sender, signal, **kwargs): +def handle_badging_event(sender, signal, **kwargs): # pylint: disable=unused-argument """ Generic handler for incoming from the Event bus public signals. """ @@ -47,7 +47,7 @@ def handle_badging_event(sender, signal, **kwargs): @receiver(BADGE_REQUIREMENT_FULFILLED) -def handle_requirement_fulfilled(sender, username, **kwargs): # pylint: disable=unused-argument +def handle_requirement_fulfilled(sender, username, **kwargs): """ On user's Badge progression (completion). """ @@ -55,7 +55,7 @@ def handle_requirement_fulfilled(sender, username, **kwargs): # pylint: disable @receiver(BADGE_REQUIREMENT_REGRESSED) -def handle_requirement_regressed(sender, username, **kwargs): # pylint: disable=unused-argument +def handle_requirement_regressed(sender, username, **kwargs): """ On user's Badge regression (incompletion). """ diff --git a/credentials/apps/badges/signals/signals.py b/credentials/apps/badges/signals/signals.py index c7faf9eaef..db224ff549 100644 --- a/credentials/apps/badges/signals/signals.py +++ b/credentials/apps/badges/signals/signals.py @@ -71,7 +71,7 @@ def notify_progress_incomplete(sender, username, badge_template_id): ) -def notify_badge_awarded(user_credential): # pylint: disable=unused-argument +def notify_badge_awarded(user_credential): """ Emits a public signal about the badge template completion for user. @@ -82,7 +82,7 @@ def notify_badge_awarded(user_credential): # pylint: disable=unused-argument BADGE_AWARDED.send_event(badge=user_credential.as_badge_data()) -def notify_badge_revoked(user_credential): # pylint: disable=unused-argument +def notify_badge_revoked(user_credential): """ Emit public event about badge template regression. diff --git a/credentials/apps/badges/tests/test_admin_forms.py b/credentials/apps/badges/tests/test_admin_forms.py index 2c5fede194..6a5914e936 100644 --- a/credentials/apps/badges/tests/test_admin_forms.py +++ b/credentials/apps/badges/tests/test_admin_forms.py @@ -76,9 +76,7 @@ def test_clean_requirements_different_template(self): with self.assertRaises(forms.ValidationError) as cm: form.clean() - self.assertEqual( - str(cm.exception), "['All requirements must belong to the same template.']" - ) + self.assertEqual(str(cm.exception), "['All requirements must belong to the same template.']") @override_settings(BADGES_CONFIG={"credly": {"ORGANIZATIONS": {}}}) def test_clean(self): @@ -93,9 +91,7 @@ def test_clean(self): ) as mock_get_orgs: mock_get_orgs.return_value = {} - with patch( - "credentials.apps.badges.admin_forms.CredlyAPIClient" - ) as mock_client: + with patch("credentials.apps.badges.admin_forms.CredlyAPIClient") as mock_client: mock_client.return_value = MagicMock() form.clean() @@ -103,9 +99,7 @@ def test_clean(self): mock_get_orgs.assert_called_once() mock_client.assert_called_once_with("test_uuid", "test_api_key") - @override_settings( - BADGES_CONFIG={"credly": {"ORGANIZATIONS": {"test_uuid": "test_api_key"}}} - ) + @override_settings(BADGES_CONFIG={"credly": {"ORGANIZATIONS": {"test_uuid": "test_api_key"}}}) def test_clean_with_configured_organization(self): form = CredlyOrganizationAdminForm() form.cleaned_data = { @@ -118,9 +112,7 @@ def test_clean_with_configured_organization(self): ) as mock_get_orgs: mock_get_orgs.return_value = {"test_uuid": "test_org"} - with patch( - "credentials.apps.badges.admin_forms.CredlyAPIClient" - ) as mock_client: + with patch("credentials.apps.badges.admin_forms.CredlyAPIClient") as mock_client: mock_client.return_value = MagicMock() form.clean() @@ -143,9 +135,7 @@ def test_clean_with_invalid_organization(self): with self.assertRaises(forms.ValidationError) as cm: form.clean() - self.assertIn( - "You specified an invalid authorization token.", str(cm.exception) - ) + self.assertIn("You specified an invalid authorization token.", str(cm.exception)) def test_clean_cannot_provide_api_key_for_configured_organization(self): form = CredlyOrganizationAdminForm() @@ -172,7 +162,7 @@ def test_ensure_organization_exists(self): api_client = MagicMock() api_client.fetch_organization.return_value = {"data": {"org_id": "test_org_id"}} - form._ensure_organization_exists(api_client) + form.ensure_organization_exists(api_client) api_client.fetch_organization.assert_called_once() self.assertEqual(form.api_data, {"org_id": "test_org_id"}) @@ -183,15 +173,14 @@ def test_ensure_organization_exists_with_error(self): api_client.fetch_organization.side_effect = CredlyAPIError("API Error") with self.assertRaises(forms.ValidationError) as cm: - form._ensure_organization_exists(api_client) + form.ensure_organization_exists(api_client) api_client.fetch_organization.assert_called_once() self.assertEqual(str(cm.exception), "['API Error']") class TestParentMixin(ParentMixin): - def get_form_kwargs(self, index): - return super().get_form_kwargs(index) + pass class ParentMixinTestCase(TestCase): diff --git a/credentials/apps/badges/tests/test_api_client.py b/credentials/apps/badges/tests/test_api_client.py index 3da2806ea5..1d9c29a32a 100644 --- a/credentials/apps/badges/tests/test_api_client.py +++ b/credentials/apps/badges/tests/test_api_client.py @@ -4,10 +4,9 @@ from django.test import TestCase from faker import Faker from openedx_events.learning.data import BadgeData, BadgeTemplateData, UserData, UserPersonalData -from requests.models import Response from credentials.apps.badges.credly.api_client import CredlyAPIClient -from credentials.apps.badges.credly.exceptions import CredlyAPIError, CredlyError +from credentials.apps.badges.credly.exceptions import CredlyError from credentials.apps.badges.models import CredlyOrganization @@ -20,9 +19,7 @@ def setUp(self): user=UserData( id=1, is_active=True, - pii=UserPersonalData( - username="test_user", email="test_email@mail.com", name="Test User" - ), + pii=UserPersonalData(username="test_user", email="test_email@mail.com", name="Test User"), ), template=BadgeTemplateData( uuid=fake.uuid4(), @@ -34,21 +31,17 @@ def setUp(self): ) def test_get_organization_nonexistent(self): - with mock.patch( - "credentials.apps.badges.credly.api_client.CredlyOrganization.objects.get" - ) as mock_get: + with mock.patch("credentials.apps.badges.credly.api_client.CredlyOrganization.objects.get") as mock_get: mock_get.side_effect = CredlyOrganization.DoesNotExist with self.assertRaises(CredlyError) as cm: - self.api_client._get_organization("nonexistent_organization_id") + CredlyAPIClient("nonexistent_organization_id") self.assertEqual( str(cm.exception), "CredlyOrganization with the uuid nonexistent_organization_id does not exist!", ) def test_perform_request(self): - with mock.patch( - "credentials.apps.badges.credly.api_client.requests.request" - ) as mock_request: + with mock.patch("credentials.apps.badges.credly.api_client.requests.request") as mock_request: mock_response = mock.Mock() mock_response.json.return_value = {"key": "value"} mock_request.return_value = mock_response @@ -56,55 +49,33 @@ def test_perform_request(self): mock_request.assert_called_once_with( "GET", "https://sandbox-api.credly.com/api/endpoint", - headers=self.api_client._get_headers(), + headers={ + "Accept": "application/json", + "Content-Type": "application/json", + "Authorization": "Basic dGVzdF9hcGlfa2V5", + }, json=None, + timeout=10, ) self.assertEqual(result, {"key": "value"}) - def test_raise_for_error_success(self): - response = mock.Mock(spec=Response) - response.status_code = 200 - self.api_client._raise_for_error(response) - - def test_raise_for_error_error(self): - response = mock.Mock(spec=Response) - response.status_code = 404 - response.text = "Not Found" - response.raise_for_status.side_effect = CredlyAPIError( - f"Credly API: {response.text} ({response.status_code})" - ) - - with self.assertRaises(CredlyAPIError) as cm: - self.api_client._raise_for_error(response) - self.assertEqual(str(cm.exception), "Credly API: Not Found (404)") - def test_fetch_organization(self): - with mock.patch.object( - CredlyAPIClient, "perform_request" - ) as mock_perform_request: + with mock.patch.object(CredlyAPIClient, "perform_request") as mock_perform_request: mock_perform_request.return_value = {"organization": "data"} result = self.api_client.fetch_organization() mock_perform_request.assert_called_once_with("get", "") self.assertEqual(result, {"organization": "data"}) def test_fetch_badge_templates(self): - with mock.patch.object( - CredlyAPIClient, "perform_request" - ) as mock_perform_request: - mock_perform_request.return_value = { - "badge_templates": ["template1", "template2"] - } + with mock.patch.object(CredlyAPIClient, "perform_request") as mock_perform_request: + mock_perform_request.return_value = {"badge_templates": ["template1", "template2"]} result = self.api_client.fetch_badge_templates() - mock_perform_request.assert_called_once_with( - "get", "badge_templates/?filter=state::active" - ) + mock_perform_request.assert_called_once_with("get", "badge_templates/?filter=state::active") self.assertEqual(result, {"badge_templates": ["template1", "template2"]}) def test_fetch_event_information(self): event_id = "event123" - with mock.patch.object( - CredlyAPIClient, "perform_request" - ) as mock_perform_request: + with mock.patch.object(CredlyAPIClient, "perform_request") as mock_perform_request: mock_perform_request.return_value = {"event": "data"} result = self.api_client.fetch_event_information(event_id) mock_perform_request.assert_called_once_with("get", f"events/{event_id}/") @@ -112,25 +83,17 @@ def test_fetch_event_information(self): def test_issue_badge(self): issue_badge_data = self.badge_data - with mock.patch.object( - CredlyAPIClient, "perform_request" - ) as mock_perform_request: + with mock.patch.object(CredlyAPIClient, "perform_request") as mock_perform_request: mock_perform_request.return_value = {"badge": "issued"} result = self.api_client.issue_badge(issue_badge_data) - mock_perform_request.assert_called_once_with( - "post", "badges/", asdict(issue_badge_data) - ) + mock_perform_request.assert_called_once_with("post", "badges/", asdict(issue_badge_data)) self.assertEqual(result, {"badge": "issued"}) def test_revoke_badge(self): badge_id = "badge123" data = {"data": "value"} - with mock.patch.object( - CredlyAPIClient, "perform_request" - ) as mock_perform_request: + with mock.patch.object(CredlyAPIClient, "perform_request") as mock_perform_request: mock_perform_request.return_value = {"badge": "revoked"} result = self.api_client.revoke_badge(badge_id, data) - mock_perform_request.assert_called_once_with( - "put", f"badges/{badge_id}/revoke/", data=data - ) + mock_perform_request.assert_called_once_with("put", f"badges/{badge_id}/revoke/", data=data) self.assertEqual(result, {"badge": "revoked"}) diff --git a/credentials/apps/badges/tests/test_issuers.py b/credentials/apps/badges/tests/test_issuers.py index 77c583869e..45cc07fea4 100644 --- a/credentials/apps/badges/tests/test_issuers.py +++ b/credentials/apps/badges/tests/test_issuers.py @@ -16,7 +16,7 @@ User = get_user_model() -class CredlyBadgeTemplateIssuer(TestCase): +class CredlyBadgeTemplateIssuerTestCase(TestCase): issued_credential_type = CredlyBadgeTemplate issued_user_credential_type = CredlyBadge issuer = CredlyBadgeTemplateIssuer @@ -37,10 +37,10 @@ def setUp(self): ) User.objects.create_user(username="test_user", email="test_email@fff.com", password="test_password") - def _perform_request(self, method, endpoint, data=None): + def _perform_request(self, method, endpoint, data=None): # pylint: disable=unused-argument fake = faker.Faker() return {"data": {"id": fake.uuid4(), "state": "issued"}} - + def test_create_user_credential_with_status_awared(self): # Call create_user_credential with valid arguments with mock.patch("credentials.apps.badges.issuers.notify_badge_awarded") as mock_notify_badge_awarded: @@ -88,7 +88,7 @@ def test_create_user_credential_with_status_revoked(self): ).exists() ) - @patch.object(CredlyAPIClient, 'perform_request', _perform_request) + @patch.object(CredlyAPIClient, "perform_request", _perform_request) def test_issue_credly_badge(self): # Create a test user credential user_credential = self.issued_user_credential_type.objects.create( @@ -134,4 +134,4 @@ def test_issue_credly_badge_with_error(self): # Check if the user credential state is updated to "error" user_credential.refresh_from_db() - self.assertEqual(user_credential.state, "error") \ No newline at end of file + self.assertEqual(user_credential.state, "error") diff --git a/credentials/apps/badges/tests/test_models.py b/credentials/apps/badges/tests/test_models.py index 3be5212c5e..cf1f4a425b 100644 --- a/credentials/apps/badges/tests/test_models.py +++ b/credentials/apps/badges/tests/test_models.py @@ -102,7 +102,6 @@ def setUp(self): organization=self.organization, uuid=uuid.uuid4(), name="test_template", state="draft", site=self.site ) - def test_multiple_requirements_for_badgetemplate(self): self.requirement1 = BadgeRequirement.objects.create( template=self.badge_template, event_type="org.openedx.learning.course.passing.status.updated.v1", @@ -114,41 +113,29 @@ def test_multiple_requirements_for_badgetemplate(self): description="Test description", ) self.requirement3 = BadgeRequirement.objects.create( - template=self.badge_template, - event_type="org.openedx.learning.ccx.course.passing.status.updated.v1", - description="Test description", - ) - - requirements = BadgeRequirement.objects.filter(template=self.badge_template) - - self.assertEqual(requirements.count(), 3) - self.assertIn(self.requirement1, requirements) - self.assertIn(self.requirement2, requirements) - self.assertIn(self.requirement3, requirements) - - def test_multiple_requirements_for_credlybadgetemplate(self): - self.requirement1 = BadgeRequirement.objects.create( template=self.credlybadge_template, event_type="org.openedx.learning.ccx.course.passing.status.updated.v1", description="Test description", ) - self.requirement2 = BadgeRequirement.objects.create( + self.requirement4 = BadgeRequirement.objects.create( template=self.credlybadge_template, event_type="org.openedx.learning.ccx.course.passing.status.updated.v1", description="Test description", ) - self.requirement3 = BadgeRequirement.objects.create( - template=self.credlybadge_template, - event_type="org.openedx.learning.course.passing.status.updated.v1", - description="Test description", - ) - requirements = BadgeRequirement.objects.filter(template=self.credlybadge_template) + def test_multiple_requirements_for_badgetemplate(self): + requirements = BadgeRequirement.objects.filter(template=self.badge_template) - self.assertEqual(requirements.count(), 3) + self.assertEqual(requirements.count(), 2) self.assertIn(self.requirement1, requirements) self.assertIn(self.requirement2, requirements) + + def test_multiple_requirements_for_credlybadgetemplate(self): + requirements = BadgeRequirement.objects.filter(template=self.credlybadge_template) + + self.assertEqual(requirements.count(), 2) self.assertIn(self.requirement3, requirements) + self.assertIn(self.requirement4, requirements) class RequirementFulfillmentCheckTestCase(TestCase): @@ -184,21 +171,21 @@ def setUp(self): self.badge_requirement1 = BadgeRequirement.objects.create( template=self.badge_template, event_type="org.openedx.learning.course.passing.status.updated.v1", - group="group1", + blend="group1", ) self.badge_requirement2 = BadgeRequirement.objects.create( template=self.badge_template, event_type="org.openedx.learning.ccx.course.passing.status.updated.v1", - group="group1", + blend="group1", ) self.badge_requirement3 = BadgeRequirement.objects.create( template=self.badge_template, event_type="org.openedx.learning.course.passing.status.updated.v1" ) def test_requirement_group(self): - group = self.badge_template.requirements.filter(group="group1") - self.assertEqual(group.count(), 2) - self.assertIsNone(self.badge_requirement3.group) + groups = self.badge_template.requirements.filter(blend="group1") + self.assertEqual(groups.count(), 2) + self.assertIsNone(self.badge_requirement3.blend) class BadgeTemplateUserProgressTestCase(TestCase): @@ -217,19 +204,19 @@ def setUp(self): template=self.badge_template, event_type="org.openedx.learning.course.passing.status.updated.v1", description="Test description", - group="A", + blend="A", ) self.requirement2 = BadgeRequirement.objects.create( template=self.badge_template, event_type="org.openedx.learning.course.passing.status.updated.v1", description="Test description", - group="B", + blend="B", ) self.requirement3 = BadgeRequirement.objects.create( template=self.badge_template, event_type="org.openedx.learning.ccx.course.passing.status.updated.v1", description="Test description", - group="C", + blend="C", ) def test_user_progress_success(self): @@ -297,39 +284,39 @@ def setUp(self): template=self.badge_template, event_type="org.openedx.learning.course.passing.status.updated.v1", description="Test description", - group="A", + blend="A", ) self.requirement2 = BadgeRequirement.objects.create( template=self.badge_template, event_type="org.openedx.learning.course.passing.status.updated.v1", description="Test description", - group="B", + blend="B", ) self.group_requirement1 = BadgeRequirement.objects.create( template=self.badge_template, event_type="org.openedx.learning.course.passing.status.updated.v1", description="Test description", - group="test-group1", + blend="test-group1", ) self.group_requirement2 = BadgeRequirement.objects.create( template=self.badge_template, event_type="org.openedx.learning.course.passing.status.updated.v1", description="Test description", - group="test-group1", + blend="test-group1", ) self.group_requirement3 = BadgeRequirement.objects.create( template=self.badge_template, event_type="org.openedx.learning.course.passing.status.updated.v1", description="Test description", - group="test-group2", + blend="test-group2", ) self.group_requirement4 = BadgeRequirement.objects.create( template=self.badge_template, event_type="org.openedx.learning.course.passing.status.updated.v1", description="Test description", - group="test-group2", + blend="test-group2", ) self.progress = BadgeProgress.objects.create(username="test_user", template=self.badge_template) diff --git a/credentials/apps/badges/tests/test_services.py b/credentials/apps/badges/tests/test_services.py index 2c3271e9b1..e23af3b8d5 100644 --- a/credentials/apps/badges/tests/test_services.py +++ b/credentials/apps/badges/tests/test_services.py @@ -281,13 +281,13 @@ def test_process_one_of_grouped_requirements_penalty(self): template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="Test course passing award description", - group="a_or_b", + blend="a_or_b", ) requirement_b = BadgeRequirement.objects.create( template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="Test course passing award description", - group="a_or_b", + blend="a_or_b", ) DataRule.objects.create( requirement=requirement_a, @@ -322,13 +322,13 @@ def test_process_mixed_penalty(self): template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="Test course passing award description", - group="a_or_b", + blend="a_or_b", ) requirement_b = BadgeRequirement.objects.create( template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="Test course passing award description", - group="a_or_b", + blend="a_or_b", ) requirement_c = BadgeRequirement.objects.create( template=self.badge_template, @@ -420,13 +420,13 @@ def test_course_a_or_b_completion(self): template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="A or B course passing award description", - group="a_or_b", + blend="a_or_b", ) requirement_b = BadgeRequirement.objects.create( template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="A or B course passing award description", - group="a_or_b", + blend="a_or_b", ) DataRule.objects.create( requirement=requirement_a, @@ -450,19 +450,19 @@ def test_course_a_or_b_or_c_completion(self): template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="A or B or C course passing award description", - group="a_or_b_or_c", + blend="a_or_b_or_c", ) requirement_b = BadgeRequirement.objects.create( template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="A or B or C course passing award description", - group="a_or_b_or_c", + blend="a_or_b_or_c", ) requirement_c = BadgeRequirement.objects.create( template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="A or B or C course passing award description", - group="a_or_b_or_c", + blend="a_or_b_or_c", ) DataRule.objects.create( requirement=requirement_a, @@ -493,7 +493,7 @@ def test_course_a_or_completion(self): template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="A or course passing award description", - group="a_or", + blend="a_or", ) DataRule.objects.create( requirement=requirement, @@ -510,13 +510,13 @@ def test_course_a_or_b_and_c_completion(self): template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="A or B course passing award description", - group="a_or_b", + blend="a_or_b", ) requirement_b = BadgeRequirement.objects.create( template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="A or B course passing award description", - group="a_or_b", + blend="a_or_b", ) requirement_c = BadgeRequirement.objects.create( template=self.badge_template, @@ -562,25 +562,25 @@ def test_course_a_or_b_and_c_or_d_completion(self): template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="A or B course passing award description", - group="a_or_b", + blend="a_or_b", ) requirement_b = BadgeRequirement.objects.create( template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="A or B course passing award description", - group="a_or_b", + blend="a_or_b", ) requirement_c = BadgeRequirement.objects.create( template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="C or D course passing award description", - group="c_or_d", + blend="c_or_d", ) requirement_d = BadgeRequirement.objects.create( template=self.badge_template, event_type=COURSE_PASSING_EVENT, description="C or D course passing award description", - group="c_or_d", + blend="c_or_d", ) DataRule.objects.create( requirement=requirement_a, @@ -633,17 +633,18 @@ class TestIdentifyUser(TestCase): def test_identify_user(self): username = identify_user(event_type=COURSE_PASSING_EVENT, event_payload=COURSE_PASSING_DATA) self.assertEqual(username, "test_username") - + def test_identify_user_not_found(self): event_type = "unknown_event_type" event_payload = None with self.assertRaises(BadgesProcessingError) as cm: - identify_user(event_type="unknown_event_type", event_payload=event_payload) + identify_user(event_type="unknown_event_type", event_payload=event_payload) self.assertEqual( str(cm.exception), - f"User data cannot be found (got: None): {event_payload}. Does event {event_type} include user data at all?" + f"User data cannot be found (got: None): {event_payload}. " + f"Does event {event_type} include user data at all?", ) @@ -666,18 +667,16 @@ def setUp(self): is_active=True, ) DataRule.objects.create( - requirement=BadgeRequirement.objects.create( - template=self.badge_template, event_type=COURSE_PASSING_EVENT - ), + requirement=BadgeRequirement.objects.create(template=self.badge_template, event_type=COURSE_PASSING_EVENT), data_path="is_passing", operator="eq", - value="True" + value="True", ) PenaltyDataRule.objects.create( penalty=BadgePenalty.objects.create(template=self.badge_template, event_type=COURSE_PASSING_EVENT), data_path="is_passing", operator="eq", - value="False" + value="False", ) self.sender = MagicMock() self.sender.event_type = COURSE_PASSING_EVENT @@ -711,7 +710,6 @@ def test_process_event_not_found(self): process_event(sender=sender, kwargs=event_payload) mock_event_not_found.assert_called_once() - def test_process_event_no_user_data(self): event_payload = CoursePassingStatusData( is_passing=True, @@ -722,4 +720,3 @@ def test_process_event_no_user_data(self): with patch("credentials.apps.badges.processing.generic.logger.error") as mock_no_user_data: process_event(sender=self.sender, kwargs=event_payload) mock_no_user_data.assert_called_once() - diff --git a/credentials/apps/badges/tests/test_signals.py b/credentials/apps/badges/tests/test_signals.py index 8971563f7c..0a79663b43 100644 --- a/credentials/apps/badges/tests/test_signals.py +++ b/credentials/apps/badges/tests/test_signals.py @@ -41,7 +41,7 @@ def test_progression_signal_emission_and_receiver_execution(self): self.assertTrue(user_credential.exists()) # Check if user credential status is 'awarded' - self.assertTrue(user_credential[0].status == "awarded") + self.assertEqual(user_credential[0].status, "awarded") def test_regression_signal_emission_and_receiver_execution(self): # Emit the signal @@ -64,4 +64,4 @@ def test_regression_signal_emission_and_receiver_execution(self): self.assertTrue(user_credential.exists()) # Check if user credential status is 'revoked' - self.assertTrue(user_credential[0].status == "revoked") + self.assertEqual(user_credential[0].status, "revoked") diff --git a/credentials/apps/badges/tests/test_utils.py b/credentials/apps/badges/tests/test_utils.py index 658a06d5f5..2d79f873c9 100644 --- a/credentials/apps/badges/tests/test_utils.py +++ b/credentials/apps/badges/tests/test_utils.py @@ -134,7 +134,7 @@ def test_badges_checks_non_empty_events(self, mock_get_badging_event_types): mock_get_badging_event_types.return_value = ["event1", "event2"] errors = badges_checks() self.assertEqual(len(errors), 0) - + @patch("credentials.apps.badges.checks.credly_check") def test_badges_checks_credly_not_configured(self, mock_credly_check): mock_credly_check.return_value = False @@ -227,16 +227,16 @@ def test_get_credly_api_base_url_production(self): class TestGetEventTypeAttrTypeByKeypath(unittest.TestCase): def test_get_event_type_attr_type_by_keypath(self): - keypath = "course.course_key" - result = get_event_type_attr_type_by_keypath(COURSE_PASSING_EVENT, keypath) + key_path = "course.course_key" + result = get_event_type_attr_type_by_keypath(COURSE_PASSING_EVENT, key_path) self.assertEqual(result, CourseKey) def test_get_event_type_attr_type_by_keypath_bool(self): - keypath = "is_passing" - result = get_event_type_attr_type_by_keypath(COURSE_PASSING_EVENT, keypath) + key_path = "is_passing" + result = get_event_type_attr_type_by_keypath(COURSE_PASSING_EVENT, key_path) self.assertEqual(result, bool) def test_get_event_type_attr_type_by_keypath_not_found(self): - keypath = "course.id" - result = get_event_type_attr_type_by_keypath(COURSE_PASSING_EVENT, keypath) + key_path = "course.id" + result = get_event_type_attr_type_by_keypath(COURSE_PASSING_EVENT, key_path) self.assertIsNone(result) diff --git a/credentials/apps/badges/tests/test_webhooks.py b/credentials/apps/badges/tests/test_webhooks.py index 51f6e84cb2..db95b8d740 100644 --- a/credentials/apps/badges/tests/test_webhooks.py +++ b/credentials/apps/badges/tests/test_webhooks.py @@ -13,14 +13,14 @@ def mocked_handle_event(**kwargs): return "test" -def get_organization(self, organization_id): - organization = MagicMock(spec=CredlyOrganization) # Create a mocked instance of CredlyOrganization +def get_organization(self, organization_id): # pylint: disable=unused-argument + organization = MagicMock(spec=CredlyOrganization) organization.uuid = organization_id organization.api_key = "test_api_key" return organization -def perform_request(self, method, endpoint, data=None): +def perform_request(self, method, endpoint, data=None): # pylint: disable=unused-argument return {"key": "value"} @@ -90,9 +90,7 @@ def test_webhook_deleted_event(self): @patch.object(CredlyAPIClient, "_get_organization", get_organization) @patch.object(CredlyAPIClient, "perform_request", perform_request) def test_webhook_nonexistent_event(self): - with patch( - "credentials.apps.badges.credly.webhooks.logger.error" - ) as mock_handle: + with patch("credentials.apps.badges.credly.webhooks.logger.error") as mock_handle: req = self.rf.post( "/credly/webhookd/", data={ diff --git a/credentials/apps/badges/toggles.py b/credentials/apps/badges/toggles.py index bd54217ded..b82d510d5f 100644 --- a/credentials/apps/badges/toggles.py +++ b/credentials/apps/badges/toggles.py @@ -32,5 +32,6 @@ def check_badges_enabled(func): def wrapper(*args, **kwargs): if is_badges_enabled(): return func(*args, **kwargs) + return None return wrapper diff --git a/credentials/apps/badges/utils.py b/credentials/apps/badges/utils.py index fc1ad8f6d2..719ae66ea5 100644 --- a/credentials/apps/badges/utils.py +++ b/credentials/apps/badges/utils.py @@ -1,4 +1,5 @@ import inspect +from typing import Union import attr from attrs import asdict @@ -29,7 +30,7 @@ def credly_check(): "CREDLY_SANDBOX_API_BASE_URL", "USE_SANDBOX", ) - return all([key in credly_settings.keys() for key in keys]) + return all(key in credly_settings.keys() for key in keys) def keypath(payload, keys_path): @@ -100,7 +101,7 @@ def get_user_data(data: attr.s) -> UserData: return None -def extract_payload(public_signal_kwargs: dict) -> attr.s: +def extract_payload(public_signal_kwargs: dict) -> Union[None, attr.s]: """ Extracts the event payload from the event data. @@ -113,6 +114,7 @@ def extract_payload(public_signal_kwargs: dict) -> attr.s: for value in public_signal_kwargs.values(): if attr.has(value): return value + return None def get_event_type_data(event_type: str) -> attr.s: @@ -169,13 +171,13 @@ def get_data_keypaths(data): return keypaths -def get_event_type_attr_type_by_keypath(event_type: str, keypath: str): +def get_event_type_attr_type_by_keypath(event_type: str, key_path: str): """ Extracts the attribute type for a given keypath in the event type. Parameters: - event_type: The event type to extract dataclass for. - - keypath: The keypath to extract attribute type for. + - key_path: The keypath to extract attribute type for. Returns: type: The attribute type for the given keypath in the event data. @@ -184,12 +186,12 @@ def get_event_type_attr_type_by_keypath(event_type: str, keypath: str): data = get_event_type_data(event_type) data_attrs = attr.fields(data) - def get_attr_type_by_keypath(data_attrs, keypath): + def get_attr_type_by_keypath(data_attrs, key_path): """ Extracts the attribute type for a given keypath in the dataclass. """ - keypath_parts = keypath.split(".") + keypath_parts = key_path.split(".") for attr_ in data_attrs: if attr_.name == keypath_parts[0]: if len(keypath_parts) == 1: @@ -198,4 +200,4 @@ def get_attr_type_by_keypath(data_attrs, keypath): return get_attr_type_by_keypath(attr.fields(attr_.type), ".".join(keypath_parts[1:])) return None - return get_attr_type_by_keypath(data_attrs, keypath) + return get_attr_type_by_keypath(data_attrs, key_path)