From: Andy Allan Date: Wed, 3 Oct 2018 12:04:12 +0000 (+0200) Subject: Merge branch 'master' into messages X-Git-Tag: live~3888^2 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/5e407dfb34f47e6fbbbf3c11c1a8318256abb5cd?ds=inline;hp=-c Merge branch 'master' into messages --- 5e407dfb34f47e6fbbbf3c11c1a8318256abb5cd diff --combined .rubocop_todo.yml index 78ed84ac3,293135907..d3e51600d --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@@ -1,23 -1,11 +1,11 @@@ # This configuration was generated by # `rubocop --auto-gen-config` - # on 2018-06-19 09:02:55 +0100 using RuboCop version 0.57.2. + # on 2018-09-19 14:24:02 +0100 using RuboCop version 0.58.2. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new # versions of RuboCop, may require this file to be generated again. - # Offense count: 34 - Lint/AmbiguousOperator: - Exclude: - - 'test/controllers/amf_controller_test.rb' - - 'test/controllers/changeset_controller_test.rb' - - 'test/lib/bounding_box_test.rb' - - 'test/lib/country_test.rb' - - # Offense count: 96 - Lint/AmbiguousRegexpLiteral: - Enabled: false - # Offense count: 32 # Configuration parameters: AllowSafeAssignment. Lint/AssignmentInCondition: @@@ -42,19 -30,15 +30,15 @@@ Lint/HandleExceptions - 'app/controllers/amf_controller.rb' - 'app/controllers/user_controller.rb' - # Offense count: 2 - Lint/ShadowingOuterLocalVariable: - Exclude: - - 'app/views/changeset/list.atom.builder' - - # Offense count: 690 + # Offense count: 692 Metrics/AbcSize: Max: 280 - # Offense count: 41 + # Offense count: 40 # Configuration parameters: CountComments, ExcludedMethods. + # ExcludedMethods: refine Metrics/BlockLength: - Max: 259 + Max: 262 # Offense count: 11 # Configuration parameters: CountBlocks. @@@ -70,7 -54,7 +54,7 @@@ Metrics/ClassLength Metrics/CyclomaticComplexity: Max: 20 - # Offense count: 688 + # Offense count: 691 # Configuration parameters: CountComments. Metrics/MethodLength: Max: 179 @@@ -85,7 -69,7 +69,7 @@@ Metrics/ModuleLength Metrics/ParameterLists: Max: 9 - # Offense count: 71 + # Offense count: 72 Metrics/PerceivedComplexity: Max: 23 @@@ -178,10 -162,17 +162,17 @@@ Style/AsciiComments Exclude: - 'test/models/message_test.rb' - # Offense count: 229 + # Offense count: 230 Style/Documentation: Enabled: false + # Offense count: 462 + # Cop supports --auto-correct. + # Configuration parameters: EnforcedStyle. + # SupportedStyles: when_needed, always, never + Style/FrozenStringLiteralComment: + Enabled: false + # Offense count: 2 # Cop supports --auto-correct. Style/IfUnlessModifier: @@@ -194,7 -185,7 +185,7 @@@ Style/NumericLiterals: MinDigits: 11 - # Offense count: 3064 + # Offense count: 3080 # Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns. # URISchemes: http, https Metrics/LineLength: diff --combined app/models/notifier.rb index 11658cfd9,3cc1be1e6..01b807607 --- a/app/models/notifier.rb +++ b/app/models/notifier.rb @@@ -70,7 -70,7 +70,7 @@@ class Notifier < ActionMailer::Bas @text = message.body @title = message.title @readurl = message_url(message) - @replyurl = reply_message_url(message) + @replyurl = message_reply_url(message) @author = @from_user attach_user_avatar(message.sender) @@@ -87,16 -87,8 +87,8 @@@ @from_user = comment.user.display_name @text = comment.body @title = comment.diary_entry.title - @readurl = url_for(:controller => "diary_entry", - :action => "view", - :display_name => comment.diary_entry.user.display_name, - :id => comment.diary_entry.id, - :anchor => "comment#{comment.id}") - @commenturl = url_for(:controller => "diary_entry", - :action => "view", - :display_name => comment.diary_entry.user.display_name, - :id => comment.diary_entry.id, - :anchor => "newcomment") + @readurl = diary_entry_url(comment.diary_entry.user, comment.diary_entry, :anchor => "comment#{comment.id}") + @commenturl = diary_entry_url(comment.diary_entry.user, comment.diary_entry, :anchor => "newcomment") @replyurl = new_message_url(comment.user, :message => { :title => "Re: #{comment.diary_entry.title}" }) @author = @from_user @@@ -112,8 -104,7 +104,7 @@@ def friend_notification(friend) with_recipient_locale friend.befriendee do @friend = friend - @viewurl = url_for(:controller => "user", :action => "view", - :display_name => @friend.befriender.display_name) + @viewurl = user_url(@friend.befriender) @friendurl = url_for(:controller => "user", :action => "make_friend", :display_name => @friend.befriender.display_name) @author = @friend.befriender.display_name @@@ -190,8 -181,8 +181,8 @@@ end def user_avatar_file_path(user) - image = user && user.image - if image && image.file? + image = user&.image + if image&.file? return image.path(:small) else return Rails.root.join("app", "assets", "images", "users", "images", "small.png") diff --combined config/routes.rb index 1a5fb8272,e60994863..b2a0f84c3 --- a/config/routes.rb +++ b/config/routes.rb @@@ -119,11 -119,11 +119,11 @@@ OpenStreetMap::Application.routes.draw get "/changeset/:id/comments/feed" => "changeset#comments_feed", :as => :changeset_comments_feed, :id => /\d*/, :defaults => { :format => "rss" } get "/note/:id" => "browse#note", :id => /\d+/, :as => "browse_note" get "/note/new" => "browse#new_note" - get "/user/:display_name/history" => "changeset#list" + get "/user/:display_name/history" => "changeset#index" get "/user/:display_name/history/feed" => "changeset#feed", :defaults => { :format => :atom } get "/user/:display_name/notes" => "notes#mine" - get "/history/friends" => "changeset#list", :friends => true, :as => "friend_changesets", :defaults => { :format => :html } - get "/history/nearby" => "changeset#list", :nearby => true, :as => "nearby_changesets", :defaults => { :format => :html } + get "/history/friends" => "changeset#index", :friends => true, :as => "friend_changesets", :defaults => { :format => :html } + get "/history/nearby" => "changeset#index", :nearby => true, :as => "nearby_changesets", :defaults => { :format => :html } get "/browse/way/:id", :to => redirect(:path => "/way/%{id}") get "/browse/way/:id/history", :to => redirect(:path => "/way/%{id}/history") @@@ -150,7 -150,7 +150,7 @@@ get "/fixthemap" => "site#fixthemap" get "/help" => "site#help" get "/about" => "site#about" - get "/history" => "changeset#list" + get "/history" => "changeset#index" get "/history/feed" => "changeset#feed", :defaults => { :format => :atom } get "/history/comments/feed" => "changeset#comments_feed", :as => :changesets_comments_feed, :defaults => { :format => "rss" } get "/export" => "site#export" @@@ -208,26 -208,24 +208,24 @@@ get "/traces/mine/tag/:tag" => "traces#mine" get "/traces/mine/page/:page" => "traces#mine" get "/traces/mine" => "traces#mine" - post "/trace/create" => "traces#create" # remove after deployment get "/trace/create", :to => redirect(:path => "/traces/new") get "/trace/:id/data" => "traces#data", :id => /\d+/, :as => "trace_data" - post "trace/:id/edit" => "traces#update" # remove after deployment get "/trace/:id/edit", :to => redirect(:path => "/traces/%{id}/edit") post "/trace/:id/delete" => "traces#delete", :id => /\d+/ # diary pages match "/diary/new" => "diary_entry#new", :via => [:get, :post] - get "/diary/friends" => "diary_entry#list", :friends => true, :as => "friend_diaries" - get "/diary/nearby" => "diary_entry#list", :nearby => true, :as => "nearby_diaries" + get "/diary/friends" => "diary_entry#index", :friends => true, :as => "friend_diaries" + get "/diary/nearby" => "diary_entry#index", :nearby => true, :as => "nearby_diaries" get "/user/:display_name/diary/rss" => "diary_entry#rss", :defaults => { :format => :rss } get "/diary/:language/rss" => "diary_entry#rss", :defaults => { :format => :rss } get "/diary/rss" => "diary_entry#rss", :defaults => { :format => :rss } get "/user/:display_name/diary/comments/:page" => "diary_entry#comments", :page => /[1-9][0-9]*/ get "/user/:display_name/diary/comments/" => "diary_entry#comments" - get "/user/:display_name/diary" => "diary_entry#list" - get "/diary/:language" => "diary_entry#list" - get "/diary" => "diary_entry#list" - get "/user/:display_name/diary/:id" => "diary_entry#view", :id => /\d+/, :as => :diary_entry + get "/user/:display_name/diary" => "diary_entry#index" + get "/diary/:language" => "diary_entry#index" + get "/diary" => "diary_entry#index" + get "/user/:display_name/diary/:id" => "diary_entry#show", :id => /\d+/, :as => :diary_entry post "/user/:display_name/diary/:id/newcomment" => "diary_entry#comment", :id => /\d+/ match "/user/:display_name/diary/:id/edit" => "diary_entry#edit", :via => [:get, :post], :id => /\d+/ post "/user/:display_name/diary/:id/hide" => "diary_entry#hide", :id => /\d+/, :as => :hide_diary_entry @@@ -236,7 -234,7 +234,7 @@@ post "/user/:display_name/diary/:id/unsubscribe" => "diary_entry#unsubscribe", :as => :diary_entry_unsubscribe, :id => /\d+/ # user pages - get "/user/:display_name" => "user#view", :as => "user" + get "/user/:display_name" => "user#show", :as => "user" match "/user/:display_name/make_friend" => "user#make_friend", :via => [:get, :post], :as => "make_friend" match "/user/:display_name/remove_friend" => "user#remove_friend", :via => [:get, :post], :as => "remove_friend" match "/user/:display_name/account" => "user#account", :via => [:get, :post] @@@ -244,8 -242,8 +242,8 @@@ get "/user/:display_name/delete" => "user#delete", :as => :delete_user # user lists - match "/users" => "user#list", :via => [:get, :post] - match "/users/:status" => "user#list", :via => [:get, :post] + match "/users" => "user#index", :via => [:get, :post] + match "/users/:status" => "user#index", :via => [:get, :post] # geocoder get "/search" => "geocoder#search" @@@ -264,9 -262,7 +262,9 @@@ get "/export/embed" => "export#embed" # messages - resources :messages, :only => [:create, :show] do + resources :messages, :only => [:create, :show, :destroy] do + post :mark + match :reply, :via => [:get, :post] collection do get :inbox get :outbox @@@ -276,8 -272,9 +274,8 @@@ get "/user/:display_name/outbox", :to => redirect(:path => "/messages/outbox") get "/message/new/:display_name" => "messages#new", :as => "new_message" get "/message/read/:message_id", :to => redirect(:path => "/messages/%{message_id}") - post "/message/mark/:message_id" => "messages#mark", :as => "mark_message" - match "/message/reply/:message_id" => "messages#reply", :via => [:get, :post], :as => "reply_message" - post "/message/delete/:message_id" => "messages#destroy", :as => "destroy_message" + post "/message/mark/:message_id" => "messages#mark" # remove after deployment + match "/message/reply/:message_id" => "messages#reply", :via => [:get, :post] # remove after deployment # oauth admin pages (i.e: for setting up new clients, etc...) scope "/user/:display_name" do diff --combined test/controllers/messages_controller_test.rb index 6d4e808f6,940144a17..8536a1b01 --- a/test/controllers/messages_controller_test.rb +++ b/test/controllers/messages_controller_test.rb @@@ -25,20 -25,20 +25,20 @@@ class MessagesControllerTest < ActionCo { :controller => "messages", :action => "show", :id => "1" } ) assert_routing( - { :path => "/message/mark/1", :method => :post }, + { :path => "/messages/1/mark", :method => :post }, { :controller => "messages", :action => "mark", :message_id => "1" } ) assert_routing( - { :path => "/message/reply/1", :method => :get }, + { :path => "/messages/1/reply", :method => :get }, { :controller => "messages", :action => "reply", :message_id => "1" } ) assert_routing( - { :path => "/message/reply/1", :method => :post }, + { :path => "/messages/1/reply", :method => :post }, { :controller => "messages", :action => "reply", :message_id => "1" } ) assert_routing( - { :path => "/message/delete/1", :method => :post }, - { :controller => "messages", :action => "destroy", :message_id => "1" } + { :path => "/messages/1", :method => :delete }, + { :controller => "messages", :action => "destroy", :id => "1" } ) end @@@ -180,8 -180,8 +180,8 @@@ e = ActionMailer::Base.deliveries.first assert_equal [recipient_user.email], e.to assert_equal "[OpenStreetMap] Test Message", e.subject - assert_match /Test message body/, e.text_part.decoded - assert_match /Test message body/, e.html_part.decoded + assert_match(/Test message body/, e.text_part.decoded) + assert_match(/Test message body/, e.html_part.decoded) assert_match %r{#{SERVER_URL}/messages/[0-9]+}, e.text_part.decoded ActionMailer::Base.deliveries.clear m = Message.last @@@ -232,14 -232,14 +232,14 @@@ # Check that the message reply page requires us to login get :reply, :params => { :message_id => unread_message.id } - assert_redirected_to login_path(:referer => reply_message_path(:message_id => unread_message.id)) + assert_redirected_to login_path(:referer => message_reply_path(:message_id => unread_message.id)) # Login as the wrong user session[:user] = other_user.id # Check that we can't reply to somebody else's message get :reply, :params => { :message_id => unread_message.id } - assert_redirected_to login_path(:referer => reply_message_path(:message_id => unread_message.id)) + assert_redirected_to login_path(:referer => message_reply_path(:message_id => unread_message.id)) assert_equal "You are logged in as `#{other_user.display_name}' but the message you have asked to reply to was not sent to that user. Please login as the correct user in order to reply.", flash[:notice] # Login as the right user @@@ -429,14 -429,14 +429,14 @@@ sent_message = create(:message, :unread, :recipient => second_user, :sender => user) # Check that destroying a message requires us to login - post :destroy, :params => { :message_id => read_message.id } + delete :destroy, :params => { :id => read_message.id } assert_response :forbidden # Login as a user with no messages session[:user] = other_user.id # Check that destroying a message we didn't send or receive fails - post :destroy, :params => { :message_id => read_message.id } + delete :destroy, :params => { :id => read_message.id } assert_response :not_found assert_template "no_such_message" @@@ -444,7 -444,7 +444,7 @@@ session[:user] = user.id # Check that the destroy a received message works - post :destroy, :params => { :message_id => read_message.id } + delete :destroy, :params => { :id => read_message.id } assert_redirected_to inbox_messages_path assert_equal "Message deleted", flash[:notice] m = Message.find(read_message.id) @@@ -452,7 -452,7 +452,7 @@@ assert_equal false, m.to_user_visible # Check that the destroying a sent message works - post :destroy, :params => { :message_id => sent_message.id, :referer => outbox_messages_path } + delete :destroy, :params => { :id => sent_message.id, :referer => outbox_messages_path } assert_redirected_to outbox_messages_path assert_equal "Message deleted", flash[:notice] m = Message.find(sent_message.id) @@@ -465,7 -465,7 +465,7 @@@ end # Asking to destroy a message with a bogus ID should fail - post :destroy, :params => { :message_id => 99999 } + delete :destroy, :params => { :id => 99999 } assert_response :not_found assert_template "no_such_message" end