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

Allow Notification#target to be optional to allow more flexible sending to a dynamic "group" #52

Closed
TylerRick opened this issue Jan 11, 2019 · 1 comment · Fixed by #94
Labels

Comments

@TylerRick
Copy link
Collaborator

As a follow-up thought to #50, I wonder if instead of requiring target to always be present, I wonder if we should allow it to be omitted, and just fix notification-settings to not break if target is missing...

`notification_setting' for nil:NilClass

The specific use case I'm considering currently is that I have some notifications that should go out to a group of people (multiple recipients) in a single email (so that all the recipients can see each other's names in the To: line of the email.

I don't see an easy way to do that currently.

I see that you can actually specify a group for a notification instead of a target — but it looks like that (before_validation :create_for_group) actually just "flattens" your group, duplicates the notification, and creates a copy of the same notification with target set to each member of the group. So yes, it provides a facade of sending to a group, but ultimately the "send to group" request gets translated into a bunch of "send to individual" operations.

I don't think that would work for my use case, because (I think) it would try to push an individual/separate email for each one.

Options that come to mind...

  • don't go through / create a Notification for these emails at all for these — just "special case" them in the app
  • create a special GroupNotification class/subclass that knows how to send a single notification/email to a gorup. Have this single notification record represent/handle sending itself to a "group".
  • have multiple discrete notifications, but group/aggregate these together somehow in the pusher and not allow the individual notifications to get pushed individually
  • create a Group record that we can use as the target

The other reason why it seems the current "group" feature wouldn't work for my case is that the groups are dynamic. The groups I need are more like a combination of a role + a group/company where they have that role — for example, one group is everyone matching {role: :manager, at_company: Company.find(3)} and another group is {role: :member, at_company: Company.find(4)}. There could be any number of them; they can't really be enumerated in the configuration file...

config.define_group :members_company_4, User.with_role(:member, Company.find(4))

Since I can't use the existing group feature, it seems like I need to build another way of sending to groups... which led me to realize that having target be strictly required could actually be a problem.

If only there were a database record for each group that I needed to send to... Then I could simply set the polymorphic target to that Group or Company record and it would just work. But if I need to be more specific and say "send to all {role} people at {company}", then I don't know how to get that to work.

One crazy idea I just had would be to add a new serialized group_spec column that let you specify any arbitrary data structure as the group target. (Can't use target_type/target_id columns for that since they're strongly typed as just a type + id.) Then you could store something like {role: :member, at_company: 4} and be able to interpret that at "push" time.

Or, if you could safely serialize an actual ActiveRecord::Relation (store in a new target_relation column?), that might be an even better option...

I don't want to make things crazy complicated though.

I'll have to keep thinking about this...

@jonhue
Copy link
Owner

jonhue commented Jan 11, 2019

I'm not sure if that helps. But you can send one notification to multiple recipients. Just do notification.deliver(:email, to: ['a', 'b', 'c']).

@jonhue jonhue added the enhancement New feature or request label Jan 11, 2019
@jonhue jonhue pinned this issue May 16, 2019
@jonhue jonhue unpinned this issue May 29, 2019
jonhue added a commit that referenced this issue May 29, 2019
jonhue added a commit that referenced this issue May 30, 2019
… sending to a dynamic "group" (#94)

* [#52] Allow Notification#target to be optional to allow more flexible sending to a dynamic "group"

* some fixes
@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
Projects
None yet
2 participants