Merge pull request #967 from hennevogel/refactor-comment-save

Refactor comments

src/api/Gemfile

@@ -49,6 +49,8 @@ gem "pundit"
 gem 'responders', '~> 2.0'
 # needed for travis-ci.org, must be global for scripts
 gem 'bundler'
+# for threaded comments
+gem 'acts_as_tree'
 
 group :production do
   # if you have an account, it can be configured by

src/api/Gemfile.lock

@@ -38,6 +38,8 @@ GEM
       tzinfo (~> 1.1)
     acts_as_list (0.7.2)
       activerecord (>= 3.0)
+    acts_as_tree (2.2.0)
+      activerecord (>= 3.0.0)
     addressable (2.3.8)
     arel (6.0.0)
     binding_of_caller (0.7.2)
@@ -252,6 +254,7 @@ PLATFORMS
 DEPENDENCIES
   actionmailer
   acts_as_list
+  acts_as_tree
   bundler
   capybara_minitest_spec
   chunky_png

src/api/app/assets/stylesheets/webui/application/comments.css.scss

@@ -6,13 +6,11 @@
   float: right;
 }
 
-.comment_thread {
-}
-
 .comment{
   margin-right: 2em;
   border-top: 1px solid lightgrey;
   padding-top: 10px;
+  padding-bottom: 10px;
 }
 
 .comment_0 {
@@ -24,9 +22,24 @@
 
 .comment_child{
   background: #fff;
+}
+
+.thread_level_1{
   margin-left: 2em;
 }
 
+.thread_level_2{
+  margin-left: 4em;
+}
+
+.thread_level_3{
+  margin-left: 6em;
+}
+
+.thread_level_4{
+  margin-left: 8em;
+}
+
 .comment_odd{
 }
 

src/api/app/controllers/webui/package_controller.rb

@@ -54,8 +54,8 @@ def show
       redirect_back_or_to :controller => 'package', :action => 'show', :project => @project, :package => @package and return
     end
 
-    sort_comments(@package.comments)
 
+    @comments = @package.comments
     @requests = []
     # TODO!!!
     #BsRequest.list({:states => %w(review), :reviewstates => %w(new), :roles => %w(reviewer), :project => @project.name, :package => @package.name}) +

src/api/app/controllers/webui/project_controller.rb

@@ -363,7 +363,7 @@ def show
         @has_patchinfo = true if e['name'] == '_patchinfo'
       end
     end
-    sort_comments(@project.api_obj.comments)
+    @comments = @project.api_obj.comments
     render :show, status: params[:nextstatus] if params[:nextstatus]
   end
 

src/api/app/controllers/webui/request_controller.rb

@@ -119,8 +119,7 @@ def show
       # will be nul for after end
       @request_after = request_list[index+1]
     end
-
-    sort_comments(BsRequest.find(params[:id]).comments)
+    @comments = @bsreq.comments
   end
 
   def sourcediff

src/api/app/controllers/webui/webui_controller.rb

@@ -83,6 +83,10 @@ def initialize( _xml, _errors )
     end
   end
 
+
+  # FIXME: This is more than stupid. Why do we tell the user that something isn't found
+  # just because there is some data missing to compute the request? Someone needs to read
+  # http://guides.rubyonrails.org/active_record_validations.html
   class MissingParameterError < Exception; end
   rescue_from MissingParameterError do |exception|
     logger.debug "#{exception.class.name} #{exception.message} #{exception.backtrace.join('\n')}"

src/api/app/helpers/comment_helper.rb

@@ -4,7 +4,6 @@ module CommentHelper
   def comment_body(comment)
     # Initializes a Markdown parser, if needed
     @md_parser ||= Redcarpet::Markdown.new(OBSApi::MarkdownRenderer, autolink: true)
-    raw_comment = comment.is_a?(String) ? comment : comment[:body]
-    @md_parser.render(raw_comment).html_safe
+    @md_parser.render(comment.to_s).html_safe
   end
 end

src/api/app/helpers/webui/comment_helper.rb

@@ -1,5 +1,7 @@
 module Webui::CommentHelper
 
+  # This could also be solved with 
+  # http://apidock.com/rails/ActiveRecord/NestedAttributes/ClassMethods/accepts_nested_attributes_for 
   def save_comment_form
     opts = {action: 'save_comment'}
     opts[:controller] = params[:controller]

src/api/app/helpers/webui/webui_helper.rb

@@ -503,6 +503,7 @@ def user_with_realname_and_icon(user, opts = {})
     end
   end
 
+  # If there is any content add the ul tag
   def possibly_empty_ul(html_opts, &block)
     content = capture(&block)
     if content.blank?

src/api/app/mixins/webui/has_comments.rb

@@ -1,85 +1,21 @@
 module Webui::HasComments
 
+  # This is a nice but useless reimplementation of nested objects. Good work but nahhhhh
   def save_comment
     require_login || return
 
-    required_parameters :body
-
     comment = main_object.comments.build(body: params[:body], parent_id: params[:parent_id])
     comment.user = User.current
-    comment.save!
 
     respond_to do |format|
-      format.js do
-        render json: 'ok'
-      end
-      format.html do
-        flash[:notice] = 'Comment added successfully'
-      end
-    end
-    redirect_to :back
-  end
-
-  protected
-
-  def sort_comments(comments)
-    @all_comments = Hash.new
-    @all_comments[:parents] = []
-    @all_comments[:children] = []
-
-    # separate parents from children. How cruel, I know.
-    comments.each do |com|
-      # No valid parent
-      if !com.parent_id.nil? && Comment.exists?(com.parent_id)
-        @all_comments[:children] << [com.body, com.user.login, com.parent_id, com.id, com.created_at]
+      if comment.save
+        format.html { redirect_to :back , notice: 'Comment was successfully created.' }
+        format.json { render json: 'ok' }
       else
-        @all_comments[:parents] << [com.body, com.id, com.user.login, com.created_at]
+        format.html { redirect_to :back, error: "Comment can't be saved: #{comment.errors.full_messages.to_sentence}." }
+        format.json { render json: comment.errors, status: :unprocessable_entity }
       end
     end
-
-    @all_comments[:parents].sort_by! { |c| c[3] } # sorting by created_at
-    @all_comments[:children].sort_by! { |c| c[3] } # sorting by created_at
-
-    thread_comments
-  end
-
-  def thread_comments
-    @comments = []
-    # now pushing sorted and final list of first/top/parent level comments into to a hash to
-    @all_comments[:parents].each do |first_level|
-      @comments << {
-          created_at: first_level[3],
-          id: first_level[1],
-          body: first_level[0],
-          parent_id: nil,
-          user: first_level[2],
-          children: find_children(first_level[1])
-      }
-    end
-  end
-
-  def find_children(parent_id = nil)
-    return [] unless parent_id
-    current_children = []
-
-    # get children of current top comment
-    child_comments = @all_comments[:children].select do |c|
-      c[2] == parent_id
-    end
-
-    # pushing children coments into hash
-
-    child_comments.each do |child|
-      current_children << {
-          created_at: child[4],
-          id: child[3],
-          body: child[0],
-          parent_id: child[2],
-          user: child[1],
-          children: find_children(child[3])
-      }
-    end
-    return current_children
   end
 
 end

src/api/app/models/comment.rb

@@ -14,6 +14,12 @@ class Comment < ActiveRecord::Base
 
   has_many :children, dependent: :destroy, :class_name => 'Comment', :foreign_key => 'parent_id'
 
+  extend ActsAsTree::TreeWalker
+  acts_as_tree order: "created_at"
+
+  def to_s
+    body
+  end
 
   def create_notification(params = {})
     params[:commenter] = self.user.id

src/api/app/views/webui/comment/_links.html.erb

@@ -1,21 +1,21 @@
 <%= possibly_empty_ul class: 'horizontal-list comment-actions' do %>
   <% unless User.current.is_nobody? %>
-   <li class="togglable_comment" data-toggle="reply_form_of_<%= comment[:id] %>">
-    <a id="reply_link_id_<%= comment[:id] %>">
+   <li class="togglable_comment" data-toggle="reply_form_of_<%= comment.id %>">
+    <a id="reply_link_id_<%= comment.id %>">
       <%= sprited_text('comment_add', 'Reply') %>
     </a>
    </li>
   <% end %>
-  <% if User.current.login == comment[:user] || User.current.is_admin? %>
+  <% if User.current == comment.user || User.current.is_admin? %>
       <!-- editing is missing and as such broken
-         <li class="togglable_comment" data-toggle="edit_form_of_<%= comment[:id] %>">
+         <li class="togglable_comment" data-toggle="edit_form_of_<%= comment.id %>">
             <%= link_to(sprited_text('comment_edit', 'Edit'), '#') %>
         </li>
       -->
       <li>
         <%= link_to(sprited_text('comment_delete', 'Delete'),
                     {controller: :comments, action: :destroy,
-                     id: comment[:id]}, method: :delete) %>
+                     id: comment.id}, method: :delete) %>
       </li>
   <% end %>
 <% end %>

src/api/app/views/webui/comment/_reply.html.erb

@@ -1,10 +1,9 @@
-<% cid = comment[:id] %>
-<div id="<%="reply_form_of_#{cid}"%>" style="display: none; <%= 'padding-bottom: 20px;' if level >= 1 %>">
+<div id="<%="reply_form_of_#{comment.id}"%>" style="display: none;">
   <%= save_comment_form do %>
     <p>
-      <%= text_area_tag 'body', nil, size: '80x1', :onkeyup => 'sz(this);', :onclick => 'sz(this);', id: "reply_body_#{cid}", :placeholder => 'Comment Text', :required => 'required' %>
-      <%= hidden_field_tag 'parent_id', comment[:id], id: "reply_parent_id_#{cid}" %>
+      <%= text_area_tag 'body', nil, size: '80x1', :onkeyup => 'sz(this);', :onclick => 'sz(this);', id: "reply_body_#{comment.id}", :placeholder => 'Comment Text', :required => 'required' %>
+      <%= hidden_field_tag 'parent_id', comment.id, id: "reply_parent_id_#{comment.id}" %>
     </p>
-    <p><%= submit_tag 'Add reply', id: "add_reply_#{cid}"%></p>
+    <p><%= submit_tag 'Add reply', id: "add_reply_#{comment.id}"%></p>
   <% end %>
 </div>

