From: Tom Hughes <tom@compton.nu>
Date: Sun, 19 Oct 2014 20:31:23 +0000 (+0100)
Subject: Tidy up changeset comment code
X-Git-Tag: live~4303^2~2
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/8598db3233baefa9e455da331ca2579a8df17709?hp=14ac1babc2517320a2c90fa9b4ac36a5a6e68018

Tidy up changeset comment code
---

diff --git a/app/assets/stylesheets/common.css.scss b/app/assets/stylesheets/common.css.scss
index 0276b3e55..37a6aa7a9 100644
--- a/app/assets/stylesheets/common.css.scss
+++ b/app/assets/stylesheets/common.css.scss
@@ -1112,11 +1112,11 @@ header .search_form {
   }
 
   .comments-header {
-    float:left;
+    float: left;
   }
 
   .subscribe-buttons {
-    float:left;
+    float: left;
     margin: 18px 10px 10px;
     min-width: 80px;
   }
@@ -1128,8 +1128,8 @@ header .search_form {
   }
 
   span.action-button:hover {
-    cursor:pointer;
-    text-decoration:underline;
+    cursor: pointer;
+    text-decoration: underline;
   }
 
   .note-description {
diff --git a/app/controllers/browse_controller.rb b/app/controllers/browse_controller.rb
index 7056539cb..f0b92f4b4 100644
--- a/app/controllers/browse_controller.rb
+++ b/app/controllers/browse_controller.rb
@@ -58,8 +58,11 @@ class BrowseController < ApplicationController
   def changeset
     @type = "changeset"
     @changeset = Changeset.find(params[:id])
-    @comments = @changeset.comments.unscope(:where => :visible).includes(:author) if @user and @user.moderator?
-    @comments ||= @changeset.comments.includes(:author)
+    if @user and @user.moderator?
+      @comments = @changeset.comments.unscope(:where => :visible).includes(:author)
+    else
+      @comments = @changeset.comments.includes(:author)
+    end
     @node_pages, @nodes = paginate(:old_nodes, :conditions => {:changeset_id => @changeset.id}, :per_page => 20, :parameter => 'node_page')
     @way_pages, @ways = paginate(:old_ways, :conditions => {:changeset_id => @changeset.id}, :per_page => 20, :parameter => 'way_page')
     @relation_pages, @relations = paginate(:old_relations, :conditions => {:changeset_id => @changeset.id}, :per_page => 20, :parameter => 'relation_page')
diff --git a/app/controllers/changeset_controller.rb b/app/controllers/changeset_controller.rb
index ba7cb0cbf..646ba2fa3 100644
--- a/app/controllers/changeset_controller.rb
+++ b/app/controllers/changeset_controller.rb
@@ -327,27 +327,27 @@ class ChangesetController < ApplicationController
     raise OSM::APIChangesetNotYetClosedError.new(changeset) if changeset.is_open?
 
     # Add a comment to the changeset
-    attributes = {
+    comment = changeset.comments.create({
       :changeset => changeset,
       :body => body,
       :author => @user
-    }
-
-    comment = changeset.comments.create(attributes)
+    })
 
+    # Notify current subscribers of the new comment
     changeset.subscribers.each do |user|
       if @user != user
         Notifier.changeset_comment_notification(comment, user).deliver
       end
     end
 
+    # Add the commenter to the subscribers if necessary
     changeset.subscribers << @user unless changeset.subscribers.exists?(@user)
 
     # Return a copy of the updated changeset
     render :text => changeset.to_xml.to_s, :content_type => "text/xml"
   end
 
-  ## 
+  ##
   # Adds a subscriber to the changeset
   def subscribe
     # Check the arguments are sane
@@ -361,14 +361,16 @@ class ChangesetController < ApplicationController
     raise OSM::APIChangesetNotYetClosedError.new(changeset) if changeset.is_open?
     raise OSM::APIChangesetAlreadySubscribedError.new(changeset) if changeset.subscribers.exists?(@user)
 
+    # Add the subscriber
     changeset.subscribers << @user
+
     # Return a copy of the updated changeset
     render :text => changeset.to_xml.to_s, :content_type => "text/xml"
   end
 
-  ## 
+  ##
   # Removes a subscriber from the changeset
-  def unsubscribe 
+  def unsubscribe
     # Check the arguments are sane
     raise OSM::APIBadUserInput.new("No id was given") unless params[:id]
 
@@ -380,13 +382,14 @@ class ChangesetController < ApplicationController
     raise OSM::APIChangesetNotYetClosedError.new(changeset) if changeset.is_open?
     raise OSM::APIChangesetNotSubscribedError.new(changeset) unless changeset.subscribers.exists?(@user)
 
+    # Remove the subscriber
     changeset.subscribers.delete(@user)
 
     # Return a copy of the updated changeset
     render :text => changeset.to_xml.to_s, :content_type => "text/xml"
   end
 
-  ## 
+  ##
   # Sets visible flag on comment to false
   def hide_comment
     # Check the arguments are sane
@@ -396,16 +399,16 @@ class ChangesetController < ApplicationController
     id = params[:id].to_i
 
     # Find the changeset
-    @comment = ChangesetComment.find(id)
-    changeset = @comment.changeset
+    comment = ChangesetComment.find(id)
 
-    @comment.update(:visible => false)
+    # Hide the comment
+    comment.update(:visible => false)
 
     # Return a copy of the updated changeset
-    render :text => changeset.to_xml.to_s, :content_type => "text/xml"
+    render :text => comment.changeset.to_xml.to_s, :content_type => "text/xml"
   end
 
-  ## 
+  ##
   # Sets visible flag on comment to true
   def unhide_comment
     # Check the arguments are sane
@@ -415,13 +418,13 @@ class ChangesetController < ApplicationController
     id = params[:id].to_i
 
     # Find the changeset
-    @comment = ChangesetComment.find(id)
-    changeset = @comment.changeset
+    comment = ChangesetComment.find(id)
 
-    @comment.update :visible => true
+    # Unhide the comment
+    comment.update(:visible => true)
 
     # Return a copy of the updated changeset
-    render :text => changeset.to_xml.to_s, :content_type => "text/xml"
+    render :text => comment.changeset.to_xml.to_s, :content_type => "text/xml"
   end
 
   ##
@@ -434,9 +437,10 @@ class ChangesetController < ApplicationController
       # Find the changeset
       changeset = Changeset.find(id)
 
-      # Find the comments we want to return
+      # Return comments for this changeset only
       @comments = changeset.comments.includes(:author, :changeset).limit(comments_limit)
     else
+      # Return comments
       @comments = ChangesetComment.includes(:author, :changeset).where(:visible => :true).order("created_at DESC").limit(comments_limit).preload(:changeset)
     end
 
@@ -589,5 +593,4 @@ private
       100
     end
   end
-
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index e5ff1917d..ed0813bee 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -12,7 +12,7 @@ class User < ActiveRecord::Base
   has_many :tokens, :class_name => "UserToken"
   has_many :preferences, :class_name => "UserPreference"
   has_many :changesets, -> { order(:created_at => :desc) }
-  has_many :changeset_comments, :foreign_key =>  'author_id'
+  has_many :changeset_comments, :foreign_key =>  :author_id
   has_and_belongs_to_many :changeset_subscriptions, :class_name => 'Changeset', :join_table => 'changesets_subscribers', :foreign_key => 'subscriber_id'
   has_many :note_comments, :foreign_key => :author_id
   has_many :notes, :through => :note_comments
diff --git a/app/views/changeset/_comments.rss.builder b/app/views/changeset/_comments.rss.builder
index 5effbc788..8ad5cbaa7 100644
--- a/app/views/changeset/_comments.rss.builder
+++ b/app/views/changeset/_comments.rss.builder
@@ -1,5 +1,4 @@
 comments.each do |comment|
-
   xml.item do
     xml.title t("changeset.rss.comment", :author => comment.author.display_name, :changeset_id => comment.changeset.id.to_s)
     
@@ -16,4 +15,4 @@ comments.each do |comment|
 
     xml.pubDate comment.created_at.to_s(:rfc822)
   end
-end
\ No newline at end of file
+end
diff --git a/app/views/changeset/comments_feed.rss.builder b/app/views/changeset/comments_feed.rss.builder
index 5224d9f93..60a229a30 100644
--- a/app/views/changeset/comments_feed.rss.builder
+++ b/app/views/changeset/comments_feed.rss.builder
@@ -9,6 +9,6 @@ xml.rss("version" => "2.0",
     xml.link url_for(:controller => "site", :action => "index", :only_path => false)
 
     xml << render(:partial => "comments", :object => @comments)
-
   end
-end
\ No newline at end of file
+end
+
diff --git a/app/views/notifier/changeset_comment_notification.html.erb b/app/views/notifier/changeset_comment_notification.html.erb
index 2a07032c3..b7646a886 100644
--- a/app/views/notifier/changeset_comment_notification.html.erb
+++ b/app/views/notifier/changeset_comment_notification.html.erb
@@ -1,6 +1,5 @@
 <p><%= t 'notifier.changeset_comment_notification.greeting' %></p>
 
-
 <p>
   <% if @owner %>
     <%= t "notifier.changeset_comment_notification.commented.your_changeset", :commenter => @commenter, :time => @time %>
@@ -14,7 +13,6 @@
   <% end %>
 </p>
 
-
 ==
 <%= @comment.to_html %>
 ==
diff --git a/db/migrate/20140507110937_create_changeset_comments.rb b/db/migrate/20140507110937_create_changeset_comments.rb
index a44fd61ce..8d2a4599d 100644
--- a/db/migrate/20140507110937_create_changeset_comments.rb
+++ b/db/migrate/20140507110937_create_changeset_comments.rb
@@ -9,8 +9,10 @@ class CreateChangesetComments < ActiveRecord::Migration
       t.timestamp :created_at, :null => false
       t.boolean :visible, :null => false
     end
+
     add_foreign_key :changeset_comments, [:changeset_id], :changesets, [:id]
     add_foreign_key :changeset_comments, [:author_id], :users, [:id]
+
     add_index :changeset_comments, :created_at
   end
 end
diff --git a/db/migrate/20140519141742_add_join_table_between_users_and_changesets.rb b/db/migrate/20140519141742_add_join_table_between_users_and_changesets.rb
index 37ec1a220..d07c6aae9 100644
--- a/db/migrate/20140519141742_add_join_table_between_users_and_changesets.rb
+++ b/db/migrate/20140519141742_add_join_table_between_users_and_changesets.rb
@@ -1,13 +1,16 @@
 require 'migrate'
+
 class AddJoinTableBetweenUsersAndChangesets < ActiveRecord::Migration
   def change
     create_table :changesets_subscribers, id: false do |t|
       t.column :subscriber_id, :bigint, null: false
       t.column :changeset_id, :bigint, null: false
     end
+
     add_foreign_key :changesets_subscribers, [:subscriber_id], :users, [:id]
     add_foreign_key :changesets_subscribers, [:changeset_id], :changesets, [:id]
-    add_index :changesets_subscribers, [:subscriber_id, :changeset_id], { :unique => true }
+
+    add_index :changesets_subscribers, [:subscriber_id, :changeset_id], :unique => true
     add_index :changesets_subscribers, [:changeset_id]
   end
 end