From 8afb797ec979fd7c7ceb31b8b642a12d59ca4cf2 Mon Sep 17 00:00:00 2001 From: Mike Coutermarsh Date: Tue, 25 Jun 2019 06:45:30 -0600 Subject: [PATCH] =?UTF-8?q?Improve=20social=20card=20caching=20?= =?UTF-8?q?=F0=9F=9A=80=20(#3265)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add failing tests for social previews controller. HTML changes between requests, which breaks caching. Since we use the HTML to generate the cache key. When the views have any randomness in them, it breaks the cache, which means we regenerate images on each request, rather than using what's in memcached. Adding test to protect from regressions * Fix randomness in social card templates The aria tags are randomly generated. Since this is for rendering images, they aren't needed. * Make the linter happy by using the sanitize helper --- app/views/social_previews/article.html.erb | 2 +- app/views/social_previews/listing.html.erb | 4 +- app/views/social_previews/tag.html.erb | 2 +- app/views/social_previews/user.html.erb | 2 +- spec/requests/social_previews_spec.rb | 55 ++++++++++++++++++++++ 5 files changed, 60 insertions(+), 5 deletions(-) diff --git a/app/views/social_previews/article.html.erb b/app/views/social_previews/article.html.erb index 8b41b0627..c6229ccec 100644 --- a/app/views/social_previews/article.html.erb +++ b/app/views/social_previews/article.html.erb @@ -128,7 +128,7 @@ <%= truncate @article.user.name, length: 25 %>・<%= @article.readable_publish_date %> diff --git a/app/views/social_previews/listing.html.erb b/app/views/social_previews/listing.html.erb index 43716ea4f..523b71a2b 100644 --- a/app/views/social_previews/listing.html.erb +++ b/app/views/social_previews/listing.html.erb @@ -74,13 +74,13 @@

<%= @listing.title %>

- <%= @listing.processed_html.html_safe %> + <%= sanitize_rendered_markdown(@listing.processed_html) %>
<%= @category %>
diff --git a/app/views/social_previews/tag.html.erb b/app/views/social_previews/tag.html.erb index 1a956861a..0f224cd53 100644 --- a/app/views/social_previews/tag.html.erb +++ b/app/views/social_previews/tag.html.erb @@ -102,7 +102,7 @@ <% end %> diff --git a/app/views/social_previews/user.html.erb b/app/views/social_previews/user.html.erb index 881c618df..4c0bbd862 100644 --- a/app/views/social_previews/user.html.erb +++ b/app/views/social_previews/user.html.erb @@ -90,7 +90,7 @@ <%= @user.name %> diff --git a/spec/requests/social_previews_spec.rb b/spec/requests/social_previews_spec.rb index e9d78fa4c..40acb1197 100644 --- a/spec/requests/social_previews_spec.rb +++ b/spec/requests/social_previews_spec.rb @@ -21,6 +21,17 @@ RSpec.describe "SocialPreviews", type: :request do expect(response.body).to include CGI.escapeHTML(article.title) end + it "renders consistent HTML between requests" do + # We use the HTML for caching. It needs to be deterministic (if data is unchanged, the HTML should be the same) + get "/social_previews/article/#{article.id}" + first_request_body = response.body + + get "/social_previews/article/#{article.id}" + second_request_body = response.body + + expect(first_request_body).to eq second_request_body + end + it "renders shecoded template when tagged with shecoded" do she_coded_article = create(:article, tags: "shecoded") @@ -42,6 +53,17 @@ RSpec.describe "SocialPreviews", type: :request do expect(response.body).to include CGI.escapeHTML(user.name) end + it "renders consistent HTML between requests" do + # We use the HTML for caching. It needs to be deterministic (if data is unchanged, the HTML should be the same) + get "/social_previews/user/#{user.id}" + first_request_body = response.body + + get "/social_previews/user/#{user.id}" + second_request_body = response.body + + expect(first_request_body).to eq second_request_body + end + it "renders an image when requested and redirects to image url" do get "/social_previews/user/#{user.id}.png" @@ -55,6 +77,17 @@ RSpec.describe "SocialPreviews", type: :request do expect(response.body).to include CGI.escapeHTML(organization.name) end + it "renders consistent HTML between requests" do + # We use the HTML for caching. It needs to be deterministic (if data is unchanged, the HTML should be the same) + get "/social_previews/organization/#{organization.id}" + first_request_body = response.body + + get "/social_previews/organization/#{organization.id}" + second_request_body = response.body + + expect(first_request_body).to eq second_request_body + end + it "renders an image when requested and redirects to image url" do get "/social_previews/organization/#{organization.id}.png" @@ -68,6 +101,17 @@ RSpec.describe "SocialPreviews", type: :request do expect(response.body).to include CGI.escapeHTML(tag.name) end + it "renders consistent HTML between requests" do + # We use the HTML for caching. It needs to be deterministic (if data is unchanged, the HTML should be the same) + get "/social_previews/tag/#{tag.id}" + first_request_body = response.body + + get "/social_previews/tag/#{tag.id}" + second_request_body = response.body + + expect(first_request_body).to eq second_request_body + end + it "renders an image when requested and redirects to image url" do get "/social_previews/tag/#{tag.id}.png" @@ -81,6 +125,17 @@ RSpec.describe "SocialPreviews", type: :request do expect(response.body).to include CGI.escapeHTML("Call For Proposal") end + it "renders consistent HTML between requests" do + # We use the HTML for caching. It needs to be deterministic (if data is unchanged, the HTML should be the same) + get "/social_previews/listing/#{listing.id}" + first_request_body = response.body + + get "/social_previews/listing/#{listing.id}" + second_request_body = response.body + + expect(first_request_body).to eq second_request_body + end + it "renders and image when requested and redirects to iamge url" do get "/social_previews/listing/#{listing.id}.png" expect(response).to redirect_to(image_url)