Subscribe the newsletter if email_newsletter is changed (#14226)

* Check if email_newsletter is changed

* Check if Settings::General.mailchimp_api_key is defined

* Add spec for Users::NotificationSetting

* Remove spec for boolean column

http://www.chrisrolle.com/en/blog/boolean-attribute-validation

* Check if email is set when changing newsletter subscription setting

* Make it easy to read

* Remove specs for a case never happening

app/models/users/notification_setting.rb

@@ -13,7 +13,9 @@ module Users
     after_commit :subscribe_to_mailchimp_newsletter
 
     def subscribe_to_mailchimp_newsletter
-      return unless email_newsletter
+      return if Settings::General.mailchimp_api_key.blank?
+      return unless saved_changes.key?(:email_newsletter)
+      return if user.email.blank?
 
       Users::SubscribeToMailchimpNewsletterWorker.perform_async(user.id)
     end

spec/models/users/notification_setting_spec.rb

@@ -0,0 +1,43 @@
+require "rails_helper"
+
+RSpec.describe Users::NotificationSetting, type: :model do
+  let!(:user) { create(:user) }
+  let(:notification_setting) { user.notification_setting.reload }
+
+  context "when callbacks are triggered after commit" do
+    describe "subscribing to mailchimp newsletter" do
+      it "enqueues SubscribeToMailchimpNewsletterWorker when updating email_newsletter to true" do
+        sidekiq_assert_enqueued_with(job: Users::SubscribeToMailchimpNewsletterWorker, args: [user.id]) do
+          notification_setting.update(email_newsletter: true)
+        end
+      end
+
+      it "enqueues SubscribeToMailchimpNewsletterWorker when updating email_newsletter to false" do
+        notification_setting.update(email_newsletter: true)
+        sidekiq_assert_enqueued_jobs(1, only: Users::SubscribeToMailchimpNewsletterWorker) do
+          notification_setting.update(email_newsletter: false)
+        end
+      end
+
+      it "does not enqueue if email is not set" do
+        user.update(email: "")
+        sidekiq_assert_no_enqueued_jobs(only: Users::SubscribeToMailchimpNewsletterWorker) do
+          notification_setting.update(email_newsletter: !notification_setting.email_newsletter)
+        end
+      end
+
+      it "does not enqueue if Mailchimp is not enabled" do
+        allow(Settings::General).to receive(:mailchimp_api_key).and_return(nil)
+        sidekiq_assert_no_enqueued_jobs(only: Users::SubscribeToMailchimpNewsletterWorker) do
+          notification_setting.update(email_newsletter: !notification_setting.email_newsletter)
+        end
+      end
+
+      it "does not enqueue without updating email_newsletter" do
+        sidekiq_assert_no_enqueued_jobs(only: Users::SubscribeToMailchimpNewsletterWorker) do
+          notification_setting.update(email_badge_notifications: !notification_setting.email_badge_notifications)
+        end
+      end
+    end
+  end
+end

spec/requests/user/user_notification_settings_spec.rb

@@ -36,13 +36,6 @@ RSpec.describe "UserNotificationSettings", type: :request do
           params: { users_notification_setting: { tab: "notifications", welcome_notifications: 1 } }
       expect(user.reload.subscribed_to_welcome_notifications?).to be(true)
     end
-
-    it "returns error message if settings can't be saved" do
-      put users_notification_settings_path(user.notification_setting.id),
-          params: { users_notification_setting: { tab: "notifications", email_digest_periodic: nil } }
-
-      expect(flash[:error]).not_to be_blank
-    end
   end
 
   describe "PATCH /onboarding_notifications_checkbox_update" do
@@ -81,13 +74,5 @@ RSpec.describe "UserNotificationSettings", type: :request do
               params: { notifications: { tab: "notifications", email_digest_periodic: 0 } }
       end.to change { user.notification_setting.reload.email_digest_periodic }.from(true).to(false)
     end
-
-    it "returns 422 status and errors if errors occur" do
-      patch onboarding_notifications_checkbox_update_path(format: :json),
-            params: { notifications: { tab: "notifications", email_digest_periodic: nil } }
-
-      expect(response.status).to eq(422)
-      expect(response.parsed_body["errors"]).not_to be_blank
-    end
   end
 end