Poll comments #1961

zamuro · 2017-10-02T11:36:16Z

Where

Related Issue: Polls: comments #1953

What

Adding comments functionality to polls

How

Created models, controllers and views for comments module into polls
Created migrations to new models
Generated new controller methods

Screenshots

Test

Generated test in feature spec

Deployment

Need to run rake db:migrate in order to get new tables created

Warnings

Pending comment features for Polls

Sending an email to the author of the parent resource
Flagging comments
Link back to commentable from comment's show
Comment as moderator or administrator

Review soon:

Email notifications won't be sent for Poll comments, since there should be no author to notify
Duplicated content for specs and views, refactor soon!
Exception 1267

bertocq · 2017-10-02T11:40:45Z

app/views/polls/_comments.html.erb

@@ -0,0 +1,31 @@
+<% cache [locale_and_user_status, @current_order, commentable_cache_key(@commentable), @comment_tree.comments, @comment_tree.comment_authors, @commentable.comments_count, @comment_flags] do %>


Why duplicate /app/views/polls/questions/_comments.html.erb content in a new file, instead of trying to reuse a single file on both Questions and Polls?

At first, it was done that way in case views needed to be tweaked for this specific feature. Nevertheless, since this seems not to be the case, it's better to use actual questions/comments partial instead of duplicate this one. I'll make the changes.

bertocq · 2017-10-02T13:56:08Z

app/views/polls/show.html.erb

@@ -45,6 +45,6 @@
 </aside>
 </div>
 <div class="tabs-panel is-active" id="tab-comments">
- <%= render "comments" %>
+ <%= render "polls/questions/comments" %>


I didn't state it clearly in my previous comment, I was trying to say that since the partial is common to both Poll and Poll Questions, one single shared file should be used. In this case you're reusing polls/questions/comments, that's a bit strange, maybe a polls/comments in the polls questions show would be less strange until we can do a shared comments partial for all commentables

Since it's the exact same file for both polls and polls/questions, would it be better to place the partial in the polls' views directory and render that in both cases? Or place it in views/shared directory that both cases can use.

<div class="tabs-panel is-active" id="tab-comments"> <%= render "shared/comments" %> </div>

Yes, since there is about 4-5 more comments partials, and one day we'll get them all joined together, its a nice start to have a shared comments partial that will be used for now only from Polls and Polls Questions 👏 to serve as base for future refactors 👍

Nice. Then pushed 341b2fa84 that implements the suggested feature. Thanks for the feedback.

voodoorai2000 · 2017-10-02T17:59:40Z

app/controllers/polls_controller.rb


 load_and_authorize_resource

 has_filters %w{current expired incoming}
+ has_orders %w{hot_score confidence_score created_at relevance archival_date}, only: :index
+ has_orders %w{most_voted newest oldest}, only: :show


Only these orders are necessary:
has_orders %w{most_voted newest oldest}, only: :show

voodoorai2000 · 2017-10-02T18:01:41Z

app/models/comment_notifier.rb

@@ -22,8 +22,10 @@ def send_reply_email
 end

 def email_on_comment?
- commentable_author = @comment.commentable.author
- commentable_author != @author && commentable_author.email_on_comment?
+ unless @comment.commentable.is_a?(Poll)


This is fine... maybe it's a little clearer this way:
return false if @comment.commentable.is_a?(Poll)

voodoorai2000 · 2017-10-02T18:03:13Z

app/models/poll.rb

@@ -1,4 +1,13 @@
 class Poll < ActiveRecord::Base
+ include HasPublicAuthor
+ include Flaggable


A Poll is not flaggable, its something that the Government creates.

voodoorai2000 · 2017-10-02T18:03:45Z

app/models/poll.rb

@@ -1,4 +1,13 @@
 class Poll < ActiveRecord::Base
+ include HasPublicAuthor


A Poll does not need to have a PublicAuthor, this is only used for the API.

voodoorai2000 · 2017-10-02T18:04:15Z

app/models/poll.rb

@@ -1,4 +1,13 @@
 class Poll < ActiveRecord::Base
+ include HasPublicAuthor
+ include Flaggable
+ acts_as_votable


