From 9748ce301c9488b9cd32439448770c3d4a38ebd6 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 25 Jan 2023 21:46:35 +0000 Subject: [PATCH] Move browse#note to notes#show This allows a more resourceful routing approach. --- app/abilities/ability.rb | 4 +- app/controllers/browse_controller.rb | 14 --- app/controllers/notes_controller.rb | 14 +++ app/controllers/site_controller.rb | 2 +- app/helpers/issues_helper.rb | 2 +- app/helpers/note_helper.rb | 4 +- app/mailers/user_mailer.rb | 2 +- app/views/api/notes/_note.gpx.builder | 4 +- app/views/api/notes/_note.rss.builder | 2 +- app/views/api/notes/feed.rss.builder | 4 +- app/views/notes/index.html.erb | 2 +- app/views/notes/new.html.erb | 4 +- .../note.html.erb => notes/show.html.erb} | 0 config/locales/en.yml | 37 +++--- config/routes.rb | 3 +- test/controllers/api/notes_controller_test.rb | 2 +- test/controllers/browse_controller_test.rb | 76 ------------ test/controllers/notes_controller_test.rb | 115 +++++++++++++++++- test/controllers/site_controller_test.rb | 2 +- test/system/index_test.rb | 6 +- test/system/report_note_test.rb | 12 +- test/system/report_user_test.rb | 2 +- 22 files changed, 175 insertions(+), 138 deletions(-) rename app/views/{browse/note.html.erb => notes/show.html.erb} (100%) diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index aee44e6e5..5d196fa98 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -5,8 +5,8 @@ class Ability def initialize(user) can [:relation, :relation_history, :way, :way_history, :node, :node_history, - :changeset, :note, :query], :browse - can [:new], Note + :changeset, :query], :browse + can [:show, :new], Note can :search, :direction can [:index, :permalink, :edit, :help, :fixthemap, :offline, :export, :about, :communities, :preview, :copyright, :key, :id], :site can [:finish, :embed], :export diff --git a/app/controllers/browse_controller.rb b/app/controllers/browse_controller.rb index 6c5336908..88871a9e1 100644 --- a/app/controllers/browse_controller.rb +++ b/app/controllers/browse_controller.rb @@ -76,19 +76,5 @@ class BrowseController < ApplicationController render :action => "not_found", :status => :not_found end - def note - @type = "note" - - if current_user&.moderator? - @note = Note.find(params[:id]) - @note_comments = @note.comments.unscope(:where => :visible) - else - @note = Note.visible.find(params[:id]) - @note_comments = @note.comments - end - rescue ActiveRecord::RecordNotFound - render :action => "not_found", :status => :not_found - end - def query; end end diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index 4c1bac193..440a620e8 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -33,5 +33,19 @@ class NotesController < ApplicationController end end + def show + @type = "note" + + if current_user&.moderator? + @note = Note.find(params[:id]) + @note_comments = @note.comments.unscope(:where => :visible) + else + @note = Note.visible.find(params[:id]) + @note_comments = @note.comments + end + rescue ActiveRecord::RecordNotFound + render :template => "browse/not_found", :status => :not_found + end + def new; end end diff --git a/app/controllers/site_controller.rb b/app/controllers/site_controller.rb index 9d9cf0f39..a05fe376b 100644 --- a/app/controllers/site_controller.rb +++ b/app/controllers/site_controller.rb @@ -140,7 +140,7 @@ class SiteController < ApplicationController elsif params[:relation] redirect_to relation_path(params[:relation]) elsif params[:note] - redirect_to browse_note_path(params[:note]) + redirect_to note_path(params[:note]) elsif params[:query] redirect_to search_path(:query => params[:query]) end diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index e6c1adb16..dcec4990b 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -8,7 +8,7 @@ module IssuesHelper when DiaryComment diary_entry_url(reportable.diary_entry.user, reportable.diary_entry, :anchor => "comment#{reportable.id}") when Note - url_for(:controller => :browse, :action => :note, :id => reportable.id) + note_url(reportable) end end diff --git a/app/helpers/note_helper.rb b/app/helpers/note_helper.rb index 809480a04..86e5c40f8 100644 --- a/app/helpers/note_helper.rb +++ b/app/helpers/note_helper.rb @@ -3,11 +3,11 @@ module NoteHelper def note_event(event, at, by) if by.nil? - t("browse.note.#{event}_by_anonymous_html", + t("notes.show.#{event}_by_anonymous_html", :when => friendly_date_ago(at), :exact_time => l(at)) else - t("browse.note.#{event}_by_html", + t("notes.show.#{event}_by_html", :when => friendly_date_ago(at), :exact_time => l(at), :user => note_author(by)) diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index 33fcc7465..c5c0118e6 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -123,7 +123,7 @@ class UserMailer < ApplicationMailer def note_comment_notification(comment, recipient) with_recipient_locale recipient do - @noteurl = browse_note_url(comment.note) + @noteurl = note_url(comment.note) @place = Nominatim.describe_location(comment.note.lat, comment.note.lon, 14, I18n.locale) @comment = comment.body @owner = recipient == comment.note.author diff --git a/app/views/api/notes/_note.gpx.builder b/app/views/api/notes/_note.gpx.builder index 0753b1914..bb5cac1c9 100644 --- a/app/views/api/notes/_note.gpx.builder +++ b/app/views/api/notes/_note.gpx.builder @@ -1,12 +1,12 @@ xml.wpt("lon" => note.lon, "lat" => note.lat) do xml.time note.created_at.to_fs(:iso8601) - xml.name t("browse.note.title", :id => note.id) + xml.name t("notes.show.title", :id => note.id) xml.desc do xml.cdata! render(:partial => "description", :object => note, :formats => [:html]) end - xml.link("href" => browse_note_url(note, :only_path => false)) + xml.link("href" => note_url(note, :only_path => false)) xml.extensions do xml.id note.id diff --git a/app/views/api/notes/_note.rss.builder b/app/views/api/notes/_note.rss.builder index a26f54aa8..fa70536f7 100644 --- a/app/views/api/notes/_note.rss.builder +++ b/app/views/api/notes/_note.rss.builder @@ -9,7 +9,7 @@ xml.item do xml.title t("api.notes.rss.opened", :place => location) end - xml.link browse_note_url(note) + xml.link note_url(note) xml.guid api_note_url(note) xml.description render(:partial => "description", :object => note, :formats => [:html]) diff --git a/app/views/api/notes/feed.rss.builder b/app/views/api/notes/feed.rss.builder index 89f888f5b..cf380be31 100644 --- a/app/views/api/notes/feed.rss.builder +++ b/app/views/api/notes/feed.rss.builder @@ -15,8 +15,8 @@ xml.rss("version" => "2.0", xml.item do xml.title t("api.notes.rss.#{comment.event}", :place => location) - xml.link url_for(:controller => "/browse", :action => "note", :id => comment.note.id, :anchor => "c#{comment.id}", :only_path => false) - xml.guid url_for(:controller => "/browse", :action => "note", :id => comment.note.id, :anchor => "c#{comment.id}", :only_path => false) + xml.link url_for(:controller => "/notes", :action => "show", :id => comment.note.id, :anchor => "c#{comment.id}", :only_path => false) + xml.guid url_for(:controller => "/notes", :action => "show", :id => comment.note.id, :anchor => "c#{comment.id}", :only_path => false) xml.description do xml.cdata! render(:partial => "entry", :object => comment, :formats => [:html]) diff --git a/app/views/notes/index.html.erb b/app/views/notes/index.html.erb index a627e1c54..2eb99b812 100644 --- a/app/views/notes/index.html.erb +++ b/app/views/notes/index.html.erb @@ -29,7 +29,7 @@ <%= image_tag("open_note_marker.png", :alt => "open", :size => "25x40") %> <% end %> - <%= link_to note.id, browse_note_path(note) %> + <%= link_to note.id, note %> <%= note_author(note.author) %> <%= note.comments.first.body.to_html %> <%= friendly_date_ago(note.created_at) %> diff --git a/app/views/notes/new.html.erb b/app/views/notes/new.html.erb index c8f658f53..7b3b807e3 100644 --- a/app/views/notes/new.html.erb +++ b/app/views/notes/new.html.erb @@ -1,6 +1,6 @@ -<% set_title(t("browse.note.new_note")) %> +<% set_title(t(".title")) %> -<%= render "sidebar_header", :title => t("browse.note.new_note") %> +<%= render "sidebar_header", :title => t(".title") %>

<%= t("javascripts.notes.new.intro") %>

diff --git a/app/views/browse/note.html.erb b/app/views/notes/show.html.erb similarity index 100% rename from app/views/browse/note.html.erb rename to app/views/notes/show.html.erb diff --git a/config/locales/en.yml b/config/locales/en.yml index b1752834b..6a405170d 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -407,24 +407,6 @@ en: telephone_link: "Call %{phone_number}" colour_preview: "Colour %{colour_value} preview" email_link: "Email %{email}" - note: - title: "Note: %{id}" - new_note: "New Note" - description: "Description" - open_title: "Unresolved note #%{note_name}" - closed_title: "Resolved note #%{note_name}" - hidden_title: "Hidden note #%{note_name}" - opened_by_html: "Created by %{user} %{when}" - opened_by_anonymous_html: "Created by anonymous %{when}" - commented_by_html: "Comment from %{user} %{when}" - commented_by_anonymous_html: "Comment from anonymous %{when}" - closed_by_html: "Resolved by %{user} %{when}" - closed_by_anonymous_html: "Resolved by anonymous %{when}" - reopened_by_html: "Reactivated by %{user} %{when}" - reopened_by_anonymous_html: "Reactivated by anonymous %{when}" - hidden_by_html: "Hidden by %{user} %{when}" - report: report this note - coordinates_html: "%{latitude}, %{longitude}" query: title: "Query Features" introduction: "Click on the map to find nearby features." @@ -2837,6 +2819,25 @@ en: description: "Description" created_at: "Created at" last_changed: "Last changed" + show: + title: "Note: %{id}" + description: "Description" + open_title: "Unresolved note #%{note_name}" + closed_title: "Resolved note #%{note_name}" + hidden_title: "Hidden note #%{note_name}" + opened_by_html: "Created by %{user} %{when}" + opened_by_anonymous_html: "Created by anonymous %{when}" + commented_by_html: "Comment from %{user} %{when}" + commented_by_anonymous_html: "Comment from anonymous %{when}" + closed_by_html: "Resolved by %{user} %{when}" + closed_by_anonymous_html: "Resolved by anonymous %{when}" + reopened_by_html: "Reactivated by %{user} %{when}" + reopened_by_anonymous_html: "Reactivated by anonymous %{when}" + hidden_by_html: "Hidden by %{user} %{when}" + report: report this note + coordinates_html: "%{latitude}, %{longitude}" + new: + title: "New Note" javascripts: close: Close share: diff --git a/config/routes.rb b/config/routes.rb index d60202670..c0fbf2020 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -113,8 +113,7 @@ OpenStreetMap::Application.routes.draw do get "/relation/:id/history" => "browse#relation_history", :id => /\d+/, :as => :relation_history get "/changeset/:id" => "browse#changeset", :as => :changeset, :id => /\d+/ get "/changeset/:id/comments/feed" => "changeset_comments#index", :as => :changeset_comments_feed, :id => /\d*/, :defaults => { :format => "rss" } - get "/note/:id" => "browse#note", :id => /\d+/, :as => "browse_note" - resources :notes, :path => "note", :only => [:new] + resources :notes, :path => "note", :only => [:show, :new] get "/user/:display_name/history" => "changesets#index" get "/user/:display_name/history/feed" => "changesets#feed", :defaults => { :format => :atom } diff --git a/test/controllers/api/notes_controller_test.rb b/test/controllers/api/notes_controller_test.rb index f5f060977..da2478169 100644 --- a/test/controllers/api/notes_controller_test.rb +++ b/test/controllers/api/notes_controller_test.rb @@ -492,7 +492,7 @@ module Api assert_select "rss", :count => 1 do assert_select "channel", :count => 1 do assert_select "item", :count => 1 do - assert_select "link", browse_note_url(open_note) + assert_select "link", note_url(open_note) assert_select "guid", api_note_url(open_note) assert_select "pubDate", open_note.created_at.to_fs(:rfc822) assert_select "geo|lat", open_note.lat.to_s diff --git a/test/controllers/browse_controller_test.rb b/test/controllers/browse_controller_test.rb index 82068629d..578e78fa2 100644 --- a/test/controllers/browse_controller_test.rb +++ b/test/controllers/browse_controller_test.rb @@ -32,10 +32,6 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest { :path => "/changeset/1", :method => :get }, { :controller => "browse", :action => "changeset", :id => "1" } ) - assert_routing( - { :path => "/note/1", :method => :get }, - { :controller => "browse", :action => "note", :id => "1" } - ) assert_routing( { :path => "/query", :method => :get }, { :controller => "browse", :action => "query" } @@ -102,78 +98,6 @@ class BrowseControllerTest < ActionDispatch::IntegrationTest assert_select "div.changeset-comments ul li", :count => 4 end - def test_read_note - open_note = create(:note_with_comments) - - browse_check :browse_note_path, open_note.id, "browse/note" - end - - def test_read_hidden_note - hidden_note_with_comment = create(:note_with_comments, :status => "hidden") - - get browse_note_path(:id => hidden_note_with_comment) - assert_response :not_found - assert_template "browse/not_found" - assert_template :layout => "map" - - get browse_note_path(:id => hidden_note_with_comment), :xhr => true - assert_response :not_found - assert_template "browse/not_found" - assert_template :layout => "xhr" - - session_for(create(:moderator_user)) - - browse_check :browse_note_path, hidden_note_with_comment.id, "browse/note" - end - - def test_read_note_hidden_comments - note_with_hidden_comment = create(:note_with_comments, :comments_count => 2) do |note| - create(:note_comment, :note => note, :visible => false) - end - - browse_check :browse_note_path, note_with_hidden_comment.id, "browse/note" - assert_select "div.note-comments ul li", :count => 1 - - session_for(create(:moderator_user)) - - browse_check :browse_note_path, note_with_hidden_comment.id, "browse/note" - assert_select "div.note-comments ul li", :count => 2 - end - - def test_read_note_hidden_user_comment - hidden_user = create(:user, :deleted) - note_with_hidden_user_comment = create(:note_with_comments, :comments_count => 2) do |note| - create(:note_comment, :note => note, :author => hidden_user) - end - - browse_check :browse_note_path, note_with_hidden_user_comment.id, "browse/note" - assert_select "div.note-comments ul li", :count => 1 - - session_for(create(:moderator_user)) - - browse_check :browse_note_path, note_with_hidden_user_comment.id, "browse/note" - assert_select "div.note-comments ul li", :count => 1 - end - - def test_read_closed_note - user = create(:user) - closed_note = create(:note_with_comments, :status => "closed", :closed_at => Time.now.utc, :comments_count => 2) do |note| - create(:note_comment, :event => "closed", :note => note, :author => user) - end - - browse_check :browse_note_path, closed_note.id, "browse/note" - assert_select "div.note-comments ul li", :count => 2 - assert_select "div.details", /Resolved by #{user.display_name}/ - - user.soft_destroy! - - reset! - - browse_check :browse_note_path, closed_note.id, "browse/note" - assert_select "div.note-comments ul li", :count => 1 - assert_select "div.details", /Resolved by deleted/ - end - ## # Methods to check redaction. # diff --git a/test/controllers/notes_controller_test.rb b/test/controllers/notes_controller_test.rb index 4032b538d..5cffbd702 100644 --- a/test/controllers/notes_controller_test.rb +++ b/test/controllers/notes_controller_test.rb @@ -15,7 +15,10 @@ class NotesControllerTest < ActionDispatch::IntegrationTest { :path => "/user/username/notes", :method => :get }, { :controller => "notes", :action => "index", :display_name => "username" } ) - + assert_routing( + { :path => "/note/1", :method => :get }, + { :controller => "notes", :action => "show", :id => "1" } + ) assert_routing( { :path => "/note/new", :method => :get }, { :controller => "notes", :action => "new" } @@ -86,9 +89,119 @@ class NotesControllerTest < ActionDispatch::IntegrationTest assert_select "h4", :html => "No notes" end + def test_read_note + open_note = create(:note_with_comments) + + browse_check :note_path, open_note.id, "notes/show" + end + + def test_read_hidden_note + hidden_note_with_comment = create(:note_with_comments, :status => "hidden") + + get note_path(:id => hidden_note_with_comment) + assert_response :not_found + assert_template "browse/not_found" + assert_template :layout => "map" + + get note_path(:id => hidden_note_with_comment), :xhr => true + assert_response :not_found + assert_template "browse/not_found" + assert_template :layout => "xhr" + + session_for(create(:moderator_user)) + + browse_check :note_path, hidden_note_with_comment.id, "notes/show" + end + + def test_read_note_hidden_comments + note_with_hidden_comment = create(:note_with_comments, :comments_count => 2) do |note| + create(:note_comment, :note => note, :visible => false) + end + + browse_check :note_path, note_with_hidden_comment.id, "notes/show" + assert_select "div.note-comments ul li", :count => 1 + + session_for(create(:moderator_user)) + + browse_check :note_path, note_with_hidden_comment.id, "notes/show" + assert_select "div.note-comments ul li", :count => 2 + end + + def test_read_note_hidden_user_comment + hidden_user = create(:user, :deleted) + note_with_hidden_user_comment = create(:note_with_comments, :comments_count => 2) do |note| + create(:note_comment, :note => note, :author => hidden_user) + end + + browse_check :note_path, note_with_hidden_user_comment.id, "notes/show" + assert_select "div.note-comments ul li", :count => 1 + + session_for(create(:moderator_user)) + + browse_check :note_path, note_with_hidden_user_comment.id, "notes/show" + assert_select "div.note-comments ul li", :count => 1 + end + + def test_read_closed_note + user = create(:user) + closed_note = create(:note_with_comments, :status => "closed", :closed_at => Time.now.utc, :comments_count => 2) do |note| + create(:note_comment, :event => "closed", :note => note, :author => user) + end + + browse_check :note_path, closed_note.id, "notes/show" + assert_select "div.note-comments ul li", :count => 2 + assert_select "div.details", /Resolved by #{user.display_name}/ + + user.soft_destroy! + + reset! + + browse_check :note_path, closed_note.id, "notes/show" + assert_select "div.note-comments ul li", :count => 1 + assert_select "div.details", /Resolved by deleted/ + end + def test_new_note get new_note_path assert_response :success assert_template "notes/new" end + + private + + # This is a convenience method for most of the above checks + # First we check that when we don't have an id, it will correctly return a 404 + # then we check that we get the correct 404 when a non-existant id is passed + # then we check that it will get a successful response, when we do pass an id + def browse_check(path, id, template) + path_method = method(path) + + assert_raise ActionController::UrlGenerationError do + get path_method.call + end + + # assert_raise ActionController::UrlGenerationError do + # get path_method.call(:id => -10) # we won't have an id that's negative + # end + + get path_method.call(:id => 0) + assert_response :not_found + assert_template "browse/not_found" + assert_template :layout => "map" + + get path_method.call(:id => 0), :xhr => true + assert_response :not_found + assert_template "browse/not_found" + assert_template :layout => "xhr" + + get path_method.call(:id => id) + assert_response :success + assert_template template + assert_template :layout => "map" + + get path_method.call(:id => id), :xhr => true + assert_response :success + assert_template template + assert_template :layout => "xhr" + end end diff --git a/test/controllers/site_controller_test.rb b/test/controllers/site_controller_test.rb index ccd8ba254..8a38da5c0 100644 --- a/test/controllers/site_controller_test.rb +++ b/test/controllers/site_controller_test.rb @@ -98,7 +98,7 @@ class SiteControllerTest < ActionDispatch::IntegrationTest assert_redirected_to :controller => :browse, :action => :relation, :id => 123 get root_path(:note => 123) - assert_redirected_to :controller => :browse, :action => :note, :id => 123 + assert_redirected_to :controller => :notes, :action => :show, :id => 123 get root_path(:query => "test") assert_redirected_to :controller => :geocoder, :action => :search, :query => "test" diff --git a/test/system/index_test.rb b/test/system/index_test.rb index 1167797c5..1de18c9ed 100644 --- a/test/system/index_test.rb +++ b/test/system/index_test.rb @@ -12,7 +12,7 @@ class IndexTest < ApplicationSystemTestCase test "note included in edit link" do note = create(:note_with_comments) - visit browse_note_path(note) + visit note_path(note) assert_selector "#editanchor[href*='?note=#{note.id}#']" find("#sidebar .btn-close").click @@ -27,8 +27,8 @@ class IndexTest < ApplicationSystemTestCase visible_note = create(:note, :latitude => position, :longitude => position) create(:note_comment, :note => visible_note, :body => "this-is-a-visible-note") - visit root_path(:anchor => "map=15/1/1") # view place of hidden note in case it is not rendered during browse_note_path(hidden_note) - visit browse_note_path(hidden_note) + visit root_path(:anchor => "map=15/1/1") # view place of hidden note in case it is not rendered during note_path(hidden_note) + visit note_path(hidden_note) find(".leaflet-control.control-layers .control-button").click find("#map-ui .overlay-layers .form-check-label", :text => "Map Notes").click visible_note_marker = find(".leaflet-marker-icon[title=this-is-a-visible-note]") diff --git a/test/system/report_note_test.rb b/test/system/report_note_test.rb index 2f407ca60..79894eb89 100644 --- a/test/system/report_note_test.rb +++ b/test/system/report_note_test.rb @@ -3,18 +3,18 @@ require "application_system_test_case" class ReportNoteTest < ApplicationSystemTestCase def test_no_link_when_not_logged_in note = create(:note_with_comments) - visit browse_note_path(note) + visit note_path(note) assert_content note.comments.first.body - assert_no_content I18n.t("browse.note.report") + assert_no_content I18n.t("notes.show.report") end def test_can_report_anonymous_notes note = create(:note_with_comments) sign_in_as(create(:user)) - visit browse_note_path(note) + visit note_path(note) - click_on I18n.t("browse.note.report") + click_on I18n.t("notes.show.report") assert_content "Report" assert_content I18n.t("reports.new.disclaimer.intro") @@ -33,9 +33,9 @@ class ReportNoteTest < ApplicationSystemTestCase def test_can_report_notes_with_author note = create(:note_comment, :author => create(:user)).note sign_in_as(create(:user)) - visit browse_note_path(note) + visit note_path(note) - click_on I18n.t("browse.note.report") + click_on I18n.t("notes.show.report") assert_content "Report" assert_content I18n.t("reports.new.disclaimer.intro") diff --git a/test/system/report_user_test.rb b/test/system/report_user_test.rb index 879558ab5..7a9e800c8 100644 --- a/test/system/report_user_test.rb +++ b/test/system/report_user_test.rb @@ -3,7 +3,7 @@ require "application_system_test_case" class ReportUserTest < ApplicationSystemTestCase def test_no_link_when_not_logged_in note = create(:note_with_comments) - visit browse_note_path(note) + visit note_path(note) assert_content note.comments.first.body assert_no_content I18n.t("users.show.report") -- 2.39.5