Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically build notification_setting if user doesn't have one yet #33

Closed
TylerRick opened this issue Dec 28, 2018 · 2 comments
Closed
Labels
bug Something isn't working notification-settings

Comments

@TylerRick
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

The Readme has this example:

s = User.first.notification_setting
s.settings[:enabled] = false

But when I tried this example after adding notification-settings to an existing project, I got this error instead:

main > s = User.first.notification_setting
=> nil

main > s.settings[:enabled] = false
NoMethodError: undefined method `settings' for nil:NilClass

Describe the solution you'd like

It would be nice if a settings target/object could safely assume that it always has an associated notification_setting.

It looks like this callback was added in order to help ensure that a notification_setting always exists for new users:

before_create :create_notification_setting

... but that doesn't help in the case of pre-existing user records.

I propose (instead of that) overriding notification_setting such that it builds a new setting if one doesn't already exist:

      def notification_setting
        super || build_notification_setting
      end

I've tried it locally and it works fine...

Describe alternatives you've considered

Update the migration generator to create settings records for any existing [user] records.

@jonhue
Copy link
Owner

jonhue commented Dec 28, 2018

Would you be willing to submit a pull request?

@jonhue jonhue added bug Something isn't working notification-settings labels Dec 28, 2018
TylerRick added a commit to TylerRick/notifications-rails that referenced this issue Jan 2, 2019
@TylerRick
Copy link
Collaborator Author

Sure! Submitted #40...

@jonhue jonhue closed this as completed in #40 Jan 2, 2019
jonhue pushed a commit that referenced this issue Jan 2, 2019
@jonhue jonhue mentioned this issue May 3, 2019
@jonhue jonhue mentioned this issue Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working notification-settings
Projects
None yet
Development

No branches or pull requests

2 participants