src/api/app/views/webui/comment/_show.html.erb

@@ -1,31 +1,19 @@
 <div class="comments">
-  <% @comments.each_with_index do |comment, index| %>
-      <%= javascript_tag do %>
-          <%= render partial: 'webui/comment/expand_collapse.js.erb', locals: { comment_id: comment[:id] } %>
-      <% end %>
-      <div class="comment_thread">
-        <div class="comment comment_<%= index %>">
-          <%=  user_with_realname_and_icon(comment[:user], no_icon: true, short: true) %>
-          wrote <span class="comment_time"><%= fuzzy_time(comment[:created_at]) %></span>
-
-          <%= comment_body(comment) %>
-          <%= render :partial => 'webui/comment/links', :locals => { comment: comment } %>
-          <%= render :partial => 'webui/comment/reply', :locals => { comment: comment, level: 0 } %>
-        </div>
-        <% unless comment[:children].blank? %>
-            <div id="reply_of_comment_<%= comment[:id] %>">
-              <%= render partial: 'webui/comment/child', locals: { children: comment[:children] } %>
-            </div>
-        <% end %>
-      </div>
+  <% @comments.walk_tree do |comment, level| %>
+    <div class="comment thread_level_<%= level >= 4 ? 4 : level %>">
+      <%=  user_with_realname_and_icon(comment.user, no_icon: true, short: true) %>
+      wrote <span class="comment_time"><%= fuzzy_time(comment.created_at) %></span>
+      <%= comment_body(comment) %>
+      <%= render :partial => 'webui/comment/links', :locals => { comment: comment } %>
+      <%= render :partial => 'webui/comment/reply', :locals => { comment: comment, level: 0 } %>
+    </div>
   <% end %>
 
   <div class="comment_new grid_16 alpha omega">
     <%= render partial: 'webui/comment/new' %>
   </div>
-
 </div>
 
 <% content_for :ready_function do %>
-    setup_comment_toggles();
+  setup_comment_toggles();
 <% end %>

src/api/test/functional/webui/package_controller_test.rb

@@ -86,15 +86,15 @@ def delete_and_recreate_kdelibs
   def fill_comment(body = 'Comment Body')
     fill_in 'body', with: body
     find_button('Add comment').click
-    find('#flash-messages').must_have_text 'Comment added successfully '
+    find('#flash-messages').must_have_text 'Comment was successfully created.'
   end
 
   test 'succesful comment creation' do
     use_js
     login_Iggy
     visit root_path + '/package/show/home:Iggy/TestPack'
     fill_comment "Write some http://link.com\n\nand some other\n\n* Markdown\n* markup\n\nReferencing sr#23, bco#24, fate#25, @_nobody_, @a-dashed-user and @Iggy. https://anotherlink.com"
-    within('div.comment_0') do
+    within('div.thread_level_0') do
       page.must_have_link "http://link.com"
       page.must_have_xpath '//ul//li[text()="Markdown"]'
       page.must_have_xpath '//p[text()="and some other"]'
@@ -115,7 +115,7 @@ def fill_comment(body = 'Comment Body')
     visit root_path + '/package/show?project=home:Iggy&package=TestPack'
     # @Iggy works at the very beginning and requests are case insensitive
     fill_comment "@Iggy likes to mention himself and to write request#23 with capital 'R', like Request#23."
-    within('div.comment_0') do
+    within('div.thread_level_0') do
       page.must_have_xpath '//a[contains(@href, "/request/show/23") and text()="request#23"]'
       page.must_have_xpath '//a[contains(@href, "/request/show/23") and text()="Request#23"]'
       page.must_have_xpath '//a[contains(@href, "user/show/Iggy") and text()="@Iggy"]'
@@ -137,7 +137,7 @@ def fill_comment(body = 'Comment Body')
     find(:id, 'reply_link_id_201').click
     fill_in 'reply_body_201', with: 'Comment Body'
     find(:id, 'add_reply_201').click
-    find('#flash-messages').must_have_text 'Comment added successfully '
+    find('#flash-messages').must_have_text 'Comment was successfully created.'
   end
 
   test 'diff is empty' do