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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add notifications when a Result is commented #1543

Merged
merged 9 commits into from Jul 6, 2017

Conversation

@beagleknight
Copy link
Contributor

@beagleknight beagleknight commented Jun 29, 2017

馃帺 What? Why?

When a Result is commented all participatory process admins should be notified. As @josepjaume suggested I created an abstraction called Notifiable with two methods:

  • notifiable?: Whether a notification should be sent or not.
  • users_to_notify: A collection of users to notify.

I implemented the Notifiable concern methods to all the Commentable resources so the author is not checked to sent notifications.
I refactored the CreateComment command code so it doesn't check flags anymore. The code was pushed to the model.

馃搶 Related Issues

馃搵 Subtasks

None

馃摲 Screenshots (optional)

N/A

馃懟 GIF

@beagleknight beagleknight self-assigned this Jun 29, 2017
@codecov
Copy link

@codecov codecov bot commented Jun 29, 2017

Codecov Report

Merging #1543 into master will decrease coverage by 32.26%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master    #1543       +/-   ##
===========================================
- Coverage   96.99%   64.73%   -32.27%     
===========================================
  Files         494       56      -438     
  Lines        8430      984     -7446     
===========================================
- Hits         8177      637     -7540     
- Misses        253      347       +94

@codecov
Copy link

@codecov codecov bot commented Jun 29, 2017

Codecov Report

Merging #1543 into master will decrease coverage by 0.01%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master    #1543      +/-   ##
==========================================
- Coverage   97.01%   96.99%   -0.02%     
==========================================
  Files         495      496       +1     
  Lines        8434     8461      +27     
==========================================
+ Hits         8182     8207      +25     
- Misses        252      254       +2

@beagleknight beagleknight changed the title Feature/results notifications Add notifications when a Result is commented Jun 29, 2017

# Public: Overrides the `notifiable?` Notifiable concern method.
def notifiable?(_context)
false
Copy link
Contributor

@mrcasals mrcasals Jun 29, 2017

So is this notifiable or not? I mean, I feel it's weird having the Notifiable module included, but it not actually being notifiable. Maybe a better naming would solve this, but the current state feels very confusing to me :/

Copy link
Contributor Author

@beagleknight beagleknight Jun 29, 2017

Well.. I followed the same pattern as the Commentable module. I think it's just an issue about naming because the method is pretty handy to add some logic.

Copy link
Contributor

@josepjaume josepjaume Jun 29, 2017

I believe the method refers to the instance: A particular resource might not be notifiable in some circumstances, but the resource itself is.

Copy link
Contributor

@josepjaume josepjaume Jun 29, 2017

I mean the instance might not be notifiable, but the model is, in general terms.

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez Jun 29, 2017

I find this confusing too. I interpret that Notifiable as a module means that the including object needs to define the interface (#notifiable? and #users_to_notify), whereas notifiable? itself means whether the class is actually notifiable or not. Is that it?

Also, should this inclusion be moved to the Commentable's module?

Copy link
Contributor Author

@beagleknight beagleknight Jun 29, 2017

Yeah I thought the same @deivid-rodriguez . I think the Commentable module can include the Notifiable but then I should rename it to something like NotifiableOnComment or something like that (I'm very bad at naming things).

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez Jun 29, 2017

Well, I don't think the module is really coupled to comments in any way. It's just that the only usage right now is on every commentable, so I guess it's more DRY to let Comentable do the inclusion instead of having

include Decidim::Notifiable
include Decidim::Comments::Commentable

everywhere.

Copy link
Contributor Author

@beagleknight beagleknight Jun 30, 2017

The module itself is not coupled but their implementation on every model is right now. Let's keep it DRY for now and we can do renames on a later PR when we have other use cases.

extend ActiveSupport::Concern

included do
# Public: Whether the object's comments are visible or not.
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez Jun 29, 2017

Copy/pasted?


# Public: Overrides the `notifiable?` Notifiable concern method.
# When a proposal is commented the proposal's author is notified if it is not the same
# who has commented the proposal and if the proposal's author has comment notifiations enabled.
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez Jun 29, 2017

s/notifiations/notifications/

@beagleknight beagleknight force-pushed the feature/results-notifications branch from 93072ba to 23e89e1 Jun 30, 2017
@josepjaume josepjaume merged commit 77220ec into master Jul 6, 2017
4 checks passed
@josepjaume josepjaume deleted the feature/results-notifications branch Jul 6, 2017
leio10 added a commit to podemos-info/decidim that referenced this issue Jul 26, 2017
* Add Notifiable concern

* Send notifications to users to notify

* Add some tests

* Fix rubocop issues

* Add missing params to CommentNotificationMailer specs

* Fix specs

* Fix specs

* Add some feedback

* Specs: add process admin to test results notifications
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants