From 9009484a35332f8ea1e9f4a35fab048f8bf5cc27 Mon Sep 17 00:00:00 2001 From: Yash Kumar Date: Sat, 21 Mar 2026 20:42:18 +0530 Subject: [PATCH] [fix] Resync org settings on repeated global email disable #431 Preserve explicit global preference updates from the preferences API so reapplying the same global state still propagates to organization notification settings, add API regression coverage for repeated global email and web updates, and include the CI-requested formatting fix. Fixes #431 --- openwisp_notifications/api/serializers.py | 10 ++ openwisp_notifications/base/models.py | 20 +++- openwisp_notifications/tests/test_api.py | 92 +++++++++++++++++++ openwisp_notifications/tests/test_selenium.py | 6 +- 4 files changed, 121 insertions(+), 7 deletions(-) diff --git a/openwisp_notifications/api/serializers.py b/openwisp_notifications/api/serializers.py index 42d91348..cd799e9c 100644 --- a/openwisp_notifications/api/serializers.py +++ b/openwisp_notifications/api/serializers.py @@ -91,6 +91,16 @@ def to_representation(self, instance): data["email"] = instance.email_notification return data + def update(self, instance, validated_data): + explicit_global_fields = { + field for field in ["web", "email"] if field in validated_data + } + if explicit_global_fields and not instance.organization and not instance.type: + # The preferences UI can explicitly reapply the same global state. + # Preserve that intent so the model can resync organization rows. + instance._explicit_global_update_fields = explicit_global_fields + return super().update(instance, validated_data) + class IgnoreObjectNotificationSerializer(serializers.ModelSerializer): class Meta: diff --git a/openwisp_notifications/base/models.py b/openwisp_notifications/base/models.py index 002611fa..3bf1f016 100644 --- a/openwisp_notifications/base/models.py +++ b/openwisp_notifications/base/models.py @@ -445,15 +445,26 @@ def validate_global_setting(self): raise ValidationError("There can only be one global setting per user.") def save(self, *args, **kwargs): + explicit_global_update_fields = set( + getattr(self, "_explicit_global_update_fields", []) + ) + if hasattr(self, "_explicit_global_update_fields"): + delattr(self, "_explicit_global_update_fields") if not self.web_notification: self.email = self.web_notification with transaction.atomic(): if not self.organization and not self.type: try: - previous_state = self.__class__.objects.only("email").get( + previous_state = self.__class__.objects.only("web", "email").get( pk=self.pk ) - updates = {"web": self.web} + updates = {} + + if ( + self.web != previous_state.web + or "web" in explicit_global_update_fields + ): + updates["web"] = self.web # If global web notifications are disabled, then disable email notifications as well if not self.web: @@ -462,7 +473,10 @@ def save(self, *args, **kwargs): # Update email notifiations only if it's different from the previous state # Otherwise, it would overwrite the email notification settings for specific # setting that were enabled by the user after disabling global email notifications - if self.email != previous_state.email: + if ( + self.email != previous_state.email + or "email" in explicit_global_update_fields + ): updates["email"] = self.email self.user.notificationsetting_set.exclude(pk=self.pk).update( diff --git a/openwisp_notifications/tests/test_api.py b/openwisp_notifications/tests/test_api.py index 34a514be..1de366a9 100644 --- a/openwisp_notifications/tests/test_api.py +++ b/openwisp_notifications/tests/test_api.py @@ -794,6 +794,98 @@ def test_update_notification_setting_api(self): ) self.assertEqual(response.status_code, 403) + def test_reapplying_global_email_setting_updates_organization_settings(self): + org = self._get_org("default") + global_setting = NotificationSetting.objects.get( + user=self.admin, type=None, organization=None + ) + global_setting_url = self._get_path("notification_setting", global_setting.pk) + org_setting_url = self._get_path( + "user_org_notification_setting", self.admin.pk, org.pk + ) + + response = self.client.patch( + global_setting_url, + data={"email": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 200) + self.assertFalse( + NotificationSetting.objects.filter( + user=self.admin, organization=org, email=True + ).exists() + ) + + response = self.client.post( + org_setting_url, + data={"web": True, "email": True}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 200) + self.assertTrue( + NotificationSetting.objects.filter( + user=self.admin, organization=org, email=True + ).exists() + ) + + response = self.client.patch( + global_setting_url, + data={"email": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 200) + self.assertFalse( + NotificationSetting.objects.filter( + user=self.admin, organization=org, email=True + ).exists() + ) + + def test_reapplying_global_web_setting_updates_organization_settings(self): + org = self._get_org("default") + global_setting = NotificationSetting.objects.get( + user=self.admin, type=None, organization=None + ) + global_setting_url = self._get_path("notification_setting", global_setting.pk) + org_setting_url = self._get_path( + "user_org_notification_setting", self.admin.pk, org.pk + ) + + response = self.client.patch( + global_setting_url, + data={"web": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 200) + self.assertFalse( + NotificationSetting.objects.filter( + user=self.admin, organization=org, web=True + ).exists() + ) + + response = self.client.post( + org_setting_url, + data={"web": True, "email": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 200) + self.assertTrue( + NotificationSetting.objects.filter( + user=self.admin, organization=org, web=True + ).exists() + ) + + response = self.client.patch( + global_setting_url, + data={"web": False}, + content_type="application/json", + ) + self.assertEqual(response.status_code, 200) + self.assertFalse( + NotificationSetting.objects.filter( + user=self.admin, organization=org, web=True + ).exists() + ) + def test_disallowed_change_types_absent_in_notification_setting_api(self): with self.subTest("disallowed type setting not present in list"): path = self._get_path("notification_setting_list") diff --git a/openwisp_notifications/tests/test_selenium.py b/openwisp_notifications/tests/test_selenium.py index 885d0931..e3e3d0d4 100644 --- a/openwisp_notifications/tests/test_selenium.py +++ b/openwisp_notifications/tests/test_selenium.py @@ -171,13 +171,11 @@ def test_email_unsubscribe_page(self): with self.subTest("Network request fails"): self.open(unsubscribe_link) - self.web_driver.execute_script( - """ + self.web_driver.execute_script(""" window.fetch = function() { return Promise.reject(new Error('Simulated fetch failure')); }; - """ - ) + """) self.web_driver.find_element(By.ID, "toggle-btn").click() self.wait_for_visibility(By.ID, "error-msg") browser_logs = self.get_browser_logs()