From 8b31c1052bab442e5cc4344c6594633838332c2b Mon Sep 17 00:00:00 2001 From: MueezKhan246 <93375917+MueezKhan246@users.noreply.github.com> Date: Tue, 10 Sep 2024 14:22:47 +0500 Subject: [PATCH 1/8] Making unencrypted sap client credentials nullable (#2236) * refactor: making unencrypted credentials nullable so after removing refs tests can run --- CHANGELOG.rst | 4 ++++ enterprise/__init__.py | 2 +- .../migrations/0023_auto_20240909_1556.py | 23 +++++++++++++++++++ .../sap_success_factors/models.py | 6 +++-- 4 files changed, 32 insertions(+), 3 deletions(-) create mode 100644 integrated_channels/sap_success_factors/migrations/0023_auto_20240909_1556.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 29762d812..002c361f9 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,6 +17,10 @@ Unreleased ---------- * nothing unreleased +[4.25.6] +---------- +* refactor: making unencrypted credentials nullable so after removing refs tests can run + [4.25.5] ---------- * feat: changing django enterprise customer summary columns diff --git a/enterprise/__init__.py b/enterprise/__init__.py index 2d8e12f09..45e7ced47 100644 --- a/enterprise/__init__.py +++ b/enterprise/__init__.py @@ -2,4 +2,4 @@ Your project description goes here. """ -__version__ = "4.25.5" +__version__ = "4.25.6" diff --git a/integrated_channels/sap_success_factors/migrations/0023_auto_20240909_1556.py b/integrated_channels/sap_success_factors/migrations/0023_auto_20240909_1556.py new file mode 100644 index 000000000..042e01ef8 --- /dev/null +++ b/integrated_channels/sap_success_factors/migrations/0023_auto_20240909_1556.py @@ -0,0 +1,23 @@ +# Generated by Django 3.2.23 on 2024-09-09 15:56 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('sap_success_factors', '0022_auto_20240906_1349'), + ] + + operations = [ + migrations.AlterField( + model_name='sapsuccessfactorsenterprisecustomerconfiguration', + name='key', + field=models.CharField(blank=True, default='', help_text='OAuth client identifier.', max_length=255, null=True, verbose_name='Client ID'), + ), + migrations.AlterField( + model_name='sapsuccessfactorsenterprisecustomerconfiguration', + name='secret', + field=models.CharField(blank=True, default='', help_text='OAuth client secret.', max_length=255, null=True, verbose_name='Client Secret'), + ), + ] diff --git a/integrated_channels/sap_success_factors/models.py b/integrated_channels/sap_success_factors/models.py index aaa8ff365..002ee57ce 100644 --- a/integrated_channels/sap_success_factors/models.py +++ b/integrated_channels/sap_success_factors/models.py @@ -79,7 +79,8 @@ class SAPSuccessFactorsEnterpriseCustomerConfiguration(EnterpriseCustomerPluginC blank=True, default='', verbose_name="Client ID", - help_text=_("OAuth client identifier.") + help_text=_("OAuth client identifier."), + null=True ) decrypted_key = EncryptedCharField( @@ -142,7 +143,8 @@ def encrypted_key(self, value): blank=True, default='', verbose_name="Client Secret", - help_text=_("OAuth client secret.") + help_text=_("OAuth client secret."), + null=True ) decrypted_secret = EncryptedCharField( From fa7bfe82dafb02759a9a6dc7c8e7efb95be1478b Mon Sep 17 00:00:00 2001 From: MueezKhan246 <93375917+MueezKhan246@users.noreply.github.com> Date: Tue, 10 Sep 2024 17:25:03 +0500 Subject: [PATCH 2/8] Removing unencrypted credentials from model (#2220) * refactor: removed unencrypted credentials from sap config model --- CHANGELOG.rst | 4 ++++ enterprise/__init__.py | 2 +- .../sap_success_factors/models.py | 16 ---------------- 3 files changed, 5 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 002c361f9..29f99164d 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,6 +17,10 @@ Unreleased ---------- * nothing unreleased +[4.25.7] +---------- +* refactor: removed unencrypted credentials from sap config model + [4.25.6] ---------- * refactor: making unencrypted credentials nullable so after removing refs tests can run diff --git a/enterprise/__init__.py b/enterprise/__init__.py index 45e7ced47..a2e3e77a7 100644 --- a/enterprise/__init__.py +++ b/enterprise/__init__.py @@ -2,4 +2,4 @@ Your project description goes here. """ -__version__ = "4.25.6" +__version__ = "4.25.7" diff --git a/integrated_channels/sap_success_factors/models.py b/integrated_channels/sap_success_factors/models.py index 002ee57ce..43fb6f765 100644 --- a/integrated_channels/sap_success_factors/models.py +++ b/integrated_channels/sap_success_factors/models.py @@ -74,14 +74,6 @@ class SAPSuccessFactorsEnterpriseCustomerConfiguration(EnterpriseCustomerPluginC (USER_TYPE_USER, 'User'), (USER_TYPE_ADMIN, 'Admin'), ) - key = models.CharField( - max_length=255, - blank=True, - default='', - verbose_name="Client ID", - help_text=_("OAuth client identifier."), - null=True - ) decrypted_key = EncryptedCharField( max_length=255, @@ -138,14 +130,6 @@ def encrypted_key(self, value): verbose_name="SAP User ID", help_text=_("Success factors user identifier.") ) - secret = models.CharField( - max_length=255, - blank=True, - default='', - verbose_name="Client Secret", - help_text=_("OAuth client secret."), - null=True - ) decrypted_secret = EncryptedCharField( max_length=255, From 0f361aeb4abc6a4c8f36431cb227f04798f00938 Mon Sep 17 00:00:00 2001 From: MueezKhan246 <93375917+MueezKhan246@users.noreply.github.com> Date: Tue, 10 Sep 2024 19:02:59 +0500 Subject: [PATCH 3/8] Remove unencrypted sap client credentials (#2217) * feat: added migration for removing unencrypted client credentials --- CHANGELOG.rst | 4 ++++ enterprise/__init__.py | 2 +- .../migrations/0024_auto_20240909_1614.py | 21 +++++++++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) create mode 100644 integrated_channels/sap_success_factors/migrations/0024_auto_20240909_1614.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 29f99164d..6a573b88d 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,6 +17,10 @@ Unreleased ---------- * nothing unreleased +[4.25.8] +---------- +* feat: added migration for removing unencrypted client credentials + [4.25.7] ---------- * refactor: removed unencrypted credentials from sap config model diff --git a/enterprise/__init__.py b/enterprise/__init__.py index a2e3e77a7..faf5cc4ba 100644 --- a/enterprise/__init__.py +++ b/enterprise/__init__.py @@ -2,4 +2,4 @@ Your project description goes here. """ -__version__ = "4.25.7" +__version__ = "4.25.8" diff --git a/integrated_channels/sap_success_factors/migrations/0024_auto_20240909_1614.py b/integrated_channels/sap_success_factors/migrations/0024_auto_20240909_1614.py new file mode 100644 index 000000000..cb1ec57b0 --- /dev/null +++ b/integrated_channels/sap_success_factors/migrations/0024_auto_20240909_1614.py @@ -0,0 +1,21 @@ +# Generated by Django 3.2.23 on 2024-09-09 16:14 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('sap_success_factors', '0023_auto_20240909_1556'), + ] + + operations = [ + migrations.RemoveField( + model_name='sapsuccessfactorsenterprisecustomerconfiguration', + name='key', + ), + migrations.RemoveField( + model_name='sapsuccessfactorsenterprisecustomerconfiguration', + name='secret', + ), + ] From 987c81bbf84b467c1992ed85ff75f5eb15fbe11e Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Fri, 6 Sep 2024 12:03:21 -0700 Subject: [PATCH 4/8] fix: send LEARNER_CREDIT_COURSE_ENROLLMENT_REVOKED from the correct place. I had originally tried emitting this event from the `/cancel_enrollment` API endpoint, but in reality the LMS and Enterprise dashboards were calling the `/change_enrollment` endpoint. The former is called by enterprise-subsidy on transaction reversal, i.e. NOT learner-initiated. The latter is learner-initiated, and that's the original goal of this work. Changes in this commit: * Update the event emission to live closer to the fulfillment model itself (inside `revoke()`). * Ensure that when an EnterpriseCourseEnrollment is unenrolled, the related Fulfillment subclass is revoked. This is a best-effort to improve internal consistency during learner-initiated unenrollment. * Consumers of the various enrollment models are now expected to call helper functions to unenroll/reenroll/revoke/reactivate rather than directly set internal fields. ENT-9213 --- CHANGELOG.rst | 4 + enterprise/__init__.py | 2 +- enterprise/models.py | 123 +++++++++++++++++++----- enterprise/signals.py | 18 +--- tests/test_enterprise/api/test_views.py | 2 + tests/test_enterprise/test_signals.py | 43 ++++++++- 6 files changed, 149 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 6a573b88d..e41faf57e 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,6 +17,10 @@ Unreleased ---------- * nothing unreleased +[4.25.9] +---------- +* fix: send LEARNER_CREDIT_COURSE_ENROLLMENT_REVOKED from the correct place. + [4.25.8] ---------- * feat: added migration for removing unencrypted client credentials diff --git a/enterprise/__init__.py b/enterprise/__init__.py index faf5cc4ba..487580cd9 100644 --- a/enterprise/__init__.py +++ b/enterprise/__init__.py @@ -2,4 +2,4 @@ Your project description goes here. """ -__version__ = "4.25.8" +__version__ = "4.25.9" diff --git a/enterprise/models.py b/enterprise/models.py index f5758a1a4..c81824b2e 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -2097,7 +2097,7 @@ def is_audit_enrollment(self): @property def license(self): """ - Returns the license associated with this enterprise course enrollment if one exists. + Returns the license fulfillment associated with this enterprise course enrollment if one exists. """ try: associated_license = self.licensedenterprisecourseenrollment_enrollment_fulfillment # pylint: disable=no-member @@ -2105,6 +2105,24 @@ def license(self): associated_license = None return associated_license + @property + def learner_credit_fulfillment(self): + """ + Returns the Learner Credit fulfillment associated with this enterprise course enrollment if one exists. + """ + try: + associated_fulfillment = self.learnercreditenterprisecourseenrollment_enrollment_fulfillment # pylint: disable=no-member + except LearnerCreditEnterpriseCourseEnrollment.DoesNotExist: + associated_fulfillment = None + return associated_fulfillment + + @property + def fulfillment(self): + """ + Find and return the related EnterpriseFulfillmentSource subclass, or None. + """ + return self.license or self.learner_credit_fulfillment + @cached_property def course_enrollment(self): """ @@ -2196,6 +2214,46 @@ def get_enterprise_uuids_with_user_and_course(cls, user_id, course_run_id, is_cu ) return [] + def set_unenrolled(self, desired_unenrolled): + """ + Idempotently set this object's fields to appear (un)enrolled and (un)saved-for-later. + + Also, attempt to revoke any related fulfillment, which in turn is also idempotent. + + This method and the fulfillment's revoke() call each other!!! If you edit either method, make sure to preserve + base cases that terminate infinite recursion. + + TODO: revoke entitlements as well? + """ + changed = False + if desired_unenrolled: + if not self.unenrolled or not self.saved_for_later: + self.saved_for_later = True + self.unenrolled = True + self.unenrolled_at = localized_utcnow() + changed = True + else: + if self.unenrolled or self.saved_for_later: + self.saved_for_later = False + self.unenrolled = False + self.unenrolled_at = None + changed = True + if changed: + LOGGER.info( + f"Marking EnterpriseCourseEnrollment as unenrolled={desired_unenrolled} " + f"for LMS user {self.enterprise_customer_user.user_id} " + f"and course {self.course_id}" + ) + self.save() + # Find and revoke/reactivate any related fulfillment. + # By only updating the related object on updates to self, we prevent infinite recursion. + if fulfillment := self.fulfillment: + if desired_unenrolled and not fulfillment.is_revoked: + fulfillment.revoke() + # Fulfillment reactivation on ECE reenrollment is unsupported. We'd need to collect a + # transaction UUID from the caller, but the caller at the time of writing is not aware of any + # transaction. + def __str__(self): """ Create string representation of the enrollment. @@ -2285,34 +2343,50 @@ def enterprise_customer_user(self): def revoke(self): """ - Marks this object as revoked and marks the associated EnterpriseCourseEnrollment - as "saved for later". This object and the associated EnterpriseCourseEnrollment are both saved. + Idempotently unenroll/revoke this fulfillment and associated EnterpriseCourseEnrollment. - Subclasses may override this function to additionally emit revocation events. + This method and EnterpriseCourseEnrollment.set_unenrolled() call each other!!! If you edit either method, make + sure to preserve base cases that terminate infinite recursion. - TODO: revoke entitlements as well? - """ - if self.enterprise_course_enrollment: - self.enterprise_course_enrollment.saved_for_later = True - self.enterprise_course_enrollment.unenrolled = True - self.enterprise_course_enrollment.unenrolled_at = localized_utcnow() - self.enterprise_course_enrollment.save() + Notes: + * This object and the associated EnterpriseCourseEnrollment may both be saved. + * Subclasses may override this function to additionally emit revocation events. - self.is_revoked = True - self.save() + Returns: + bool: True if self.is_revoked was changed. + """ + changed = False + if not self.is_revoked: + LOGGER.info(f"Marking fulfillment {str(self)} as revoked.") + changed = True + self.is_revoked = True + self.save() + # Find and unenroll any related EnterpriseCourseEnrollment. + # By only updating the related object on updates to self, we prevent infinite recursion. + if ece := self.enterprise_course_enrollment: + if not ece.unenrolled: # redundant base case to terminate loops. + ece.set_unenrolled(True) + return changed def reactivate(self, **kwargs): """ Idempotently reactivates this enterprise fulfillment source. - """ - if self.enterprise_course_enrollment: - self.enterprise_course_enrollment.saved_for_later = False - self.enterprise_course_enrollment.unenrolled = False - self.enterprise_course_enrollment.unenrolled_at = None - self.enterprise_course_enrollment.save() - self.is_revoked = False - self.save() + Returns: + bool: True if self.is_revoked was changed. + """ + changed = False + if self.is_revoked: + LOGGER.info(f"Marking fulfillment {str(self)} as reactivated.") + changed = True + self.is_revoked = False + self.save() + # Find and REenroll any related EnterpriseCourseEnrollment. + # By only updating the related object on updates to self, we prevent infinite recursion. + if ece := self.enterprise_course_enrollment: + if ece.unenrolled: # redundant base case to terminate loops. + ece.set_unenrolled(False) + return changed def __str__(self): """ @@ -2337,8 +2411,9 @@ def revoke(self): """ Revoke this LearnerCreditEnterpriseCourseEnrollment, and emit a revoked event. """ - super().revoke() - send_learner_credit_course_enrollment_revoked_event(self) + if changed := super().revoke(): + send_learner_credit_course_enrollment_revoked_event(self) + return changed def reactivate(self, transaction_id=None, **kwargs): """ @@ -2354,7 +2429,7 @@ def reactivate(self, transaction_id=None, **kwargs): f"getting this enrollment for free." ) self.transaction_id = transaction_id - super().reactivate() + return super().reactivate() transaction_id = models.UUIDField( primary_key=False, diff --git a/enterprise/signals.py b/enterprise/signals.py index d16c4ea4d..7de8cd927 100644 --- a/enterprise/signals.py +++ b/enterprise/signals.py @@ -17,7 +17,6 @@ from enterprise.utils import ( NotConnectedToOpenEdX, get_default_catalog_content_filter, - localized_utcnow, unset_enterprise_learner_language, unset_language_of_all_enterprise_learners, ) @@ -364,14 +363,7 @@ def course_enrollment_changed_receiver(sender, **kwargs): # pylint: disable= enterprise_customer_user__user_id=enrollment.user.id, ).first() if enterprise_enrollment and enrollment.is_active: - logger.info( - f"Marking EnterpriseCourseEnrollment as enrolled (unenrolled_at=NULL) for user {enrollment.user} and " - f"course {enrollment.course.course_key}" - ) - enterprise_enrollment.unenrolled = False - enterprise_enrollment.unenrolled_at = None - enterprise_enrollment.saved_for_later = False - enterprise_enrollment.save() + enterprise_enrollment.set_unenrolled(False) # Note: If the CourseEnrollment is being flipped to is_active=False, then this handler is a no-op. # In that case, the `enterprise_unenrollment_receiver` signal handler below will run. @@ -386,13 +378,7 @@ def enterprise_unenrollment_receiver(sender, **kwargs): # pylint: disable=un enterprise_customer_user__user_id=enrollment.user.id, ).first() if enterprise_enrollment: - logger.info( - f"Marking EnterpriseCourseEnrollment as unenrolled for user {enrollment.user} and " - f"course {enrollment.course.course_key}" - ) - enterprise_enrollment.unenrolled = True - enterprise_enrollment.unenrolled_at = localized_utcnow() - enterprise_enrollment.save() + enterprise_enrollment.set_unenrolled(True) def create_enterprise_enrollment_receiver(sender, instance, **kwargs): # pylint: disable=unused-argument diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index 6e8efe7e4..27c9493d8 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -4152,6 +4152,7 @@ def test_requested_recently_unenrolled_subsidy_fulfillment(self): # user. Because the requesting user is an operator user, they should be able to see this enrollment. second_lc_enrollment = factories.LearnerCreditEnterpriseCourseEnrollmentFactory( enterprise_course_enrollment=second_enterprise_course_enrollment, + is_revoked=True, ) self.enterprise_course_enrollment.unenrolled = True @@ -4209,6 +4210,7 @@ def test_recently_unenrolled_fulfillment_endpoint_can_filter_for_modified_after( ) old_learner_credit_enrollment = factories.LearnerCreditEnterpriseCourseEnrollmentFactory( enterprise_course_enrollment=old_enterprise_course_enrollment, + is_revoked=True, ) response = self.client.get( reverse('enterprise-subsidy-fulfillment-unenrolled') + self.unenrolled_after_filter diff --git a/tests/test_enterprise/test_signals.py b/tests/test_enterprise/test_signals.py index 5e5b5a949..0bdf34f19 100644 --- a/tests/test_enterprise/test_signals.py +++ b/tests/test_enterprise/test_signals.py @@ -41,6 +41,7 @@ EnterpriseCustomerFactory, EnterpriseCustomerUserFactory, EnterpriseGroupMembershipFactory, + LearnerCreditEnterpriseCourseEnrollmentFactory, PendingEnrollmentFactory, PendingEnterpriseCustomerAdminUserFactory, PendingEnterpriseCustomerUserFactory, @@ -830,6 +831,7 @@ def test_delete_enterprise_learner_role_assignment_no_user_associated(self): @mark.django_db +@ddt.ddt class TestCourseEnrollmentSignals(TestCase): """ Tests signals associated with CourseEnrollments (that are found in edx-platform). @@ -897,17 +899,27 @@ def test_create_ece_receiver_does_not_call_task_if_ecu_not_exists(self, mock_tas create_enterprise_enrollment_receiver(sender, instance, **kwargs) mock_task.assert_not_called() - def test_course_enrollment_changed_receiver(self): + @ddt.data(True, False) + def test_course_enrollment_changed_receiver(self, create_related_lc_fulfillment): """ Test receiver that is supposed to handle course enrollments being reactivated (re-enrolled). """ # Create an unenrolled EnterpriseCourseEnrollment. enterprise_enrollment = EnterpriseCourseEnrollmentFactory( enterprise_customer_user=self.enterprise_customer_user, + saved_for_later=True, unenrolled=True, unenrolled_at=datetime.now() - timedelta(days=1), ) + # Optionally create a revoked LearnerCreditEnterpriseCourseEnrollment. + lc_fulfillment = None + if create_related_lc_fulfillment: + lc_fulfillment = LearnerCreditEnterpriseCourseEnrollmentFactory( + enterprise_course_enrollment=enterprise_enrollment, + is_revoked=True, + ) + # Simulate a previously inactive course enrollment being re-activated. mock_enrollment_data = mock.Mock() mock_enrollment_data.course.course_key = enterprise_enrollment.course_id @@ -921,20 +933,36 @@ def test_course_enrollment_changed_receiver(self): # Make sure the previously inactive enterprise enrollment has now been re-activated in response to the system # enrollment being re-activated. enterprise_enrollment.refresh_from_db() + assert enterprise_enrollment.saved_for_later is False assert enterprise_enrollment.unenrolled is False assert enterprise_enrollment.unenrolled_at is None - def test_enterprise_unenrollment_receiver(self): + # Make sure the related LC fulfillment is SILL REVOKED. Fulfillment re-activation is unsupported. + if create_related_lc_fulfillment: + lc_fulfillment.refresh_from_db() + assert lc_fulfillment.is_revoked is True + + @mock.patch('enterprise.models.send_learner_credit_course_enrollment_revoked_event') + @ddt.data(True, False) + def test_enterprise_unenrollment_receiver(self, create_related_lc_fulfillment, mock_send_revoked_event): """ Test receiver that is supposed to handle course enrollments being deactivated (unenrolled). """ # Create an enrolled EnterpriseCourseEnrollment. enterprise_enrollment = EnterpriseCourseEnrollmentFactory( enterprise_customer_user=self.enterprise_customer_user, + saved_for_later=False, unenrolled=None, unenrolled_at=None, ) + # Optionally create an enrolled LearnerCreditEnterpriseCourseEnrollment. + lc_fulfillment = None + if create_related_lc_fulfillment: + lc_fulfillment = LearnerCreditEnterpriseCourseEnrollmentFactory( + enterprise_course_enrollment=enterprise_enrollment, + ) + # Simulate a previously active course enrollment being deactivated. mock_enrollment_data = mock.Mock() mock_enrollment_data.course.course_key = enterprise_enrollment.course_id @@ -948,9 +976,20 @@ def test_enterprise_unenrollment_receiver(self): # Make sure the previously active enterprise enrollment has now been deactivated in response to the system # enrollment being deactivated. enterprise_enrollment.refresh_from_db() + assert enterprise_enrollment.saved_for_later is True assert enterprise_enrollment.unenrolled is True assert enterprise_enrollment.unenrolled_at is not None + # Make sure the related LC fulfillment is also revoked, and a signal sent. + if create_related_lc_fulfillment: + lc_fulfillment.refresh_from_db() + assert lc_fulfillment.is_revoked is True + mock_send_revoked_event.assert_called_once() + event_fulfillment = mock_send_revoked_event.call_args.args[0] + assert event_fulfillment.uuid == lc_fulfillment.uuid + else: + mock_send_revoked_event.assert_not_called() + @mark.django_db class TestEnterpriseCatalogSignals(unittest.TestCase): From 9dc750281081323f95306b1c5cd3a33202adc111 Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Tue, 10 Sep 2024 11:07:27 -0700 Subject: [PATCH 5/8] feat: edge case of revoking potentially multiple related fulfillments --- enterprise/models.py | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/enterprise/models.py b/enterprise/models.py index c81824b2e..74d3b03a8 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -2117,11 +2117,16 @@ def learner_credit_fulfillment(self): return associated_fulfillment @property - def fulfillment(self): + def fulfillments(self): """ - Find and return the related EnterpriseFulfillmentSource subclass, or None. + Find and return the related EnterpriseFulfillmentSource subclass, or empty list if there are none. + + Returns: + list of EnterpriseFulfillmentSource subclass instances: all existing related fulfillments """ - return self.license or self.learner_credit_fulfillment + possible_fulfillments = [self.license, self.learner_credit_fulfillment] + existing_fulfillments = [f for f in possible_fulfillments if f] + return existing_fulfillments @cached_property def course_enrollment(self): @@ -2245,14 +2250,16 @@ def set_unenrolled(self, desired_unenrolled): f"and course {self.course_id}" ) self.save() - # Find and revoke/reactivate any related fulfillment. + # Find and revoke/reactivate any related fulfillment if unenrolling the EnterpriseCourseEnrollment. # By only updating the related object on updates to self, we prevent infinite recursion. - if fulfillment := self.fulfillment: - if desired_unenrolled and not fulfillment.is_revoked: - fulfillment.revoke() - # Fulfillment reactivation on ECE reenrollment is unsupported. We'd need to collect a - # transaction UUID from the caller, but the caller at the time of writing is not aware of any - # transaction. + if desired_unenrolled: + for fulfillment in self.fulfillments: + if not fulfillment.is_revoked: # redundant base case to terminate loops. + fulfillment.revoke() + # Fulfillment reactivation on ECE reenrollment is unsupported. We'd need to collect a + # transaction UUID from the caller, but the caller at the time of writing is not aware of any + # transaction. Furthermore, we wouldn't know which fulfillment to reactivate, if there were multiple + # related fulfillment types. def __str__(self): """ From 54e6a30b2f3e13ba2b915b30a2ce1bf9875a54ed Mon Sep 17 00:00:00 2001 From: Prashant Makwana Date: Thu, 12 Sep 2024 10:34:44 -0400 Subject: [PATCH 6/8] Feat: update Braze API key for edx-enterprise (#2238) * chore: updating braze api key variable for enterprise * style: lint fix * chore: bumping up enterprise version * chore: rebase fix * chore: updating version number --- CHANGELOG.rst | 4 ++++ enterprise/__init__.py | 2 +- enterprise/api_client/braze.py | 4 ++-- enterprise/settings/test.py | 4 ++-- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e41faf57e..07032d0ad 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,6 +17,10 @@ Unreleased ---------- * nothing unreleased +[4.25.10] +---------- +* feat: update Enterprise Braze API key + [4.25.9] ---------- * fix: send LEARNER_CREDIT_COURSE_ENROLLMENT_REVOKED from the correct place. diff --git a/enterprise/__init__.py b/enterprise/__init__.py index 487580cd9..06ef6c1a1 100644 --- a/enterprise/__init__.py +++ b/enterprise/__init__.py @@ -2,4 +2,4 @@ Your project description goes here. """ -__version__ = "4.25.9" +__version__ = "4.25.10" diff --git a/enterprise/api_client/braze.py b/enterprise/api_client/braze.py index ca4f33c78..84ab5367c 100644 --- a/enterprise/api_client/braze.py +++ b/enterprise/api_client/braze.py @@ -19,9 +19,9 @@ class BrazeAPIClient(BrazeClient): API client for calls to Braze. """ def __init__(self): - braze_api_key = getattr(settings, 'EDX_BRAZE_API_KEY', None) + braze_api_key = getattr(settings, 'ENTERPRISE_BRAZE_API_KEY', None) braze_api_url = getattr(settings, 'EDX_BRAZE_API_SERVER', None) - required_settings = ['EDX_BRAZE_API_KEY', 'EDX_BRAZE_API_SERVER'] + required_settings = ['ENTERPRISE_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.' diff --git a/enterprise/settings/test.py b/enterprise/settings/test.py index 409c3cd29..8952ae926 100644 --- a/enterprise/settings/test.py +++ b/enterprise/settings/test.py @@ -374,7 +374,7 @@ def root(*args): ENTERPRISE_SSO_ORCHESTRATOR_CONFIGURE_PATH = 'configure' ENTERPRISE_SSO_ORCHESTRATOR_CONFIGURE_EDX_OAUTH_PATH = 'configure-edx-oauth' -EDX_BRAZE_API_KEY='test-api-key' -EDX_BRAZE_API_SERVER='test-api-server' +ENTERPRISE_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' From 81dfa71a121b201861825dfab9edf12844804194 Mon Sep 17 00:00:00 2001 From: Katrina Nguyen <71999631+katrinan029@users.noreply.github.com> Date: Thu, 12 Sep 2024 13:46:00 -0700 Subject: [PATCH 7/8] feat: add username query to filter (#2240) * feat: add username query * feat: add username query to filter --- CHANGELOG.rst | 4 ++++ enterprise/__init__.py | 2 +- .../api/v1/views/enterprise_customer_support.py | 6 ++++-- tests/test_enterprise/api/test_views.py | 13 +++++++++++-- 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 07032d0ad..80ad71a72 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,6 +17,10 @@ Unreleased ---------- * nothing unreleased +[4.25.11] +---------- +* feat: add username query to enterprise customer user query + [4.25.10] ---------- * feat: update Enterprise Braze API key diff --git a/enterprise/__init__.py b/enterprise/__init__.py index 06ef6c1a1..a5d812bd1 100644 --- a/enterprise/__init__.py +++ b/enterprise/__init__.py @@ -2,4 +2,4 @@ Your project description goes here. """ -__version__ = "4.25.10" +__version__ = "4.25.11" diff --git a/enterprise/api/v1/views/enterprise_customer_support.py b/enterprise/api/v1/views/enterprise_customer_support.py index b01474c7f..2141ca38c 100644 --- a/enterprise/api/v1/views/enterprise_customer_support.py +++ b/enterprise/api/v1/views/enterprise_customer_support.py @@ -73,8 +73,10 @@ def filter_queryset_by_user_query(self, queryset, is_pending_user=False): if user_query: if not is_pending_user: - queryset = queryset.filter( - user_id__in=User.objects.filter(Q(email__icontains=user_query)) + queryset = models.EnterpriseCustomerUser.objects.filter( + user_id__in=User.objects.filter( + Q(email__icontains=user_query) | Q(username__icontains=user_query) + ) ) else: queryset = queryset.filter(user_email=user_query) diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index 27c9493d8..ea31413fd 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -9725,8 +9725,17 @@ def test_list_users_filtered(self): 'role_assignments': [ENTERPRISE_LEARNER_ROLE], 'is_admin': False }] - user_query_string = f'?user_query={enterprise_customer_user.user_email}' - url = reverse(self.ECS_ENDPOINT, kwargs={self.ECS_KWARG: enterprise_customer.uuid}) + user_query_string + # search by email + user_query_email = f'?user_query={enterprise_customer_user.user_email}' + url = reverse(self.ECS_ENDPOINT, kwargs={self.ECS_KWARG: enterprise_customer.uuid}) + user_query_email + response = self.client.get(settings.TEST_SERVER + url) + + assert expected_json == response.json().get('results') + assert response.json().get('count') == 1 + + # search by username + user_query_username = f'?user_query={enterprise_customer_user.username}' + url = reverse(self.ECS_ENDPOINT, kwargs={self.ECS_KWARG: enterprise_customer.uuid}) + user_query_username response = self.client.get(settings.TEST_SERVER + url) assert expected_json == response.json().get('results') From 1745b40740e7d32e6077e1ec9ab0e84904dcbda1 Mon Sep 17 00:00:00 2001 From: Hamza Waleed Date: Fri, 13 Sep 2024 12:48:21 +0500 Subject: [PATCH 8/8] fix: add id in pending_enterprise_customer_admin_user serializer (#2239) --- CHANGELOG.rst | 4 ++++ enterprise/__init__.py | 2 +- enterprise/api/v1/serializers.py | 2 +- tests/test_enterprise/api/test_views.py | 9 ++++----- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 80ad71a72..a5296117b 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,6 +17,10 @@ Unreleased ---------- * nothing unreleased +[4.25.12] +---------- +* feat: add username query to enterprise customer user query + [4.25.11] ---------- * feat: add username query to enterprise customer user query diff --git a/enterprise/__init__.py b/enterprise/__init__.py index a5d812bd1..d3dc97873 100644 --- a/enterprise/__init__.py +++ b/enterprise/__init__.py @@ -2,4 +2,4 @@ Your project description goes here. """ -__version__ = "4.25.11" +__version__ = "4.25.12" diff --git a/enterprise/api/v1/serializers.py b/enterprise/api/v1/serializers.py index 98466be51..36ffed664 100644 --- a/enterprise/api/v1/serializers.py +++ b/enterprise/api/v1/serializers.py @@ -1671,7 +1671,7 @@ class PendingEnterpriseCustomerAdminUserSerializer(serializers.ModelSerializer): class Meta: model = PendingEnterpriseCustomerAdminUser fields = ( - 'enterprise_customer', 'user_email' + 'id', 'enterprise_customer', 'user_email' ) def validate(self, attrs): diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index ea31413fd..aa45b85f7 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -891,6 +891,7 @@ def test_post_pending_enterprise_customer_admin_user_creation(self): data = { 'enterprise_customer': self.enterprise_customer.uuid, 'user_email': self.user.email, + 'id': 2 } response = self.client.post(settings.TEST_SERVER + PENDING_ENTERPRISE_CUSTOMER_ADMIN_LIST_ENDPOINT, data=data) @@ -949,11 +950,9 @@ def test_get_pending_enterprise_customer_admin_user(self): self.assertEqual(response.status_code, status.HTTP_200_OK) - expected_data = { - 'enterprise_customer': self.enterprise_customer.uuid, - 'user_email': 'test@example.com' - } - self.assertEqual(response.data, expected_data) + expected_keys = ['enterprise_customer', 'user_email', 'id'] + for key in expected_keys: + self.assertIn(key, response.data) def test_patch_pending_enterprise_customer_admin_user(self): """