Configure default subscriptions for emails and optional targets

[kiskoza]

Issue #, if available: #159

Summary

I added two new entry to ActivityNotification::Config, subscribe_to_email_as_default to change default subscriptions for emails (if you want to differ from subscribe_as_default) and subscribe_to_optional_targets_as_default to change default subscriptions for optional targets. Then I went through the gem, looked for usages of subscribe_as_default and changed them to use the new configs. After introducing the new defaults to every reasonable place, the tests remained green (that was the expected result as the new configs have a fallback to the common default)

Other Information

I still need to do a few steps to finish the PR, but an initial review would be nice

• Keep all existing specs as they are
• Add new specs to check the behavior of the new configs
• Make some manual tests on example rails app

[coveralls]

Coverage remained the same at 100.0% when pulling b06ef28 on kiskoza:configure_default_subscriptions into dde5103 on simukappu:master.

[kiskoza]

Hi there,

I'm ready with this PR. I added new rspec tests to exercise the new configs, added them to Functions.md and made the views aware of the new optional defaults.

I hope I did everything I had to and we could merge it in soon, just ping me if I need to change anything :)

[simukappu]

I've added several comments for your PR. Thank you for your contribution!

[simukappu]

Why do you set default parameters in a view? It seems to be too complex and it might become breaking changes for the existing users using customized views. I think subscription records should be initialized with default subscription in server side business logic, not front end views. What do you think about it?

[kiskoza]

Nice catch. I think I just haven't found the right place at first. There are two separate changes in this line.

option_params.merge(with_email_subscription: ActivityNotification.config.subscribe_to_email_as_default, with_optional_targets: ActivityNotification.config.subscribe_to_optional_targets_as_default)

This should be reverted, there's a better place for it in subscriptions_controller.rb#L93-L97, I just need to change the to_boolean(true) conversions to use the right default from config.

.find("input").prop("checked", #{ActivityNotification.config.subscribe_to_email_as_default.to_s});

This code should remain here to remove a flick in the transitions. Without this, for a few seconds the frontend could show a wrong state, then it got rewritten with the response from the backend.

I'm going to make a new commit about these.

[simukappu]

Same as above

[kiskoza]

Here the original code already contained with_email_subscription: false, so I thought we either need to remove that or add optional_targets too. On current master, it looks like this with config.subscribe_as_default = false in the initializer:

1. I open the page
   
2. Immediately after I turn on the notifications
   
3. A few moments later, when the server responded with a partial
   

With the changes I added to solve your other comment, these with_* overrides will become unneccessary, I'm going to remove them in a commit.

[kiskoza]

Hi @simukappu . Thanks for the review, I found them very helpful. I replied to your comments and made changes where I felt necessary.

[simukappu]

LGTM. Also worked fine with my env.