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

Remove the usage of instance variables from Noticed::Base #126

Merged
merged 1 commit into from
Jun 1, 2021

Conversation

manojmj92
Copy link
Contributor

@manojmj92 manojmj92 commented May 30, 2021

Since we are attaching record and recipient to the Notification object just before the delivery, there is no longer a requirement to set these from within Noticed::Base.

recipient was already being passed as an argument to run_delivery_method. Now record, if it exists (ie, the database record) is also passed as an argument to the same run_delivery_method method.

Both of these will now be available in the perform method of DeliveryMethods::Base, where it will be attached to the @notification object.

I'm trying implement #89, and this cleanup helps :)

@manojmj92
Copy link
Contributor Author

@excid3 Hi Chris, this is ready for review 🙂

@excid3 excid3 merged commit e33aa24 into excid3:master Jun 1, 2021
@silva96
Copy link
Contributor

silva96 commented Nov 15, 2021

Hey I'm a bit too late here but, removing @record on the database notification (former line 80) breaks my code:

notification = MyNotification.with(my_param: 'something').deliver(some_user)
if some_user.special_thing
  notification.record.mark_as_read!
end

In the new code, calling notification.record is nil

@excid3 @manojmj92

@excid3
Copy link
Owner

excid3 commented Nov 15, 2021

#176

@silva96
Copy link
Contributor

silva96 commented Nov 15, 2021

thanks @excid3 i'll have to wait then until that is merged.

clinejj added a commit to clinejj/noticed that referenced this pull request Nov 18, 2021
excid3 pushed a commit that referenced this pull request Nov 19, 2021
* Revert instance @recipient removal from #126

* Add tests/lint

Signed-off-by: John Cline <[email protected]>

* Prefer self for consistency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants