From 5da01b92c563e412fe67962a5cea90bc54df5507 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 16 May 2018 12:59:16 +0800 Subject: [PATCH 1/2] Fix test which was accidentally testing as the entry user twice --- test/controllers/diary_entry_controller_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/controllers/diary_entry_controller_test.rb b/test/controllers/diary_entry_controller_test.rb index f95c57d7cc..ee74c42633 100644 --- a/test/controllers/diary_entry_controller_test.rb +++ b/test/controllers/diary_entry_controller_test.rb @@ -335,7 +335,7 @@ def test_edit # and when not logged in as the user who wrote the entry get :view, :params => { :display_name => entry.user.display_name, :id => entry.id }, - :session => { :user => entry.user } + :session => { :user => create(:user) } assert_response :success assert_template "diary_entry/view" assert_select "title", :text => /Users' diaries | /, :count => 1 From e1c62f1bf21fa80d5873c0a75583450ae88b16d7 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 16 May 2018 13:05:20 +0800 Subject: [PATCH 2/2] Remove if_user and similar methods Rather than hiding features based on CSS, just avoid including them in the output. Fixes #1862 --- app/assets/stylesheets/common.scss | 4 - app/helpers/application_helper.rb | 38 ------- app/views/browse/changeset.html.erb | 50 +++++---- app/views/diary_entry/_diary_comment.html.erb | 6 +- app/views/diary_entry/_diary_entry.html.erb | 8 +- app/views/diary_entry/list.html.erb | 12 ++- app/views/diary_entry/view.html.erb | 34 +++--- app/views/layouts/_head.html.erb | 1 - app/views/traces/list.html.erb | 4 +- app/views/traces/view.html.erb | 6 +- .../diary_entry_controller_test.rb | 4 +- test/helpers/application_helper_test.rb | 102 ------------------ .../user_changeset_comments_test.rb | 4 +- 13 files changed, 72 insertions(+), 201 deletions(-) diff --git a/app/assets/stylesheets/common.scss b/app/assets/stylesheets/common.scss index 0a5d9164e2..313d6c94b1 100644 --- a/app/assets/stylesheets/common.scss +++ b/app/assets/stylesheets/common.scss @@ -1668,10 +1668,6 @@ tr.turn:hover { } } -.content-heading .hide_unless_logged_in { // hacky selector, better to just add a new class to this div - display: inline; -} - .pagination { padding-top: $lineheight; } diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index adcf5c6c01..c78c3ea464 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -17,44 +17,6 @@ def atom_link_to(*args) link_to(image_tag("RSS.png", :size => "16x16", :border => 0), Hash[*args], :class => "rsssmall") end - def style_rules - css = "" - - css << ".hidden { display: none !important }" - css << ".hide_unless_logged_in { display: none !important }" unless current_user - css << ".hide_if_logged_in { display: none !important }" if current_user - css << ".hide_if_user_#{current_user.id} { display: none !important }" if current_user - css << ".show_if_user_#{current_user.id} { display: inline !important }" if current_user - css << ".hide_unless_administrator { display: none !important }" unless current_user && current_user.administrator? - css << ".hide_unless_moderator { display: none !important }" unless current_user && current_user.moderator? - - content_tag(:style, css, :type => "text/css") - end - - def if_logged_in(tag = :div, &block) - content_tag(tag, capture(&block), :class => "hide_unless_logged_in") - end - - def if_not_logged_in(tag = :div, &block) - content_tag(tag, capture(&block), :class => "hide_if_logged_in") - end - - def if_user(user, tag = :div, &block) - content_tag(tag, capture(&block), :class => "hidden show_if_user_#{user.id}") if user - end - - def unless_user(user, tag = :div, &block) - if user - content_tag(tag, capture(&block), :class => "hide_if_user_#{user.id}") - else - content_tag(tag, capture(&block)) - end - end - - def if_administrator(tag = :div, &block) - content_tag(tag, capture(&block), :class => "hide_unless_administrator") - end - def richtext_area(object_name, method, options = {}) id = "#{object_name}_#{method}" type = options.delete(:format) || "markdown" diff --git a/app/views/browse/changeset.html.erb b/app/views/browse/changeset.html.erb index ed1e596ee1..85b9515dfe 100644 --- a/app/views/browse/changeset.html.erb +++ b/app/views/browse/changeset.html.erb @@ -13,15 +13,17 @@

<%= t('.discussion') %>

