-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Poll comments #1961
Poll comments #1961
Changes from 9 commits
63cbe2f
c3d7d47
8277e3c
2b58875
faa2f31
23397ee
d405d70
37cae41
843048a
ffdbdac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the Warning of the PR, please state that this feature is not available. |
||
else | ||
Flag.flagged?(current_user, flaggable) | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good! |
||
|
||
acts_as_paranoid column: :hidden_at | ||
include ActsAsParanoidAliases | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ def send_reply_email | |
end | ||
|
||
def email_on_comment? | ||
return false if @comment.commentable.is_a?(Poll) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that emails will not be send, should also be included in the Warning section of the PR |
||
commentable_author = @comment.commentable.author | ||
commentable_author != @author && commentable_author.email_on_comment? | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
class Poll < ActiveRecord::Base | ||
include Imageable | ||
|
||
acts_as_paranoid column: :hidden_at | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no requirement to hide Polls. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you remove the association, it raises an error ActionView::Template::Error (undefined local variable or method `with_hidden' for #<Poll::ActiveRecord_Relation:0x007ff01d39d0b8>): There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove both:
|
||
include ActsAsParanoidAliases | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zamuro this won't be neccesary, its not a requirement to make the Poll hidden There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When removing hidden_at from migration and these two lines:
which comes from comment model: belongs_to :commentable, -> { with_hidden }, polymorphic: true, counter_cache: true There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two lines, involve the Gem https://github.com/rubysherpas/paranoia Polls, do not need to have this functionality. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two lines are required by comments model, and while removing it, raises errors in polls comments claiming it needs hidden_at field. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As stated in #1961 (comment) |
||
has_many :booth_assignments, class_name: "Poll::BoothAssignment" | ||
has_many :booths, through: :booth_assignments | ||
has_many :partial_results, through: :booth_assignments | ||
|
@@ -9,8 +10,10 @@ class Poll < ActiveRecord::Base | |
has_many :officer_assignments, through: :booth_assignments | ||
has_many :officers, through: :officer_assignments | ||
has_many :questions | ||
has_many :comments, as: :commentable | ||
|
||
has_and_belongs_to_many :geozones | ||
belongs_to :author, -> { with_hidden }, class_name: 'User', foreign_key: 'author_id' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A Poll does not have an author. It is always the Government. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When removed, polls show view raises an error requiring "author_id" field There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to remove this as well as the |
||
|
||
validates :name, presence: true | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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" %> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. State this in the Warning section too, please |
||
</div> | ||
</div> | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
<% cache [locale_and_user_status, @current_order, commentable_cache_key(@poll), @comment_tree.comments, @comment_tree.comment_authors, @poll.comments_count, @comment_flags] do %> | ||
<div class="row comments"> | ||
<div id="comments" class="small-12 column"> | ||
<%= render 'shared/wide_order_selector', i18n_namespace: "comments" %> | ||
|
||
<% if user_signed_in? %> | ||
<%= render 'comments/form', {commentable: @poll, parent_id: nil, toggeable: false} %> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we've a @commentable variable already instead of using @poll |
||
<% else %> | ||
<br> | ||
|
||
<div data-alert class="callout primary"> | ||
<%= t("polls.show.login_to_comment", | ||
signin: link_to(t("votes.signin"), new_user_session_path), | ||
signup: link_to(t("votes.signup"), new_user_registration_path)).html_safe %> | ||
</div> | ||
<% end %> | ||
|
||
<% @comment_tree.root_comments.each do |comment| %> | ||
<%= render 'comments/comment', comment: comment %> | ||
<% end %> | ||
<%= paginate @comment_tree.root_comments %> | ||
</div> | ||
</div> | ||
<% end %> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
<div class="row"> | ||
<div class="small-12 column"> | ||
<ul class="tabs" data-tabs id="polls-tabs"> | ||
<li class="tabs-title is-active"> | ||
<%= link_to "#tab-comments" do %> | ||
<h3> | ||
<%= t("polls.show.comments_tab") %> | ||
<span class="js-comments-count">(<%= @poll.comments_count %>)</span> | ||
</h3> | ||
<% end %> | ||
</li> | ||
</ul> | ||
</div> | ||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,5 +128,14 @@ | |
</div> | ||
<% end %> | ||
</div> | ||
|
||
</div> | ||
</div> | ||
|
||
<div class="tabs-content" data-tabs-content="proposals-tabs" role="tablist"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be |
||
<%= render "filter_subnav" %> | ||
|
||
<div class="tabs-panel is-active" id="tab-comments"> | ||
<%= render "comments" %> | ||
</div> | ||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
class AddCommentsCountToPolls < ActiveRecord::Migration | ||
def change | ||
add_column :polls, :comments_count, :integer, default: 0 | ||
add_column :polls, :author_id, :integer | ||
add_column :polls, :hidden_at, :datetime | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, remove these two lines:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh sorry @zamuro you are right! |
||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -801,6 +801,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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the hidden_at is part of the unnecesary ActsAsParanoidAliases There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update schema after removing the previous columns from the migration file. |
||
end | ||
|
||
add_index "polls", ["starts_at", "ends_at"], name: "index_polls_on_starts_at_and_ends_at", using: :btree | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only these orders are necessary:
has_orders %w{most_voted newest oldest}, only: :show