From b61b4e6a2dbb4c984dbefe8a7dc81efd78e00000 Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Fri, 15 Mar 2024 20:19:54 +0000 Subject: [PATCH 01/17] feat: send group membership invite, remove, and reminder emails feat: send group membership invite, remove, and reminder emails feat: send group membership invite, remove, and reminder emails feat: send group membership invite, remove, and reminder emails fix: file name chore: refactored chore: refactored chore: refactored sending group reminder email to realized learners fix: deleted unused file chore: update fix: updated tests chore: removed comment chore: removed empty line --- enterprise/api/v1/serializers.py | 3 +- enterprise/api/v1/views/enterprise_group.py | 20 ++- enterprise/api_client/braze.py | 86 +++++++++-- enterprise/models.py | 4 +- enterprise/settings/test.py | 5 + enterprise/tasks.py | 153 +++++++++++++++++++- tests/test_enterprise/api/test_views.py | 85 +++++++++-- tests/test_enterprise/test_tasks.py | 96 +++++++++++- 8 files changed, 418 insertions(+), 34 deletions(-) diff --git a/enterprise/api/v1/serializers.py b/enterprise/api/v1/serializers.py index 5bdab56b1f..41bccb2f63 100644 --- a/enterprise/api/v1/serializers.py +++ b/enterprise/api/v1/serializers.py @@ -599,10 +599,11 @@ class EnterpriseGroupMembershipSerializer(serializers.ModelSerializer): learner_id = serializers.IntegerField(source='enterprise_customer_user.id', allow_null=True) pending_learner_id = serializers.IntegerField(source='pending_enterprise_customer_user.id', allow_null=True) enterprise_group_membership_uuid = serializers.UUIDField(source='uuid', allow_null=True, read_only=True) + enterprise_customer = EnterpriseCustomerSerializer(source='group.enterprise_customer', read_only=True) class Meta: model = models.EnterpriseGroupMembership - fields = ('learner_id', 'pending_learner_id', 'enterprise_group_membership_uuid') + fields = ('learner_id', 'pending_learner_id', 'enterprise_group_membership_uuid', 'enterprise_customer') class EnterpriseCustomerUserReadOnlySerializer(serializers.ModelSerializer): diff --git a/enterprise/api/v1/views/enterprise_group.py b/enterprise/api/v1/views/enterprise_group.py index 8c324b1225..0e8e3bbdd5 100644 --- a/enterprise/api/v1/views/enterprise_group.py +++ b/enterprise/api/v1/views/enterprise_group.py @@ -17,6 +17,7 @@ from enterprise.api.v1 import serializers from enterprise.api.v1.views.base_views import EnterpriseReadWriteModelViewSet from enterprise.logging import getEnterpriseLogger +from enterprise.tasks import send_group_membership_invitation_notification, send_group_membership_removal_notification LOGGER = getEnterpriseLogger(__name__) @@ -111,6 +112,7 @@ def get_learners(self, *args, **kwargs): 'learner_uuid': 'enterprise_customer_user_id', 'pending_learner_id': 'pending_enterprise_customer_user_id', 'enterprise_group_membership_uuid': 'enterprise_group_membership_uuid', + 'enterprise_customer': EnterpriseCustomerSerializer }, ], } @@ -118,9 +120,11 @@ def get_learners(self, *args, **kwargs): """ group_uuid = kwargs.get('group_uuid') + # GET api/v1/enterprise-group//learners?filter_for_pecus=true + filter_for_pecus = self.request.query_params.get('filter_for_pecus', None) try: group_object = self.get_queryset().get(uuid=group_uuid) - members = group_object.get_all_learners() + members = group_object.get_all_learners(filter_for_pecus) page = self.paginate_queryset(members) serializer = serializers.EnterpriseGroupMembershipSerializer(page, many=True) response = self.get_paginated_response(serializer.data) @@ -157,6 +161,8 @@ def assign_learners(self, request, group_uuid): except models.EnterpriseGroup.DoesNotExist as exc: raise Http404 from exc if requested_emails := request.POST.dict().get('learner_emails'): + budget_expiration = request.POST.dict().get('budget_expiration') + catalog_uuid = request.POST.dict().get('catalog_uuid') total_records_processed = 0 total_existing_users_processed = 0 total_new_users_processed = 0 @@ -167,7 +173,6 @@ def assign_learners(self, request, group_uuid): ecus = [] # Gather all existing User objects associated with the email batch existing_users = User.objects.filter(email__in=user_email_batch) - # Build and create a list of EnterpriseCustomerUser objects for the emails of existing Users # Ignore conflicts in case any of the ent customer user objects already exist ecu_by_email = { @@ -237,6 +242,14 @@ def assign_learners(self, request, group_uuid): 'new_learners': total_new_users_processed, 'existing_learners': total_existing_users_processed, } + if budget_expiration is not None and catalog_uuid is not None: + for membership in memberships: + send_group_membership_invitation_notification.delay( + customer.uuid, + membership.uuid, + budget_expiration, + catalog_uuid + ) return Response(data, status=201) return Response(data="Error: missing request data: `learner_emails`.", status=400) @@ -259,6 +272,7 @@ def remove_learners(self, request, group_uuid): """ try: group = self.get_queryset().get(uuid=group_uuid) + customer = group.enterprise_customer except models.EnterpriseGroup.DoesNotExist as exc: raise Http404 from exc if requested_emails := request.POST.dict().get('learner_emails'): @@ -272,6 +286,8 @@ def remove_learners(self, request, group_uuid): group_q & (ecu_in_q | pecu_in_q), ) records_deleted += len(records_to_delete) + for record in records_to_delete: + send_group_membership_removal_notification.delay(customer.uuid, record.uuid) records_to_delete.delete() data = { 'records_deleted': records_deleted, diff --git a/enterprise/api_client/braze.py b/enterprise/api_client/braze.py index c8a05e53dc..3549743a35 100644 --- a/enterprise/api_client/braze.py +++ b/enterprise/api_client/braze.py @@ -9,26 +9,88 @@ logger = logging.getLogger(__name__) +ENTERPRISE_BRAZE_ALIAS_LABEL = 'Enterprise' # Do Not change this, this is consistent with other uses across edX repos. -class BrazeAPIClient: + +class BrazeAPIClient(BrazeClient): """ API client for calls to Braze. """ - @classmethod - def get_braze_client(cls): - """ Returns a Braze client. """ - if not BrazeClient: - return None - - # fetching them from edx-platform settings + def __init__(self): braze_api_key = getattr(settings, 'EDX_BRAZE_API_KEY', None) braze_api_url = getattr(settings, 'EDX_BRAZE_API_SERVER', None) + required_settings = ['EDX_BRAZE_API_KEY', 'EDX_BRAZE_API_SERVER'] + for setting in required_settings: + if not getattr(settings, setting, None): + msg = f'Missing {setting} in settings required for Braze API Client.' + logger.error(msg) + raise ValueError(msg) - if not braze_api_key or not braze_api_url: - return None - - return BrazeClient( + super().__init__( api_key=braze_api_key, api_url=braze_api_url, app_id='', ) + + def generate_mailto_link(self, emails): + """ + Generate a mailto link for the given emails. + """ + if emails: + return f'mailto:{",".join(emails)}' + + return None + + def create_recipient( + self, + user_email, + lms_user_id, + trigger_properties=None, + ): + """ + Create a recipient object using the given user_email and lms_user_id. + Identifies the given email address with any existing Braze alias records + via the provided ``lms_user_id``. + """ + + user_alias = { + 'alias_label': ENTERPRISE_BRAZE_ALIAS_LABEL, + 'alias_name': user_email, + } + + # Identify the user alias in case it already exists. This is necessary so + # we don't accidentally create a duplicate Braze profile. + self.identify_users([{ + 'external_id': lms_user_id, + 'user_alias': user_alias + }]) + + attributes = { + "user_alias": user_alias, + "email": user_email, + "is_enterprise_learner": True, + "_update_existing_only": False, + } + + return { + 'external_user_id': lms_user_id, + 'attributes': attributes, + # If a profile does not already exist, Braze will create a new profile before sending a message. + 'send_to_existing_only': False, + 'trigger_properties': trigger_properties or {}, + } + + def create_recipient_no_external_id(self, user_email): + """ + Create a Braze recipient dict identified only by an alias based on their email. + """ + return { + 'attributes': { + 'email': user_email, + 'is_enterprise_learner': True, + }, + 'user_alias': { + 'alias_label': ENTERPRISE_BRAZE_ALIAS_LABEL, + 'alias_name': user_email, + }, + } diff --git a/enterprise/models.py b/enterprise/models.py index c47b9caf05..8cb4f96bb8 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -4249,7 +4249,7 @@ class Meta: unique_together = (("name", "enterprise_customer"),) ordering = ['-modified'] - def get_all_learners(self): + def get_all_learners(self, filter_for_pecus): """ Returns all users associated with the group, whether the group specifies the entire org else all associated membership records. @@ -4277,6 +4277,8 @@ def get_all_learners(self): )) return members else: + if filter_for_pecus is not None: + return self.members.filter(is_removed=False, enterprise_customer_user_id__isnull=True) return self.members.filter(is_removed=False) diff --git a/enterprise/settings/test.py b/enterprise/settings/test.py index e95dcfad49..c60947507c 100644 --- a/enterprise/settings/test.py +++ b/enterprise/settings/test.py @@ -364,3 +364,8 @@ def root(*args): ENTERPRISE_SSO_ORCHESTRATOR_BASE_URL = 'https://foobar.com' ENTERPRISE_SSO_ORCHESTRATOR_CONFIGURE_PATH = 'configure' ENTERPRISE_SSO_ORCHESTRATOR_CONFIGURE_EDX_OAUTH_PATH = 'configure-edx-oauth' + +EDX_BRAZE_API_KEY='894e2287-66d5-4e41-b04e-67aba70dabf4' +EDX_BRAZE_API_SERVER=None +BRAZE_GROUPS_INVITATION_EMAIL_CAMPAIGN_ID = 'test-invitation-campaign-id' +BRAZE_GROUPS_REMOVAL_EMAIL_CAMPAIGN_ID = 'test-removal-campaign-id' diff --git a/enterprise/tasks.py b/enterprise/tasks.py index c829965257..3c8ef9fa00 100644 --- a/enterprise/tasks.py +++ b/enterprise/tasks.py @@ -9,10 +9,13 @@ from edx_django_utils.monitoring import set_code_owner_attribute from django.apps import apps +from django.conf import settings from django.core import mail from django.db import IntegrityError -from enterprise.api_client.braze import BrazeAPIClient +from enterprise.api_client.braze import ENTERPRISE_BRAZE_ALIAS_LABEL, BrazeAPIClient +from enterprise.api_client.enterprise_catalog import EnterpriseCatalogApiClient + from enterprise.constants import SSO_BRAZE_CAMPAIGN_ID from enterprise.utils import get_enterprise_customer, send_email_notification_message @@ -127,6 +130,22 @@ def enterprise_customer_user_model(): return apps.get_model('enterprise', 'EnterpriseCustomerUser') +def pending_enterprise_customer_user_model(): + """ + Returns the ``PendingEnterpriseCustomerUser`` class. + This function is needed to avoid circular ref issues when model classes call tasks in this module. + """ + return apps.get_model('enterprise', 'PendingEnterpriseCustomerUser') + + +def enterprise_group_membership_model(): + """ + Returns the ``EnterpriseGroupMembership`` class. + This function is needed to avoid circular ref issues when model classes call tasks in this module. + """ + return apps.get_model('enterprise', 'EnterpriseGroupMembership') + + def enterprise_course_enrollment_model(): """ Returns the ``EnterpriseCourseEnrollment`` class. @@ -169,7 +188,7 @@ def send_sso_configured_email( } try: - braze_client_instance = BrazeAPIClient.get_braze_client() + braze_client_instance = BrazeAPIClient() braze_client_instance.send_campaign_message( braze_campaign_id, recipients=[contact_email], @@ -182,3 +201,133 @@ def send_sso_configured_email( ) LOGGER.exception(message) raise exc + + +@shared_task +@set_code_owner_attribute +def send_group_membership_invitation_notification( + enterprise_customer_uuid, + group_membership_uuid, + budget_expiration, + catalog_uuid +): + """ + Send braze email notification when member is invited to a group. + + Arguments: + * enterprise_customer_uuid (string) + * group_membership_uuid (string) + * budget_expiration (datetime) + * catalog_uuid (string) + """ + enterprise_customer = get_enterprise_customer(enterprise_customer_uuid) + group_membership = enterprise_group_membership_model().objects.filter( + uuid=group_membership_uuid + ).values().last() + braze_client_instance = BrazeAPIClient() + enterprise_catalog_client = EnterpriseCatalogApiClient() + braze_trigger_properties = {} + contact_email = enterprise_customer.contact_email + enterprise_customer_name = enterprise_customer.name + braze_trigger_properties['contact_admin_link'] = braze_client_instance.generate_mailto_link(contact_email) + braze_trigger_properties['enterprise_customer_name'] = enterprise_customer_name + braze_trigger_properties[ + 'catalog_content_count' + ] = enterprise_catalog_client.get_catalog_content_count(catalog_uuid) + braze_trigger_properties['budget_end_date'] = budget_expiration + if group_membership['enterprise_customer_user_id'] is None: + pending_enterprise_customer_user = pending_enterprise_customer_user_model().objects.get( + id=group_membership['pending_enterprise_customer_user_id']) + learner_email = pending_enterprise_customer_user.user_email + recipient = braze_client_instance.create_recipient_no_external_id( + learner_email, + ) + # We need an alias record to exist in Braze before + # sending to any previously-unidentified users. + braze_client_instance.create_braze_alias( + [learner_email], + ENTERPRISE_BRAZE_ALIAS_LABEL, + ) + else: + enterprise_customer_user = enterprise_customer_user_model().objects.get( + user_id=group_membership['enterprise_customer_user_id']) + learner_email = enterprise_customer_user.user_email + recipient = braze_client_instance.create_recipient( + user_email=learner_email, + lms_user_id=enterprise_customer_user.user_id, + ) + + try: + braze_client_instance.send_campaign_message( + settings.BRAZE_GROUPS_INVITATION_EMAIL_CAMPAIGN_ID, + recipients=[recipient], + trigger_properties=braze_trigger_properties, + ) + except BrazeClientError as exc: + message = ( + "Groups learner invitation email could not be sent " + f"to {learner_email} for enterprise {enterprise_customer_name}." + ) + LOGGER.exception(message) + raise exc + + +@shared_task +@set_code_owner_attribute +def send_group_membership_removal_notification(enterprise_customer_uuid, group_membership_uuid): + """ + Send braze email notification when learner is removed from a group. + + Arguments: + * enterprise_customer_uuid (string) + * group_membership_uuid (string) + """ + enterprise_customer = get_enterprise_customer(enterprise_customer_uuid) + group_membership = enterprise_group_membership_model().objects.filter( + uuid=group_membership_uuid + ).values().last() + braze_client_instance = BrazeAPIClient() + braze_trigger_properties = {} + braze_campaign_id = settings.BRAZE_GROUPS_REMOVAL_EMAIL_CAMPAIGN_ID + contact_email = enterprise_customer.contact_email + enterprise_customer_name = enterprise_customer.name + braze_trigger_properties['contact_admin_link'] = braze_client_instance.generate_mailto_link(contact_email) + braze_trigger_properties['enterprise_customer_name'] = enterprise_customer_name + + if group_membership['enterprise_customer_user_id'] is None: + pending_enterprise_customer_user = pending_enterprise_customer_user_model().objects.get( + id=group_membership['pending_enterprise_customer_user_id'] + ) + learner_email = pending_enterprise_customer_user.user_email + recipient = braze_client_instance.create_recipient_no_external_id( + learner_email, + ) + # We need an alias record to exist in Braze before + # sending to any previously-unidentified users. + braze_client_instance.create_braze_alias( + [learner_email], + ENTERPRISE_BRAZE_ALIAS_LABEL, + ) + else: + enterprise_customer_user = enterprise_customer_user_model().objects.get( + user_id=group_membership['enterprise_customer_user_id'] + ) + learner_email = enterprise_customer_user.user_email + recipient = braze_client_instance.create_recipient( + user_email=learner_email, + lms_user_id=enterprise_customer_user.user_id, + ) + + try: + braze_client_instance.send_campaign_message( + braze_campaign_id, + recipients=[recipient], + trigger_properties=braze_trigger_properties, + ) + except BrazeClientError as exc: + message = ( + "Groups learner removal email could not be sent " + f"to {learner_email} for enterprise {enterprise_customer_name}." + ) + LOGGER.exception(message) + raise exc diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index c08a05d97c..d7947bb68f 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -7296,6 +7296,7 @@ def setUp(self): is_active=True, is_staff=False, ) + self.pending_enterprise_customer_user = PendingEnterpriseCustomerUserFactory() self.enterprise_customer_user = EnterpriseCustomerUserFactory( user_id=self.user.id, enterprise_customer=self.enterprise_customer ) @@ -7369,6 +7370,9 @@ def test_successful_list_learners(self): 'learner_id': self.enterprise_group_memberships[i].enterprise_customer_user.id, 'pending_learner_id': None, 'enterprise_group_membership_uuid': str(self.enterprise_group_memberships[i].uuid), + 'enterprise_customer': { + 'name': self.enterprise_customer.name, + } }, ) expected_response = { @@ -7378,7 +7382,15 @@ def test_successful_list_learners(self): 'results': results_list, } response = self.client.get(url) - assert response.json() == expected_response + for i in range(10): + assert response.json()['results'][i]['learner_id'] == expected_response['results'][i]['learner_id'] + assert response.json()['results'][i]['pending_learner_id'] == ( + expected_response['results'][i]['pending_learner_id']) + assert (response.json()['results'][i]['enterprise_group_membership_uuid'] + == expected_response['results'][i]['enterprise_group_membership_uuid']) + assert (response.json()['results'][i]['enterprise_customer']['name'] + == expected_response['results'][i]['enterprise_customer']['name']) + # verify page 2 of paginated response url_page_2 = settings.TEST_SERVER + reverse( 'enterprise-group-learners', @@ -7394,15 +7406,55 @@ def test_successful_list_learners(self): 'learner_id': self.enterprise_group_memberships[0].enterprise_customer_user.id, 'pending_learner_id': None, 'enterprise_group_membership_uuid': str(self.enterprise_group_memberships[0].uuid), + 'enterprise_customer': { + 'name': self.enterprise_customer.name, + } } ], } - assert page_2_response.json() == expected_response_page_2 - + assert page_2_response.json()['count'] == expected_response_page_2['count'] + assert page_2_response.json()['previous'] == expected_response_page_2['previous'] + assert page_2_response.json()['results'][0]['learner_id'] == ( + expected_response_page_2['results'][0]['learner_id']) + assert page_2_response.json()['results'][0]['pending_learner_id'] == ( + expected_response_page_2['results'][0]['pending_learner_id']) + assert (page_2_response.json()['results'][0]['enterprise_group_membership_uuid'] + == expected_response_page_2['results'][0]['enterprise_group_membership_uuid']) + assert (page_2_response.json()['results'][0]['enterprise_customer']['name'] + == expected_response_page_2['results'][0]['enterprise_customer']['name']) self.enterprise_group_memberships[0].delete() response = self.client.get(url) assert response.json()['count'] == 10 + # url: 'http://testserver/enterprise/api/v1/enterprise_group//learners/?filter_for_pecus=true' + # verify filtered response for only pending users + self.enterprise_group_memberships.append(EnterpriseGroupMembershipFactory( + group=self.group_1, + pending_enterprise_customer_user=self.pending_enterprise_customer_user, + enterprise_customer_user=None + )) + filter_for_pecus_url = settings.TEST_SERVER + reverse( + 'enterprise-group-learners', + kwargs={'group_uuid': self.group_1.uuid}, + ) + '/?filter_for_pecus=true' + filter_for_pecus_response = self.client.get(filter_for_pecus_url) + expected_filtered_for_pecus_response = { + 'count': 1, + 'next': None, + 'previous': None, + 'results': [ + { + 'learner_id': self.enterprise_group_memberships[0].enterprise_customer_user.id, + 'pending_learner_id': self.pending_enterprise_customer_user, + 'enterprise_group_membership_uuid': str(self.enterprise_group_memberships[0].uuid), + 'enterprise_customer': { + 'name': self.enterprise_customer.name, + } + }, + ], + } + assert filter_for_pecus_response.json()['count'] == expected_filtered_for_pecus_response['count'] + def test_group_uuid_not_found(self): """ Verify that the endpoint api/v1/enterprise_group//learners/ @@ -7534,7 +7586,8 @@ def test_assign_learners_requires_learner_emails(self): assert response.status_code == 400 assert response.data == "Error: missing request data: `learner_emails`." - def test_successful_assign_learners_to_group(self): + @mock.patch('enterprise.tasks.send_group_membership_invitation_notification.delay', return_value=mock.MagicMock()) + def test_successful_assign_learners_to_group(self, mock_send_group_membership_invitation_notification): """ Test that both existing and new learners assigned to groups properly creates membership records """ @@ -7545,10 +7598,14 @@ def test_successful_assign_learners_to_group(self): existing_emails = ",".join([(UserFactory().email) for _ in range(10)]) new_emails = ",".join([(f"email_{x}@example.com") for x in range(10)]) - - request_data = {'learner_emails': f"{new_emails},{existing_emails}"} + budget_expiration = datetime.now() + catalog_uuid = uuid.uuid4() + request_data = { + 'learner_emails': f"{new_emails},{existing_emails}", + 'budget_expiration': budget_expiration, + 'catalog_uuid': catalog_uuid, + } response = self.client.post(url, data=request_data) - assert response.status_code == 201 assert response.data == {'records_processed': 20, 'new_learners': 10, 'existing_learners': 10} assert len( @@ -7563,6 +7620,7 @@ def test_successful_assign_learners_to_group(self): enterprise_customer_user__isnull=True ) ) == 10 + assert mock_send_group_membership_invitation_notification.call_count == 20 def test_remove_learners_404(self): """ @@ -7615,7 +7673,8 @@ def test_patch_with_bad_request_customer_to_change_to(self): response = self.client.patch(url, data=request_data) assert response.status_code == 401 - def test_successful_remove_learners_from_group(self): + @mock.patch('enterprise.tasks.send_group_membership_removal_notification.delay', return_value=mock.MagicMock()) + def test_successful_remove_learners_from_group(self, mock_send_group_membership_removal_notification): """ Test that both existing and new learners in groups are properly removed by the remove_learners endpoint """ @@ -7634,9 +7693,7 @@ def test_successful_remove_learners_from_group(self): response = self.client.post(url, data=request_data) assert response.status_code == 200 assert response.data == {'records_deleted': 10} - for membership in memberships_to_delete: - with self.assertRaises(EnterpriseGroupMembership.DoesNotExist): - EnterpriseGroupMembership.objects.get(pk=membership.pk) + assert mock_send_group_membership_removal_notification.call_count == 10 def test_remove_learners_from_group_only_removes_from_specified_group(self): """ @@ -7779,7 +7836,7 @@ def test_sso_configuration_oauth_orchestration_complete_not_found(self): response = self.post_sso_configuration_complete(config_pk) assert response.status_code == 404 - @mock.patch("enterprise.api_client.braze.BrazeAPIClient.get_braze_client") + @mock.patch("enterprise.api_client.braze.BrazeAPIClient") def test_sso_configuration_oauth_orchestration_complete_error(self, mock_braze_client): """ Verify that the endpoint is able to mark an sso config as errored. @@ -7800,7 +7857,7 @@ def test_sso_configuration_oauth_orchestration_complete_error(self, mock_braze_c assert enterprise_sso_orchestration_config.errored_at is not None assert response.status_code == status.HTTP_200_OK - @mock.patch("enterprise.api_client.braze.BrazeAPIClient.get_braze_client") + @mock.patch("enterprise.api_client.braze.BrazeAPIClient") def test_sso_configuration_oauth_orchestration_complete(self, mock_braze_client): """ Verify that the endpoint returns the correct response when the oauth orchestration is complete. @@ -7821,7 +7878,7 @@ def test_sso_configuration_oauth_orchestration_complete(self, mock_braze_client) assert enterprise_sso_orchestration_config.is_pending_configuration() is False assert response.status_code == status.HTTP_200_OK - @mock.patch("enterprise.api_client.braze.BrazeAPIClient.get_braze_client") + @mock.patch("enterprise.api_client.braze.BrazeAPIClient") def test_sso_configuration_oauth_orchestration_email(self, mock_braze_client): """ Assert sso configuration calls Braze API with the correct arguments. diff --git a/tests/test_enterprise/test_tasks.py b/tests/test_enterprise/test_tasks.py index eba0f8b6ca..bb288a4333 100644 --- a/tests/test_enterprise/test_tasks.py +++ b/tests/test_enterprise/test_tasks.py @@ -4,16 +4,26 @@ import copy import unittest +import uuid +from datetime import datetime from unittest import mock from pytest import mark from enterprise.models import EnterpriseCourseEnrollment, EnterpriseEnrollmentSource -from enterprise.tasks import create_enterprise_enrollment, send_enterprise_email_notification +from enterprise.settings.test import BRAZE_GROUPS_INVITATION_EMAIL_CAMPAIGN_ID, BRAZE_GROUPS_REMOVAL_EMAIL_CAMPAIGN_ID +from enterprise.tasks import ( + create_enterprise_enrollment, + send_enterprise_email_notification, + send_group_membership_invitation_notification, + send_group_membership_removal_notification, +) from enterprise.utils import serialize_notification_content from test_utils.factories import ( EnterpriseCustomerFactory, EnterpriseCustomerUserFactory, + EnterpriseGroupFactory, + EnterpriseGroupMembershipFactory, PendingEnterpriseCustomerUserFactory, UserFactory, ) @@ -30,14 +40,28 @@ def setUp(self): """ Setup for `TestEnterpriseTasks` test. """ - self.user = UserFactory(id=2, email='user@example.com') self.enterprise_customer = EnterpriseCustomerFactory( name='Team Titans', ) + self.user = UserFactory(email='user@example.com') + self.pending_enterprise_customer_user = PendingEnterpriseCustomerUserFactory() + self.enterprise_group = EnterpriseGroupFactory(enterprise_customer=self.enterprise_customer) self.enterprise_customer_user = EnterpriseCustomerUserFactory( user_id=self.user.id, enterprise_customer=self.enterprise_customer, ) + self.enterprise_group_memberships = [] + + self.enterprise_group_memberships.append(EnterpriseGroupMembershipFactory( + group=self.enterprise_group, + pending_enterprise_customer_user=None, + enterprise_customer_user__enterprise_customer=self.enterprise_customer, + )) + self.enterprise_group_memberships.append(EnterpriseGroupMembershipFactory( + group=self.enterprise_group, + pending_enterprise_customer_user=self.pending_enterprise_customer_user, + enterprise_customer_user=None, + )) super().setUp() @mock.patch('enterprise.models.EnterpriseCustomer.catalog_contains_course') @@ -180,3 +204,71 @@ def test_send_enterprise_email_notification(self, mock_send_notification, mock_e admin_enrollment=admin_enrollment, ) for item in email_items] mock_send_notification.assert_has_calls(calls) + + @mock.patch('enterprise.tasks.EnterpriseCatalogApiClient', return_value=mock.MagicMock()) + @mock.patch('enterprise.tasks.BrazeAPIClient', return_value=mock.MagicMock()) + def test_send_group_membership_invitation_notification(self, mock_braze_api_client, mock_enterprise_catalog_client): + """ + Verify send_group_membership_invitation_notification hits braze client with expected args + """ + admin_email = 'edx@example.org' + mock_recipients = [{'external_user_id': 1}, self.pending_enterprise_customer_user.user_email] + mock_catalog_content_count = 5 + mock_admin_mailto = f'mailto:{admin_email}' + mock_braze_api_client().create_recipient.return_value = mock_recipients[0] + mock_braze_api_client().generate_mailto_link.return_value = mock_admin_mailto + mock_braze_api_client().create_recipient_no_external_id.return_value = ( + self.pending_enterprise_customer_user.user_email) + mock_enterprise_catalog_client().get_catalog_content_count.return_value = ( + mock_catalog_content_count) + budget_expiration = datetime.now() + catalog_uuid = uuid.uuid4() + for membership in self.enterprise_group_memberships: + send_group_membership_invitation_notification( + self.enterprise_customer.uuid, + membership.uuid, + budget_expiration, + catalog_uuid, + ) + calls = [mock.call( + BRAZE_GROUPS_INVITATION_EMAIL_CAMPAIGN_ID, + recipients=[recipient], + trigger_properties={ + 'contact_admin_link': mock_admin_mailto, + 'enterprise_customer_name': self.enterprise_customer.name, + 'catalog_content_count': mock_catalog_content_count, + 'budget_end_date': budget_expiration, + }, + ) for recipient in mock_recipients] + mock_braze_api_client().send_campaign_message.assert_has_calls(calls) + + @mock.patch('enterprise.tasks.EnterpriseCatalogApiClient', return_value=mock.MagicMock()) + @mock.patch('enterprise.tasks.BrazeAPIClient', return_value=mock.MagicMock()) + def test_send_group_membership_removal_notification(self, mock_braze_api_client, mock_enterprise_catalog_client): + """ + Verify send_group_membership_removal_notification hits braze client with expected args + """ + admin_email = 'edx@example.org' + mock_recipients = [{'external_user_id': 1}, self.pending_enterprise_customer_user.user_email] + mock_catalog_content_count = 5 + mock_admin_mailto = f'mailto:{admin_email}' + mock_braze_api_client().create_recipient.return_value = mock_recipients[0] + mock_braze_api_client().generate_mailto_link.return_value = mock_admin_mailto + mock_braze_api_client().create_recipient_no_external_id.return_value = ( + self.pending_enterprise_customer_user.user_email) + mock_enterprise_catalog_client().get_catalog_content_count.return_value = ( + mock_catalog_content_count) + for membership in self.enterprise_group_memberships: + send_group_membership_removal_notification( + self.enterprise_customer.uuid, + membership.uuid, + ) + calls = [mock.call( + BRAZE_GROUPS_REMOVAL_EMAIL_CAMPAIGN_ID, + recipients=[recipient], + trigger_properties={ + 'contact_admin_link': mock_admin_mailto, + 'enterprise_customer_name': self.enterprise_customer.name, + }, + ) for recipient in mock_recipients] + mock_braze_api_client().send_campaign_message.assert_has_calls(calls) From b5dcadc564f860f71d85e4a0c607f47342460de8 Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Wed, 27 Mar 2024 22:57:47 +0000 Subject: [PATCH 02/17] fix: updated failing test --- enterprise/settings/test.py | 4 +-- enterprise/tasks.py | 10 ++++-- tests/test_enterprise/api/test_views.py | 41 ------------------------- tests/test_enterprise/test_tasks.py | 25 +++++++++++++++ 4 files changed, 35 insertions(+), 45 deletions(-) diff --git a/enterprise/settings/test.py b/enterprise/settings/test.py index c60947507c..f6dae73273 100644 --- a/enterprise/settings/test.py +++ b/enterprise/settings/test.py @@ -365,7 +365,7 @@ def root(*args): ENTERPRISE_SSO_ORCHESTRATOR_CONFIGURE_PATH = 'configure' ENTERPRISE_SSO_ORCHESTRATOR_CONFIGURE_EDX_OAUTH_PATH = 'configure-edx-oauth' -EDX_BRAZE_API_KEY='894e2287-66d5-4e41-b04e-67aba70dabf4' -EDX_BRAZE_API_SERVER=None +EDX_BRAZE_API_KEY='test-api-key' +EDX_BRAZE_API_SERVER='test-api-server' BRAZE_GROUPS_INVITATION_EMAIL_CAMPAIGN_ID = 'test-invitation-campaign-id' BRAZE_GROUPS_REMOVAL_EMAIL_CAMPAIGN_ID = 'test-removal-campaign-id' diff --git a/enterprise/tasks.py b/enterprise/tasks.py index 3c8ef9fa00..28cbb1ed3d 100644 --- a/enterprise/tasks.py +++ b/enterprise/tasks.py @@ -15,7 +15,6 @@ from enterprise.api_client.braze import ENTERPRISE_BRAZE_ALIAS_LABEL, BrazeAPIClient from enterprise.api_client.enterprise_catalog import EnterpriseCatalogApiClient - from enterprise.constants import SSO_BRAZE_CAMPAIGN_ID from enterprise.utils import get_enterprise_customer, send_email_notification_message @@ -189,9 +188,16 @@ def send_sso_configured_email( try: braze_client_instance = BrazeAPIClient() + recipient = braze_client_instance.create_recipient_no_external_id( + contact_email, + ) + braze_client_instance.create_braze_alias( + [contact_email], + ENTERPRISE_BRAZE_ALIAS_LABEL, + ) braze_client_instance.send_campaign_message( braze_campaign_id, - recipients=[contact_email], + recipients=[recipient], trigger_properties=braze_trigger_properties, ) except BrazeClientError as exc: diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index 3ee8e6bfbe..f76feada57 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -41,7 +41,6 @@ ENTERPRISE_OPERATOR_ROLE, ENTERPRISE_REPORTING_CONFIG_ADMIN_ROLE, PATHWAY_CUSTOMER_ADMIN_ENROLLMENT, - SSO_BRAZE_CAMPAIGN_ID, ) from enterprise.models import ( ChatGPTResponse, @@ -7961,46 +7960,6 @@ def test_sso_configuration_oauth_orchestration_complete(self, mock_braze_client) assert enterprise_sso_orchestration_config.is_pending_configuration() is False assert response.status_code == status.HTTP_200_OK - @mock.patch("enterprise.api_client.braze.BrazeAPIClient") - def test_sso_configuration_oauth_orchestration_email(self, mock_braze_client): - """ - Assert sso configuration calls Braze API with the correct arguments. - """ - mock_braze_client.return_value.get_braze_client.return_value = mock.MagicMock() - mock_send_campaign_message = mock_braze_client.return_value.send_campaign_message - - self.set_jwt_cookie(ENTERPRISE_OPERATOR_ROLE, "*") - config_pk = uuid.uuid4() - enterprise_sso_orchestration_config = EnterpriseCustomerSsoConfigurationFactory( - uuid=config_pk, - enterprise_customer=self.enterprise_customer, - configured_at=None, - submitted_at=localized_utcnow(), - ) - url = settings.TEST_SERVER + reverse( - self.SSO_CONFIGURATION_COMPLETE_ENDPOINT, - kwargs={'configuration_uuid': config_pk} - ) - assert enterprise_sso_orchestration_config.is_pending_configuration() - self.client.post(url) - - expected_trigger_properties = { - 'enterprise_customer_slug': self.enterprise_customer.slug, - 'enterprise_customer_name': self.enterprise_customer.name, - 'enterprise_sender_alias': self.enterprise_customer.sender_alias, - 'enterprise_contact_email': self.enterprise_customer.contact_email, - } - - mock_send_campaign_message.assert_any_call( - SSO_BRAZE_CAMPAIGN_ID, - recipients=[self.enterprise_customer.contact_email], - trigger_properties=expected_trigger_properties, - ) - enterprise_sso_orchestration_config.refresh_from_db() - assert enterprise_sso_orchestration_config.configured_at is not None - - # -------------------------- retrieve test suite -------------------------- - def test_sso_configuration_retrieve(self): """ Test expected response when successfully retrieving an existing sso configuration. diff --git a/tests/test_enterprise/test_tasks.py b/tests/test_enterprise/test_tasks.py index bb288a4333..70a4ccafc6 100644 --- a/tests/test_enterprise/test_tasks.py +++ b/tests/test_enterprise/test_tasks.py @@ -10,6 +10,9 @@ from pytest import mark +from enterprise.constants import ( + SSO_BRAZE_CAMPAIGN_ID, +) from enterprise.models import EnterpriseCourseEnrollment, EnterpriseEnrollmentSource from enterprise.settings.test import BRAZE_GROUPS_INVITATION_EMAIL_CAMPAIGN_ID, BRAZE_GROUPS_REMOVAL_EMAIL_CAMPAIGN_ID from enterprise.tasks import ( @@ -17,6 +20,7 @@ send_enterprise_email_notification, send_group_membership_invitation_notification, send_group_membership_removal_notification, + send_sso_configured_email, ) from enterprise.utils import serialize_notification_content from test_utils.factories import ( @@ -272,3 +276,24 @@ def test_send_group_membership_removal_notification(self, mock_braze_api_client, }, ) for recipient in mock_recipients] mock_braze_api_client().send_campaign_message.assert_has_calls(calls) + + @mock.patch('enterprise.tasks.BrazeAPIClient', return_value=mock.MagicMock()) + def test_sso_configuration_oauth_orchestration_email(self, mock_braze_client): + """ + Assert sso configuration calls Braze API with the correct arguments. + """ + mock_braze_client().create_recipient_no_external_id.return_value = ( + self.enterprise_customer.contact_email) + expected_trigger_properties = { + 'enterprise_customer_slug': self.enterprise_customer.slug, + 'enterprise_customer_name': self.enterprise_customer.name, + 'enterprise_sender_alias': self.enterprise_customer.sender_alias, + 'enterprise_contact_email': self.enterprise_customer.contact_email, + } + send_sso_configured_email(self.enterprise_customer.uuid) + call = [mock.call( + SSO_BRAZE_CAMPAIGN_ID, + recipients=[self.enterprise_customer.contact_email], + trigger_properties=expected_trigger_properties + )] + mock_braze_client().send_campaign_message.assert_has_calls(call) From 68e1bd302f57234fff9034725cda7d87443bc9bd Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Thu, 28 Mar 2024 15:12:28 +0000 Subject: [PATCH 03/17] fix: fix lint error --- tests/test_enterprise/test_tasks.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_enterprise/test_tasks.py b/tests/test_enterprise/test_tasks.py index 70a4ccafc6..94e44a91ad 100644 --- a/tests/test_enterprise/test_tasks.py +++ b/tests/test_enterprise/test_tasks.py @@ -10,9 +10,7 @@ from pytest import mark -from enterprise.constants import ( - SSO_BRAZE_CAMPAIGN_ID, -) +from enterprise.constants import SSO_BRAZE_CAMPAIGN_ID from enterprise.models import EnterpriseCourseEnrollment, EnterpriseEnrollmentSource from enterprise.settings.test import BRAZE_GROUPS_INVITATION_EMAIL_CAMPAIGN_ID, BRAZE_GROUPS_REMOVAL_EMAIL_CAMPAIGN_ID from enterprise.tasks import ( From 3aa7450be51b22a2e98a37d57388935ac31c13eb Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Tue, 2 Apr 2024 08:18:01 +0000 Subject: [PATCH 04/17] chore: refactor --- enterprise/api/v1/views/enterprise_group.py | 22 ++-- enterprise/tasks.py | 125 ++++++++++---------- tests/test_enterprise/api/test_views.py | 18 ++- tests/test_enterprise/test_tasks.py | 86 +++++++------- 4 files changed, 133 insertions(+), 118 deletions(-) diff --git a/enterprise/api/v1/views/enterprise_group.py b/enterprise/api/v1/views/enterprise_group.py index cf8e9f82cf..e41575c77d 100644 --- a/enterprise/api/v1/views/enterprise_group.py +++ b/enterprise/api/v1/views/enterprise_group.py @@ -161,9 +161,9 @@ def assign_learners(self, request, group_uuid): customer = group.enterprise_customer except models.EnterpriseGroup.DoesNotExist as exc: raise Http404 from exc - if requested_emails := request.POST.dict().get('learner_emails'): - budget_expiration = request.POST.dict().get('budget_expiration') - catalog_uuid = request.POST.dict().get('catalog_uuid') + if requested_emails := request.data.get('learner_emails'): + act_by_date = request.data.get('act_by_date') + catalog_uuid = request.data.get('catalog_uuid') total_records_processed = 0 total_existing_users_processed = 0 total_new_users_processed = 0 @@ -244,12 +244,13 @@ def assign_learners(self, request, group_uuid): 'new_learners': total_new_users_processed, 'existing_learners': total_existing_users_processed, } - if budget_expiration is not None and catalog_uuid is not None: - for membership in memberships: + membership_uuids = [membership.uuid for membership in memberships] + if act_by_date is not None and catalog_uuid is not None: + for membership_uuid_batch in utils.batch(membership_uuids, batch_size=200): send_group_membership_invitation_notification.delay( customer.uuid, - membership.uuid, - budget_expiration, + membership_uuid_batch, + act_by_date, catalog_uuid ) return Response(data, status=201) @@ -273,6 +274,8 @@ def remove_learners(self, request, group_uuid): Number of membership records removed """ try: + import pdb + pdb.set_trace() group = self.get_queryset().get(uuid=group_uuid) customer = group.enterprise_customer except models.EnterpriseGroup.DoesNotExist as exc: @@ -288,8 +291,9 @@ def remove_learners(self, request, group_uuid): group_q & (ecu_in_q | pecu_in_q), ) records_deleted += len(records_to_delete) - for record in records_to_delete: - send_group_membership_removal_notification.delay(customer.uuid, record.uuid) + records_to_delete_uuids = [record.uuid for record in records_to_delete] + for records_to_delete_uuids_batch in utils.batch(records_to_delete_uuids, batch_size=200): + send_group_membership_removal_notification.delay(customer.uuid, records_to_delete_uuids_batch) records_to_delete.delete() data = { 'records_deleted': records_deleted, diff --git a/enterprise/tasks.py b/enterprise/tasks.py index 28cbb1ed3d..c604b24677 100644 --- a/enterprise/tasks.py +++ b/enterprise/tasks.py @@ -213,8 +213,8 @@ def send_sso_configured_email( @set_code_owner_attribute def send_group_membership_invitation_notification( enterprise_customer_uuid, - group_membership_uuid, - budget_expiration, + membership_uuids, + act_by_date, catalog_uuid ): """ @@ -222,14 +222,11 @@ def send_group_membership_invitation_notification( Arguments: * enterprise_customer_uuid (string) - * group_membership_uuid (string) - * budget_expiration (datetime) + * memberships (list) + * act_by_date (datetime) * catalog_uuid (string) """ enterprise_customer = get_enterprise_customer(enterprise_customer_uuid) - group_membership = enterprise_group_membership_model().objects.filter( - uuid=group_membership_uuid - ).values().last() braze_client_instance = BrazeAPIClient() enterprise_catalog_client = EnterpriseCatalogApiClient() braze_trigger_properties = {} @@ -240,39 +237,42 @@ def send_group_membership_invitation_notification( braze_trigger_properties[ 'catalog_content_count' ] = enterprise_catalog_client.get_catalog_content_count(catalog_uuid) - braze_trigger_properties['budget_end_date'] = budget_expiration - if group_membership['enterprise_customer_user_id'] is None: - pending_enterprise_customer_user = pending_enterprise_customer_user_model().objects.get( - id=group_membership['pending_enterprise_customer_user_id']) - learner_email = pending_enterprise_customer_user.user_email - recipient = braze_client_instance.create_recipient_no_external_id( - learner_email, - ) - # We need an alias record to exist in Braze before - # sending to any previously-unidentified users. - braze_client_instance.create_braze_alias( - [learner_email], - ENTERPRISE_BRAZE_ALIAS_LABEL, - ) - else: - enterprise_customer_user = enterprise_customer_user_model().objects.get( - user_id=group_membership['enterprise_customer_user_id']) - learner_email = enterprise_customer_user.user_email - recipient = braze_client_instance.create_recipient( - user_email=learner_email, - lms_user_id=enterprise_customer_user.user_id, - ) + braze_trigger_properties['act_by_date'] = act_by_date + pecu_emails = [] + ecus = [] + for membership_uuid in membership_uuids: + group_membership = enterprise_group_membership_model().objects.filter( + uuid=membership_uuid + ) + if group_membership[0].pending_enterprise_customer_user is not None: + pecu_emails.append(group_membership[0].pending_enterprise_customer_user.user_email) + else: + ecus.append({ + 'user_email': group_membership[0].enterprise_customer_user.user_email, + 'user_id': group_membership[0].enterprise_customer_user.user_id + }) + recipients = [] + for pecu_email in pecu_emails: + recipients.append(braze_client_instance.create_recipient_no_external_id(pecu_email)) + braze_client_instance.create_braze_alias( + [pecu_emails], + ENTERPRISE_BRAZE_ALIAS_LABEL, + ) + for ecu in ecus: + recipients.append(braze_client_instance.create_recipient( + user_email=ecu['user_email'], + lms_user_id=ecu['user_id'])) try: braze_client_instance.send_campaign_message( settings.BRAZE_GROUPS_INVITATION_EMAIL_CAMPAIGN_ID, - recipients=[recipient], + recipients=recipients, trigger_properties=braze_trigger_properties, ) except BrazeClientError as exc: message = ( "Groups learner invitation email could not be sent " - f"to {learner_email} for enterprise {enterprise_customer_name}." + f"to {recipients} for enterprise {enterprise_customer_name}." ) LOGGER.exception(message) raise exc @@ -280,7 +280,7 @@ def send_group_membership_invitation_notification( @shared_task @set_code_owner_attribute -def send_group_membership_removal_notification(enterprise_customer_uuid, group_membership_uuid): +def send_group_membership_removal_notification(enterprise_customer_uuid, membership_uuids): """ Send braze email notification when learner is removed from a group. @@ -289,51 +289,48 @@ def send_group_membership_removal_notification(enterprise_customer_uuid, group_m * group_membership_uuid (string) """ enterprise_customer = get_enterprise_customer(enterprise_customer_uuid) - group_membership = enterprise_group_membership_model().objects.filter( - uuid=group_membership_uuid - ).values().last() braze_client_instance = BrazeAPIClient() braze_trigger_properties = {} - braze_campaign_id = settings.BRAZE_GROUPS_REMOVAL_EMAIL_CAMPAIGN_ID contact_email = enterprise_customer.contact_email enterprise_customer_name = enterprise_customer.name braze_trigger_properties['contact_admin_link'] = braze_client_instance.generate_mailto_link(contact_email) braze_trigger_properties['enterprise_customer_name'] = enterprise_customer_name - - if group_membership['enterprise_customer_user_id'] is None: - pending_enterprise_customer_user = pending_enterprise_customer_user_model().objects.get( - id=group_membership['pending_enterprise_customer_user_id'] + pecu_emails = [] + ecus = [] + for membership_uuid in membership_uuids: + group_membership = enterprise_group_membership_model().objects.filter( + uuid=membership_uuid ) - learner_email = pending_enterprise_customer_user.user_email - recipient = braze_client_instance.create_recipient_no_external_id( - learner_email, - ) - # We need an alias record to exist in Braze before - # sending to any previously-unidentified users. - braze_client_instance.create_braze_alias( - [learner_email], - ENTERPRISE_BRAZE_ALIAS_LABEL, - ) - else: - enterprise_customer_user = enterprise_customer_user_model().objects.get( - user_id=group_membership['enterprise_customer_user_id'] - ) - learner_email = enterprise_customer_user.user_email - recipient = braze_client_instance.create_recipient( - user_email=learner_email, - lms_user_id=enterprise_customer_user.user_id, - ) - + if group_membership[0].pending_enterprise_customer_user is not None: + pecu_emails.append(group_membership[0].pending_enterprise_customer_user.user_email) + else: + ecus.append({ + 'user_email': group_membership[0].enterprise_customer_user.user_email, + 'user_id': group_membership[0].enterprise_customer_user.user_id + }) + + recipients = [] + for pecu_email in pecu_emails: + recipients.append(braze_client_instance.create_recipient_no_external_id(pecu_email)) + braze_client_instance.create_braze_alias( + [pecu_emails], + ENTERPRISE_BRAZE_ALIAS_LABEL, + ) + for ecu in ecus: + recipients.append(braze_client_instance.create_recipient( + user_email=ecu['user_email'], + lms_user_id=ecu['user_id'] + )) try: braze_client_instance.send_campaign_message( - braze_campaign_id, - recipients=[recipient], + settings.BRAZE_GROUPS_REMOVAL_EMAIL_CAMPAIGN_ID, + recipients=recipients, trigger_properties=braze_trigger_properties, ) except BrazeClientError as exc: message = ( - "Groups learner removal email could not be sent " - f"to {learner_email} for enterprise {enterprise_customer_name}." + "Groups learner invitation email could not be sent " + f"to {recipients} for enterprise {enterprise_customer_name}." ) LOGGER.exception(message) raise exc diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index f76feada57..d0f058cd30 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -7658,11 +7658,11 @@ def test_successful_assign_learners_to_group(self, mock_send_group_membership_in existing_emails = ",".join([(UserFactory().email) for _ in range(10)]) new_emails = ",".join([(f"email_{x}@example.com") for x in range(10)]) - budget_expiration = datetime.now() - catalog_uuid = uuid.uuid4() + act_by_date = datetime.now().strftime("%m-%d-%Y, %H:%M:%S") + catalog_uuid = str(uuid.uuid4()) request_data = { 'learner_emails': f"{new_emails},{existing_emails}", - 'budget_expiration': budget_expiration, + 'act_by_date': act_by_date, 'catalog_uuid': catalog_uuid, } response = self.client.post(url, data=request_data) @@ -7680,7 +7680,15 @@ def test_successful_assign_learners_to_group(self, mock_send_group_membership_in enterprise_customer_user__isnull=True ) ) == 10 - assert mock_send_group_membership_invitation_notification.call_count == 20 + assert mock_send_group_membership_invitation_notification.call_count == 1 + group_uuids = list(EnterpriseGroupMembership.objects.filter(group=self.group_2).values_list('uuid', flat=True)) + mock_send_group_membership_invitation_notification.assert_has_calls([ + mock.call(self.enterprise_customer.uuid, + list(reversed(group_uuids)), + act_by_date, + catalog_uuid, + ), + ], any_order=True) def test_remove_learners_404(self): """ @@ -7753,7 +7761,7 @@ def test_successful_remove_learners_from_group(self, mock_send_group_membership_ response = self.client.post(url, data=request_data) assert response.status_code == 200 assert response.data == {'records_deleted': 10} - assert mock_send_group_membership_removal_notification.call_count == 10 + assert mock_send_group_membership_removal_notification.call_count == 1 def test_remove_learners_from_group_only_removes_from_specified_group(self): """ diff --git a/tests/test_enterprise/test_tasks.py b/tests/test_enterprise/test_tasks.py index 94e44a91ad..f84d2eca64 100644 --- a/tests/test_enterprise/test_tasks.py +++ b/tests/test_enterprise/test_tasks.py @@ -11,7 +11,7 @@ from pytest import mark from enterprise.constants import SSO_BRAZE_CAMPAIGN_ID -from enterprise.models import EnterpriseCourseEnrollment, EnterpriseEnrollmentSource +from enterprise.models import EnterpriseCourseEnrollment, EnterpriseEnrollmentSource, EnterpriseGroupMembership from enterprise.settings.test import BRAZE_GROUPS_INVITATION_EMAIL_CAMPAIGN_ID, BRAZE_GROUPS_REMOVAL_EMAIL_CAMPAIGN_ID from enterprise.tasks import ( create_enterprise_enrollment, @@ -52,18 +52,6 @@ def setUp(self): user_id=self.user.id, enterprise_customer=self.enterprise_customer, ) - self.enterprise_group_memberships = [] - - self.enterprise_group_memberships.append(EnterpriseGroupMembershipFactory( - group=self.enterprise_group, - pending_enterprise_customer_user=None, - enterprise_customer_user__enterprise_customer=self.enterprise_customer, - )) - self.enterprise_group_memberships.append(EnterpriseGroupMembershipFactory( - group=self.enterprise_group, - pending_enterprise_customer_user=self.pending_enterprise_customer_user, - enterprise_customer_user=None, - )) super().setUp() @mock.patch('enterprise.models.EnterpriseCustomer.catalog_contains_course') @@ -213,66 +201,84 @@ def test_send_group_membership_invitation_notification(self, mock_braze_api_clie """ Verify send_group_membership_invitation_notification hits braze client with expected args """ + EnterpriseGroupMembershipFactory( + group=self.enterprise_group, + pending_enterprise_customer_user=PendingEnterpriseCustomerUserFactory(), + enterprise_customer_user=None, + ) + EnterpriseGroupMembershipFactory( + group=self.enterprise_group, + pending_enterprise_customer_user=None, + enterprise_customer_user__enterprise_customer=self.enterprise_customer, + activated_at=datetime.now() + ) admin_email = 'edx@example.org' - mock_recipients = [{'external_user_id': 1}, self.pending_enterprise_customer_user.user_email] + mock_recipients = [self.pending_enterprise_customer_user.user_email, + mock_braze_api_client().create_recipient.return_value] mock_catalog_content_count = 5 mock_admin_mailto = f'mailto:{admin_email}' - mock_braze_api_client().create_recipient.return_value = mock_recipients[0] mock_braze_api_client().generate_mailto_link.return_value = mock_admin_mailto mock_braze_api_client().create_recipient_no_external_id.return_value = ( self.pending_enterprise_customer_user.user_email) mock_enterprise_catalog_client().get_catalog_content_count.return_value = ( mock_catalog_content_count) - budget_expiration = datetime.now() + act_by_date = datetime.now() catalog_uuid = uuid.uuid4() - for membership in self.enterprise_group_memberships: - send_group_membership_invitation_notification( - self.enterprise_customer.uuid, - membership.uuid, - budget_expiration, - catalog_uuid, - ) + membership_uuids = EnterpriseGroupMembership.objects.values_list('uuid', flat=True) + send_group_membership_invitation_notification( + self.enterprise_customer.uuid, + membership_uuids, + act_by_date, + catalog_uuid, + ) calls = [mock.call( BRAZE_GROUPS_INVITATION_EMAIL_CAMPAIGN_ID, - recipients=[recipient], + recipients=mock_recipients, trigger_properties={ 'contact_admin_link': mock_admin_mailto, 'enterprise_customer_name': self.enterprise_customer.name, 'catalog_content_count': mock_catalog_content_count, - 'budget_end_date': budget_expiration, + 'act_by_date': act_by_date, }, - ) for recipient in mock_recipients] + )] mock_braze_api_client().send_campaign_message.assert_has_calls(calls) - @mock.patch('enterprise.tasks.EnterpriseCatalogApiClient', return_value=mock.MagicMock()) @mock.patch('enterprise.tasks.BrazeAPIClient', return_value=mock.MagicMock()) - def test_send_group_membership_removal_notification(self, mock_braze_api_client, mock_enterprise_catalog_client): + def test_send_group_membership_removal_notification(self, mock_braze_api_client): """ Verify send_group_membership_removal_notification hits braze client with expected args """ + EnterpriseGroupMembershipFactory( + group=self.enterprise_group, + pending_enterprise_customer_user=PendingEnterpriseCustomerUserFactory(), + enterprise_customer_user=None, + ) + EnterpriseGroupMembershipFactory( + group=self.enterprise_group, + pending_enterprise_customer_user=None, + enterprise_customer_user__enterprise_customer=self.enterprise_customer, + activated_at=datetime.now() + ) admin_email = 'edx@example.org' - mock_recipients = [{'external_user_id': 1}, self.pending_enterprise_customer_user.user_email] - mock_catalog_content_count = 5 + mock_recipients = [self.pending_enterprise_customer_user.user_email, + mock_braze_api_client().create_recipient.return_value] mock_admin_mailto = f'mailto:{admin_email}' - mock_braze_api_client().create_recipient.return_value = mock_recipients[0] mock_braze_api_client().generate_mailto_link.return_value = mock_admin_mailto mock_braze_api_client().create_recipient_no_external_id.return_value = ( self.pending_enterprise_customer_user.user_email) - mock_enterprise_catalog_client().get_catalog_content_count.return_value = ( - mock_catalog_content_count) - for membership in self.enterprise_group_memberships: - send_group_membership_removal_notification( - self.enterprise_customer.uuid, - membership.uuid, - ) + membership_uuids = EnterpriseGroupMembership.objects.values_list('uuid', flat=True) + send_group_membership_removal_notification( + self.enterprise_customer.uuid, + membership_uuids, + ) calls = [mock.call( BRAZE_GROUPS_REMOVAL_EMAIL_CAMPAIGN_ID, - recipients=[recipient], + recipients=mock_recipients, trigger_properties={ 'contact_admin_link': mock_admin_mailto, 'enterprise_customer_name': self.enterprise_customer.name, }, - ) for recipient in mock_recipients] + )] mock_braze_api_client().send_campaign_message.assert_has_calls(calls) @mock.patch('enterprise.tasks.BrazeAPIClient', return_value=mock.MagicMock()) From 7b26bcfd8d76921118369fcf43e8f9cd07c6ffa7 Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Tue, 2 Apr 2024 16:12:36 +0000 Subject: [PATCH 05/17] chore: removed pdb --- enterprise/api/v1/views/enterprise_group.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/enterprise/api/v1/views/enterprise_group.py b/enterprise/api/v1/views/enterprise_group.py index e41575c77d..b7f5ab802a 100644 --- a/enterprise/api/v1/views/enterprise_group.py +++ b/enterprise/api/v1/views/enterprise_group.py @@ -274,8 +274,6 @@ def remove_learners(self, request, group_uuid): Number of membership records removed """ try: - import pdb - pdb.set_trace() group = self.get_queryset().get(uuid=group_uuid) customer = group.enterprise_customer except models.EnterpriseGroup.DoesNotExist as exc: From 46d0b6c10163a26e4622396ab29608218feb7760 Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Wed, 3 Apr 2024 23:56:51 +0000 Subject: [PATCH 06/17] fix: add serializer for request data --- enterprise/api/v1/serializers.py | 12 ++++++ enterprise/api/v1/views/enterprise_group.py | 48 ++++++++++++++++----- enterprise/tasks.py | 3 +- tests/test_enterprise/api/test_views.py | 17 ++++---- 4 files changed, 60 insertions(+), 20 deletions(-) diff --git a/enterprise/api/v1/serializers.py b/enterprise/api/v1/serializers.py index 41968b3fe8..77b055fe0d 100644 --- a/enterprise/api/v1/serializers.py +++ b/enterprise/api/v1/serializers.py @@ -1701,3 +1701,15 @@ class Meta: client_secret = serializers.CharField(read_only=True, default=generate_client_secret()) redirect_uris = serializers.CharField(required=False) updated = serializers.DateTimeField(required=False, read_only=True) + + +class EnterpriseGroupAssignLearnersRequestQuerySerializer(serializers.Serializer): + """ + Serializer for the Enterprise Group Assign Learners endpoint query params + """ + catalog_uuid = serializers.UUIDField(required=False) + act_by_date = serializers.DateTimeField(required=False) + learner_emails = serializers.ListField( + child=serializers.EmailField(required=False), + allow_empty=True + ) diff --git a/enterprise/api/v1/views/enterprise_group.py b/enterprise/api/v1/views/enterprise_group.py index b7f5ab802a..ad8162503f 100644 --- a/enterprise/api/v1/views/enterprise_group.py +++ b/enterprise/api/v1/views/enterprise_group.py @@ -95,13 +95,19 @@ def create(self, request, *args, **kwargs): return super().create(request, *args, **kwargs) @action(detail=True, methods=['get']) - def get_learners(self, *args, **kwargs): + def get_learners(self, request, *args, **kwargs): """ - Endpoint Location: GET api/v1/enterprise-group//learners/ + Endpoint Location to return all learners: GET api/v1/enterprise-group//learners/ + + Endpoint Location to return pending learners: + GET api/v1/enterprise-group//learners?pending_users_only=true Request Arguments: - ``group_uuid`` (URL location, required): The uuid of the group from which learners should be listed. + Optional query params: + - ``pending_users_only`` (string, optional): Specify that results should only contain pending learners + Returns: Paginated list of learners that are associated with the enterprise group uuid:: { @@ -110,10 +116,16 @@ def get_learners(self, *args, **kwargs): 'previous': null, 'results': [ { - 'learner_uuid': 'enterprise_customer_user_id', - 'pending_learner_id': 'pending_enterprise_customer_user_id', - 'enterprise_group_membership_uuid': 'enterprise_group_membership_uuid', - 'enterprise_customer': EnterpriseCustomerSerializer + 'learner_id': integer or None, + 'pending_learner_id': integer or None, + 'enterprise_group_membership_uuid': UUID, + 'enterprise_customer': EnterpriseCustomerSerializer, + 'member_details': { + 'user_email': string, + 'user_name': string, + }, + 'recent_action': string, + 'member_status': string, }, ], } @@ -121,8 +133,7 @@ def get_learners(self, *args, **kwargs): """ group_uuid = kwargs.get('group_uuid') - # GET api/v1/enterprise-group//learners?filter_for_pecus=true - filter_for_pecus = self.request.query_params.get('filter_for_pecus', None) + filter_for_pecus = request.query_params.get('pending_users_only', None) try: group_object = self.get_queryset().get(uuid=group_uuid) members = group_object.get_all_learners(filter_for_pecus) @@ -161,13 +172,28 @@ def assign_learners(self, request, group_uuid): customer = group.enterprise_customer except models.EnterpriseGroup.DoesNotExist as exc: raise Http404 from exc + if requested_emails := request.data.get('learner_emails'): - act_by_date = request.data.get('act_by_date') - catalog_uuid = request.data.get('catalog_uuid') + request_data = {} + if act_by_date := request.data.get('act_by_date'): + request_data['act_by_date'] = act_by_date + if catalog_uuid := request.data.get('catalog_uuid'): + request_data['catalog_uuid'] = catalog_uuid + request_data['learner_emails'] = requested_emails.split(',') + + param_serializers = serializers.EnterpriseGroupAssignLearnersRequestQuerySerializer( + data=request_data + ) + param_serializers.is_valid() + if not param_serializers.is_valid(): + return Response(param_serializers.errors, status=400) + + act_by_date = param_serializers.validated_data.get('act_by_date') + catalog_uuid = param_serializers.validated_data.get('catalog_uuid') total_records_processed = 0 total_existing_users_processed = 0 total_new_users_processed = 0 - for user_email_batch in utils.batch(requested_emails.rstrip(',').split(',')[: 1000], batch_size=200): + for user_email_batch in utils.batch(requested_emails[: 1000], batch_size=200): user_emails_to_create = [] memberships_to_create = [] # ecus: enterprise customer users diff --git a/enterprise/tasks.py b/enterprise/tasks.py index c604b24677..143b37cc69 100644 --- a/enterprise/tasks.py +++ b/enterprise/tasks.py @@ -2,6 +2,7 @@ Django tasks. """ +from datetime import datetime from logging import getLogger from braze.exceptions import BrazeClientError @@ -238,7 +239,7 @@ def send_group_membership_invitation_notification( 'catalog_content_count' ] = enterprise_catalog_client.get_catalog_content_count(catalog_uuid) - braze_trigger_properties['act_by_date'] = act_by_date + braze_trigger_properties['act_by_date'] = datetime.strptime(act_by_date, "%B %d, %Y") pecu_emails = [] ecus = [] for membership_uuid in membership_uuids: diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index d0f058cd30..3c09803725 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -14,6 +14,7 @@ from urllib.parse import parse_qs, urlencode, urljoin, urlsplit, urlunsplit import ddt +import pytz import responses from edx_toggles.toggles.testutils import override_waffle_flag from faker import Faker @@ -7486,19 +7487,19 @@ def test_successful_list_learners(self): response = self.client.get(url) assert response.json()['count'] == 10 - # url: 'http://testserver/enterprise/api/v1/enterprise_group//learners/?filter_for_pecus=true' + # url: 'http://testserver/enterprise/api/v1/enterprise_group//learners/?pending_users_only=true' # verify filtered response for only pending users self.enterprise_group_memberships.append(EnterpriseGroupMembershipFactory( group=self.group_1, pending_enterprise_customer_user=self.pending_enterprise_customer_user, enterprise_customer_user=None )) - filter_for_pecus_url = settings.TEST_SERVER + reverse( + pending_users_only_url = settings.TEST_SERVER + reverse( 'enterprise-group-learners', kwargs={'group_uuid': self.group_1.uuid}, - ) + '/?filter_for_pecus=true' - filter_for_pecus_response = self.client.get(filter_for_pecus_url) - expected_filtered_for_pecus_response = { + ) + '/?pending_users_only=true' + pending_users_only_response = self.client.get(pending_users_only_url) + expected_pending_users_only_response = { 'count': 1, 'next': None, 'previous': None, @@ -7513,7 +7514,7 @@ def test_successful_list_learners(self): }, ], } - assert filter_for_pecus_response.json()['count'] == expected_filtered_for_pecus_response['count'] + assert pending_users_only_response.json()['count'] == expected_pending_users_only_response['count'] def test_group_uuid_not_found(self): """ @@ -7658,8 +7659,8 @@ def test_successful_assign_learners_to_group(self, mock_send_group_membership_in existing_emails = ",".join([(UserFactory().email) for _ in range(10)]) new_emails = ",".join([(f"email_{x}@example.com") for x in range(10)]) - act_by_date = datetime.now().strftime("%m-%d-%Y, %H:%M:%S") - catalog_uuid = str(uuid.uuid4()) + act_by_date = datetime.now(pytz.UTC) + catalog_uuid = uuid.uuid4() request_data = { 'learner_emails': f"{new_emails},{existing_emails}", 'act_by_date': act_by_date, From 1ab79361a6e77c1519908b5857119ce63daab706 Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Wed, 3 Apr 2024 23:58:32 +0000 Subject: [PATCH 07/17] fix: require emails --- enterprise/api/v1/serializers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/enterprise/api/v1/serializers.py b/enterprise/api/v1/serializers.py index 77b055fe0d..fdbeabb402 100644 --- a/enterprise/api/v1/serializers.py +++ b/enterprise/api/v1/serializers.py @@ -1710,6 +1710,6 @@ class EnterpriseGroupAssignLearnersRequestQuerySerializer(serializers.Serializer catalog_uuid = serializers.UUIDField(required=False) act_by_date = serializers.DateTimeField(required=False) learner_emails = serializers.ListField( - child=serializers.EmailField(required=False), - allow_empty=True + child=serializers.EmailField(required=True), + allow_empty=False ) From 3d1aa71ebb42de3bf29d333cf0dd0b58315867d6 Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Thu, 4 Apr 2024 23:05:10 +0000 Subject: [PATCH 08/17] chore: group membership serializer to remove enterprise customer and updated tests --- enterprise/api/v1/serializers.py | 8 +++----- enterprise/api/v1/views/enterprise_group.py | 17 +++++++---------- enterprise/tasks.py | 3 +-- tests/test_enterprise/api/test_views.py | 17 ++++++----------- tests/test_enterprise/test_tasks.py | 4 ++-- 5 files changed, 19 insertions(+), 30 deletions(-) diff --git a/enterprise/api/v1/serializers.py b/enterprise/api/v1/serializers.py index fdbeabb402..4298ab31ad 100644 --- a/enterprise/api/v1/serializers.py +++ b/enterprise/api/v1/serializers.py @@ -599,7 +599,6 @@ class EnterpriseGroupMembershipSerializer(serializers.ModelSerializer): learner_id = serializers.IntegerField(source='enterprise_customer_user.id', allow_null=True) pending_learner_id = serializers.IntegerField(source='pending_enterprise_customer_user.id', allow_null=True) enterprise_group_membership_uuid = serializers.UUIDField(source='uuid', allow_null=True, read_only=True) - enterprise_customer = EnterpriseCustomerSerializer(source='group.enterprise_customer', read_only=True) member_details = serializers.SerializerMethodField() recent_action = serializers.SerializerMethodField() @@ -614,7 +613,6 @@ class Meta: 'member_details', 'recent_action', 'member_status', - 'enterprise_customer' ) def get_member_details(self, obj): @@ -1703,12 +1701,12 @@ class Meta: updated = serializers.DateTimeField(required=False, read_only=True) -class EnterpriseGroupAssignLearnersRequestQuerySerializer(serializers.Serializer): +class EnterpriseGroupAssignLearnersRequestDataSerializer(serializers.Serializer): """ Serializer for the Enterprise Group Assign Learners endpoint query params """ - catalog_uuid = serializers.UUIDField(required=False) - act_by_date = serializers.DateTimeField(required=False) + catalog_uuid = serializers.UUIDField(required=False, allow_null=True) + act_by_date = serializers.DateTimeField(required=False, allow_null=True) learner_emails = serializers.ListField( child=serializers.EmailField(required=True), allow_empty=False diff --git a/enterprise/api/v1/views/enterprise_group.py b/enterprise/api/v1/views/enterprise_group.py index ad8162503f..f0ed125cf8 100644 --- a/enterprise/api/v1/views/enterprise_group.py +++ b/enterprise/api/v1/views/enterprise_group.py @@ -119,7 +119,6 @@ def get_learners(self, request, *args, **kwargs): 'learner_id': integer or None, 'pending_learner_id': integer or None, 'enterprise_group_membership_uuid': UUID, - 'enterprise_customer': EnterpriseCustomerSerializer, 'member_details': { 'user_email': string, 'user_name': string, @@ -172,16 +171,13 @@ def assign_learners(self, request, group_uuid): customer = group.enterprise_customer except models.EnterpriseGroup.DoesNotExist as exc: raise Http404 from exc - - if requested_emails := request.data.get('learner_emails'): + if requested_emails := request.data.getlist('learner_emails'): request_data = {} - if act_by_date := request.data.get('act_by_date'): - request_data['act_by_date'] = act_by_date - if catalog_uuid := request.data.get('catalog_uuid'): - request_data['catalog_uuid'] = catalog_uuid - request_data['learner_emails'] = requested_emails.split(',') + request_data['act_by_date'] = request.data.get('act_by_date') + request_data['catalog_uuid'] = request.data.get('catalog_uuid') + request_data['learner_emails'] = requested_emails - param_serializers = serializers.EnterpriseGroupAssignLearnersRequestQuerySerializer( + param_serializers = serializers.EnterpriseGroupAssignLearnersRequestDataSerializer( data=request_data ) param_serializers.is_valid() @@ -190,10 +186,11 @@ def assign_learners(self, request, group_uuid): act_by_date = param_serializers.validated_data.get('act_by_date') catalog_uuid = param_serializers.validated_data.get('catalog_uuid') + learner_emails = param_serializers.validated_data.get('learner_emails') total_records_processed = 0 total_existing_users_processed = 0 total_new_users_processed = 0 - for user_email_batch in utils.batch(requested_emails[: 1000], batch_size=200): + for user_email_batch in utils.batch(learner_emails[: 1000], batch_size=200): user_emails_to_create = [] memberships_to_create = [] # ecus: enterprise customer users diff --git a/enterprise/tasks.py b/enterprise/tasks.py index 143b37cc69..e2905602d9 100644 --- a/enterprise/tasks.py +++ b/enterprise/tasks.py @@ -2,7 +2,6 @@ Django tasks. """ -from datetime import datetime from logging import getLogger from braze.exceptions import BrazeClientError @@ -239,7 +238,7 @@ def send_group_membership_invitation_notification( 'catalog_content_count' ] = enterprise_catalog_client.get_catalog_content_count(catalog_uuid) - braze_trigger_properties['act_by_date'] = datetime.strptime(act_by_date, "%B %d, %Y") + braze_trigger_properties['act_by_date'] = act_by_date.strftime('%B %d, %Y') pecu_emails = [] ecus = [] for membership_uuid in membership_uuids: diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index 3c09803725..ef1b1d6737 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -7656,13 +7656,12 @@ def test_successful_assign_learners_to_group(self, mock_send_group_membership_in 'enterprise-group-assign-learners', kwargs={'group_uuid': self.group_2.uuid}, ) - - existing_emails = ",".join([(UserFactory().email) for _ in range(10)]) - new_emails = ",".join([(f"email_{x}@example.com") for x in range(10)]) + existing_emails = [UserFactory().email for _ in range(10)] + new_emails = [f"email_{x}@example.com" for x in range(10)] act_by_date = datetime.now(pytz.UTC) catalog_uuid = uuid.uuid4() request_data = { - 'learner_emails': f"{new_emails},{existing_emails}", + 'learner_emails': existing_emails + new_emails, 'act_by_date': act_by_date, 'catalog_uuid': catalog_uuid, } @@ -7682,14 +7681,10 @@ def test_successful_assign_learners_to_group(self, mock_send_group_membership_in ) ) == 10 assert mock_send_group_membership_invitation_notification.call_count == 1 - group_uuids = list(EnterpriseGroupMembership.objects.filter(group=self.group_2).values_list('uuid', flat=True)) + group_uuids = list(reversed(list( + EnterpriseGroupMembership.objects.filter(group=self.group_2).values_list('uuid', flat=True)))) mock_send_group_membership_invitation_notification.assert_has_calls([ - mock.call(self.enterprise_customer.uuid, - list(reversed(group_uuids)), - act_by_date, - catalog_uuid, - ), - ], any_order=True) + mock.call(self.enterprise_customer.uuid, group_uuids, act_by_date, catalog_uuid)], any_order=True) def test_remove_learners_404(self): """ diff --git a/tests/test_enterprise/test_tasks.py b/tests/test_enterprise/test_tasks.py index f84d2eca64..ff9e8e84a0 100644 --- a/tests/test_enterprise/test_tasks.py +++ b/tests/test_enterprise/test_tasks.py @@ -222,7 +222,7 @@ def test_send_group_membership_invitation_notification(self, mock_braze_api_clie self.pending_enterprise_customer_user.user_email) mock_enterprise_catalog_client().get_catalog_content_count.return_value = ( mock_catalog_content_count) - act_by_date = datetime.now() + act_by_date = datetime.today() catalog_uuid = uuid.uuid4() membership_uuids = EnterpriseGroupMembership.objects.values_list('uuid', flat=True) send_group_membership_invitation_notification( @@ -238,7 +238,7 @@ def test_send_group_membership_invitation_notification(self, mock_braze_api_clie 'contact_admin_link': mock_admin_mailto, 'enterprise_customer_name': self.enterprise_customer.name, 'catalog_content_count': mock_catalog_content_count, - 'act_by_date': act_by_date, + 'act_by_date': act_by_date.strftime('%B %d, %Y'), }, )] mock_braze_api_client().send_campaign_message.assert_has_calls(calls) From a2e87d86e07e410f4e04f31fb3ba96f4124cf6e1 Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Thu, 4 Apr 2024 23:31:48 +0000 Subject: [PATCH 09/17] fix: failing tests --- enterprise/api/v1/views/enterprise_group.py | 4 +++- tests/test_enterprise/api/test_views.py | 12 +----------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/enterprise/api/v1/views/enterprise_group.py b/enterprise/api/v1/views/enterprise_group.py index f0ed125cf8..9358ee8aea 100644 --- a/enterprise/api/v1/views/enterprise_group.py +++ b/enterprise/api/v1/views/enterprise_group.py @@ -171,7 +171,9 @@ def assign_learners(self, request, group_uuid): customer = group.enterprise_customer except models.EnterpriseGroup.DoesNotExist as exc: raise Http404 from exc - if requested_emails := request.data.getlist('learner_emails'): + import pdb + pdb.set_trace() + if requested_emails := request.POST.getlist('learner_emails'): request_data = {} request_data['act_by_date'] = request.data.get('act_by_date') request_data['catalog_uuid'] = request.data.get('catalog_uuid') diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index ef1b1d6737..8fddeed241 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -7418,9 +7418,6 @@ def test_successful_list_learners(self): 'learner_id': member_user.id, 'pending_learner_id': None, 'enterprise_group_membership_uuid': str(self.enterprise_group_memberships[i].uuid), - 'enterprise_customer': { - 'name': self.enterprise_customer.name, - }, 'member_details': { 'user_name': member_user.name, 'user_email': member_user.user_email @@ -7442,8 +7439,6 @@ def test_successful_list_learners(self): expected_response['results'][i]['pending_learner_id']) assert (response.json()['results'][i]['enterprise_group_membership_uuid'] == expected_response['results'][i]['enterprise_group_membership_uuid']) - assert (response.json()['results'][i]['enterprise_customer']['name'] - == expected_response['results'][i]['enterprise_customer']['name']) # verify page 2 of paginated response url_page_2 = settings.TEST_SERVER + reverse( @@ -7461,9 +7456,6 @@ def test_successful_list_learners(self): 'learner_id': user.id, 'pending_learner_id': None, 'enterprise_group_membership_uuid': str(self.enterprise_group_memberships[0].uuid), - 'enterprise_customer': { - 'name': self.enterprise_customer.name, - }, 'member_details': { 'user_name': user.name, 'user_email': user.user_email @@ -7481,8 +7473,6 @@ def test_successful_list_learners(self): expected_response_page_2['results'][0]['pending_learner_id']) assert (page_2_response.json()['results'][0]['enterprise_group_membership_uuid'] == expected_response_page_2['results'][0]['enterprise_group_membership_uuid']) - assert (page_2_response.json()['results'][0]['enterprise_customer']['name'] - == expected_response_page_2['results'][0]['enterprise_customer']['name']) self.enterprise_group_memberships[0].delete() response = self.client.get(url) assert response.json()['count'] == 10 @@ -7820,7 +7810,7 @@ def test_group_assign_realized_learner_adds_activated_at(self): 'enterprise-group-assign-learners', kwargs={'group_uuid': self.group_2.uuid}, ) - request_data = {'learner_emails': f"{UserFactory().email},email@example.com"} + request_data = {'learner_emails': [UserFactory().email, 'email@example.com']} self.client.post(url, data=request_data) membership = EnterpriseGroupMembership.objects.filter( group=self.group_2, From 4d23516e97ec757cf1df288cc2a8defffe5b7432 Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Thu, 4 Apr 2024 23:39:09 +0000 Subject: [PATCH 10/17] chore: removed pdb --- enterprise/api/v1/views/enterprise_group.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/enterprise/api/v1/views/enterprise_group.py b/enterprise/api/v1/views/enterprise_group.py index 9358ee8aea..d419656abb 100644 --- a/enterprise/api/v1/views/enterprise_group.py +++ b/enterprise/api/v1/views/enterprise_group.py @@ -171,8 +171,6 @@ def assign_learners(self, request, group_uuid): customer = group.enterprise_customer except models.EnterpriseGroup.DoesNotExist as exc: raise Http404 from exc - import pdb - pdb.set_trace() if requested_emails := request.POST.getlist('learner_emails'): request_data = {} request_data['act_by_date'] = request.data.get('act_by_date') From f32642b80d285b09162f0c8d2f339c0c1f9aad66 Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Thu, 4 Apr 2024 23:53:36 +0000 Subject: [PATCH 11/17] fix: linter --- enterprise/api/v1/views/enterprise_group.py | 1 - 1 file changed, 1 deletion(-) diff --git a/enterprise/api/v1/views/enterprise_group.py b/enterprise/api/v1/views/enterprise_group.py index d419656abb..2d641b323b 100644 --- a/enterprise/api/v1/views/enterprise_group.py +++ b/enterprise/api/v1/views/enterprise_group.py @@ -183,7 +183,6 @@ def assign_learners(self, request, group_uuid): param_serializers.is_valid() if not param_serializers.is_valid(): return Response(param_serializers.errors, status=400) - act_by_date = param_serializers.validated_data.get('act_by_date') catalog_uuid = param_serializers.validated_data.get('catalog_uuid') learner_emails = param_serializers.validated_data.get('learner_emails') From f02998157b71699b2c9e5f8036a61a450a39021a Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Fri, 5 Apr 2024 17:16:54 +0000 Subject: [PATCH 12/17] fix: refactored task and enterprise_group --- enterprise/api/v1/serializers.py | 2 +- enterprise/api/v1/views/enterprise_group.py | 189 +++++++++----------- enterprise/tasks.py | 28 ++- tests/test_enterprise/api/test_views.py | 8 +- 4 files changed, 106 insertions(+), 121 deletions(-) diff --git a/enterprise/api/v1/serializers.py b/enterprise/api/v1/serializers.py index 3fa6d15f50..743a4b6ddb 100644 --- a/enterprise/api/v1/serializers.py +++ b/enterprise/api/v1/serializers.py @@ -1706,7 +1706,7 @@ class EnterpriseGroupAssignLearnersRequestDataSerializer(serializers.Serializer) Serializer for the Enterprise Group Assign Learners endpoint query params """ catalog_uuid = serializers.UUIDField(required=False, allow_null=True) - act_by_date = serializers.DateTimeField(required=False, allow_null=True) + act_by_date = serializers.DateTimeField(required=False, allow_null=True, format='%Y-%m-%dT%H:%M:%SZ') learner_emails = serializers.ListField( child=serializers.EmailField(required=True), allow_empty=False diff --git a/enterprise/api/v1/views/enterprise_group.py b/enterprise/api/v1/views/enterprise_group.py index 2d641b323b..3ea1b9ea05 100644 --- a/enterprise/api/v1/views/enterprise_group.py +++ b/enterprise/api/v1/views/enterprise_group.py @@ -171,112 +171,101 @@ def assign_learners(self, request, group_uuid): customer = group.enterprise_customer except models.EnterpriseGroup.DoesNotExist as exc: raise Http404 from exc - if requested_emails := request.POST.getlist('learner_emails'): - request_data = {} - request_data['act_by_date'] = request.data.get('act_by_date') - request_data['catalog_uuid'] = request.data.get('catalog_uuid') - request_data['learner_emails'] = requested_emails - - param_serializers = serializers.EnterpriseGroupAssignLearnersRequestDataSerializer( - data=request_data + param_serializer = serializers.EnterpriseGroupAssignLearnersRequestDataSerializer(data=request.data) + param_serializer.is_valid(raise_exception=True) + act_by_date = param_serializer.data['act_by_date'] + catalog_uuid = param_serializer.data['catalog_uuid'] + learner_emails = param_serializer.data['learner_emails'] + total_records_processed = 0 + total_existing_users_processed = 0 + total_new_users_processed = 0 + for user_email_batch in utils.batch(learner_emails[: 1000], batch_size=200): + user_emails_to_create = [] + memberships_to_create = [] + # ecus: enterprise customer users + ecus = [] + # Gather all existing User objects associated with the email batch + existing_users = User.objects.filter(email__in=user_email_batch) + # Build and create a list of EnterpriseCustomerUser objects for the emails of existing Users + # Ignore conflicts in case any of the ent customer user objects already exist + ecu_by_email = { + user.email: models.EnterpriseCustomerUser( + enterprise_customer=customer, user_id=user.id, active=True + ) for user in existing_users + } + models.EnterpriseCustomerUser.objects.bulk_create( + ecu_by_email.values(), + ignore_conflicts=True, ) - param_serializers.is_valid() - if not param_serializers.is_valid(): - return Response(param_serializers.errors, status=400) - act_by_date = param_serializers.validated_data.get('act_by_date') - catalog_uuid = param_serializers.validated_data.get('catalog_uuid') - learner_emails = param_serializers.validated_data.get('learner_emails') - total_records_processed = 0 - total_existing_users_processed = 0 - total_new_users_processed = 0 - for user_email_batch in utils.batch(learner_emails[: 1000], batch_size=200): - user_emails_to_create = [] - memberships_to_create = [] - # ecus: enterprise customer users - ecus = [] - # Gather all existing User objects associated with the email batch - existing_users = User.objects.filter(email__in=user_email_batch) - # Build and create a list of EnterpriseCustomerUser objects for the emails of existing Users - # Ignore conflicts in case any of the ent customer user objects already exist - ecu_by_email = { - user.email: models.EnterpriseCustomerUser( - enterprise_customer=customer, user_id=user.id, active=True - ) for user in existing_users - } - models.EnterpriseCustomerUser.objects.bulk_create( - ecu_by_email.values(), - ignore_conflicts=True, - ) - # Fetch all ent customer users related to existing users provided by requester - # whether they were created above or already existed - ecus.extend( - models.EnterpriseCustomerUser.objects.filter( - user_id__in=existing_users.values_list('id', flat=True) - ) + # Fetch all ent customer users related to existing users provided by requester + # whether they were created above or already existed + ecus.extend( + models.EnterpriseCustomerUser.objects.filter( + user_id__in=existing_users.values_list('id', flat=True) ) + ) + + # Extend the list of emails that don't have User objects associated and need to be turned into + # new PendingEnterpriseCustomerUser objects + user_emails_to_create.extend(set(user_email_batch).difference(set(ecu_by_email.keys()))) - # Extend the list of emails that don't have User objects associated and need to be turned into - # new PendingEnterpriseCustomerUser objects - user_emails_to_create.extend(set(user_email_batch).difference(set(ecu_by_email.keys()))) - - # Extend the list of memberships that need to be created associated with existing Users - ent_customer_users = [ - models.EnterpriseGroupMembership( - activated_at=localized_utcnow(), - enterprise_customer_user=ecu, - group=group - ) - for ecu in ecus - ] - total_existing_users_processed += len(ent_customer_users) - memberships_to_create.extend(ent_customer_users) - - # Go over (in batches) all emails that don't have User objects - for emails_to_create_batch in utils.batch(user_emails_to_create, batch_size=200): - # Create the PendingEnterpriseCustomerUser objects - pecu_records = [ - models.PendingEnterpriseCustomerUser( - enterprise_customer=customer, user_email=user_email - ) for user_email in emails_to_create_batch - ] - # According to Django docs, bulk created objects can't be used in future bulk creates as the in memory - # objects returned by bulk_create won't have PK's assigned. - models.PendingEnterpriseCustomerUser.objects.bulk_create(pecu_records) - pecus = models.PendingEnterpriseCustomerUser.objects.filter( - user_email__in=emails_to_create_batch, - enterprise_customer=customer, + # Extend the list of memberships that need to be created associated with existing Users + ent_customer_users = [ + models.EnterpriseGroupMembership( + activated_at=localized_utcnow(), + enterprise_customer_user=ecu, + group=group ) - total_new_users_processed += len(pecus) - # Extend the list of memberships that need to be created associated with the new pending users - memberships_to_create.extend([ - models.EnterpriseGroupMembership( - pending_enterprise_customer_user=pecu, - group=group - ) for pecu in pecus - ]) - - # Create all our memberships, bulk_create will batch for us. - memberships = models.EnterpriseGroupMembership.objects.bulk_create( - memberships_to_create, ignore_conflicts=True + for ecu in ecus + ] + total_existing_users_processed += len(ent_customer_users) + memberships_to_create.extend(ent_customer_users) + + # Go over (in batches) all emails that don't have User objects + for emails_to_create_batch in utils.batch(user_emails_to_create, batch_size=200): + # Create the PendingEnterpriseCustomerUser objects + pecu_records = [ + models.PendingEnterpriseCustomerUser( + enterprise_customer=customer, user_email=user_email + ) for user_email in emails_to_create_batch + ] + # According to Django docs, bulk created objects can't be used in future bulk creates as the in memory + # objects returned by bulk_create won't have PK's assigned. + models.PendingEnterpriseCustomerUser.objects.bulk_create(pecu_records) + pecus = models.PendingEnterpriseCustomerUser.objects.filter( + user_email__in=emails_to_create_batch, + enterprise_customer=customer, ) - total_records_processed += len(memberships) - data = { - 'records_processed': total_records_processed, - 'new_learners': total_new_users_processed, - 'existing_learners': total_existing_users_processed, - } - membership_uuids = [membership.uuid for membership in memberships] - if act_by_date is not None and catalog_uuid is not None: - for membership_uuid_batch in utils.batch(membership_uuids, batch_size=200): - send_group_membership_invitation_notification.delay( - customer.uuid, - membership_uuid_batch, - act_by_date, - catalog_uuid - ) - return Response(data, status=201) - return Response(data="Error: missing request data: `learner_emails`.", status=400) + total_new_users_processed += len(pecus) + # Extend the list of memberships that need to be created associated with the new pending users + memberships_to_create.extend([ + models.EnterpriseGroupMembership( + pending_enterprise_customer_user=pecu, + group=group + ) for pecu in pecus + ]) + + # Create all our memberships, bulk_create will batch for us. + memberships = models.EnterpriseGroupMembership.objects.bulk_create( + memberships_to_create, ignore_conflicts=True + ) + total_records_processed += len(memberships) + data = { + 'records_processed': total_records_processed, + 'new_learners': total_new_users_processed, + 'existing_learners': total_existing_users_processed, + } + membership_uuids = [membership.uuid for membership in memberships] + if act_by_date is not None and catalog_uuid is not None: + for membership_uuid_batch in utils.batch(membership_uuids, batch_size=200): + send_group_membership_invitation_notification.delay( + customer.uuid, + membership_uuid_batch, + act_by_date, + catalog_uuid + ) + return Response(data, status=201) @action(methods=['post'], detail=False, permission_classes=[permissions.IsAuthenticated]) @permission_required( diff --git a/enterprise/tasks.py b/enterprise/tasks.py index e2905602d9..ac8a8b8b92 100644 --- a/enterprise/tasks.py +++ b/enterprise/tasks.py @@ -241,16 +241,14 @@ def send_group_membership_invitation_notification( braze_trigger_properties['act_by_date'] = act_by_date.strftime('%B %d, %Y') pecu_emails = [] ecus = [] - for membership_uuid in membership_uuids: - group_membership = enterprise_group_membership_model().objects.filter( - uuid=membership_uuid - ) - if group_membership[0].pending_enterprise_customer_user is not None: - pecu_emails.append(group_membership[0].pending_enterprise_customer_user.user_email) + membership_records = enterprise_group_membership_model().objects.filter(uuid__in=membership_uuids) + for group_membership in membership_records: + if group_membership.pending_enterprise_customer_user is not None: + pecu_emails.append(group_membership.pending_enterprise_customer_user.user_email) else: ecus.append({ - 'user_email': group_membership[0].enterprise_customer_user.user_email, - 'user_id': group_membership[0].enterprise_customer_user.user_id + 'user_email': group_membership.enterprise_customer_user.user_email, + 'user_id': group_membership.enterprise_customer_user.user_id }) recipients = [] for pecu_email in pecu_emails: @@ -297,16 +295,14 @@ def send_group_membership_removal_notification(enterprise_customer_uuid, members braze_trigger_properties['enterprise_customer_name'] = enterprise_customer_name pecu_emails = [] ecus = [] - for membership_uuid in membership_uuids: - group_membership = enterprise_group_membership_model().objects.filter( - uuid=membership_uuid - ) - if group_membership[0].pending_enterprise_customer_user is not None: - pecu_emails.append(group_membership[0].pending_enterprise_customer_user.user_email) + membership_records = enterprise_group_membership_model().objects.filter(uuid__in=membership_uuids) + for group_membership in membership_records: + if group_membership.pending_enterprise_customer_user is not None: + pecu_emails.append(group_membership.pending_enterprise_customer_user.user_email) else: ecus.append({ - 'user_email': group_membership[0].enterprise_customer_user.user_email, - 'user_id': group_membership[0].enterprise_customer_user.user_id + 'user_email': group_membership.enterprise_customer_user.user_email, + 'user_id': group_membership.enterprise_customer_user.user_id }) recipients = [] diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index 8fddeed241..ddfc0f5cc6 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -14,7 +14,6 @@ from urllib.parse import parse_qs, urlencode, urljoin, urlsplit, urlunsplit import ddt -import pytz import responses from edx_toggles.toggles.testutils import override_waffle_flag from faker import Faker @@ -7635,7 +7634,8 @@ def test_assign_learners_requires_learner_emails(self): ) response = self.client.post(url) assert response.status_code == 400 - assert response.data == "Error: missing request data: `learner_emails`." + + assert response.json() == {'learner_emails': ['This field is required.']} @mock.patch('enterprise.tasks.send_group_membership_invitation_notification.delay', return_value=mock.MagicMock()) def test_successful_assign_learners_to_group(self, mock_send_group_membership_invitation_notification): @@ -7648,8 +7648,8 @@ def test_successful_assign_learners_to_group(self, mock_send_group_membership_in ) existing_emails = [UserFactory().email for _ in range(10)] new_emails = [f"email_{x}@example.com" for x in range(10)] - act_by_date = datetime.now(pytz.UTC) - catalog_uuid = uuid.uuid4() + act_by_date = datetime.now().strftime('%Y-%m-%dT%H:%M:%SZ') + catalog_uuid = str(uuid.uuid4()) request_data = { 'learner_emails': existing_emails + new_emails, 'act_by_date': act_by_date, From ea5802e0af3b802ebaa71c2fb45cc7b2d46e1b7e Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Fri, 5 Apr 2024 17:38:35 +0000 Subject: [PATCH 13/17] fix: add catalog content count to removal email --- enterprise/api/v1/serializers.py | 4 +- enterprise/api/v1/views/enterprise_group.py | 48 ++++++++++++--------- enterprise/tasks.py | 6 ++- tests/test_enterprise/api/test_views.py | 6 +-- tests/test_enterprise/test_tasks.py | 9 +++- 5 files changed, 45 insertions(+), 28 deletions(-) diff --git a/enterprise/api/v1/serializers.py b/enterprise/api/v1/serializers.py index 743a4b6ddb..9e70557df3 100644 --- a/enterprise/api/v1/serializers.py +++ b/enterprise/api/v1/serializers.py @@ -1701,12 +1701,12 @@ class Meta: updated = serializers.DateTimeField(required=False, read_only=True) -class EnterpriseGroupAssignLearnersRequestDataSerializer(serializers.Serializer): +class EnterpriseGroupRequestDataSerializer(serializers.Serializer): """ Serializer for the Enterprise Group Assign Learners endpoint query params """ catalog_uuid = serializers.UUIDField(required=False, allow_null=True) - act_by_date = serializers.DateTimeField(required=False, allow_null=True, format='%Y-%m-%dT%H:%M:%SZ') + act_by_date = serializers.DateTimeField(required=False, allow_null=True) learner_emails = serializers.ListField( child=serializers.EmailField(required=True), allow_empty=False diff --git a/enterprise/api/v1/views/enterprise_group.py b/enterprise/api/v1/views/enterprise_group.py index 3ea1b9ea05..962b253df3 100644 --- a/enterprise/api/v1/views/enterprise_group.py +++ b/enterprise/api/v1/views/enterprise_group.py @@ -171,7 +171,7 @@ def assign_learners(self, request, group_uuid): customer = group.enterprise_customer except models.EnterpriseGroup.DoesNotExist as exc: raise Http404 from exc - param_serializer = serializers.EnterpriseGroupAssignLearnersRequestDataSerializer(data=request.data) + param_serializer = serializers.EnterpriseGroupRequestDataSerializer(data=request.data) param_serializer.is_valid(raise_exception=True) act_by_date = param_serializer.data['act_by_date'] catalog_uuid = param_serializer.data['catalog_uuid'] @@ -289,23 +289,29 @@ def remove_learners(self, request, group_uuid): customer = group.enterprise_customer except models.EnterpriseGroup.DoesNotExist as exc: raise Http404 from exc - if requested_emails := request.POST.dict().get('learner_emails'): - records_deleted = 0 - for user_email_batch in utils.batch(requested_emails.split(','), batch_size=200): - existing_users = User.objects.filter(email__in=user_email_batch).values_list("id", flat=True) - group_q = Q(group=group) - ecu_in_q = Q(enterprise_customer_user__user_id__in=existing_users) - pecu_in_q = Q(pending_enterprise_customer_user__user_email__in=user_email_batch) - records_to_delete = models.EnterpriseGroupMembership.objects.filter( - group_q & (ecu_in_q | pecu_in_q), - ) - records_deleted += len(records_to_delete) - records_to_delete_uuids = [record.uuid for record in records_to_delete] - for records_to_delete_uuids_batch in utils.batch(records_to_delete_uuids, batch_size=200): - send_group_membership_removal_notification.delay(customer.uuid, records_to_delete_uuids_batch) - records_to_delete.delete() - data = { - 'records_deleted': records_deleted, - } - return Response(data, status=200) - return Response(data="Error: missing request data: `learner_emails`.", status=400) + param_serializer = serializers.EnterpriseGroupRequestDataSerializer(data=request.data) + param_serializer.is_valid(raise_exception=True) + catalog_uuid = param_serializer.data['catalog_uuid'] + learner_emails = param_serializer.data['learner_emails'] + + records_deleted = 0 + for user_email_batch in utils.batch(learner_emails[: 1000], batch_size=200): + existing_users = User.objects.filter(email__in=user_email_batch).values_list("id", flat=True) + group_q = Q(group=group) + ecu_in_q = Q(enterprise_customer_user__user_id__in=existing_users) + pecu_in_q = Q(pending_enterprise_customer_user__user_email__in=user_email_batch) + records_to_delete = models.EnterpriseGroupMembership.objects.filter( + group_q & (ecu_in_q | pecu_in_q), + ) + records_deleted += len(records_to_delete) + records_to_delete_uuids = [record.uuid for record in records_to_delete] + for records_to_delete_uuids_batch in utils.batch(records_to_delete_uuids, batch_size=200): + send_group_membership_removal_notification.delay( + customer.uuid, + records_to_delete_uuids_batch, + catalog_uuid) + records_to_delete.delete() + data = { + 'records_deleted': records_deleted, + } + return Response(data, status=200) diff --git a/enterprise/tasks.py b/enterprise/tasks.py index ac8a8b8b92..bfd7328631 100644 --- a/enterprise/tasks.py +++ b/enterprise/tasks.py @@ -278,7 +278,7 @@ def send_group_membership_invitation_notification( @shared_task @set_code_owner_attribute -def send_group_membership_removal_notification(enterprise_customer_uuid, membership_uuids): +def send_group_membership_removal_notification(enterprise_customer_uuid, membership_uuids, catalog_uuid): """ Send braze email notification when learner is removed from a group. @@ -288,11 +288,15 @@ def send_group_membership_removal_notification(enterprise_customer_uuid, members """ enterprise_customer = get_enterprise_customer(enterprise_customer_uuid) braze_client_instance = BrazeAPIClient() + enterprise_catalog_client = EnterpriseCatalogApiClient() braze_trigger_properties = {} contact_email = enterprise_customer.contact_email enterprise_customer_name = enterprise_customer.name braze_trigger_properties['contact_admin_link'] = braze_client_instance.generate_mailto_link(contact_email) braze_trigger_properties['enterprise_customer_name'] = enterprise_customer_name + braze_trigger_properties[ + 'catalog_content_count' + ] = enterprise_catalog_client.get_catalog_content_count(catalog_uuid) pecu_emails = [] ecus = [] membership_records = enterprise_group_membership_model().objects.filter(uuid__in=membership_uuids) diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index ddfc0f5cc6..2baaefe49a 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -7696,7 +7696,7 @@ def test_remove_learners_requires_learner_emails(self): ) response = self.client.post(url) assert response.status_code == 400 - assert response.data == "Error: missing request data: `learner_emails`." + assert response.json() == {'learner_emails': ['This field is required.']} def test_patch_with_bad_request_customer_to_change_to(self): """ @@ -7736,12 +7736,12 @@ def test_successful_remove_learners_from_group(self, mock_send_group_membership_ 'enterprise-group-remove-learners', kwargs={'group_uuid': self.group_2.uuid}, ) - existing_emails = "" + existing_emails = [] memberships_to_delete = [] for _ in range(10): membership = EnterpriseGroupMembershipFactory(group=self.group_2) memberships_to_delete.append(membership) - existing_emails += membership.enterprise_customer_user.user.email + ',' + existing_emails.append(membership.enterprise_customer_user.user.email) request_data = {'learner_emails': existing_emails} response = self.client.post(url, data=request_data) diff --git a/tests/test_enterprise/test_tasks.py b/tests/test_enterprise/test_tasks.py index ff9e8e84a0..eaa460b706 100644 --- a/tests/test_enterprise/test_tasks.py +++ b/tests/test_enterprise/test_tasks.py @@ -243,8 +243,9 @@ def test_send_group_membership_invitation_notification(self, mock_braze_api_clie )] mock_braze_api_client().send_campaign_message.assert_has_calls(calls) + @mock.patch('enterprise.tasks.EnterpriseCatalogApiClient', return_value=mock.MagicMock()) @mock.patch('enterprise.tasks.BrazeAPIClient', return_value=mock.MagicMock()) - def test_send_group_membership_removal_notification(self, mock_braze_api_client): + def test_send_group_membership_removal_notification(self, mock_braze_api_client, mock_enterprise_catalog_client): """ Verify send_group_membership_removal_notification hits braze client with expected args """ @@ -266,10 +267,15 @@ def test_send_group_membership_removal_notification(self, mock_braze_api_client) mock_braze_api_client().generate_mailto_link.return_value = mock_admin_mailto mock_braze_api_client().create_recipient_no_external_id.return_value = ( self.pending_enterprise_customer_user.user_email) + mock_catalog_content_count = 5 + mock_enterprise_catalog_client().get_catalog_content_count.return_value = ( + mock_catalog_content_count) membership_uuids = EnterpriseGroupMembership.objects.values_list('uuid', flat=True) + catalog_uuid = uuid.uuid4() send_group_membership_removal_notification( self.enterprise_customer.uuid, membership_uuids, + catalog_uuid, ) calls = [mock.call( BRAZE_GROUPS_REMOVAL_EMAIL_CAMPAIGN_ID, @@ -277,6 +283,7 @@ def test_send_group_membership_removal_notification(self, mock_braze_api_client) trigger_properties={ 'contact_admin_link': mock_admin_mailto, 'enterprise_customer_name': self.enterprise_customer.name, + 'catalog_content_count': mock_catalog_content_count, }, )] mock_braze_api_client().send_campaign_message.assert_has_calls(calls) From fb15f0e7a25819051133af1186d901a2d6fe6f4e Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Mon, 8 Apr 2024 20:35:24 +0000 Subject: [PATCH 14/17] fix: failing test --- enterprise/api/v1/serializers.py | 1 + enterprise/api/v1/views/enterprise_group.py | 23 ++++++++++++++++----- enterprise/models.py | 13 ++++++++++-- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/enterprise/api/v1/serializers.py b/enterprise/api/v1/serializers.py index 9ae393009c..b17a6864c2 100644 --- a/enterprise/api/v1/serializers.py +++ b/enterprise/api/v1/serializers.py @@ -1716,3 +1716,4 @@ class EnterpriseGroupLearnersRequestQuerySerializer(serializers.Serializer): ], required=False, ) + pending_users_only = serializers.BooleanField(required=False, default=False) diff --git a/enterprise/api/v1/views/enterprise_group.py b/enterprise/api/v1/views/enterprise_group.py index 2d7d8c04a1..af551da40e 100644 --- a/enterprise/api/v1/views/enterprise_group.py +++ b/enterprise/api/v1/views/enterprise_group.py @@ -100,7 +100,7 @@ def get_learners(self, request, *args, **kwargs): Endpoint Location to return all learners: GET api/v1/enterprise-group//learners/ Endpoint Location to return pending learners: - GET api/v1/enterprise-group//learners?pending_users_only=true + GET api/v1/enterprise-group//learners/?pending_users_only=true Request Arguments: - ``group_uuid`` (URL location, required): The uuid of the group from which learners should be listed. @@ -129,7 +129,7 @@ def get_learners(self, request, *args, **kwargs): 'user_name': string, }, 'recent_action': string, - 'member_status': string, + 'status': string, }, ], } @@ -147,13 +147,16 @@ def get_learners(self, request, *args, **kwargs): user_query = param_serializers.validated_data.get('user_query') sort_by = param_serializers.validated_data.get('sort_by') + pending_users_only = param_serializers.validated_data.get('pending_users_only') group_uuid = kwargs.get('group_uuid') - filter_for_pecus = request.query_params.get('pending_users_only', None) try: group_object = self.get_queryset().get(uuid=group_uuid) - if filter_for_pecus is not None: - members = group_object.get_all_learners(filter_for_pecus) + if pending_users_only is not None: + members = group_object.get_all_learners(user_query, + sort_by, + desc_order=is_reversed, + pending_users_only=pending_users_only) else: members = group_object.get_all_learners(user_query, sort_by, desc_order=is_reversed) page = self.paginate_queryset(members) @@ -304,6 +307,8 @@ def remove_learners(self, request, group_uuid): - ``records_deleted``: Number of membership records removed """ + # import pdb + # pdb.set_trace() try: group = self.get_queryset().get(uuid=group_uuid) customer = group.enterprise_customer @@ -331,6 +336,14 @@ def remove_learners(self, request, group_uuid): records_to_delete_uuids_batch, catalog_uuid) records_to_delete.delete() + # Woohoo! Records removed! Now to update the soft deleted records + deleted_records = models.EnterpriseGroupMembership.all_objects.filter( + group_q & (ecu_in_q | pecu_in_q), + ) + deleted_records.update( + status=constants.GROUP_MEMBERSHIP_REMOVED_STATUS, + removed_at=localized_utcnow() + ) data = { 'records_deleted': records_deleted, } diff --git a/enterprise/models.py b/enterprise/models.py index 75157378ca..4367c2742c 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -4330,7 +4330,7 @@ def _get_implicit_group_members(self, user_query=None): )) return members - def _get_explicit_group_members(self, user_query=None, fetch_removed=False): + def _get_explicit_group_members(self, user_query=None, fetch_removed=False, pending_users_only=None,): """ Fetch explicitly defined members of a group, indicated by an existing membership record """ @@ -4345,9 +4345,16 @@ def _get_explicit_group_members(self, user_query=None, fetch_removed=False): # pecu has user_email as a field, so we can filter directly through the ORM with the user_query pecu_filter = Q(pending_enterprise_customer_user__user_email__icontains=user_query) members = members.filter(ecu_filter | pecu_filter) + if pending_users_only: + members = members.filter(is_removed=False, enterprise_customer_user_id__isnull=True) return members - def get_all_learners(self, user_query=None, sort_by=None, desc_order=False, fetch_removed=False): + def get_all_learners(self, + user_query=None, + sort_by=None, + desc_order=False, + fetch_removed=False, + pending_users_only=False): """ Returns all users associated with the group, whether the group specifies the entire org else all associated membership records. @@ -4369,6 +4376,8 @@ def get_all_learners(self, user_query=None, sort_by=None, desc_order=False, fetc 'recent_action': lambda t: t.recent_action, } members = sorted(members, key=lambda_keys.get(sort_by), reverse=desc_order) + if pending_users_only: + members = self._get_explicit_group_members(user_query, fetch_removed, pending_users_only) return members From 7c425a72733395dd7b3260d43a3de0a1158c9c7f Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Mon, 15 Apr 2024 18:02:01 +0000 Subject: [PATCH 15/17] fix: update model logic --- enterprise/api/v1/views/enterprise_group.py | 39 ++++++++++++--------- enterprise/models.py | 13 +++---- 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/enterprise/api/v1/views/enterprise_group.py b/enterprise/api/v1/views/enterprise_group.py index af551da40e..0dbec4cf1e 100644 --- a/enterprise/api/v1/views/enterprise_group.py +++ b/enterprise/api/v1/views/enterprise_group.py @@ -152,13 +152,11 @@ def get_learners(self, request, *args, **kwargs): group_uuid = kwargs.get('group_uuid') try: group_object = self.get_queryset().get(uuid=group_uuid) - if pending_users_only is not None: - members = group_object.get_all_learners(user_query, - sort_by, - desc_order=is_reversed, - pending_users_only=pending_users_only) - else: - members = group_object.get_all_learners(user_query, sort_by, desc_order=is_reversed) + members = group_object.get_all_learners(user_query, + sort_by, + desc_order=is_reversed, + pending_users_only=pending_users_only) + page = self.paginate_queryset(members) serializer = serializers.EnterpriseGroupMembershipSerializer(page, many=True) response = self.get_paginated_response(serializer.data) @@ -177,10 +175,14 @@ def assign_learners(self, request, group_uuid): """ POST /enterprise/api/v1/enterprise-group//assign_learners - Required Arguments: + Request Arguments: - ``learner_emails``: List of learner emails to associate with the group. Note: only processes the first 1000 records provided. + Optional request data: + - ``act_by_date`` (datetime, optional): The expiration date for the subsidy. + - ``catalog_uuid`` (string, optional): The uuid of the catalog that is part of the subsidy. + Returns: - ``records_processed``: Total number of group membership records processed. - ``new_learners``: Total number of group membership records associated with new pending enterprise learners @@ -196,9 +198,10 @@ def assign_learners(self, request, group_uuid): raise Http404 from exc param_serializer = serializers.EnterpriseGroupRequestDataSerializer(data=request.data) param_serializer.is_valid(raise_exception=True) - act_by_date = param_serializer.data['act_by_date'] - catalog_uuid = param_serializer.data['catalog_uuid'] - learner_emails = param_serializer.data['learner_emails'] + # act_by_date and catalog_uuid values are needed for Braze email trigger properties + act_by_date = param_serializer.data.get('act_by_date') + catalog_uuid = param_serializer.data.get('catalog_uuid') + learner_emails = param_serializer.data.get('learner_emails') total_records_processed = 0 total_existing_users_processed = 0 total_new_users_processed = 0 @@ -280,7 +283,7 @@ def assign_learners(self, request, group_uuid): 'existing_learners': total_existing_users_processed, } membership_uuids = [membership.uuid for membership in memberships] - if act_by_date is not None and catalog_uuid is not None: + if act_by_date and catalog_uuid: for membership_uuid_batch in utils.batch(membership_uuids, batch_size=200): send_group_membership_invitation_notification.delay( customer.uuid, @@ -303,12 +306,13 @@ def remove_learners(self, request, group_uuid): - ``learner_emails``: List of learner emails to associate with the group. + Optional request data: + - ``catalog_uuid`` (string, optional): The uuid of the catalog that is part of the subsidy. + Returns: - ``records_deleted``: Number of membership records removed """ - # import pdb - # pdb.set_trace() try: group = self.get_queryset().get(uuid=group_uuid) customer = group.enterprise_customer @@ -316,8 +320,9 @@ def remove_learners(self, request, group_uuid): raise Http404 from exc param_serializer = serializers.EnterpriseGroupRequestDataSerializer(data=request.data) param_serializer.is_valid(raise_exception=True) - catalog_uuid = param_serializer.data['catalog_uuid'] - learner_emails = param_serializer.data['learner_emails'] + + catalog_uuid = param_serializer.data.get('catalog_uuid') + learner_emails = param_serializer.data.get('learner_emails') records_deleted = 0 for user_email_batch in utils.batch(learner_emails[: 1000], batch_size=200): @@ -330,12 +335,12 @@ def remove_learners(self, request, group_uuid): ) records_deleted += len(records_to_delete) records_to_delete_uuids = [record.uuid for record in records_to_delete] + records_to_delete.delete() for records_to_delete_uuids_batch in utils.batch(records_to_delete_uuids, batch_size=200): send_group_membership_removal_notification.delay( customer.uuid, records_to_delete_uuids_batch, catalog_uuid) - records_to_delete.delete() # Woohoo! Records removed! Now to update the soft deleted records deleted_records = models.EnterpriseGroupMembership.all_objects.filter( group_q & (ecu_in_q | pecu_in_q), diff --git a/enterprise/models.py b/enterprise/models.py index 4367c2742c..e30a76a6b2 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -4292,7 +4292,7 @@ def _get_filtered_ecu_ids(self, user_query): ecus = EnterpriseCustomerUser.objects.raw(sql_string, (customer_id, var_q)) return [ecu.id for ecu in ecus] - def _get_implicit_group_members(self, user_query=None): + def _get_implicit_group_members(self, user_query=None, pending_users_only=False): """ Fetches all implicit members of a group, indicated by a (pending) enterprise customer user records. """ @@ -4315,6 +4315,9 @@ def _get_implicit_group_members(self, user_query=None): enterprise_customer=self.enterprise_customer, active=True, ) + # Setting customer_users to be an empty array so we only get back pecu members + if pending_users_only: + customer_users = [] # Build an in memory array of all the implicit memberships for ent_user in customer_users: members.append(EnterpriseGroupMembership( @@ -4330,7 +4333,7 @@ def _get_implicit_group_members(self, user_query=None): )) return members - def _get_explicit_group_members(self, user_query=None, fetch_removed=False, pending_users_only=None,): + def _get_explicit_group_members(self, user_query=None, fetch_removed=False, pending_users_only=False,): """ Fetch explicitly defined members of a group, indicated by an existing membership record """ @@ -4366,9 +4369,9 @@ def get_all_learners(self, beginning of the sorting value ie `-memberStatus`. """ if self.applies_to_all_contexts: - members = self._get_implicit_group_members(user_query) + members = self._get_implicit_group_members(user_query, pending_users_only) else: - members = self._get_explicit_group_members(user_query, fetch_removed) + members = self._get_explicit_group_members(user_query, fetch_removed, pending_users_only) if sort_by: lambda_keys = { 'member_details': lambda t: t.member_email, @@ -4376,8 +4379,6 @@ def get_all_learners(self, 'recent_action': lambda t: t.recent_action, } members = sorted(members, key=lambda_keys.get(sort_by), reverse=desc_order) - if pending_users_only: - members = self._get_explicit_group_members(user_query, fetch_removed, pending_users_only) return members From bc420a498f42781c68b522fc67a3e404d02176f1 Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Mon, 15 Apr 2024 18:50:08 +0000 Subject: [PATCH 16/17] fix: use validated_data from serializer --- enterprise/api/v1/views/enterprise_group.py | 10 +++++----- tests/test_enterprise/api/test_views.py | 5 +++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/enterprise/api/v1/views/enterprise_group.py b/enterprise/api/v1/views/enterprise_group.py index 0dbec4cf1e..0cac9a5af8 100644 --- a/enterprise/api/v1/views/enterprise_group.py +++ b/enterprise/api/v1/views/enterprise_group.py @@ -199,9 +199,9 @@ def assign_learners(self, request, group_uuid): param_serializer = serializers.EnterpriseGroupRequestDataSerializer(data=request.data) param_serializer.is_valid(raise_exception=True) # act_by_date and catalog_uuid values are needed for Braze email trigger properties - act_by_date = param_serializer.data.get('act_by_date') - catalog_uuid = param_serializer.data.get('catalog_uuid') - learner_emails = param_serializer.data.get('learner_emails') + act_by_date = param_serializer.validated_data.get('act_by_date') + catalog_uuid = param_serializer.validated_data.get('catalog_uuid') + learner_emails = param_serializer.validated_data.get('learner_emails') total_records_processed = 0 total_existing_users_processed = 0 total_new_users_processed = 0 @@ -321,8 +321,8 @@ def remove_learners(self, request, group_uuid): param_serializer = serializers.EnterpriseGroupRequestDataSerializer(data=request.data) param_serializer.is_valid(raise_exception=True) - catalog_uuid = param_serializer.data.get('catalog_uuid') - learner_emails = param_serializer.data.get('learner_emails') + catalog_uuid = param_serializer.validated_data.get('catalog_uuid') + learner_emails = param_serializer.validated_data.get('learner_emails') records_deleted = 0 for user_email_batch in utils.batch(learner_emails[: 1000], batch_size=200): diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index b87bd14a30..80d160c1be 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -14,6 +14,7 @@ from urllib.parse import parse_qs, urlencode, urljoin, urlsplit, urlunsplit import ddt +import pytz import responses from edx_toggles.toggles.testutils import override_waffle_flag from faker import Faker @@ -7793,8 +7794,8 @@ def test_successful_assign_learners_to_group(self, mock_send_group_membership_in ) existing_emails = [UserFactory().email for _ in range(10)] new_emails = [f"email_{x}@example.com" for x in range(10)] - act_by_date = datetime.now().strftime('%Y-%m-%dT%H:%M:%SZ') - catalog_uuid = str(uuid.uuid4()) + act_by_date = datetime.now(pytz.UTC) + catalog_uuid = uuid.uuid4() request_data = { 'learner_emails': existing_emails + new_emails, 'act_by_date': act_by_date, From df20e95141f61fb6ecf0b621887cb028c7223636 Mon Sep 17 00:00:00 2001 From: katrinan029 Date: Thu, 18 Apr 2024 18:06:21 +0000 Subject: [PATCH 17/17] chore: update version --- CHANGELOG.rst | 4 ++++ enterprise/__init__.py | 2 +- enterprise/tasks.py | 3 +++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b08d01a5a6..1b03e20598 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -15,6 +15,10 @@ Change Log Unreleased ---------- +[4.15.7] +-------- +* feat: add send group membership invite and removal braze emails + [4.15.6] -------- * perf: update user preferences inside an async task to void request timeout diff --git a/enterprise/__init__.py b/enterprise/__init__.py index d86d11e771..be646193b2 100644 --- a/enterprise/__init__.py +++ b/enterprise/__init__.py @@ -2,4 +2,4 @@ Your project description goes here. """ -__version__ = "4.15.6" +__version__ = "4.15.7" diff --git a/enterprise/tasks.py b/enterprise/tasks.py index 3bb74a2b96..f6646ad1ec 100644 --- a/enterprise/tasks.py +++ b/enterprise/tasks.py @@ -339,6 +339,9 @@ def send_group_membership_removal_notification(enterprise_customer_uuid, members LOGGER.exception(message) raise exc + +@shared_task +@set_code_owner_attribute def update_enterprise_learners_user_preference(enterprise_customer_uuid): """ Update the user preference `pref-lang` attribute for all enterprise learners linked with an enterprise.