From eda23c719a5a4900f229d8ad1c4808d93b79f387 Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 19 Nov 2021 22:40:34 -0600 Subject: [PATCH] votes: fixup various minor issues. * Add a gap between thumbnails on mobile. * Adjust CSS for scores and vote buttons. * Include "Private favorites" as an incentive on the user upgrade page. * Fix vote buttons not being visible beneath thumbnails on mobile. * Fix the "Show scores" link not preserving the current page number. * Fix vote buttons being unintentionally enabled for all thumbnails by default. * Fix banned and restricted users being able to favorite posts by tagging them with `fav:self`. * Fix search engines being able to crawl /posts?view=score pages. * Fix broken tests. --- .../favorites_tooltip_component.scss | 5 -- app/components/post_preview_component.rb | 2 +- .../post_preview_component.html.erb | 4 +- .../post_preview_component.scss | 46 +++++-------------- .../post_votes_component.scss | 13 ++++-- .../post_votes_tooltip_component.html.erb | 4 +- .../src/styles/common/utilities.scss | 3 ++ .../src/styles/specific/z_responsive.scss | 14 +++--- app/logical/post_sets/post.rb | 1 + app/models/post.rb | 2 + app/views/posts/index.html.erb | 4 +- .../posts/partials/index/_posts.html.erb | 4 +- .../partials/index/_seo_meta_tags.html.erb | 2 + app/views/user_upgrades/new.html.erb | 6 +++ test/components/post_navbar_component_test.rb | 7 ++- test/components/post_votes_component_test.rb | 14 +++--- test/factories/favorite.rb | 4 ++ test/unit/favorite_test.rb | 13 ++---- test/unit/post_query_builder_test.rb | 9 ++-- test/unit/post_test.rb | 45 ++++++++++++++---- 20 files changed, 108 insertions(+), 94 deletions(-) diff --git a/app/components/favorites_tooltip_component/favorites_tooltip_component.scss b/app/components/favorites_tooltip_component/favorites_tooltip_component.scss index 4876c8cdd..ccdc2b5a1 100644 --- a/app/components/favorites_tooltip_component/favorites_tooltip_component.scss +++ b/app/components/favorites_tooltip_component/favorites_tooltip_component.scss @@ -6,8 +6,3 @@ max-width: 160px; } } - -span.post-favcount a { - color: var(--text-color); - &:hover { text-decoration: underline; } -} diff --git a/app/components/post_preview_component.rb b/app/components/post_preview_component.rb index 44ef74963..8c9e7369a 100644 --- a/app/components/post_preview_component.rb +++ b/app/components/post_preview_component.rb @@ -9,7 +9,7 @@ class PostPreviewComponent < ApplicationComponent delegate :image_width, :image_height, :file_ext, :file_size, :duration, :is_animated?, to: :media_asset delegate :media_asset, to: :post - def initialize(post:, tags: "", show_deleted: false, show_cropped: true, show_votes: true, link_target: post, pool: nil, similarity: nil, recommended: nil, compact: nil, size: nil, current_user: CurrentUser.user, **options) + def initialize(post:, tags: "", show_deleted: false, show_cropped: true, show_votes: false, link_target: post, pool: nil, similarity: nil, recommended: nil, compact: nil, size: nil, current_user: CurrentUser.user, **options) super @post = post @tags = tags.presence diff --git a/app/components/post_preview_component/post_preview_component.html.erb b/app/components/post_preview_component/post_preview_component.html.erb index 2fdf0e55a..49d90e5c7 100644 --- a/app/components/post_preview_component/post_preview_component.html.erb +++ b/app/components/post_preview_component/post_preview_component.html.erb @@ -55,8 +55,8 @@ <% end %>

<% elsif show_votes -%> -

+

<%= render_post_votes post, current_user: current_user %> -