A Poll is not votable, we use a different voting system for Polls.

Is that method not intended for votes in comments? Placed it there because in other places that uses comments has it.

The Comment model has already acts_as_votable, that makes comments votable, but this is on the Poll model, the Poll is not votable in the same way as comments, acts_as_votable is a gem that acts on the model you put it not over comments or other related entities

Right. I understand.

voodoorai2000 · 2017-10-02T18:05:10Z

app/models/poll.rb

+ include Flaggable
+ acts_as_votable
+
+ acts_as_paranoid column: :hidden_at


There is no requirement to hide Polls.

If you remove the association, it raises an error

ActionView::Template::Error (undefined local variable or method `with_hidden' for #<Poll::ActiveRecord_Relation:0x007ff01d39d0b8>):

Remove both:

acts_as_paranoid column: :hidden_at include ActsAsParanoidAliases

add_column :polls, :hidden_at, :datetime from migration

voodoorai2000 · 2017-10-02T18:05:40Z

db/migrate/20170929131411_add_fields_to_polls.rb

+class AddFieldsToPolls < ActiveRecord::Migration
+ def change
+ add_column :polls, :comments_count, :integer, default: 0
+ add_column :polls, :hidden_at, :datetime


No need to hide polls

Better to remove de migration file from the PR (and revert the db/schema.rb) than adding a migration that reverts the changes 🙏

@bertocq the whole migration? comments_count is not needed elsewhere?

Right, not the entire migration, seems like comments_count its needed indeed 👍

After running a migration that removes both hidden_at and author_id, it raises an error:

PG::UndefinedColumn: ERROR: no existe la columna polls.hidden_at LINE 1: SELECT "polls".* FROM "polls" WHERE "polls"."hidden_at" IS ... ^ : SELECT "polls".* FROM "polls" WHERE "polls"."hidden_at" IS NULL AND "polls"."id" = $1 LIMIT 1

voodoorai2000 · 2017-10-02T18:07:08Z

db/migrate/20170929131411_add_fields_to_polls.rb

+ def change
+ add_column :polls, :comments_count, :integer, default: 0
+ add_column :polls, :hidden_at, :datetime
+ add_column :polls, :author_id, :integer


There is no requirement for a poll author, it is always the Government.

voodoorai2000 · 2017-10-02T18:08:34Z

spec/features/polls/polls_spec.rb

+ expect(page).to have_content comment.body
+ end
+ end
+


All comment scenarios should be tested https://github.com/consul/consul/tree/master/spec/features/comments

Either add to the Warning section of this PR that only the basic scenarios have been tested
or test all the scenarios that a comment can have https://github.com/consul/consul/blob/master/spec/features/comments/proposals_spec.rb

bertocq · 2017-10-04T13:36:38Z

app/models/poll.rb

@@ -1,4 +1,7 @@
 class Poll < ActiveRecord::Base
+ acts_as_paranoid column: :hidden_at
+ include ActsAsParanoidAliases


@zamuro this won't be neccesary, its not a requirement to make the Poll hidden

When removing hidden_at from migration and these two lines:

5) Polls Show user b can access to comments from user a Failure/Error: belongs_to :commentable, -> { with_hidden }, polymorphic: true, counter_cache: true ActionView::Template::Error: undefined local variable or method `with_hidden' for #<Poll::ActiveRecord_Relation:0x0055f8a558f4a0> # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/relation/delegation.rb:136:in `method_missing' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/relation/delegation.rb:99:in `method_missing' # ./app/models/comment.rb:22:in `block in <class:Comment>' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/builder/association.rb:65:in `instance_exec' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/builder/association.rb:65:in `block in initialize' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association_scope.rb:191:in `instance_exec' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association_scope.rb:191:in `eval_scope' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association_scope.rb:155:in `block (2 levels) in add_constraints' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association_scope.rb:154:in `each' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association_scope.rb:154:in `block in add_constraints' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association_scope.rb:141:in `each' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association_scope.rb:141:in `each_with_index' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association_scope.rb:141:in `add_constraints' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association_scope.rb:39:in `scope' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association_scope.rb:5:in `scope' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association.rb:97:in `association_scope' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association.rb:86:in `scope' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/singular_association.rb:42:in `get_records' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/singular_association.rb:57:in `find_target' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association.rb:138:in `load_target' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/association.rb:53:in `reload' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/singular_association.rb:9:in `reader' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/bullet-5.5.1/lib/bullet/active_record42.rb:215:in `reader' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/associations/builder/association.rb:115:in `commentable' # ./app/views/comments/_comment.html.erb:2:in `_app_views_comments__comment_html_erb___2900209252357399508_47263206653840' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/template.rb:145:in `block in render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications.rb:166:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/template.rb:333:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/template.rb:143:in `render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/partial_renderer.rb:339:in `render_partial' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/partial_renderer.rb:310:in `block in render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/abstract_renderer.rb:39:in `block in instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications.rb:164:in `block in instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications/instrumenter.rb:20:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications.rb:164:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/abstract_renderer.rb:39:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/partial_renderer.rb:309:in `render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/renderer.rb:51:in `render_partial' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/helpers/rendering_helper.rb:35:in `render' # ./app/views/shared/_comments.html.erb:25:in `block (2 levels) in _app_views_shared__comments_html_erb___3346234190162093915_47263181704260' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/relation/delegation.rb:46:in `each' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/relation/delegation.rb:46:in `each' # ./app/views/shared/_comments.html.erb:24:in `block in _app_views_shared__comments_html_erb___3346234190162093915_47263181704260' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/helpers/cache_helper.rb:117:in `cache' # ./app/views/shared/_comments.html.erb:1:in `_app_views_shared__comments_html_erb___3346234190162093915_47263181704260' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/template.rb:145:in `block in render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications.rb:166:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/template.rb:333:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/template.rb:143:in `render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/partial_renderer.rb:339:in `render_partial' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/partial_renderer.rb:310:in `block in render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/abstract_renderer.rb:39:in `block in instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications.rb:164:in `block in instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications/instrumenter.rb:20:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications.rb:164:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/abstract_renderer.rb:39:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/partial_renderer.rb:309:in `render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/renderer.rb:51:in `render_partial' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/helpers/rendering_helper.rb:35:in `render' # ./app/views/polls/show.html.erb:46:in `_app_views_polls_show_html_erb__504409128911861219_47263182546540' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/template.rb:145:in `block in render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications.rb:166:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/template.rb:333:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/template.rb:143:in `render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/template_renderer.rb:54:in `block (2 levels) in render_template' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/abstract_renderer.rb:39:in `block in instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications.rb:164:in `block in instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications/instrumenter.rb:20:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications.rb:164:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/abstract_renderer.rb:39:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/template_renderer.rb:53:in `block in render_template' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/template_renderer.rb:61:in `render_with_layout' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/template_renderer.rb:52:in `render_template' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/template_renderer.rb:14:in `render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/renderer.rb:46:in `render_template' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/renderer/renderer.rb:27:in `render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/rendering.rb:100:in `_render_template' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/streaming.rb:217:in `_render_template' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/rendering.rb:83:in `render_to_body' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/rendering.rb:32:in `render_to_body' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/renderers.rb:37:in `render_to_body' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/abstract_controller/rendering.rb:25:in `render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/rendering.rb:16:in `render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/instrumentation.rb:44:in `block (2 levels) in render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/core_ext/benchmark.rb:12:in `block in ms' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/core_ext/benchmark.rb:12:in `ms' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/instrumentation.rb:44:in `block in render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/instrumentation.rb:87:in `cleanup_view_runtime' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/railties/controller_runtime.rb:25:in `cleanup_view_runtime' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/instrumentation.rb:43:in `render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/implicit_render.rb:10:in `default_render' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/implicit_render.rb:5:in `send_action' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/abstract_controller/base.rb:198:in `process_action' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/rendering.rb:10:in `process_action' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/abstract_controller/callbacks.rb:20:in `block in process_action' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/callbacks.rb:117:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/callbacks.rb:555:in `block (2 levels) in compile' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/callbacks.rb:505:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/callbacks.rb:92:in `__run_callbacks__' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/callbacks.rb:778:in `_run_process_action_callbacks' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/callbacks.rb:81:in `run_callbacks' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/abstract_controller/callbacks.rb:19:in `process_action' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/rescue.rb:29:in `process_action' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/instrumentation.rb:32:in `block in process_action' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications.rb:164:in `block in instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications/instrumenter.rb:20:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/notifications.rb:164:in `instrument' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/instrumentation.rb:30:in `process_action' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/params_wrapper.rb:250:in `process_action' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/railties/controller_runtime.rb:18:in `process_action' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/abstract_controller/base.rb:137:in `process' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionview-4.2.9/lib/action_view/rendering.rb:30:in `process' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal.rb:196:in `dispatch' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal/rack_delegation.rb:13:in `dispatch' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_controller/metal.rb:237:in `block in action' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/routing/route_set.rb:74:in `dispatch' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/routing/route_set.rb:43:in `serve' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/journey/router.rb:43:in `block in serve' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/journey/router.rb:30:in `each' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/journey/router.rb:30:in `serve' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/routing/route_set.rb:817:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/turnout-2.4.0/lib/rack/turnout.rb:25:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/omniauth-1.6.1/lib/omniauth/strategy.rb:407:in `call_app!' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/omniauth-1.6.1/lib/omniauth/strategy.rb:265:in `mock_call!' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/omniauth-1.6.1/lib/omniauth/strategy.rb:184:in `call!' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/omniauth-1.6.1/lib/omniauth/strategy.rb:167:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/omniauth-1.6.1/lib/omniauth/strategy.rb:407:in `call_app!' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/omniauth-1.6.1/lib/omniauth/strategy.rb:265:in `mock_call!' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/omniauth-1.6.1/lib/omniauth/strategy.rb:184:in `call!' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/omniauth-1.6.1/lib/omniauth/strategy.rb:167:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/omniauth-1.6.1/lib/omniauth/strategy.rb:407:in `call_app!' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/omniauth-1.6.1/lib/omniauth/strategy.rb:265:in `mock_call!' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/omniauth-1.6.1/lib/omniauth/strategy.rb:184:in `call!' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/omniauth-1.6.1/lib/omniauth/strategy.rb:167:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-attack-5.0.1/lib/rack/attack.rb:147:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/bullet-5.5.1/lib/bullet/rack.rb:12:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/warden-1.2.7/lib/warden/manager.rb:36:in `block in call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/warden-1.2.7/lib/warden/manager.rb:35:in `catch' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/warden-1.2.7/lib/warden/manager.rb:35:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-1.6.8/lib/rack/etag.rb:24:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-1.6.8/lib/rack/conditionalget.rb:25:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-1.6.8/lib/rack/head.rb:13:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/middleware/params_parser.rb:27:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/middleware/flash.rb:260:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-1.6.8/lib/rack/session/abstract/id.rb:225:in `context' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-1.6.8/lib/rack/session/abstract/id.rb:220:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/middleware/cookies.rb:560:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/query_cache.rb:36:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/connection_adapters/abstract/connection_pool.rb:653:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/middleware/callbacks.rb:29:in `block in call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/callbacks.rb:88:in `__run_callbacks__' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/callbacks.rb:778:in `_run_call_callbacks' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/callbacks.rb:81:in `run_callbacks' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/middleware/callbacks.rb:27:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/middleware/remote_ip.rb:78:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rollbar-2.14.1/lib/rollbar/middleware/rails/rollbar.rb:24:in `block in call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rollbar-2.14.1/lib/rollbar.rb:145:in `scoped' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rollbar-2.14.1/lib/rollbar/middleware/rails/rollbar.rb:22:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/middleware/debug_exceptions.rb:17:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rollbar-2.14.1/lib/rollbar/middleware/rails/show_exceptions.rb:22:in `call_with_rollbar' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/middleware/show_exceptions.rb:30:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/railties-4.2.9/lib/rails/rack/logger.rb:38:in `call_app' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/railties-4.2.9/lib/rails/rack/logger.rb:20:in `block in call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/tagged_logging.rb:68:in `block in tagged' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/tagged_logging.rb:26:in `tagged' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/tagged_logging.rb:68:in `tagged' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/railties-4.2.9/lib/rails/rack/logger.rb:20:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/ahoy_matey-1.6.0/lib/ahoy/engine.rb:22:in `call_with_quiet_ahoy' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/quiet_assets-1.1.0/lib/quiet_assets.rb:27:in `call_with_quiet_assets' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/request_store-1.3.2/lib/request_store/middleware.rb:9:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/middleware/request_id.rb:21:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-1.6.8/lib/rack/methodoverride.rb:22:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-1.6.8/lib/rack/runtime.rb:18:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activesupport-4.2.9/lib/active_support/cache/strategy/local_cache_middleware.rb:28:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-1.6.8/lib/rack/lock.rb:17:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/actionpack-4.2.9/lib/action_dispatch/middleware/static.rb:120:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-1.6.8/lib/rack/sendfile.rb:113:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/railties-4.2.9/lib/rails/engine.rb:518:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/railties-4.2.9/lib/rails/application.rb:165:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-1.6.8/lib/rack/urlmap.rb:66:in `block in call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-1.6.8/lib/rack/urlmap.rb:50:in `each' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-1.6.8/lib/rack/urlmap.rb:50:in `call' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-test-0.6.3/lib/rack/mock_session.rb:30:in `request' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-test-0.6.3/lib/rack/test.rb:244:in `process_request' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/rack-test-0.6.3/lib/rack/test.rb:58:in `get' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/capybara-2.14.4/lib/capybara/rack_test/browser.rb:64:in `process' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/capybara-2.14.4/lib/capybara/rack_test/browser.rb:36:in `process_and_follow_redirects' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/capybara-2.14.4/lib/capybara/rack_test/browser.rb:22:in `visit' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/capybara-2.14.4/lib/capybara/rack_test/driver.rb:44:in `visit' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/capybara-2.14.4/lib/capybara/session.rb:269:in `visit' # /home/manuel/.rvm/gems/ruby-2.3.2/gems/capybara-2.14.4/lib/capybara/dsl.rb:50:in `block (2 levels) in <module:DSL>' # ./spec/features/polls/polls_spec.rb:238:in `block (3 levels) in <top (required)>' # ------------------ # --- Caused by: --- # NameError: # undefined local variable or method `with_hidden' for #<Poll::ActiveRecord_Relation:0x0055f8a558f4a0> # /home/manuel/.rvm/gems/ruby-2.3.2/gems/activerecord-4.2.9/lib/active_record/relation/delegation.rb:136:in `method_missing'

which comes from comment model:

belongs_to :commentable, -> { with_hidden }, polymorphic: true, counter_cache: true

bertocq · 2017-10-04T13:36:58Z

db/schema.rb

@@ -783,6 +783,9 @@
 t.datetime "ends_at"
 t.boolean "published", default: false
 t.boolean "geozone_restricted", default: false
+ t.integer "comments_count", default: 0
+ t.integer "author_id"
+ t.datetime "hidden_at"


the hidden_at is part of the unnecesary ActsAsParanoidAliases

voodoorai2000 · 2017-10-05T15:56:03Z

app/controllers/polls_controller.rb

@@ -13,6 +15,9 @@ def index
 def show
 @questions = @poll.questions.for_render.sort_for_list

+ @commentable = @poll
+ @comment_tree = CommentTree.new(@commentable, params[:page], @current_order)


voodoorai2000 · 2017-10-05T15:57:14Z

app/models/comment.rb

@@ -3,7 +3,7 @@ class Comment < ActiveRecord::Base
 include HasPublicAuthor
 include Graphqlable

- COMMENTABLE_TYPES = %w(Debate Proposal Budget::Investment Poll::Question Legislation::Question Legislation::Annotation Topic).freeze
+ COMMENTABLE_TYPES = %w(Debate Proposal Budget::Investment Poll::Question Legislation::Question Legislation::Annotation Topic Poll).freeze


voodoorai2000 · 2017-10-05T15:57:17Z

app/helpers/flags_helper.rb

@@ -12,7 +12,7 @@ def show_unflag_action?(flaggable)

 def flagged?(flaggable)
 if flaggable.is_a? Comment
- @comment_flags[flaggable.id]
+ @comment_flags[flaggable.id] unless flaggable.commentable_type == "Poll"


In the Warning of the PR, please state that this feature is not available.
We'll create a new issue to deal with it later

voodoorai2000 · 2017-10-05T15:57:50Z

app/models/comment_notifier.rb

@@ -22,6 +22,7 @@ def send_reply_email
 end

 def email_on_comment?
+ return false if @comment.commentable.is_a?(Poll)


The fact that emails will not be send, should also be included in the Warning section of the PR

voodoorai2000 · 2017-10-05T16:02:26Z

app/views/comments/show.html.erb

@@ -1,7 +1,7 @@
 <div class="row">
 <div class="small-12 column margin-top">
 <%= back_link_to commentable_path(@comment),
- t("comments.show.return_to_commentable") + @comment.commentable.title %>
+ t("comments.show.return_to_commentable") + @comment.commentable.title unless @comment != "Poll" %>


State this in the Warning section too, please

voodoorai2000 · 2017-10-05T16:03:31Z

app/views/polls/show.html.erb

-
+
+ <div class="tabs-panel is-active" id="tab-comments">
+ <%= render "shared/comments" %>


voodoorai2000 · 2017-10-05T16:06:57Z

app/views/shared/_comments.html.erb

@@ -0,0 +1,31 @@
+<% cache [locale_and_user_status, @current_order, commentable_cache_key(@commentable), @comment_tree.comments, @comment_tree.comment_authors, @commentable.comments_count, @comment_flags] do %>


Great, one day we'll refactor so that all models use this view :)

voodoorai2000 · 2017-10-05T16:07:33Z

db/migrate/20171003143034_add_comments_count_to_polls.rb

+ def change
+ add_column :polls, :comments_count, :integer, default: 0
+ add_column :polls, :author_id, :integer
+ add_column :polls, :hidden_at, :datetime


Please, remove these two lines:

add_column :polls, :author_id, :integer add_column :polls, :hidden_at, :datetime

Those two fields are required by comments model. When removing it from polls (and removing the columns from the migration), raises an error asking for either "hidden_at" or "author_id".

Oh sorry @zamuro you are right!
There is probably a way to bypass this, but it's a situation we have accepted in the past.
Don't worry about it 🙏 👍

voodoorai2000 · 2017-10-05T16:08:02Z

db/schema.rb

@@ -792,6 +792,9 @@
 t.boolean "geozone_restricted", default: false
 t.text "summary"
 t.text "description"
+ t.integer "comments_count", default: 0
+ t.integer "author_id"
+ t.datetime "hidden_at"


Update schema after removing the previous columns from the migration file.

voodoorai2000 · 2017-10-05T16:10:12Z

spec/features/polls/polls_spec.rb

+ expect(page).to have_content comment.body
+ end
+ end
+


Either add to the Warning section of this PR that only the basic scenarios have been tested
or test all the scenarios that a comment can have https://github.com/consul/consul/blob/master/spec/features/comments/proposals_spec.rb

On branch mlucena-poll-comments Changes to be committed: modified: app/views/comments/show.html.erb modified: spec/features/polls/polls_spec.rb

bertocq · 2017-10-10T10:27:11Z

app/views/polls/_comments.html.erb

+ <%= render 'shared/wide_order_selector', i18n_namespace: "comments" %>
+
+ <% if user_signed_in? %>
+ <%= render 'comments/form', {commentable: @poll, parent_id: nil, toggeable: false} %>


I think we've a @commentable variable already instead of using @poll

bertocq

👏 @voodoorai200

decabeza · 2017-10-10T10:44:14Z

app/views/polls/show.html.erb

 </div>
 </div>
+
+<div class="tabs-content" data-tabs-content="proposals-tabs" role="tablist">


This should be data-tabs-content="polls-tabs" and adds to class="tabs-content"a margin-top class 😉

…eam-fix_exception_with_wrong_token Fix exception when confirming an invalid token

bertocq reviewed Oct 2, 2017

View reviewed changes

zamuro force-pushed the mlucena-poll-comments branch from 613a86e to 418518a Compare October 2, 2017 12:02

bertocq reviewed Oct 2, 2017

View reviewed changes

voodoorai2000 requested changes Oct 2, 2017

View reviewed changes

zamuro force-pushed the mlucena-poll-comments branch from 3c83d42 to 213660d Compare October 3, 2017 16:29

bertocq reviewed Oct 4, 2017

View reviewed changes

zamuro force-pushed the mlucena-poll-comments branch 3 times, most recently from 79b2157 to ac99338 Compare October 5, 2017 14:05

voodoorai2000 reviewed Oct 5, 2017

View reviewed changes

zamuro force-pushed the mlucena-poll-comments branch from 7463c41 to 4feb2d1 Compare October 6, 2017 08:25

zamuro added 3 commits October 6, 2017 10:36

20171003 - [WIP] Functionality and tests for polls comments

63cbe2f

20171004 - Refactored specs for polls comments

c3d7d47

On branch mlucena-poll-comments Changes to be committed: modified: app/views/comments/show.html.erb modified: spec/features/polls/polls_spec.rb

removed obsolete polls questions view

8277e3c

zamuro force-pushed the mlucena-poll-comments branch from 4feb2d1 to 8277e3c Compare October 6, 2017 09:55

voodoorai2000 added 5 commits October 9, 2017 17:59

Merge branch 'master' into mlucena-poll-comments

2b58875

Adds comment's tab

faa2f31

increases consistency by moving comment's view to poll's folder

23397ee

add translations

d405d70

adds specs

37cae41

voodoorai2000 changed the title ~~Mlucena poll comments~~ Poll comments Oct 10, 2017

voodoorai2000 approved these changes Oct 10, 2017

View reviewed changes

cleans up

843048a

bertocq reviewed Oct 10, 2017

View reviewed changes

bertocq approved these changes Oct 10, 2017

View reviewed changes

decabeza reviewed Oct 10, 2017

View reviewed changes

fixes specs

ffdbdac

voodoorai2000 merged commit 0e3868c into consuldemocracy:master Oct 10, 2017

voodoorai2000 mentioned this pull request Oct 10, 2017

Consul Pull ⬇️ AyuntamientoMadrid/consul#930

Merged

decabeza mentioned this pull request Oct 10, 2017

[FROZEN] Hide/Unhide Poll comments #2035

Closed

voodoorai2000 mentioned this pull request Oct 10, 2017

Fixes exception when sending a poll comment's email #2039

Merged

bertocq mentioned this pull request Oct 11, 2017

Remove Author and usage from Poll #2045

Closed

aitbw deleted the mlucena-poll-comments branch January 19, 2018 18:32

javierm added a commit to javierm/consul that referenced this pull request Apr 25, 2019

Merge pull request consuldemocracy#1961 from AyuntamientoMadrid/upstr…

dd0f12f

…eam-fix_exception_with_wrong_token Fix exception when confirming an invalid token

		@@ -0,0 +1,31 @@
		<% cache [locale_and_user_status, @current_order, commentable_cache_key(@commentable), @comment_tree.comments, @comment_tree.comment_authors, @commentable.comments_count, @comment_flags] do %>

		@@ -1,4 +1,13 @@
		class Poll < ActiveRecord::Base
		include HasPublicAuthor



		<div class="tabs-panel is-active" id="tab-comments">
		<%= render "shared/comments" %>

Poll comments #1961

Poll comments #1961

Conversation

zamuro commented Oct 2, 2017 • edited by voodoorai2000

Where

What

How

Screenshots

Test

Deployment

Warnings

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zamuro Oct 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertocq left a comment

Choose a reason for hiding this comment

decabeza Oct 10, 2017 • edited

Choose a reason for hiding this comment

zamuro commented Oct 2, 2017 •

edited by voodoorai2000

zamuro Oct 2, 2017 •

edited

decabeza Oct 10, 2017 •

edited