From c84b2f0704ddafdbbcee3b96ee3a903801c96a24 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Mon, 29 Jan 2018 21:47:01 +0100 Subject: [PATCH 01/10] Add valuation boolean flag to Comment model Why: Budget Investment's valuators need to be able to comment on investments without making those comments public. We need a way to clearly make a distinction to avoid "leaking" internal valuation comments. How: Adding a boolean `valuation` attribute defaulted to false to the Comments table, and index on it with concurrent algorithm as explained at https://robots.thoughtbot.com/how-to-create-postgres-indexes-concurrently-in The name `valuation` was chosen instead of `internal` because of the more specific meaning as well as avoiding a collision with existing internal_comments attribute on Budget::Investment model (soon to be deprecated & removed) --- .../20180129190931_add_valuation_flag_to_comments.rb | 5 +++++ .../20180129190950_add_index_to_valuation_comments.rb | 7 +++++++ db/schema.rb | 8 +++++--- 3 files changed, 17 insertions(+), 3 deletions(-) create mode 100644 db/migrate/20180129190931_add_valuation_flag_to_comments.rb create mode 100644 db/migrate/20180129190950_add_index_to_valuation_comments.rb diff --git a/db/migrate/20180129190931_add_valuation_flag_to_comments.rb b/db/migrate/20180129190931_add_valuation_flag_to_comments.rb new file mode 100644 index 000000000..cf31a92e7 --- /dev/null +++ b/db/migrate/20180129190931_add_valuation_flag_to_comments.rb @@ -0,0 +1,5 @@ +class AddValuationFlagToComments < ActiveRecord::Migration + def change + add_column :comments, :valuation, :boolean, default: false + end +end diff --git a/db/migrate/20180129190950_add_index_to_valuation_comments.rb b/db/migrate/20180129190950_add_index_to_valuation_comments.rb new file mode 100644 index 000000000..2816d15b0 --- /dev/null +++ b/db/migrate/20180129190950_add_index_to_valuation_comments.rb @@ -0,0 +1,7 @@ +class AddIndexToValuationComments < ActiveRecord::Migration + disable_ddl_transaction! + + def change + add_index :comments, :valuation, algorithm: :concurrently + end +end diff --git a/db/schema.rb b/db/schema.rb index 219707f69..a6fc6ef88 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180119073228) do +ActiveRecord::Schema.define(version: 20180129190950) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -234,7 +234,7 @@ ActiveRecord::Schema.define(version: 20180119073228) do t.string "commentable_type" t.text "body" t.string "subject" - t.integer "user_id", null: false + t.integer "user_id", null: false t.datetime "created_at" t.datetime "updated_at" t.datetime "hidden_at" @@ -247,7 +247,8 @@ ActiveRecord::Schema.define(version: 20180119073228) do t.integer "cached_votes_down", default: 0 t.datetime "confirmed_hide_at" t.string "ancestry" - t.integer "confidence_score", default: 0, null: false + t.integer "confidence_score", default: 0, null: false + t.boolean "valuation", default: false end add_index "comments", ["ancestry"], name: "index_comments_on_ancestry", using: :btree @@ -257,6 +258,7 @@ ActiveRecord::Schema.define(version: 20180119073228) do add_index "comments", ["commentable_id", "commentable_type"], name: "index_comments_on_commentable_id_and_commentable_type", using: :btree add_index "comments", ["hidden_at"], name: "index_comments_on_hidden_at", using: :btree add_index "comments", ["user_id"], name: "index_comments_on_user_id", using: :btree + add_index "comments", ["valuation"], name: "index_comments_on_valuation", using: :btree create_table "communities", force: :cascade do |t| t.datetime "created_at", null: false From 56fc5c958336016e4dab4b9083992c3288d27be7 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Mon, 29 Jan 2018 21:53:38 +0100 Subject: [PATCH 02/10] Filter internal valuation comments from public api Why: Internal valuation comments are only for admins and valuators, not for the public view. How: Adding a `not_valuations` scope and use it at the `public_for_api` one --- app/models/comment.rb | 5 ++++- spec/factories.rb | 4 ++++ spec/models/comment_spec.rb | 6 ++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index 50f6cef66..0651b3442 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -33,7 +33,8 @@ class Comment < ActiveRecord::Base end scope :sort_by_flags, -> { order(flags_count: :desc, updated_at: :desc) } scope :public_for_api, -> do - where(%{(comments.commentable_type = 'Debate' and comments.commentable_id in (?)) or + not_valuations + .where(%{(comments.commentable_type = 'Debate' and comments.commentable_id in (?)) or (comments.commentable_type = 'Proposal' and comments.commentable_id in (?)) or (comments.commentable_type = 'Poll' and comments.commentable_id in (?))}, Debate.public_for_api.pluck(:id), @@ -50,6 +51,8 @@ class Comment < ActiveRecord::Base scope :sort_by_oldest, -> { order(created_at: :asc) } scope :sort_descendants_by_oldest, -> { order(created_at: :asc) } + scope :not_valuations, -> { where(valuation: false) } + after_create :call_after_commented def self.build(commentable, user, body, p_id = nil) diff --git a/spec/factories.rb b/spec/factories.rb index f9ffd1ef6..fe9bccd6c 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -465,6 +465,10 @@ FactoryBot.define do trait :with_confidence_score do before(:save) { |d| d.calculate_confidence_score } end + + trait :valuation do + valuation true + end end factory :legacy_legislation do diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index fb55c6a81..837464527 100644 --- a/spec/models/comment_spec.rb +++ b/spec/models/comment_spec.rb @@ -187,5 +187,11 @@ describe Comment do expect(described_class.public_for_api).not_to include(comment) end + + it "does not return internal valuation comments" do + valuation_comment = create(:comment, :valuation) + + expect(described_class.public_for_api).not_to include(valuation_comment) + end end end From 767fd04bdfe957f43dce4a3e73c273dda27a1631 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Mon, 29 Jan 2018 21:55:51 +0100 Subject: [PATCH 03/10] Add valuation comments relation at Budget Investment Why: Budget Investments already has an existing `comments` relation that is on use. We need to keep that relation unaltered after adding the internal valuation comments, that means scoping the relation to only public comments (non valuation ones) so existing code using it will remain working as expected. A new second relation will be needed to explicitly ask for valuation comments only where needed, again scoping to valuation comments. How: Adding a second `valuations` relationship and filtering on both with the new `valuation` flag from Comment model. --- app/models/budget/investment.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 58674be9b..7129c40dc 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -34,7 +34,10 @@ class Budget has_many :valuator_assignments, dependent: :destroy has_many :valuators, through: :valuator_assignments - has_many :comments, as: :commentable + + has_many :comments, -> {where(valuation: false)}, as: :commentable, class_name: 'Comment' + has_many :valuations, -> {where(valuation: true)}, as: :commentable, class_name: 'Comment' + has_many :milestones validates :title, presence: true From dff966d9b3700cdf3dae1f75da97954d97c284c5 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Mon, 29 Jan 2018 22:12:55 +0100 Subject: [PATCH 04/10] Show valuation comment thread @ Valuation show/edit Why: Budget Investment's valuators should be able to see internal valuation comments thread at both show and edit views. How: At Valuation::BudgetInvestmentsController: * Include CommentableActions to gain access to the entire feature, with required resource_model & resource_name methods. * Add the only possible order (oldest to newest) * Load comments on both show & edit actions, passing `valuations` flag to the CommentTree in order to only list those. At CommentTree: * Use `valuations` flag as instance variable to decide wich comment threat to load: valuations (if relation exists) or comments. --- .../budget_investments_controller.rb | 25 +++++++++++++++++++ .../_written_by_valuators.html.erb | 8 ++++++ .../budget_investments/edit.html.erb | 8 ++++++ lib/comment_tree.rb | 14 ++++++++--- 4 files changed, 52 insertions(+), 3 deletions(-) diff --git a/app/controllers/valuation/budget_investments_controller.rb b/app/controllers/valuation/budget_investments_controller.rb index c8d61215b..643ccbc74 100644 --- a/app/controllers/valuation/budget_investments_controller.rb +++ b/app/controllers/valuation/budget_investments_controller.rb @@ -1,11 +1,14 @@ class Valuation::BudgetInvestmentsController < Valuation::BaseController include FeatureFlags + include CommentableActions + feature_flag :budgets before_action :restrict_access_to_assigned_items, only: [:show, :edit, :valuate] before_action :load_budget before_action :load_investment, only: [:show, :edit, :valuate] + has_orders %w{oldest}, only: [:show, :edit] has_filters %w{valuating valuation_finished}, only: :index load_and_authorize_resource :investment, class: "Budget::Investment" @@ -36,8 +39,30 @@ class Valuation::BudgetInvestmentsController < Valuation::BaseController end end + def show + load_comments + end + + def edit + load_comments + end + private + def load_comments + @commentable = @investment + @comment_tree = CommentTree.new(@commentable, params[:page], @current_order, valuations: true) + set_comment_flags(@comment_tree.comments) + end + + def resource_model + Budget::Investment + end + + def resource_name + resource_model.parameterize('_') + end + def load_budget @budget = Budget.find(params[:budget_id]) end diff --git a/app/views/valuation/budget_investments/_written_by_valuators.html.erb b/app/views/valuation/budget_investments/_written_by_valuators.html.erb index 9dc5a8e8c..965f2b059 100644 --- a/app/views/valuation/budget_investments/_written_by_valuators.html.erb +++ b/app/views/valuation/budget_investments/_written_by_valuators.html.erb @@ -51,3 +51,11 @@