+
<% end -%> <% end -%> diff --git a/app/components/post_preview_component/post_preview_component.scss b/app/components/post_preview_component/post_preview_component.scss index e24646a54..c31586e0f 100644 --- a/app/components/post_preview_component/post_preview_component.scss +++ b/app/components/post_preview_component/post_preview_component.scss @@ -1,12 +1,10 @@ @import "../../javascript/src/styles/base/000_vars.scss"; article.post-preview { - width: 154px; - margin: 0 10px 10px 0; text-align: center; display: inline-block; position: relative; - vertical-align: top; + overflow: hidden; a { display: inline-block; @@ -119,38 +117,16 @@ body[data-current-user-can-approve-posts="true"] .post-preview { } } -@media screen and (max-width: 660px) { +@media screen and (min-width: 660px) { article.post-preview { - margin: 0; - text-align: center; - vertical-align: middle; - display: inline-block; - - a { - margin: 0 auto; - } - - img { - max-width: 33.3vw; - max-height: 33.3vw; - border: none !important; - } - } - - .user-disable-cropped-false { - article.post-preview { - width: 33.3%; - height: 33.3vw; - overflow: hidden; - } - - img { - width: 33.3vw; - height: 33.3vw; - - &.has-cropped-false { - object-fit: cover; - } - } + width: 154px; + margin: 0 10px 10px 0; + vertical-align: top; + } +} + +@media screen and (max-width: 660px) { + article.post-preview img { + border: none !important; } } diff --git a/app/components/post_votes_component/post_votes_component.scss b/app/components/post_votes_component/post_votes_component.scss index 0dcd21510..b5abde985 100644 --- a/app/components/post_votes_component/post_votes_component.scss +++ b/app/components/post_votes_component/post_votes_component.scss @@ -1,3 +1,5 @@ +@import "../../javascript/src/styles/base/000_vars.scss"; + .post-votes { // Fix it so that the vote buttons don't move when the score changes width. // XXX duplicated from app/components/comment_component/comment_component.scss @@ -7,10 +9,11 @@ min-width: 1.25em; white-space: nowrap; vertical-align: middle; - - a { - color: var(--text-color); - &:hover { text-decoration: underline; } - } + } +} + +.posts-container { + .post-score a { + @include inactive-link; } } diff --git a/app/components/post_votes_tooltip_component/post_votes_tooltip_component.html.erb b/app/components/post_votes_tooltip_component/post_votes_tooltip_component.html.erb index 24937ae58..a50e1e3e0 100644 --- a/app/components/post_votes_tooltip_component/post_votes_tooltip_component.html.erb +++ b/app/components/post_votes_tooltip_component/post_votes_tooltip_component.html.erb @@ -1,6 +1,6 @@
-
- +<%= post.up_score %> / <%= post.down_score %> <%= upvote_ratio %> +
+ +<%= post.up_score %> / -<%= post.down_score.abs %> <%= upvote_ratio %>
diff --git a/app/javascript/src/styles/common/utilities.scss b/app/javascript/src/styles/common/utilities.scss index beae54a77..fd09c2cc2 100644 --- a/app/javascript/src/styles/common/utilities.scss +++ b/app/javascript/src/styles/common/utilities.scss @@ -28,6 +28,8 @@ $spacer: 0.25rem; /* 4px */ white-space: nowrap; } +.whitespace-nowrap { white-space: nowrap; } + .leading-none { line-height: 1; } .absolute { position: absolute; } @@ -45,6 +47,7 @@ $spacer: 0.25rem; /* 4px */ .mx-0\.5 { margin-left: 0.5 * $spacer; margin-right: 0.5 * $spacer; } .mx-2 { margin-left: 2 * $spacer; margin-right: 2 * $spacer; } +.mt-1 { margin-top: 1 * $spacer; } .mt-2 { margin-top: 2 * $spacer; } .mt-4 { margin-top: 4 * $spacer; } .mt-8 { margin-top: 8 * $spacer; } diff --git a/app/javascript/src/styles/specific/z_responsive.scss b/app/javascript/src/styles/specific/z_responsive.scss index 80da5b38f..4bb6cc934 100644 --- a/app/javascript/src/styles/specific/z_responsive.scss +++ b/app/javascript/src/styles/specific/z_responsive.scss @@ -41,11 +41,13 @@ } } - #posts #posts-container { - width: 100%; - display: flex; - flex-wrap: wrap; - align-items: center; - justify-content: flex-start; + .posts-container { + display: grid; + grid-template-columns: repeat(3, minmax(0, 1fr)); + gap: 0.25rem; + + &.user-disable-cropped-false article.post-preview img.has-cropped-true { + object-fit: none; + } } } diff --git a/app/logical/post_sets/post.rb b/app/logical/post_sets/post.rb index 5702133b9..34f576e93 100644 --- a/app/logical/post_sets/post.rb +++ b/app/logical/post_sets/post.rb @@ -115,6 +115,7 @@ module PostSets def hide_from_crawler? return true if current_page > 50 + return true if show_votes? return true if artist.present? && artist.is_banned? return false if query.is_empty_search? || query.is_simple_tag? || query.is_metatag?(:order, :rank) true diff --git a/app/models/post.rb b/app/models/post.rb index f0e6a0e7a..4ac77452d 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -548,9 +548,11 @@ class Post < ApplicationRecord pool&.add!(self) when /^fav:(.+)$/i + raise User::PrivilegeError unless Pundit.policy!(CurrentUser.user, Favorite).create? Favorite.create(post: self, user: CurrentUser.user) when /^-fav:(.+)$/i + raise User::PrivilegeError unless Pundit.policy!(CurrentUser.user, Favorite).create? Favorite.destroy_by(post: self, user: CurrentUser.user) when /^(up|down)vote:(.+)$/i diff --git a/app/views/posts/index.html.erb b/app/views/posts/index.html.erb index aa470ba93..c2d3e3701 100644 --- a/app/views/posts/index.html.erb +++ b/app/views/posts/index.html.erb @@ -35,9 +35,9 @@ <%= render PopupMenuComponent.new do |menu| %> <% menu.item do %> <% if params[:view] == "score" %> - <%= link_to "Hide scores", posts_path(tags: params[:tags], view: nil) %> + <%= link_to "Hide scores", posts_path(tags: params[:tags], page: params[:page], limit: params[:limit], view: nil), rel: "nofollow" %> <% else %> - <%= link_to "Show scores", posts_path(tags: params[:tags], view: "score") %> + <%= link_to "Show scores", posts_path(tags: params[:tags], page: params[:page], limit: params[:limit], view: "score"), rel: "nofollow" %> <% end %> <% end %> <% end %> diff --git a/app/views/posts/partials/index/_posts.html.erb b/app/views/posts/partials/index/_posts.html.erb index 129d6f5e8..5bbf4ecbc 100644 --- a/app/views/posts/partials/index/_posts.html.erb +++ b/app/views/posts/partials/index/_posts.html.erb @@ -1,5 +1,5 @@ -
-
+
+
<% if post_set.shown_posts.empty? %> <%= render "post_sets/blank" %> <% else %> diff --git a/app/views/posts/partials/index/_seo_meta_tags.html.erb b/app/views/posts/partials/index/_seo_meta_tags.html.erb index 66755c3e0..382f13e4b 100644 --- a/app/views/posts/partials/index/_seo_meta_tags.html.erb +++ b/app/views/posts/partials/index/_seo_meta_tags.html.erb @@ -12,6 +12,8 @@ <% if params[:tags].blank? && @post_set.current_page == 1 %> <% canonical_url root_url(host: Danbooru.config.hostname) %> +<% else %> + <% canonical_url posts_url(host: Danbooru.config.hostname, tags: params[:tags], page: params[:page], limit: params[:limit]) %> <% end %> <% noindex if @post_set.hide_from_crawler? %> diff --git a/app/views/user_upgrades/new.html.erb b/app/views/user_upgrades/new.html.erb index f72ed032a..7e88565bb 100644 --- a/app/views/user_upgrades/new.html.erb +++ b/app/views/user_upgrades/new.html.erb @@ -70,6 +70,12 @@ 2,000 5,000 + + Private Favorites + no + yes + yes + Favorite Groups 3 diff --git a/test/components/post_navbar_component_test.rb b/test/components/post_navbar_component_test.rb index a27024ac0..60a37c0ec 100644 --- a/test/components/post_navbar_component_test.rb +++ b/test/components/post_navbar_component_test.rb @@ -7,7 +7,7 @@ class PostNavbarComponentTest < ViewComponent::TestCase setup do @post = create(:post) - @user = create(:user) + @user = create(:gold_user) end context "The PostNavbarComponent" do @@ -46,9 +46,8 @@ class PostNavbarComponentTest < ViewComponent::TestCase context "for a post with favgroups" do setup do as(@user) do - @favgroup1 = create(:favorite_group, creator: @user, is_public: true) - @favgroup2 = create(:favorite_group, creator: @user, is_public: false) - @post.update(tag_string: "favgroup:#{@favgroup1.id} favgroup:#{@favgroup2.id}") + @favgroup1 = create(:favorite_group, creator: @user, post_ids: [@post.id]) + @favgroup2 = create(:private_favorite_group, creator: @user, post_ids: [@post.id]) end end diff --git a/test/components/post_votes_component_test.rb b/test/components/post_votes_component_test.rb index df30aff6d..7a9118d56 100644 --- a/test/components/post_votes_component_test.rb +++ b/test/components/post_votes_component_test.rb @@ -11,12 +11,12 @@ class PostVotesComponentTest < ViewComponent::TestCase end context "for a user who can't vote" do - should "not show the vote buttons" do + should "show the vote buttons" do render_post_votes(@post, current_user: User.anonymous) assert_css(".post-score") - assert_no_css(".post-upvote-link") - assert_no_css(".post-downvote-link") + assert_css(".post-upvote-link.inactive-link") + assert_css(".post-downvote-link.inactive-link") end end @@ -34,8 +34,8 @@ class PostVotesComponentTest < ViewComponent::TestCase context "for a downvoted post" do should "highlight the downvote button as active" do - @post.vote!(-1, @user) - render_post_votes(@post, current_user: @user) + create(:post_vote, post: @post, user: @user, score: -1) + as(@user) { render_post_votes(@post, current_user: @user) } assert_css(".post-upvote-link.inactive-link") assert_css(".post-downvote-link.active-link") @@ -44,8 +44,8 @@ class PostVotesComponentTest < ViewComponent::TestCase context "for an upvoted post" do should "highlight the upvote button as active" do - @post.vote!(1, @user) - render_post_votes(@post, current_user: @user) + create(:post_vote, post: @post, user: @user, score: 1) + as(@user) { render_post_votes(@post, current_user: @user) } assert_css(".post-upvote-link.active-link") assert_css(".post-downvote-link.inactive-link") diff --git a/test/factories/favorite.rb b/test/factories/favorite.rb index 441c6f4a9..40ef97b84 100644 --- a/test/factories/favorite.rb +++ b/test/factories/favorite.rb @@ -2,5 +2,9 @@ FactoryBot.define do factory(:favorite) do user post + + factory(:private_favorite) do + user factory: :gold_user, enable_private_favorites: true + end end end diff --git a/test/unit/favorite_test.rb b/test/unit/favorite_test.rb index 9c0bed69b..8273dc8af 100644 --- a/test/unit/favorite_test.rb +++ b/test/unit/favorite_test.rb @@ -11,10 +11,13 @@ class FavoriteTest < ActiveSupport::TestCase context "Favorites: " do context "removing a favorite" do should "update the post and user favorite counts" do + @user1 = create(:restricted_user) fav = Favorite.create!(post: @p1, user: @user1) assert_equal(1, @user1.reload.favorite_count) assert_equal(1, @p1.reload.fav_count) + assert_equal(0, @p1.reload.score) + refute(PostVote.positive.exists?(post: @p1, user: @user)) Favorite.destroy_by(post: @p1, user: @user1) @@ -42,14 +45,8 @@ class FavoriteTest < ActiveSupport::TestCase end context "adding a favorite" do - should "update the post and user favorite counts" do - Favorite.create!(post: @p1, user: @user1) - - assert_equal(1, @user1.reload.favorite_count) - assert_equal(1, @p1.reload.fav_count) - end - should "not upvote the post if the user can't vote" do + @user1 = create(:restricted_user) Favorite.create!(post: @p1, user: @user1) assert_equal(1, @user1.reload.favorite_count) @@ -89,7 +86,7 @@ class FavoriteTest < ActiveSupport::TestCase assert_equal(["You have already favorited this post"], @f2.errors.full_messages) assert_equal(1, @user1.reload.favorite_count) assert_equal(1, @p1.reload.fav_count) - assert_equal(0, @p1.reload.score) + assert_equal(1, @p1.reload.score) end end end diff --git a/test/unit/post_query_builder_test.rb b/test/unit/post_query_builder_test.rb index 94958fda6..a0706f543 100644 --- a/test/unit/post_query_builder_test.rb +++ b/test/unit/post_query_builder_test.rb @@ -390,7 +390,7 @@ class PostQueryBuilderTest < ActiveSupport::TestCase favgroup1 = create(:favorite_group, creator: CurrentUser.user, post_ids: [post1.id]) favgroup2 = create(:favorite_group, creator: CurrentUser.user, post_ids: [post2.id]) - favgroup3 = create(:favorite_group, creator: create(:user), post_ids: [post3.id], is_public: false) + favgroup3 = create(:private_favorite_group, post_ids: [post3.id]) assert_tag_match([post1], "favgroup:#{favgroup1.id}") assert_tag_match([post2], "favgroup:#{favgroup2.name}") @@ -421,7 +421,7 @@ class PostQueryBuilderTest < ActiveSupport::TestCase post3 = create(:post) favgroup1 = create(:favorite_group, creator: CurrentUser.user, post_ids: [post1.id, post2.id]) - favgroup2 = create(:favorite_group, creator: create(:user), post_ids: [post2.id, post3.id], is_public: false) + favgroup2 = create(:private_favorite_group, post_ids: [post2.id, post3.id]) assert_tag_match([post1, post2], "ordfavgroup:#{favgroup1.id}") assert_tag_match([post1, post2], "ordfavgroup:#{favgroup1.name}") @@ -975,7 +975,7 @@ class PostQueryBuilderTest < ActiveSupport::TestCase context "for the upvote: metatag" do setup do - @user = create(:user) + @user = create(:gold_user) @upvote = create(:post_vote, user: @user, score: 1) @downvote = create(:post_vote, user: @user, score: -1) end @@ -1411,8 +1411,7 @@ class PostQueryBuilderTest < ActiveSupport::TestCase end should "return the correct favorite count for a fav: search for a user with private favorites" do - fav = create(:favorite) - fav.user.update!(favorite_count: 1, enable_private_favorites: true) + fav = create(:private_favorite) assert_fast_count(0, "fav:#{fav.user.name}") assert_fast_count(0, "ordfav:#{fav.user.name}") diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index a4f0a6f53..2721c52a9 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -699,10 +699,34 @@ class PostTest < ActiveSupport::TestCase context "for a fav" do should "add/remove the current user to the post's favorite listing" do @post.update(tag_string: "aaa fav:self") + assert_equal(1, @post.reload.score) assert_equal(1, @post.favorites.where(user: @user).count) + assert_equal(1, @post.votes.positive.where(user: @user).count) @post.update(tag_string: "aaa -fav:self") + assert_equal(0, @post.reload.score) assert_equal(0, @post.favorites.count) + assert_equal(0, @post.votes.positive.where(user: @user).count) + end + + should "not allow banned users to fav" do + assert_raises(User::PrivilegeError) do + as(create(:banned_user)) { @post.update(tag_string: "aaa fav:self") } + end + + assert_raises(User::PrivilegeError) do + as(create(:banned_user)) { @post.update(tag_string: "aaa -fav:self") } + end + end + + should "not allow restricted users to fav" do + assert_raises(User::PrivilegeError) do + as(create(:restricted_user)) { @post.update(tag_string: "aaa fav:self") } + end + + assert_raises(User::PrivilegeError) do + as(create(:restricted_user)) { @post.update(tag_string: "aaa -fav:self") } + end end should "not fail when the fav: metatag is used twice" do @@ -870,20 +894,20 @@ class PostTest < ActiveSupport::TestCase context "upvote:self or downvote:self" do context "by a member" do - should "not upvote the post" do - assert_no_difference("PostVote.count") do + should "upvote the post" do + assert_difference("PostVote.count") do @post.update(tag_string: "upvote:self") end - assert_equal(0, @post.reload.score) + assert_equal(1, @post.reload.score) end - should "not downvote the post" do - assert_no_difference("PostVote.count") do + should "downvote the post" do + assert_difference("PostVote.count") do @post.update(tag_string: "downvote:self") end - assert_equal(0, @post.reload.score) + assert_equal(-1, @post.reload.score) end end @@ -1476,7 +1500,8 @@ class PostTest < ActiveSupport::TestCase should "create a vote for each user who can vote" do assert(@parent.votes.where(user: @gold1).exists?) - assert_equal(1, @parent.score) + assert(@parent.votes.where(user: @user1).exists?) + assert_equal(2, @parent.score) end end end @@ -1523,13 +1548,13 @@ class PostTest < ActiveSupport::TestCase end context "Voting:" do - should "not allow members to vote" do + should "allow members to vote" do user = create(:user) post = create(:post) assert_nothing_raised { post.vote!(1, user) } - assert_equal(0, post.votes.count) - assert_equal(0, post.reload.score) + assert_equal(1, post.votes.count) + assert_equal(1, post.reload.score) end should "not allow duplicate votes" do