Subscribe the newsletter if email_newsletter is changed #14226

takmar · 2021-07-13T16:44:46Z

What type of PR is this? (check all applicable)

Description

In Users::NotificationSetting model, Users::SubscribeToMailchimpNewsletterWorker is executed ONLY the case email_newsletter is true. So a user can not unsubscribe from the newsletter by updating email_newsletter to false.

To fix this, I changed subscribe_to_mailchimp_newsletter method to check saved_changes.key?(:email_newsletter) instead of email_newsletter.

And I also make it to check Settings::General.mailchimp_api_key.blank? the same as User model.

forem/app/models/user.rb

Lines 509 to 511 in e50db31

    
           def subscribe_to_mailchimp_newsletter 
        
             return unless registered && email.present? 
        
             return if Settings::General.mailchimp_api_key.blank?

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Please replace this line with instructions on how to test your changes, a note
on the devices and browsers this has been tested on, as well as any relevant
images for UI changes.

UI accessibility concerns?

If your PR includes UI changes, please replace this line with details on how
accessibility is impacted and tested. For more info, check out the
Forem Accessibility Docs.

Added/updated tests?

Yes
No, and this is why: please replace this line with details on why tests
have not been included
I need help with writing tests

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the
development process, or any of our internal teams? If so, please note how you
will share this change with the people who need to know about it.

I've updated the Developer Docs and/or
Admin Guide, or
Storybook (for Crayons components)
I've updated the README or added inline documentation
I will share this change in a Changelog
or in a forem.dev post
I will share this change internally with the appropriate teams
I'm not sure how best to communicate this change and need help
This change does not need to be communicated, and this is why not: please
replace this line with details on why this change doesn't need to be
shared

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

github-actions · 2021-07-13T16:45:01Z

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

takmar · 2021-07-13T16:45:48Z

spec/models/users/notification_setting_spec.rb

+
+      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


I used not sidekiq_assert_enqueued_with but sidekiq_assert_enqueued_jobs for checking the jobs count. Because L24 pushes a Users::SubscribeToMailchimpNewsletterWorker job as well.

msarit

Great job, @takmar! 🚀

Just a few small changes requested.

msarit · 2021-07-13T18:06:37Z

app/models/users/notification_setting.rb

@@ -13,7 +13,8 @@ class NotificationSetting < ApplicationRecord
    after_commit :subscribe_to_mailchimp_newsletter

    def subscribe_to_mailchimp_newsletter
-      return unless email_newsletter
+      return if Settings::General.mailchimp_api_key.blank?


@takmar This is an amazing catch Takmar, thank you so much! 🙏🏾

msarit · 2021-07-13T18:09:38Z

spec/models/users/notification_setting_spec.rb

+  let!(:user) { create(:user) }
+  let(:notification_setting) { described_class.find_by(user_id: user.id) }
+
+  describe "validations" do


I'm tagging @citizen428 and @rhymes to get their take on this, @takmar, but I believe we can remove this entire describe block, and the corresponding solitary validation in app/models/users/notification_setting.rb:7 (line 7).

When running the spec, I get the following warning from shoulda-matchers gem:

Warning from shoulda-matchers: You are using `validate_inclusion_of` to assert that a boolean column allows boolean values and disallows non-boolean ones. Be aware that it is not possible to fully test this, as boolean columns will automatically convert non-boolean values to boolean ones. Hence, you should consider removing this test.

Yes, we can remove this, see http://www.chrisrolle.com/en/blog/boolean-attribute-validation.

I removed the code related to validation. Thank you!
93095a3

spec/models/users/notification_setting_spec.rb

citizen428 · 2021-07-14T03:40:14Z

spec/models/users/notification_setting_spec.rb

+  let!(:user) { create(:user) }
+  let(:notification_setting) { described_class.find_by(user_id: user.id) }
+
+  describe "validations" do


Yes, we can remove this, see http://www.chrisrolle.com/en/blog/boolean-attribute-validation.

takmar · 2021-07-14T10:56:02Z

I've noticed that the email column is nullable, so I also added the line to check an email existence to avoid enqueuing a job to subscribe to the newsletter without an email.
940601c

rhymes · 2021-07-14T14:43:01Z

@takmar thanks for the changes! Looks like there are two legit failures in the tests here https://app.travis-ci.com/github/forem/forem/jobs/524379134#L913-L914

