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

We might be sending notifications to the wrong users #4640

Closed
mrcasals opened this issue Dec 7, 2018 · 3 comments
Closed

We might be sending notifications to the wrong users #4640

mrcasals opened this issue Dec 7, 2018 · 3 comments
Labels
type: bug Issues that describe a bug

Comments

@mrcasals
Copy link
Contributor

mrcasals commented Dec 7, 2018

Describe the bug
After #4282, authors are a polymorphic resource. Now take this piece of code from current master:

def notify_author(proposal)
Decidim::EventsManager.publish(
event: "decidim.events.proposals.proposal_update_category",
event_class: Decidim::Proposals::Admin::UpdateProposalCategoryEvent,
resource: proposal,
recipient_ids: proposal.coauthorships.pluck(:decidim_author_id)
)
end

The recipient_ids gets a list of author IDs and sends the notification to those users, but it assumes they are users. But since the relation is now polymorphic, we might be getting the ID of an Organization, and treat it like a user, which is wrong.

We have two options:

  1. Ensure we're only sending IDs of users
  2. Instead of sending IDs, send the whole object (Rails will convert them with the GlobalID system), and when we receive the event we drop anything that is not a user. This system is safer, but it implies a change of the API.

Expected behavior
Notifications are sent only to users

Screenshots
None

Stacktrace
None

Extra data (please complete the following information):

  • Decidim Version: master

Additional context
Add any other context about the problem here.

@mrcasals mrcasals added the type: bug Issues that describe a bug label Dec 7, 2018
@tramuntanal
Copy link
Contributor

I'd bet for option 2, send object id and type, but then we can not ignore whatever is not a User because when the recipient is an Organization it means that we're in front of an official resource. And the event may need response from Admis, so they must be notified.

Also it would be great to log all unknown entities for better debugging problems in production environments.

@mrcasals
Copy link
Contributor Author

#4663 separates the recipients between "affected_users" and "followers" of the affected resource. When I implemented the PR I took the chance and changed how we send the notification: now instead of sending IDs, we're sending the whole resource (kind of what the Option 2 in the opening post suggests).

Closing this!

@aitorlb
Copy link
Contributor

aitorlb commented Mar 22, 2019

I'm having issues in #4771 because the recipient is an organization:

Decidim::EmailNotificationGenerator
recipient = #Decidim::Organization:0x00007f9be41a1530
recipient.respond_to?(:notification_types) = false

Decidim::NotificationGenerator
recipient = #Decidim::Organization:0x00007f9be41a1530
recipient.respond_to?(:notification_types) = false

[ActiveJob] Error performing ... from Async(events) in 54.25ms: NoMethodError (undefined method `notification_types' for #Decidim::Organization:0x00007f9be41a1530

EDITED: fixed in #5001

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Issues that describe a bug
Projects
None yet
Development

No branches or pull requests

3 participants