Skip to content

[WIP] Follow events#1695

Closed
mrcasals wants to merge 26 commits intomasterfrom
follow-meetings
Closed

[WIP] Follow events#1695
mrcasals wants to merge 26 commits intomasterfrom
follow-meetings

Conversation

@mrcasals
Copy link
Copy Markdown
Contributor

@mrcasals mrcasals commented Aug 7, 2017

🎩 What? Why?

WIP

We want users to be able to follow some resources in Decidim, we'll start by Meetings as per #1692 but the implementation should take a polymorphic account so following other resources should be easy to implement.

I'm using ActiveSupport::Notifications to notify events to the whole application and ActiveJob to act on those events.

📌 Related Issues

📋 Subtasks

  • Add the Follow entity, which relates a user and a Followable resource (unique per user/resource combo)
  • Add the Notification entity, which is related to a user and a Followable resource. Not unique. Needs to have an notification_type so that notification templates can be customized in a per-event basis.
  • Create a Followable concern/module encapsulating all the logic for followable resources (only Meetings in this PR) and make it include Notifiable
  • Add events
  • Add event_class param to Decidim::Events. The event class must be a class that wraps the event name and the resource and builds the needed information to publish the event to the different subscribers in the system.
  • When an event is triggered for a Followable resource, for each follower trigger a job to create a notification. This job should check whether the resource is notifiable for the given user or not (by default, true, but this might change for proposals - eg. the proposal author does not want to be notified when they update that proposal). This should be checked in the notifiable?(context) method in the Notifiable concern. Also, Followable should set the users_to_notify method to followers.
  • Make event emitters know which users should receive the notifications. Send an array of user IDs along with the event, and make the jobs check for that array when generating the notification. This allows us to decouple the notifications generation system from the Followable concern and generate system-wide notifications about things that are not followed by a given user.
  • Create an event_class for each event.
  • Create an EmailNotificationGenerator, with its needed jobs, so that events can generate emails.
  • Move email texts to I18n
  • Add a Notifications Dashboard where a user can check all their notifications. Use the event_class to generate the logic needed to render the notification
  • Rename fields in Notifications (followable => resource, notification_type => event_name), add "read_at" field
  • Modify the resource_icon in IconHelper to handle processes and assemblies
  • Notifications i18n
  • Set notification as read by clicking on a button and delete them
  • Add a button to mark all notifications as read
  • Filter notifications by type (?) and paginate them
  • Add missing docs
  • Make event classes know about the path of the notification icon. Try to get it from the resource.feature.manifest.icon, or from resource.manifest.icon, so it can be overwritten for specific event classes.Needs to know whether to use icon or external_icon methods, depending on the value, so it isn't that easy.
  • Add a way to follow meetings from the UI. Should be separated, so that it can be reused by other resources.
  • Update Changelog

@mrcasals mrcasals self-assigned this Aug 7, 2017
@ghost ghost added the in-progress label Aug 7, 2017
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 7, 2017

Codecov Report

Merging #1695 into master will decrease coverage by 0.12%.
The diff coverage is 91.34%.

@@            Coverage Diff             @@
##           master    #1695      +/-   ##
==========================================
- Coverage   98.64%   98.51%   -0.13%     
==========================================
  Files        1074     1088      +14     
  Lines       24342    24371      +29     
==========================================
- Hits        24013    24010       -3     
- Misses        329      361      +32

@mrcasals mrcasals force-pushed the follow-meetings branch 15 times, most recently from ca6733a to c2ef201 Compare August 10, 2017 13:35
@mrcasals mrcasals force-pushed the follow-meetings branch 5 times, most recently from 358bf8a to e6f6971 Compare August 22, 2017 07:19
@mrcasals mrcasals force-pushed the follow-meetings branch 6 times, most recently from 511d290 to dd65f19 Compare August 28, 2017 10:47
end
end
end
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This concern and "participable.rb" should probably be merged together? Also, you can probably find name.demodulize.underscore.pluralize in a method over there :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, sure! I'll merge them :)

@mrcasals mrcasals mentioned this pull request Aug 31, 2017
6 tasks
@mrcasals
Copy link
Copy Markdown
Contributor Author

Closing this, superseded by #1784. Commits are the same, but split in different mini-PRs to ease the reviews.

@mrcasals mrcasals closed this Aug 31, 2017
@ghost ghost removed the in-progress label Aug 31, 2017
@mrcasals mrcasals deleted the follow-meetings branch August 31, 2017 08:23
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.

2 participants