rspec ./spec/requests/user/user_notification_settings_spec.rb:40 # UserNotificationSettings PUT /update/:id returns error message if settings can't be saved

rspec ./spec/requests/user/user_notification_settings_spec.rb:85 # UserNotificationSettings PATCH /onboarding_notifications_checkbox_update returns 422 status and errors if errors occur

Can you take a look?

takmar · 2021-07-18T05:19:08Z

@rhymes
Thank you for the information!

The reason for both of the test failures is that the controller subjected to testing raised the ActiveRecord::NotNullViolation error when those tests set nil to the non-nullable email_digest_periodic column. This is because of the removal of the validation for email_digest_periodic.

Though the failure tests expected the controller to set an error message in case of validation failure, there are no longer validations on the model. So we can not test that case anymore.

I established the following ways to manage the failures:

Remove those tests.
Add boolean column validations to the model in another way that does not show a warning.

Though I removed the validation showing the warning, Rails Guides introduce another way to validate a boolean column.

validates :boolean_field_name, exclusion: [nil]

When I tried to implement a validation this way and added the test for it with the validate_exclusion_of matcher, the warnings no longer appeared. This will prevent unexpectedly raising the error and set an error message to a model instance, so I think this could be a better way to deal with the failures.

What do you think about these? Or do you have any other ideas?

citizen428 · 2021-07-19T02:08:41Z

validates :boolean_field_name, exclusion: [nil]

@takmar Wouldn't presence: true also work and be a bit more straightforward?

takmar · 2021-07-19T16:03:46Z

@citizen428
Though it is certainly easy to understand, In the case a boolean column value is false, presence validation always fails. Because the PresenceValidator calls the blank? method for validation. So we can't use presence validation for a boolean column.

https://github.com/rails/rails/blob/main/activemodel/lib/active_model/validations/presence.rb#L5-L8

citizen428 · 2021-07-20T03:49:02Z

Yup @takmar, my bad, I didn't properly take the context of our discussion (booleans) into account 🤦‍♂️ 😃

I'm personally of the opinion that boolean columns should always be non-nullable dealing with a tri-state is almost never what one wants.

takmar · 2021-07-26T01:18:05Z

dealing with a tri-state is almost never what one wants

@citizen428 Let me check if I understood correctly. Does this mean it's better to add a validation to check if nil for a boolean column?

citizen428 · 2021-07-26T02:53:32Z

I just meant that boolean columns should pretty much always have a non-null constraint in the DB. Whether or not we put a validation in front of that depends a lot on where/how a value is used, i.e. how important it is to communicate this back to the user in a friendlier way than an exception.

citizen428 · 2021-08-10T04:45:05Z