- + <% if current_user %> + + <% end %>
@@ -59,21 +61,25 @@ <% end %> -
- <%= link_to(t(".join_discussion"), :controller => 'user', :action => 'login', :referer => request.fullpath) %> -
+ <% unless current_user %> +
+ <%= link_to(t(".join_discussion"), :controller => 'user', :action => 'login', :referer => request.fullpath) %> +
+ <% end %> - <% unless @changeset.is_open? %> -
- -
- + <% if current_user %> + <% unless @changeset.is_open? %> + + +
+ +
+ + <% else %> +
+ <%= t('.still_open') %>
- - <% else %> -
- <%= t('.still_open') %> -
+ <% end %> <% end %> <% unless @ways.empty? %> diff --git a/app/views/diary_entry/_diary_comment.html.erb b/app/views/diary_entry/_diary_comment.html.erb index 11998ad82d..8565ecc67e 100644 --- a/app/views/diary_entry/_diary_comment.html.erb +++ b/app/views/diary_entry/_diary_comment.html.erb @@ -2,7 +2,9 @@ <%= user_thumbnail diary_comment.user %>

<%= raw(t('.comment_from', :link_user => (link_to h(diary_comment.user.display_name), user_path(diary_comment.user)), :comment_created_at => link_to(l(diary_comment.created_at, :format => :friendly), :anchor => "comment#{diary_comment.id}"))) %>

