From ba5107ebb543a396fc78069c75c431e4d3887ee1 Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Sun, 2 Dec 2012 16:29:00 +0000 Subject: [PATCH] Remove the author_name field from notes --- .../javascripts/templates/notes/show.jst.ejs | 12 +++++-- app/controllers/notes_controller.rb | 18 +++------- app/helpers/application_helper.rb | 4 +-- app/models/note.rb | 12 +------ app/models/note_comment.rb | 9 ----- app/models/notifier.rb | 20 +++++++---- app/views/notes/_note.json.jsonify | 4 +-- app/views/notes/_note.rss.builder | 6 +++- app/views/notes/_note.xml.builder | 4 +-- app/views/notes/feed.rss.builder | 6 +++- app/views/notes/mine.html.erb | 4 +-- config/locales/en.yml | 2 ++ ...09_remove_author_name_from_note_comment.rb | 9 +++++ db/structure.sql | 6 ++-- test/fixtures/note_comments.yml | 10 ------ test/functional/notes_controller_test.rb | 34 +++++++++---------- 16 files changed, 79 insertions(+), 81 deletions(-) create mode 100644 db/migrate/20121202155309_remove_author_name_from_note_comment.rb diff --git a/app/assets/javascripts/templates/notes/show.jst.ejs b/app/assets/javascripts/templates/notes/show.jst.ejs index 8b95fea2e..d85d78592 100644 --- a/app/assets/javascripts/templates/notes/show.jst.ejs +++ b/app/assets/javascripts/templates/notes/show.jst.ejs @@ -3,9 +3,15 @@ <% note.comments.forEach(function (comment) { %>

- <%- I18n.t('javascripts.notes.show.event', { - action: comment.action, user: comment.user, time: comment.date - }) %> + <% if (comment.user) { %> + <%- I18n.t('javascripts.notes.show.event', { + action: comment.action, user: comment.user, time: comment.date + }) %> + <% } else { %> + <%- I18n.t('javascripts.notes.show.anonymous_event', { + action: comment.action, time: comment.date + }) %> + <% } %>
<%- comment.text %> diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index 5a0934247..d25d9a37e 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -58,7 +58,6 @@ class NotesController < ApplicationController lon = params[:lon].to_f lat = params[:lat].to_f comment = params[:text] - name = params[:name] # Include in a transaction to ensure that there is always a note_comment for every note Note.transaction do @@ -70,7 +69,7 @@ class NotesController < ApplicationController @note.save! # Add a comment to the note - add_comment(@note, comment, name, "opened") + add_comment(@note, comment, "opened") end # Return a copy of the new note @@ -90,7 +89,6 @@ class NotesController < ApplicationController # Extract the arguments id = params[:id].to_i comment = params[:text] - name = params[:name] or "NoName" # Find the note and check it is valid @note = Note.find(id) @@ -99,7 +97,7 @@ class NotesController < ApplicationController # Add a comment to the note Note.transaction do - add_comment(@note, comment, name, "commented") + add_comment(@note, comment, "commented") end # Return a copy of the updated note @@ -118,7 +116,6 @@ class NotesController < ApplicationController # Extract the arguments id = params[:id].to_i comment = params[:text] - name = params[:name] # Find the note and check it is valid @note = Note.find_by_id(id) @@ -129,7 +126,7 @@ class NotesController < ApplicationController Note.transaction do @note.close - add_comment(@note, comment, name, "closed") + add_comment(@note, comment, "closed") end # Return a copy of the updated note @@ -192,7 +189,6 @@ class NotesController < ApplicationController # Extract the arguments id = params[:id].to_i - name = params[:name] # Find the note and check it is valid note = Note.find(id) @@ -204,7 +200,7 @@ class NotesController < ApplicationController note.status = "hidden" note.save - add_comment(note, nil, name, "hidden") + add_comment(note, nil, "hidden") end # Render the result @@ -302,17 +298,13 @@ private ## # Add a comment to a note - def add_comment(note, text, name, event) - name = "NoName" if name.nil? - + def add_comment(note, text, event) attributes = { :visible => true, :event => event, :body => text } if @user attributes[:author_id] = @user.id - attributes[:author_name] = @user.display_name else attributes[:author_ip] = request.remote_ip - attributes[:author_name] = name + " (a)" end comment = note.comments.create(attributes, :without_protection => true) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 18f233e50..e32d1bb6e 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -102,9 +102,9 @@ module ApplicationHelper def note_author(object, link_options = {}) if object.author.nil? - h(object.author_name) + "" else - link_to h(object.author_name), link_options.merge({:controller => "user", :action => "view", :display_name => object.author_name}) + link_to h(object.author.display_name), link_options.merge({:controller => "user", :action => "view", :display_name => object.author.display_name}) end end end diff --git a/app/models/note.rb b/app/models/note.rb index c32b1679b..14806be26 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -39,7 +39,7 @@ class Note < ActiveRecord::Base resp += (comment_no == 1 ? "" : separator_char) resp += comment.body if comment.body resp += " [ " - resp += comment.author_name if comment.author_name + resp += comment.author.display_name if comment.author resp += " " + comment.created_at.to_s + " ]" comment_no += 1 end @@ -62,16 +62,6 @@ class Note < ActiveRecord::Base self.comments.first.author_ip end - # Return the author id, derived from the first comment - def author_id - self.comments.first.author_id - end - - # Return the author name, derived from the first comment - def author_name - self.comments.first.author_name - end - private # Fill in default values for new notes diff --git a/app/models/note_comment.rb b/app/models/note_comment.rb index 0500acb8b..399029118 100644 --- a/app/models/note_comment.rb +++ b/app/models/note_comment.rb @@ -10,15 +10,6 @@ class NoteComment < ActiveRecord::Base validates_associated :author validates_inclusion_of :event, :in => [ "opened", "closed", "reopened", "commented", "hidden" ] - # Return the author name - def author_name - if self.author_id.nil? - self.read_attribute(:author_name) - else - self.author.display_name - end - end - # Return the comment text def body RichText.new("text", read_attribute(:body)) diff --git a/app/models/notifier.rb b/app/models/notifier.rb index ae50fa9b1..5972a700b 100644 --- a/app/models/notifier.rb +++ b/app/models/notifier.rb @@ -6,7 +6,7 @@ class Notifier < ActionMailer::Base def signup_confirm(user, token) @locale = user.preferred_language_from(I18n.available_locales) - + # If we are passed an email address verification token, create # the confirumation URL for account activation. # @@ -19,7 +19,7 @@ class Notifier < ActionMailer::Base :display_name => user.display_name, :confirm_string => token.token) end - + mail :to => user.email, :subject => I18n.t('notifier.signup_confirm.subject', :locale => @locale) end @@ -67,7 +67,7 @@ class Notifier < ActionMailer::Base mail :to => trace.user.email, :subject => I18n.t('notifier.gpx_notification.failure.subject', :locale => @locale) end - + def message_notification(message) @locale = message.recipient.preferred_language_from(I18n.available_locales) @to_user = message.recipient.display_name @@ -129,10 +129,18 @@ class Notifier < ActionMailer::Base @place = Nominatim.describe_location(comment.note.lat, comment.note.lon, 14, @locale) @comment = comment.body @owner = recipient == comment.note.author - @commenter = comment.author_name - subject = I18n.t('notifier.note_comment_notification.subject_own', :commenter => comment.author_name) if @owner - subject = I18n.t('notifier.note_comment_notification.subject_other', :commenter => comment.author_name) unless @owner + if comment.author + @commenter = comment.author.display_name + else + @commenter = I18n.t("notifier.note_comment_notification.anonymous") + end + + if @owner + subject = I18n.t('notifier.note_comment_notification.subject_own', :commenter => @commenter) + else + subject = I18n.t('notifier.note_comment_notification.subject_other', :commenter => @commenter) + end mail :to => recipient.email, :subject => subject end diff --git a/app/views/notes/_note.json.jsonify b/app/views/notes/_note.json.jsonify index f252a0dfc..ce9ddb4bf 100644 --- a/app/views/notes/_note.json.jsonify +++ b/app/views/notes/_note.json.jsonify @@ -16,8 +16,8 @@ json.properties do json.comments(note.comments) do |comment| json.date comment.created_at - json.uid comment.author_id unless comment.author_id.nil? - json.user comment.author_name + json.uid comment.author.id unless comment.author.nil? + json.user comment.author.display_name unless comment.author.nil? json.action comment.event json.text comment.body unless comment.body.nil? end diff --git a/app/views/notes/_note.rss.builder b/app/views/notes/_note.rss.builder index cffc3f271..fbd217beb 100644 --- a/app/views/notes/_note.rss.builder +++ b/app/views/notes/_note.rss.builder @@ -12,7 +12,11 @@ xml.item do xml.link browse_note_url(note) xml.guid note_url(note) xml.description render(:partial => "description", :object => note, :formats => [ :html ]) - xml.author note.comments.first.author_name + + if note.author + xml.author note.author_display_name + end + xml.pubDate note.updated_at.to_s(:rfc822) xml.geo :lat, note.lat xml.geo :long, note.lon diff --git a/app/views/notes/_note.xml.builder b/app/views/notes/_note.xml.builder index a68946bb7..187772676 100644 --- a/app/views/notes/_note.xml.builder +++ b/app/views/notes/_note.xml.builder @@ -14,8 +14,8 @@ xml.note("lon" => note.lon, "lat" => note.lat) do note.comments.each do |comment| xml.comment do xml.date comment.created_at - xml.uid comment.author_id unless comment.author_id.nil? - xml.user comment.author_name + xml.uid comment.author.id unless comment.author.nil? + xml.user comment.author.display_name unless comment.author.nil? xml.text comment.body end end diff --git a/app/views/notes/feed.rss.builder b/app/views/notes/feed.rss.builder index 7113364f0..4733c1bad 100644 --- a/app/views/notes/feed.rss.builder +++ b/app/views/notes/feed.rss.builder @@ -37,7 +37,11 @@ xml.rss("version" => "2.0", description_text += comment.note.flatten_comment("
", comment.created_at) xml.description description_text - xml.author comment.author_name + + if comment.author + xml.author comment.author.display_name + end + xml.pubDate comment.created_at.to_s(:rfc822) xml.geo :lat, comment.note.lat xml.geo :long, comment.note.lon diff --git a/app/views/notes/mine.html.erb b/app/views/notes/mine.html.erb index c58548b47..e385e942b 100644 --- a/app/views/notes/mine.html.erb +++ b/app/views/notes/mine.html.erb @@ -23,9 +23,9 @@ <%= link_to note.id.to_s, :controller => "browse", :action => "note", :id => note.id %> <% if note.author.nil? %> - <%= note.author_name %> + <% else %> - <%= link_to h(note.author_name), :controller => "user", :action => "view", :display_name => note.author_name %> + <%= link_to h(note.author.display_name), user_url(:display_name => note.author.display_name) %> <% end %> <%= note.comments.first.body.to_html %> <%= l note.created_at %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 62b6b90f8..605ff4381 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1204,6 +1204,7 @@ en: hopefully_you: "Someone (possibly you) has asked for the password to be reset on this email address's openstreetmap.org account." click_the_link: "If this is you, please click the link below to reset your password." note_comment_notification: + anonymous: An anonymous user subject_own: "[OpenStreetMap] %{commenter} has commented on one of your notes" subject_other: "[OpenStreetMap] %{commenter} has commented on a note you are interested in" greeting: "Hi," @@ -2022,6 +2023,7 @@ en: show: title: Note %{id} event: "%{action} by %{user} at %{time}" + anonymous_event: "%{action} by anonymous at %{time}" close: Close comment_and_close: Comment & Close comment: Comment diff --git a/db/migrate/20121202155309_remove_author_name_from_note_comment.rb b/db/migrate/20121202155309_remove_author_name_from_note_comment.rb new file mode 100644 index 000000000..60d0ab23b --- /dev/null +++ b/db/migrate/20121202155309_remove_author_name_from_note_comment.rb @@ -0,0 +1,9 @@ +class RemoveAuthorNameFromNoteComment < ActiveRecord::Migration + def up + remove_column :note_comments, :author_name + end + + def down + add_column :note_comments, :author_name, :string + end +end diff --git a/db/structure.sql b/db/structure.sql index 84c38c86e..eaf279f9c 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -733,7 +733,6 @@ CREATE TABLE note_comments ( note_id bigint NOT NULL, visible boolean NOT NULL, created_at timestamp without time zone NOT NULL, - author_name character varying(255), author_ip character varying(255), author_id bigint, body text, @@ -771,7 +770,6 @@ CREATE TABLE notes ( tile bigint NOT NULL, updated_at timestamp without time zone NOT NULL, created_at timestamp without time zone NOT NULL, - nearby_place character varying(255), status note_status_enum NOT NULL, closed_at timestamp without time zone ); @@ -2447,6 +2445,10 @@ INSERT INTO schema_migrations (version) VALUES ('20121005195010'); INSERT INTO schema_migrations (version) VALUES ('20121012044047'); +INSERT INTO schema_migrations (version) VALUES ('20121119165817'); + +INSERT INTO schema_migrations (version) VALUES ('20121202155309'); + INSERT INTO schema_migrations (version) VALUES ('21'); INSERT INTO schema_migrations (version) VALUES ('22'); diff --git a/test/fixtures/note_comments.yml b/test/fixtures/note_comments.yml index e078b990a..881adb6ac 100644 --- a/test/fixtures/note_comments.yml +++ b/test/fixtures/note_comments.yml @@ -3,7 +3,6 @@ t1: note_id: 1 visible: true created_at: 2007-01-01 00:00:00 - author_name: 'testname' author_ip: '192.168.1.1' body: 'This is the initial description of the note 1' @@ -12,7 +11,6 @@ t2: note_id: 2 visible: true created_at: 2007-01-01 00:00:00 - author_name: 'testname' author_ip: '192.168.1.1' body: 'This is the initial description of the note 2' @@ -21,7 +19,6 @@ t3: note_id: 2 visible: true created_at: 2007-02-01 00:00:00 - author_name: 'testname' author_ip: '192.168.1.1' body: 'This is an additional comment for note 2' @@ -30,7 +27,6 @@ t4: note_id: 3 visible: true created_at: 2007-01-01 00:00:00 - author_name: 'testname' author_ip: '192.168.1.1' body: 'This is the initial comment for note 3' @@ -39,7 +35,6 @@ t5: note_id: 4 visible: true created_at: 2007-01-01 00:00:00 - author_name: 'testname' author_ip: '192.168.1.1' body: 'Spam for note 4' @@ -48,7 +43,6 @@ t6: note_id: 5 visible: true created_at: 2007-01-01 00:00:00 - author_name: 'testname' author_ip: '192.168.1.1' body: 'Valid comment for note 5' @@ -57,7 +51,6 @@ t7: note_id: 5 visible: false created_at: 2007-02-01 00:00:00 - author_name: 'testname' author_ip: '192.168.1.1' body: 'Spam for note 5' @@ -66,7 +59,6 @@ t8: note_id: 5 visible: true created_at: 2007-02-01 00:00:00 - author_name: 'testname' author_ip: '192.168.1.1' body: 'Another valid comment for note 5' @@ -94,7 +86,6 @@ t11: visible: true event: opened created_at: 2007-01-01 00:00:00 - author_name: 'testname' author_ip: '192.168.1.1' body: 'Initial note description' @@ -104,7 +95,6 @@ t12: visible: true event: commented created_at: 2007-02-01 00:00:00 - author_name: 'testname' author_ip: '192.168.1.1' body: 'A comment description' diff --git a/test/functional/notes_controller_test.rb b/test/functional/notes_controller_test.rb index 00e887f33..1c0ded355 100644 --- a/test/functional/notes_controller_test.rb +++ b/test/functional/notes_controller_test.rb @@ -120,7 +120,7 @@ class NotesControllerTest < ActionController::TestCase def test_note_create_success assert_difference('Note.count') do assert_difference('NoteComment.count') do - post :create, {:lat => -1.0, :lon => -1.0, :name => "new_tester", :text => "This is a comment", :format => "json"} + post :create, {:lat => -1.0, :lon => -1.0, :text => "This is a comment", :format => "json"} end end assert_response :success @@ -133,7 +133,7 @@ class NotesControllerTest < ActionController::TestCase assert_equal 1, js["properties"]["comments"].count assert_equal "opened", js["properties"]["comments"].last["action"] assert_equal "This is a comment", js["properties"]["comments"].last["text"] - assert_equal "new_tester (a)", js["properties"]["comments"].last["user"] + assert_nil js["properties"]["comments"].last["user"] id = js["properties"]["id"] get :show, {:id => id, :format => "json"} @@ -148,41 +148,41 @@ class NotesControllerTest < ActionController::TestCase assert_equal 1, js["properties"]["comments"].count assert_equal "opened", js["properties"]["comments"].last["action"] assert_equal "This is a comment", js["properties"]["comments"].last["text"] - assert_equal "new_tester (a)", js["properties"]["comments"].last["user"] + assert_nil js["properties"]["comments"].last["user"] end def test_note_create_fail assert_no_difference('Note.count') do assert_no_difference('NoteComment.count') do - post :create, {:lon => -1.0, :name => "new_tester", :text => "This is a comment"} + post :create, {:lon => -1.0, :text => "This is a comment"} end end assert_response :bad_request assert_no_difference('Note.count') do assert_no_difference('NoteComment.count') do - post :create, {:lat => -1.0, :name => "new_tester", :text => "This is a comment"} + post :create, {:lat => -1.0, :text => "This is a comment"} end end assert_response :bad_request assert_no_difference('Note.count') do assert_no_difference('NoteComment.count') do - post :create, {:lat => -1.0, :lon => -1.0, :name => "new_tester"} + post :create, {:lat => -1.0, :lon => -1.0} end end assert_response :bad_request assert_no_difference('Note.count') do assert_no_difference('NoteComment.count') do - post :create, {:lat => -100.0, :lon => -1.0, :name => "new_tester", :text => "This is a comment"} + post :create, {:lat => -100.0, :lon => -1.0, :text => "This is a comment"} end end assert_response :bad_request assert_no_difference('Note.count') do assert_no_difference('NoteComment.count') do - post :create, {:lat => -1.0, :lon => -200.0, :name => "new_tester", :text => "This is a comment"} + post :create, {:lat => -1.0, :lon => -200.0, :text => "This is a comment"} end end assert_response :bad_request @@ -190,7 +190,7 @@ class NotesControllerTest < ActionController::TestCase def test_note_comment_create_success assert_difference('NoteComment.count') do - post :comment, {:id => notes(:open_note_with_comment).id, :name => "new_tester2", :text => "This is an additional comment", :format => "json"} + post :comment, {:id => notes(:open_note_with_comment).id, :text => "This is an additional comment", :format => "json"} end assert_response :success js = ActiveSupport::JSON.decode(@response.body) @@ -201,7 +201,7 @@ class NotesControllerTest < ActionController::TestCase assert_equal 3, js["properties"]["comments"].count assert_equal "commented", js["properties"]["comments"].last["action"] assert_equal "This is an additional comment", js["properties"]["comments"].last["text"] - assert_equal "new_tester2 (a)", js["properties"]["comments"].last["user"] + assert_nil js["properties"]["comments"].last["user"] get :show, {:id => notes(:open_note_with_comment).id, :format => "json"} assert_response :success @@ -213,27 +213,27 @@ class NotesControllerTest < ActionController::TestCase assert_equal 3, js["properties"]["comments"].count assert_equal "commented", js["properties"]["comments"].last["action"] assert_equal "This is an additional comment", js["properties"]["comments"].last["text"] - assert_equal "new_tester2 (a)", js["properties"]["comments"].last["user"] + assert_nil js["properties"]["comments"].last["user"] end def test_note_comment_create_fail assert_no_difference('NoteComment.count') do - post :comment, {:name => "new_tester2", :text => "This is an additional comment"} + post :comment, {:text => "This is an additional comment"} end assert_response :bad_request assert_no_difference('NoteComment.count') do - post :comment, {:id => notes(:open_note_with_comment).id, :name => "new_tester2"} + post :comment, {:id => notes(:open_note_with_comment).id} end assert_response :bad_request assert_no_difference('NoteComment.count') do - post :comment, {:id => 12345, :name => "new_tester2", :text => "This is an additional comment"} + post :comment, {:id => 12345, :text => "This is an additional comment"} end assert_response :not_found assert_no_difference('NoteComment.count') do - post :comment, {:id => notes(:hidden_note_with_comment).id, :name => "new_tester2", :text => "This is an additional comment"} + post :comment, {:id => notes(:hidden_note_with_comment).id, :text => "This is an additional comment"} end assert_response :gone end @@ -249,7 +249,7 @@ class NotesControllerTest < ActionController::TestCase assert_equal 3, js["properties"]["comments"].count assert_equal "closed", js["properties"]["comments"].last["action"] assert_equal "This is a close comment", js["properties"]["comments"].last["text"] - assert_equal "NoName (a)", js["properties"]["comments"].last["user"] + assert_nil js["properties"]["comments"].last["user"] get :show, {:id => notes(:open_note_with_comment).id, :format => "json"} assert_response :success @@ -261,7 +261,7 @@ class NotesControllerTest < ActionController::TestCase assert_equal 3, js["properties"]["comments"].count assert_equal "closed", js["properties"]["comments"].last["action"] assert_equal "This is a close comment", js["properties"]["comments"].last["text"] - assert_equal "NoName (a)", js["properties"]["comments"].last["user"] + assert_nil js["properties"]["comments"].last["user"] end def test_note_close_fail -- 2.43.2