`It seems we have legitimate spec failures right now. On a similar PR we recently made the call to keep the boolean validation in until we have a more comprehensive strategy across the app. So if you want to re-add it that's fine.

takmar · 2021-08-20T10:07:02Z

I'm sorry for my late response. I pushed another commit to fix the failures in the tests. I ended up adding validations in the way I described before.

On a similar PR we recently made the call to keep the boolean validation in until we have a more comprehensive strategy across the app. So if you want to re-add it that's fine.

Though I implemented the validations for now, feel free to remove them when you decided on the strategy.

aitchiss · 2021-09-07T07:52:18Z

@msarit @citizen428 just a quick ping as the specs are passing now 🙂

citizen428 · 2021-09-08T03:10:22Z

app/models/users/notification_setting.rb

@@ -4,7 +4,21 @@ class NotificationSetting < ApplicationRecord

    belongs_to :user, touch: true

-    validates :email_digest_periodic, inclusion: { in: [true, false] }
+    validates :email_badge_notifications, exclusion: [nil]


I'm still not super convinced by the need for these validations, as we ensure database integrity via non-null constraints and the UI should never generate a nil here. But as you said, we can decide on this later, it's ok to leave it in for now.

the UI should never generate a nil here.

When I read this, I have finally understood what I felt something wrong.

I was guessing these sending nil tests below(previously failed tests in this PR) are for a case that a user sends nil deliberately by rewriting HTML or doing something like that.

forem/spec/requests/user/user_notification_settings_spec.rb

Lines 40 to 44 in 1253d90

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

forem/spec/requests/user/user_notification_settings_spec.rb

Lines 85 to 91 in 1253d90

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

As you said, a user never sends nil usually, so I think it would be better to remove these tests.
And do so, we could also remove all the validations on the model.

What do you think?

Ah, I think this is where we previously didn't quite manage to get on the same page 😃 I always approached this from the point of our UI where this is a checkbox:

The way Rails handles these is via a hidden field with a value of "0", so an unchecked box also won't result in nil.

So yes, I think we can remove these specs and the newly added validations, if someone really manages to send a nil here I'd be ok with the exception from the DB bubbling up because it would truly be an exceptional circumstance.

I think we can remove these specs and the newly added validations

I removed the specs and the validations I added.
(I think email_digest_periodic validation is also unnecessary.)

if someone really manages to send a nil here I'd be ok with the exception from the DB bubbling up because it would truly be an exceptional circumstance.

I totally agree with you 👍

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

msarit

Great work @takmar!

takmar added 3 commits July 14, 2021 00:48

Check if email_newsletter is changed

66237ac

Check if Settings::General.mailchimp_api_key is defined

b0ee12a

Add spec for Users::NotificationSetting

3f87b66

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jul 13, 2021

takmar commented Jul 13, 2021

View reviewed changes

msarit suggested changes Jul 13, 2021

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Jul 13, 2021

citizen428 reviewed Jul 14, 2021

View reviewed changes

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Jul 14, 2021

rhymes requested review from msarit and citizen428 July 14, 2021 14:41

citizen428 approved these changes Sep 8, 2021

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Sep 8, 2021

takmar added 2 commits September 10, 2021 15:01

Remove spec for boolean column

f33a61d

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

Check if email is set when changing newsletter subscription setting

76cf4e7

takmar added 2 commits September 10, 2021 15:01

Make it easy to read

f919b24

Remove specs for a case never happening

4fce7d6

takmar force-pushed the takmar/fix_email_newsletter_of_notification_setting branch from 6100d9e to 4fce7d6 Compare September 10, 2021 06:07

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Sep 10, 2021

citizen428 requested a review from rhymes September 27, 2021 04:19

msarit approved these changes Oct 5, 2021

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 5, 2021

citizen428 merged commit baab2c0 into forem:main Oct 6, 2021

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 6, 2021

Subscribe the newsletter if email_newsletter is changed #14226

Subscribe the newsletter if email_newsletter is changed #14226

takmar commented Jul 13, 2021

github-actions bot commented Jul 13, 2021

takmar Jul 13, 2021

msarit left a comment

msarit Jul 13, 2021

msarit Jul 13, 2021

citizen428 Jul 14, 2021

takmar Jul 14, 2021

citizen428 Jul 14, 2021

takmar commented Jul 14, 2021 •

edited

rhymes commented Jul 14, 2021

takmar commented Jul 18, 2021 •

edited

citizen428 commented Jul 19, 2021

takmar commented Jul 19, 2021

citizen428 commented Jul 20, 2021

takmar commented Jul 26, 2021

citizen428 commented Jul 26, 2021

citizen428 commented Aug 10, 2021

takmar commented Aug 20, 2021

aitchiss commented Sep 7, 2021

citizen428 Sep 8, 2021

takmar Sep 8, 2021 •

edited

citizen428 Sep 10, 2021

takmar Sep 10, 2021

msarit left a comment

	def subscribe_to_mailchimp_newsletter
	return unless registered && email.present?
	return if Settings::General.mailchimp_api_key.blank?

	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

	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

Subscribe the newsletter if email_newsletter is changed #14226

Subscribe the newsletter if email_newsletter is changed #14226

Conversation

takmar commented Jul 13, 2021

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

UI accessibility concerns?

Added/updated tests?

[Forem core team only] How will this change be communicated?

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

github-actions bot commented Jul 13, 2021

Choose a reason for hiding this comment

msarit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takmar commented Jul 14, 2021 • edited

rhymes commented Jul 14, 2021

takmar commented Jul 18, 2021 • edited

citizen428 commented Jul 19, 2021

takmar commented Jul 19, 2021

citizen428 commented Jul 20, 2021

takmar commented Jul 26, 2021

citizen428 commented Jul 26, 2021

citizen428 commented Aug 10, 2021

takmar commented Aug 20, 2021

aitchiss commented Sep 7, 2021

Choose a reason for hiding this comment

takmar Sep 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msarit left a comment

Choose a reason for hiding this comment

takmar commented Jul 14, 2021 •

edited

takmar commented Jul 18, 2021 •

edited

takmar Sep 8, 2021 •

edited