<%= t("valuation.budget_investments.show.internal_comments") %>

<%= explanation_field @investment.internal_comments %> <% end %> + +
+ <% unless @comment_tree.nil? %> + <%= render partial: '/comments/comment_tree', locals: { comment_tree: @comment_tree, + comment_flags: @comment_flags, + display_comments_count: false } %> + <% end %> +
diff --git a/app/views/valuation/budget_investments/edit.html.erb b/app/views/valuation/budget_investments/edit.html.erb index 651f91c5a..5ae5ae5dd 100644 --- a/app/views/valuation/budget_investments/edit.html.erb +++ b/app/views/valuation/budget_investments/edit.html.erb @@ -103,6 +103,14 @@ <% end %> +
+ <% unless @comment_tree.nil? %> + <%= render partial: '/comments/comment_tree', locals: { comment_tree: @comment_tree, + comment_flags: @comment_flags, + display_comments_count: false } %> + <% end %> +
+

<%= @investment.title %>

<%= safe_html_with_links @investment.description %> diff --git a/lib/comment_tree.rb b/lib/comment_tree.rb index f2eadb8ab..a67a0ccc3 100644 --- a/lib/comment_tree.rb +++ b/lib/comment_tree.rb @@ -4,16 +4,24 @@ class CommentTree attr_accessor :root_comments, :comments, :commentable, :page, :order - def initialize(commentable, page, order = 'confidence_score') + def initialize(commentable, page, order = 'confidence_score', valuations: false) @commentable = commentable @page = page @order = order - + @valuations = valuations @comments = root_comments + root_descendants end def root_comments - commentable.comments.roots.send("sort_by_#{order}").page(page).per(ROOT_COMMENTS_PER_PAGE).for_render + base_comments.roots.send("sort_by_#{order}").page(page).per(ROOT_COMMENTS_PER_PAGE).for_render + end + + def base_comments + if @valuations && commentable.respond_to?('valuations') + commentable.valuations + else + commentable.comments + end end def root_descendants From 149c81371b1142e129c72291d1053d5de06e1540 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Mon, 29 Jan 2018 22:14:03 +0100 Subject: [PATCH 05/10] Allow valuation internal comments to be created How: Using a local variable at partials to set a hidden true/false value for `valuation` parameter on the comment creation form. Allowing that new param at the comment controller and using it when building a new Comment. --- app/controllers/comments_controller.rb | 5 +++-- app/models/comment.rb | 7 ++++--- app/views/comments/_comment.html.erb | 9 +++++++-- app/views/comments/_comment_tree.html.erb | 9 ++++++--- app/views/comments/_commentable_tree.html.erb | 8 ++++++-- app/views/comments/_form.html.erb | 1 + .../_written_by_valuators.html.erb | 3 ++- .../valuation/budget_investments/edit.html.erb | 3 ++- spec/features/comments/budget_investments_spec.rb | 15 ++++++++++----- 9 files changed, 41 insertions(+), 19 deletions(-) diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 98357e96e..0f5cb680b 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -45,12 +45,13 @@ class CommentsController < ApplicationController def comment_params params.require(:comment).permit(:commentable_type, :commentable_id, :parent_id, - :body, :as_moderator, :as_administrator) + :body, :as_moderator, :as_administrator, :valuation) end def build_comment @comment = Comment.build(@commentable, current_user, comment_params[:body], - comment_params[:parent_id].presence) + comment_params[:parent_id].presence, + comment_params[:valuation]) check_for_special_comments end diff --git a/app/models/comment.rb b/app/models/comment.rb index 0651b3442..3c58f966b 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -55,11 +55,12 @@ class Comment < ActiveRecord::Base after_create :call_after_commented - def self.build(commentable, user, body, p_id = nil) - new commentable: commentable, + def self.build(commentable, user, body, p_id = nil, valuation = false) + new(commentable: commentable, user_id: user.id, body: body, - parent_id: p_id + parent_id: p_id, + valuation: valuation) end def self.find_commentable(c_type, c_id) diff --git a/app/views/comments/_comment.html.erb b/app/views/comments/_comment.html.erb index 8d45b5469..a5d84166e 100644 --- a/app/views/comments/_comment.html.erb +++ b/app/views/comments/_comment.html.erb @@ -1,4 +1,5 @@ <% comment_flags ||= @comment_flags %> +<% valuation = local_assigns.fetch(:valuation, false) %> <% cache [locale_and_user_status(comment), comment, commentable_cache_key(comment.commentable), comment.author, (comment_flags[comment.id] if comment_flags)] do %>