<%= diary_comment.body.to_html %>
- <%= if_administrator(:span) do %> - <%= link_to t('.hide_link'), hide_diary_comment_path(:display_name => diary_comment.diary_entry.user.display_name, :id => diary_comment.diary_entry.id, :comment => diary_comment.id), :method => :post, :data=> { :confirm => t('.confirm') } %> + <% if current_user && current_user.administrator? %> + + <%= link_to t('.hide_link'), hide_diary_comment_path(:display_name => diary_comment.diary_entry.user.display_name, :id => diary_comment.diary_entry.id, :comment => diary_comment.id), :method => :post, :data=> { :confirm => t('.confirm') } %> + <% end %>
diff --git a/app/views/diary_entry/_diary_entry.html.erb b/app/views/diary_entry/_diary_entry.html.erb index 802dd31f33..848221a280 100644 --- a/app/views/diary_entry/_diary_entry.html.erb +++ b/app/views/diary_entry/_diary_entry.html.erb @@ -27,12 +27,12 @@
  • <%= link_to t('.comment_count', :count => diary_entry.visible_comments.count), :action => 'view', :display_name => diary_entry.user.display_name, :id => diary_entry.id, :anchor => 'comments' %>
    • <% end %> - <%= if_user(diary_entry.user, :li) do %> - <%= link_to t('.edit_link'), :action => 'edit', :display_name => diary_entry.user.display_name, :id => diary_entry.id %> + <% if current_user && current_user == diary_entry.user %> +
  • <%= link_to t('.edit_link'), :action => 'edit', :display_name => diary_entry.user.display_name, :id => diary_entry.id %>
    • <% end %> - <%= if_administrator(:li) do %> - <%= link_to t('.hide_link'), hide_diary_entry_path(:display_name => diary_entry.user.display_name, :id => diary_entry.id), :method => :post, :data => { :confirm => t('.confirm') } %> + <% if current_user && current_user.administrator? %> +
  • <%= link_to t('.hide_link'), hide_diary_entry_path(:display_name => diary_entry.user.display_name, :id => diary_entry.id), :method => :post, :data => { :confirm => t('.confirm') } %>
    • <% end %> diff --git a/app/views/diary_entry/list.html.erb b/app/views/diary_entry/list.html.erb index 8dda82b198..2d9a4b1671 100644 --- a/app/views/diary_entry/list.html.erb +++ b/app/views/diary_entry/list.html.erb @@ -11,12 +11,16 @@ <% end -%> <% if @user %> - <%= if_user(@user) do %> -
  • <%= link_to image_tag("new.png", :class => "small_icon", :border=>0) + t('.new'), {:controller => 'diary_entry', :action => 'new'}, {:title => t('.new_title')} %>
    • + <% if @user == current_user %> +
    +
  • <%= link_to image_tag("new.png", :class => "small_icon", :border=>0) + t('.new'), {:controller => 'diary_entry', :action => 'new'}, {:title => t('.new_title')} %>
    • +
    <% end %> <% else %> - <%= if_logged_in do %> -
  • <%= link_to image_tag("new.png", :class => "small_icon", :border=>0) + t('.new'), {:controller => 'diary_entry', :action => 'new'}, {:title => t('.new_title')} %>
    • + <% if current_user %> +
    +
  • <%= link_to image_tag("new.png", :class => "small_icon", :border=>0) + t('.new'), {:controller => 'diary_entry', :action => 'new'}, {:title => t('.new_title')} %>
    • +
    <% end %> <% end %> diff --git a/app/views/diary_entry/view.html.erb b/app/views/diary_entry/view.html.erb index 3e8fbe990f..9871da8ccf 100644 --- a/app/views/diary_entry/view.html.erb +++ b/app/views/diary_entry/view.html.erb @@ -12,25 +12,27 @@
    <%= render :partial => 'diary_comment', :collection => @entry.visible_comments %>
    -<%= if_logged_in(:div) do %> -

    <%= t '.leave_a_comment' %>

    - <%= error_messages_for 'diary_comment' %> - <%= form_for :diary_comment, :url => { :action => 'comment' } do |f| %> - <%= richtext_area :diary_comment, :body, :cols => 80, :rows => 15 %> - <%= submit_tag t('.save_button') %> - <% end %> - <% if current_user and @entry.subscribers.exists?(current_user.id) %> -
    <%= link_to t('javascripts.changesets.show.unsubscribe'), diary_entry_unsubscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    - <% elsif current_user %> -
    <%= link_to t('javascripts.changesets.show.subscribe'), diary_entry_subscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    - <% end %> -<% end %> +
    + <% if current_user %> +

    <%= t '.leave_a_comment' %>

    -<%= if_not_logged_in(:div) do %> -

    <%= raw t(".login_to_leave_a_comment", :login_link => link_to(t(".login"), :controller => 'user', :action => 'login', :referer => request.fullpath)) %>

    -<% end %> + <%= error_messages_for 'diary_comment' %> + + <%= form_for :diary_comment, :url => { :action => 'comment' } do |f| %> + <%= richtext_area :diary_comment, :body, :cols => 80, :rows => 15 %> + <%= submit_tag t('.save_button') %> + <% end %> + <% if @entry.subscribers.exists?(current_user.id) %> +
    <%= link_to t('javascripts.changesets.show.unsubscribe'), diary_entry_unsubscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    + <% else %> +
    <%= link_to t('javascripts.changesets.show.subscribe'), diary_entry_subscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    + <% end %> + <% else %> +

    <%= raw t(".login_to_leave_a_comment", :login_link => link_to(t(".login"), :controller => 'user', :action => 'login', :referer => request.fullpath)) %>

    + <% end %> +
    <% content_for :auto_discovery_link_tag do -%> <%= auto_discovery_link_tag :rss, :action => :rss, :display_name => @entry.user.display_name %> diff --git a/app/views/layouts/_head.html.erb b/app/views/layouts/_head.html.erb index bf9f78b437..fbc9037faa 100644 --- a/app/views/layouts/_head.html.erb +++ b/app/views/layouts/_head.html.erb @@ -33,7 +33,6 @@ <% if flash[:piwik_goal] -%> <%= tag("meta", :name => "piwik-goal", :content => flash[:piwik_goal]) %> <% end -%> - <%= style_rules %> <%= yield :head %> <%= yield :auto_discovery_link_tag %> <%= csrf_meta_tag %> diff --git a/app/views/traces/list.html.erb b/app/views/traces/list.html.erb index 4c433ce665..d63d5c52bd 100644 --- a/app/views/traces/list.html.erb +++ b/app/views/traces/list.html.erb @@ -11,8 +11,8 @@ <% if @display_name %>
  • <%= link_to t('.see_all_traces'), :controller => 'traces', :action => 'list', :display_name => nil, :tag => nil, :page => nil %>
    • <% end %> - <%= unless_user(@target_user, :li) do %> - <%= link_to t('.see_my_traces'), :action => 'mine', :tag => nil, :page => nil %> + <% if current_user && current_user != @target_user %> +
  • <%= link_to t('.see_my_traces'), :action => 'mine', :tag => nil, :page => nil %>
    • <% end %> <% end %> diff --git a/app/views/traces/view.html.erb b/app/views/traces/view.html.erb index 9566e6be8f..648160c2f6 100644 --- a/app/views/traces/view.html.erb +++ b/app/views/traces/view.html.erb @@ -56,8 +56,10 @@ <% if current_user && (current_user==@trace.user || current_user.administrator? || current_user.moderator?)%>
    - <%= if_user(@trace.user) do %> - <%= button_to t('.edit_track'), trace_edit_path(@trace) %> + <% if current_user == @trace.user %> +
    + <%= button_to t('.edit_track'), trace_edit_path(@trace) %> +
    <% end %> <%= button_to t('.delete_track'), { :controller => 'traces', :action => 'delete', :id => @trace.id }, :data => { :confirm => t('.confirm_delete') } %>
    diff --git a/test/controllers/diary_entry_controller_test.rb b/test/controllers/diary_entry_controller_test.rb index ee74c42633..141a86be33 100644 --- a/test/controllers/diary_entry_controller_test.rb +++ b/test/controllers/diary_entry_controller_test.rb @@ -349,9 +349,7 @@ def test_edit assert_select "p", :text => /#{new_body}/, :count => 1 assert_select "abbr[class=geo][title='#{number_with_precision(new_latitude, :precision => 4)}; #{number_with_precision(new_longitude, :precision => 4)}']", :count => 1 # As we're not logged in, check that you cannot edit - assert_select "li[class='hidden show_if_user_#{entry.user.id}']", :count => 1 do - assert_select "a[href='/user/#{ERB::Util.u(entry.user.display_name)}/diary/#{entry.id}/edit']", :text => "Edit this entry", :count => 1 - end + assert_select "a[href='/user/#{ERB::Util.u(entry.user.display_name)}/diary/#{entry.id}/edit']", false end end diff --git a/test/helpers/application_helper_test.rb b/test/helpers/application_helper_test.rb index d1c41d62c1..80613a81a3 100644 --- a/test/helpers/application_helper_test.rb +++ b/test/helpers/application_helper_test.rb @@ -47,108 +47,6 @@ def test_atom_link_to assert_dom_equal "\"Rss\"", link end - def test_style_rules - self.current_user = nil - - css = style_rules - assert_match /\.hidden /, css - assert_match /\.hide_unless_logged_in /, css - assert_no_match /\.hide_if_logged_in /, css - assert_no_match /\.hide_if_user_/, css - assert_no_match /\.show_if_user_/, css - assert_match /\.hide_unless_administrator /, css - assert_match /\.hide_unless_moderator /, css - - self.current_user = create(:user) - - css = style_rules - assert_match /\.hidden /, css - assert_no_match /\.hide_unless_logged_in /, css - assert_match /\.hide_if_logged_in /, css - assert_match /\.hide_if_user_#{current_user.id} /, css - assert_match /\.show_if_user_#{current_user.id} /, css - assert_match /\.hide_unless_administrator /, css - assert_match /\.hide_unless_moderator /, css - - self.current_user = create(:moderator_user) - - css = style_rules - assert_match /\.hidden /, css - assert_no_match /\.hide_unless_logged_in /, css - assert_match /\.hide_if_logged_in /, css - assert_match /\.hide_if_user_#{current_user.id} /, css - assert_match /\.show_if_user_#{current_user.id} /, css - assert_match /\.hide_unless_administrator /, css - assert_no_match /\.hide_unless_moderator /, css - - self.current_user = create(:administrator_user) - - css = style_rules - assert_match /\.hidden /, css - assert_no_match /\.hide_unless_logged_in /, css - assert_match /\.hide_if_logged_in /, css - assert_match /\.hide_if_user_#{current_user.id} /, css - assert_match /\.show_if_user_#{current_user.id} /, css - assert_no_match /\.hide_unless_administrator /, css - assert_match /\.hide_unless_moderator /, css - end - - def test_if_logged_in - html = if_logged_in { "Test 1" } - assert_dom_equal "
    Test 1
    ", html - - html = if_logged_in(:span) { "Test 2" } - assert_dom_equal "Test 2", html - end - - def test_if_not_logged_in - html = if_not_logged_in { "Test 1" } - assert_dom_equal "
    Test 1
    ", html - - html = if_not_logged_in(:span) { "Test 2" } - assert_dom_equal "Test 2", html - end - - def test_if_user - user = create(:user) - - html = if_user(user) { "Test 1" } - assert_dom_equal "
    Test 1
    ", html - - html = if_user(user, :span) { "Test 2" } - assert_dom_equal "Test 2", html - - html = if_user(nil) { "Test 3" } - assert_nil html - - html = if_user(nil, :span) { "Test 4" } - assert_nil html - end - - def test_unless_user - user = create(:user) - - html = unless_user(user) { "Test 1" } - assert_dom_equal "
    Test 1
    ", html - - html = unless_user(user, :span) { "Test 2" } - assert_dom_equal "Test 2", html - - html = unless_user(nil) { "Test 3" } - assert_dom_equal "
    Test 3
    ", html - - html = unless_user(nil, :span) { "Test 4" } - assert_dom_equal "Test 4", html - end - - def test_if_administrator - html = if_administrator { "Test 1" } - assert_dom_equal "
    Test 1
    ", html - - html = if_administrator(:span) { "Test 2" } - assert_dom_equal "Test 2", html - end - def test_richtext_area html = richtext_area(:message, :body, :cols => 40, :rows => 20) assert_not_nil html diff --git a/test/integration/user_changeset_comments_test.rb b/test/integration/user_changeset_comments_test.rb index 6030304697..5f655c4811 100644 --- a/test/integration/user_changeset_comments_test.rb +++ b/test/integration/user_changeset_comments_test.rb @@ -12,7 +12,9 @@ def test_log_in_message assert_select "div#sidebar" do assert_select "div#sidebar_content" do assert_select "div.browse-section" do - assert_select "div.notice.hide_if_logged_in" + assert_select "div.notice" do + assert_select "a[href='/login?referer=%2Fchangeset%2F#{changeset.id}']", :text => I18n.t("browse.changeset.join_discussion"), :count => 1 + end end end end