[deploy] User subscriptions API and backend updates (#8779)

* Create "blank" EmailSubscriptionTag

* Refactor liquid_tags_used with spec

* Create /user_subscriptions#create

- Update liquid tag name to UserSubscriptionTag

* Add rate limiting and specs

* Add counter_culture for user_subscriptions

* Update /async_info/base_data

- Alphabetize user_data
- Add email
- Add subscription_source_article_ids
- Cache subscription_source_article_ids on User model

* Add stale email check and specs

* Change user_email to subscriber_email for clarity

* Restrict UserSubscriptionTag

* Rename RESTRICTED_TAGS to RESTRICTED_LIQUID_TAGS

* Make TODO comment more clear

* Refactor error responses and update specs

* Update type to source_type in error message

* Use constantize over safe_constantize

* Add check for active source

* Refactor checking of current_user's subscriptions

- Remove data from async_info
- Create a new service to fetch/cache a user's existing subscriptions

* Restrict email in base_data to admin roles

* Oops! Rename liquid tag file

* Change error back to result...oops!

* It's not goodbye, it's see you later. RIP email :/

* Add current_email to /user_subscriptions/base_data

* Revert adding current_email

* Undo async_info_controller changes/fix conflict

* Move params to constant

* Refactor SubscriptionCacheChecker

* Remove duplicate status code in JSON response

* Remove duplicate status code for #subscribed

* Use response.parsed_body

* Remove user guard in SubscriptionCacheChecker
This commit is contained in:
Alex 2020-06-23 13:43:32 -04:00 committed by GitHub
parent 75e5151292
commit 8714a36d27
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
17 changed files with 413 additions and 8 deletions

View file

@ -113,6 +113,7 @@ class Internal::ConfigsController < Internal::ApplicationController
rate_limit_image_upload
rate_limit_published_article_creation
rate_limit_organization_creation
rate_limit_user_subscription_creation
]
end

View file

@ -0,0 +1,85 @@
class UserSubscriptionsController < ApplicationController
before_action :authenticate_user!
USER_SUBSCRIPTION_PARAMS = %i[source_type source_id subscriber_email].freeze
def subscribed
params.require(%i[source_type source_id])
source_type = params[:source_type]
source_id = params[:source_id]
is_subscribed = UserSubscriptions::SubscriptionCacheChecker.call(current_user, source_type, source_id)
render json: { is_subscribed: is_subscribed, success: true }, status: :ok
end
def create
rate_limit!(:user_subscription_creation)
source_type = user_subscription_params[:source_type]
return error_response("invalid source_type") unless UserSubscription::ALLOWED_TYPES.include?(source_type)
source_id = user_subscription_params[:source_id]
user_subscription_source = source_type.constantize.find_by(id: source_id)
return error_response("source not found") unless active_source?(source_type, user_subscription_source)
unless user_subscription_tag_enabled?(source_type, user_subscription_source)
return error_response("user subscriptions are not enabled for the requested source")
end
return error_response("subscriber email mismatch") if subscriber_email_stale?
@user_subscription = user_subscription_source.build_user_subscription(current_user)
if @user_subscription.save
rate_limiter.track_limit_by_action(:user_subscription_creation)
render json: { message: "success", success: true }, status: :ok
else
error_response(@user_subscription.errors.full_messages.to_sentence)
end
end
private
def user_subscription_tag_enabled?(source_type, user_subscription_source)
liquid_tags =
case source_type
when "Article"
user_subscription_source.liquid_tags_used(:body)
else
user_subscription_source.liquid_tags_used
end
liquid_tags.include?(UserSubscriptionTag)
end
def active_source?(source_type, user_subscription_source)
return false unless user_subscription_source
# Don't create new user subscriptions for inactive sources
# (i.e. unpublished Articles, deleted Comments, etc.)
case source_type
when "Article"
user_subscription_source.published?
else
false
end
end
def error_response(msg)
render json: { error: msg, success: false }, status: :unprocessable_entity
end
# This checks if the email address the user saw/consented to share is the
# same as their current email address. A mismatch occurs if a user updates
# their email address in a new/separate tab and then tries to subscribe on
# the old/stale tab without refreshing. In that case, the user would have
# consented to share their old email address instead of the current one.
def subscriber_email_stale?
current_user&.email != user_subscription_params[:subscriber_email]
end
def user_subscription_params
params.require(:user_subscription).permit(USER_SUBSCRIPTION_PARAMS)
end
end

View file

@ -59,6 +59,11 @@ module RateLimitCheckerHelper
min: 1,
placeholder: 2,
description: "The number of times we will send a confirmation email to a user in a 2 minute period"
},
rate_limit_user_subscription_creation: {
min: 0,
placeholder: 3,
description: "The number of user subscriptions a user can submit within 30 seconds"
}
}.freeze

View file

@ -0,0 +1,9 @@
class UserSubscriptionTag < LiquidTagBase
def initialize(_tag_name, cta_text, _tokens)
@cta_text = cta_text.strip
end
def render(context); end
end
Liquid::Template.register_tag("user_subscription", UserSubscriptionTag)

View file

@ -9,6 +9,7 @@ class Article < ApplicationRecord
SEARCH_SERIALIZER = Search::ArticleSerializer
SEARCH_CLASS = Search::FeedContent
DATA_SYNC_CLASS = DataSync::Elasticsearch::Article
RESTRICTED_LIQUID_TAGS = [PollTag, UserSubscriptionTag].freeze
acts_as_taggable_on :tags
resourcify
@ -351,6 +352,22 @@ class Article < ApplicationRecord
spaminess_rating: BlackBox.calculate_spaminess(self))
end
def liquid_tags_used(section = nil)
content =
case section
when :body
body_markdown
when :comments
comments_blob
else
"#{body_markdown}#{comments_blob}"
end
MarkdownParser.new(content).tags_used
rescue StandardError
[]
end
private
def search_score
@ -434,12 +451,6 @@ class Article < ApplicationRecord
Rails.logger.error(e)
end
def liquid_tags_used
MarkdownParser.new("#{body_markdown}#{comments_blob}").tags_used
rescue StandardError
[]
end
def update_notifications
Notification.update_notifications(self, "Published")
end
@ -535,9 +546,11 @@ class Article < ApplicationRecord
errors.add(:canonical_url, "must not have spaces") if canonical_url.to_s.match?(/[[:space:]]/)
end
# TODO: (Alex Smith) refactor liquid tag permissions
#
# Admin only beta tags etc.
def validate_liquid_tag_permissions
errors.add(:body_markdown, "must only use permitted tags") if liquid_tags_used.include?(PollTag) && !(user.has_role?(:super_admin) || user.has_role?(:admin))
errors.add(:body_markdown, "must only use permitted tags") if (liquid_tags_used & RESTRICTED_LIQUID_TAGS).any? && !(user.has_role?(:super_admin) || user.has_role?(:admin))
end
def create_slug

View file

@ -107,6 +107,7 @@ class SiteConfig < RailsSettings::Base
field :rate_limit_send_email_confirmation, type: :integer, default: 2
field :rate_limit_feedback_message_creation, type: :integer, default: 5
field :rate_limit_user_update, type: :integer, default: 5
field :rate_limit_user_subscription_creation, type: :integer, default: 3
# Social Media
field :staff_user_id, type: :integer, default: 1

View file

@ -4,6 +4,8 @@
class UserSubscription < ApplicationRecord
ALLOWED_TYPES = %w[Article].freeze
counter_culture :subscriber, column_name: "subscribed_to_user_subscriptions_count"
belongs_to :author, class_name: "User", inverse_of: :source_authored_user_subscriptions
belongs_to :subscriber, class_name: "User", inverse_of: :subscribed_to_user_subscriptions
belongs_to :user_subscription_sourceable, polymorphic: true

View file

@ -11,6 +11,7 @@ class RateLimitChecker
published_article_creation: { retry_after: 30 },
reaction_creation: { retry_after: 30 },
send_email_confirmation: { retry_after: 120 },
user_subscription_creation: { retry_after: 30 },
user_update: { retry_after: 30 }
}.with_indifferent_access.freeze

View file

@ -0,0 +1,26 @@
module UserSubscriptions
class SubscriptionCacheChecker
attr_accessor :user, :source_type, :source_id
def self.call(*args)
new(*args).call
end
def initialize(user, source_type, source_id)
@user = user
@source_type = source_type
@source_id = source_id
end
def call
cache_key = "user-#{user.id}-#{user.updated_at.rfc3339}-#{user.subscribed_to_user_subscriptions_count}/is_subscribed_#{source_type}_#{source_id}"
Rails.cache.fetch(cache_key, expires_in: 24.hours) do
UserSubscription.where(
subscriber_id: user.id,
user_subscription_sourceable_type: source_type,
user_subscription_sourceable_id: source_id,
).any?
end
end
end
end

View file

@ -235,6 +235,11 @@ Rails.application.routes.draw do
resources :podcasts, only: %i[new create]
resources :article_approvals, only: %i[create]
resources :video_chats, only: %i[show]
resources :user_subscriptions, only: %i[create] do
collection do
get "/subscribed", action: "subscribed"
end
end
namespace :followings, defaults: { format: :json } do
get :users
get :tags

View file

@ -0,0 +1,9 @@
class AddSubscribedToUserSubscriptionsCountToUsers < ActiveRecord::Migration[6.0]
def self.up
add_column :users, :subscribed_to_user_subscriptions_count, :integer, null: false, default: 0
end
def self.down
remove_column :users, :subscribed_to_user_subscriptions_count
end
end

View file

@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 2020_06_17_183058) do
ActiveRecord::Schema.define(version: 2020_06_18_212422) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
@ -1276,6 +1276,7 @@ ActiveRecord::Schema.define(version: 2020_06_17_183058) do
t.integer "spent_credits_count", default: 0, null: false
t.string "stackoverflow_url"
t.string "stripe_id_code"
t.integer "subscribed_to_user_subscriptions_count", default: 0, null: false
t.text "summary"
t.string "text_color_hex"
t.string "twitch_url"

View file

@ -875,4 +875,30 @@ RSpec.describe Article, type: :model do
end
end
end
describe "#liquid_tags_used" do
let(:user_liquid_tag) { "{% user #{user.username} %}" }
let(:article_body_markdown) { "---\ntitle: Tag Liquid Tag#{rand(1000)}\npublished: true\n---\n\n#{user_liquid_tag}" }
let(:article_with_user_liquid_tag) { create(:article, body_markdown: article_body_markdown) }
let(:tag) { create(:tag) }
let(:tag_liquid_tag) { "{% tag #{tag.name} %}" }
let(:comment_with_tag_liquid_tag) { create(:comment, body_markdown: tag_liquid_tag, commentable: article_with_user_liquid_tag, score: 20) }
before do
comment_with_tag_liquid_tag
article_with_user_liquid_tag.reload
end
it "returns liquid tags from both the body and comments by default" do
expect(article_with_user_liquid_tag.liquid_tags_used).to match_array([TagTag, UserTag])
end
it "returns liquid tags from only the body of the Article" do
expect(article_with_user_liquid_tag.liquid_tags_used(:body)).to match_array([UserTag])
end
it "returns liquid tags from only the comments of the Article" do
expect(article_with_user_liquid_tag.liquid_tags_used(:comments)).to match_array([TagTag])
end
end
end

View file

@ -50,4 +50,25 @@ RSpec.describe UserSubscription, type: :model do
end
end
end
describe "counter_culture" do
let(:user) { create(:user) }
context "when a UserSubscription is created" do
it "increments subscribed_to_user_subscriptions_count on user" do
expect do
create(:user_subscription, subscriber: user)
end.to change { user.reload.subscribed_to_user_subscriptions_count }.by(1)
end
end
context "when a UserSubscription is destroyed" do
it "decrements subscribed_to_user_subscriptions_count on user" do
user_subscription = create(:user_subscription, subscriber: user)
expect do
user_subscription.destroy
end.to change { user.reload.subscribed_to_user_subscriptions_count }.by(-1)
end
end
end
end

View file

@ -361,6 +361,12 @@ RSpec.describe "/internal/config", type: :request do
post "/internal/config", params: { site_config: { rate_limit_email_recipient: 3 }, confirmation: confirmation_message }
end.to change(SiteConfig, :rate_limit_email_recipient).from(5).to(3)
end
it "updates rate_limit_user_subscription_creation" do
expect do
post "/internal/config", params: { site_config: { rate_limit_user_subscription_creation: 1 }, confirmation: confirmation_message }
end.to change(SiteConfig, :rate_limit_user_subscription_creation).from(3).to(1)
end
end
describe "Social Media" do

View file

@ -0,0 +1,174 @@
require "rails_helper"
RSpec.describe "UserSubscriptions", type: :request do
# TODO: (Alex Smith) remove super_admin restriction before final release
let(:super_admin_user) { create(:user, :super_admin) }
let(:user) { create(:user) }
before { sign_in user }
describe "GET /user_subscriptions/subscribed - UserSubscriptions#subscribed" do
it "raises an error for missing params" do
expect { get subscribed_user_subscriptions_path, params: {} }.to raise_error(ActionController::ParameterMissing)
end
it "returns true if a user is already subscribed" do
article = create(:article)
create(:user_subscription,
subscriber_id: user.id,
subscriber_email: user.email,
author_id: article.user_id,
user_subscription_sourceable: article)
valid_params = { source_type: article.class_name, source_id: article.id }
get subscribed_user_subscriptions_path, params: valid_params
expect(response).to have_http_status(:ok)
expect(response.parsed_body["is_subscribed"]).to eq true
end
it "returns false if a user is not already subscribed" do
valid_params = { source_type: "Article", source_id: 999 }
get subscribed_user_subscriptions_path, params: valid_params
expect(response).to have_http_status(:ok)
expect(response.parsed_body["is_subscribed"]).to eq false
end
end
describe "POST /user_subscriptions - UserSubscriptions#create" do
it "creates a UserSubscription" do
article = create(:article, user: super_admin_user, body_markdown: "---\ntitle: User Subscription#{rand(1000)}\npublished: true\n---\n\n{% user_subscription 'CTA text' %}")
valid_attributes = { source_type: article.class_name, source_id: article.id, subscriber_email: user.email }
expect do
post user_subscriptions_path,
headers: { "Content-Type" => "application/json" },
params: { user_subscription: valid_attributes }.to_json
end.to change(UserSubscription, :count).by(1)
user_subscription = UserSubscription.last
expect(user_subscription.subscriber_id).to eq user.id
expect(user_subscription.author_id).to eq article.user_id
expect(user_subscription.user_subscription_sourceable_type).to eq article.class_name
expect(user_subscription.user_subscription_sourceable_id).to eq article.id
end
it "returns an error for an invalid source_type" do
invalid_source_type_attributes = { source_type: "NonExistentSourceType", source_id: "1", subscriber_email: user.email }
expect do
post user_subscriptions_path,
headers: { "Content-Type" => "application/json" },
params: { user_subscription: invalid_source_type_attributes }.to_json
end.to change(UserSubscription, :count).by(0)
expect(response).to have_http_status(:unprocessable_entity)
expect(response.parsed_body["error"]).to eq("invalid source_type")
end
it "returns an error for a source that can't be found" do
invalid_source_attributes = { source_type: "Article", source_id: "99999999" }
expect do
post user_subscriptions_path,
headers: { "Content-Type" => "application/json" },
params: { user_subscription: invalid_source_attributes }.to_json
end.to change(UserSubscription, :count).by(0)
expect(response).to have_http_status(:unprocessable_entity)
expect(response.parsed_body["error"]).to eq("source not found")
end
it "returns an error for an inactive source" do
unpublished_article = create(:article, user: super_admin_user, body_markdown: "---\ntitle: User Subscription#{rand(1000)}\npublished: false\n---\n\n{% user_subscription 'CTA text' %}")
invalid_source_attributes = { source_type: unpublished_article.class_name, source_id: unpublished_article.id, subscriber_email: user.email }
expect do
post user_subscriptions_path,
headers: { "Content-Type" => "application/json" },
params: { user_subscription: invalid_source_attributes }.to_json
end.to change(UserSubscription, :count).by(0)
expect(response).to have_http_status(:unprocessable_entity)
expect(response.parsed_body["error"]).to eq("source not found")
end
it "returns an error for a source that doesn't have the UserSubscription liquid tag enabled" do
article = create(:article)
invalid_source_attributes = { source_type: article.class_name, source_id: article.id, subscriber_email: user.email }
expect do
post user_subscriptions_path,
headers: { "Content-Type" => "application/json" },
params: { user_subscription: invalid_source_attributes }.to_json
end.to change(UserSubscription, :count).by(0)
expect(response).to have_http_status(:unprocessable_entity)
expect(response.parsed_body["error"]).to eq("user subscriptions are not enabled for the requested source")
end
it "returns an error for an invalid UserSubscription" do
article = create(:article, user: super_admin_user, body_markdown: "---\ntitle: User Subscription#{rand(1000)}\npublished: true\n---\n\n{% user_subscription 'CTA text' %}")
# Create the UserSubscription directly so it results in a
# duplicate/invalid record and returns an error. This mimics a user
# trying to subscribe to the same user via the same source, twice.
create(:user_subscription,
subscriber_id: user.id,
subscriber_email: user.email,
author_id: article.user.id,
user_subscription_sourceable: article)
invalid_source_attributes = { source_type: article.class_name, source_id: article.id, subscriber_email: user.email }
expect do
post user_subscriptions_path,
headers: { "Content-Type" => "application/json" },
params: { user_subscription: invalid_source_attributes }.to_json
end.to change(UserSubscription, :count).by(0)
expect(response).to have_http_status(:unprocessable_entity)
expect(response.parsed_body["error"]).to eq("Subscriber has already been taken")
end
it "returns an error for an email mismatch" do
article = create(:article, user: super_admin_user, body_markdown: "---\ntitle: User Subscription#{rand(1000)}\npublished: true\n---\n\n{% user_subscription 'CTA text' %}")
invalid_source_attributes = { source_type: article.class_name, source_id: article.id, subscriber_email: "old_email@test.com" }
expect do
post user_subscriptions_path,
headers: { "Content-Type" => "application/json" },
params: { user_subscription: invalid_source_attributes }.to_json
end.to change(UserSubscription, :count).by(0)
expect(response).to have_http_status(:unprocessable_entity)
expect(response.parsed_body["error"]).to eq("subscriber email mismatch")
end
end
context "when rate limiting" do
let(:rate_limiter) { RateLimitChecker.new(user) }
let(:article) { create(:article, user: super_admin_user, body_markdown: "---\ntitle: User Subscription#{rand(1000)}\npublished: true\n---\n\n{% user_subscription 'CTA text' %}") }
let(:valid_attributes) { { source_type: article.class_name, source_id: article.id, subscriber_email: user.email } }
before { allow(RateLimitChecker).to receive(:new).and_return(rate_limiter) }
it "increments rate limit for user_subscription_creation" do
allow(rate_limiter).to receive(:track_limit_by_action)
post user_subscriptions_path,
headers: { "Content-Type" => "application/json" },
params: { user_subscription: valid_attributes }.to_json
expect(rate_limiter).to have_received(:track_limit_by_action).with(:user_subscription_creation)
end
it "returns a 429 status when rate limit is reached" do
allow(rate_limiter).to receive(:limit_by_action).and_return(true)
post user_subscriptions_path,
headers: { "Content-Type" => "application/json" },
params: { user_subscription: valid_attributes }.to_json
expect(response).to have_http_status(:too_many_requests)
expected_retry_after = RateLimitChecker::ACTION_LIMITERS.dig(:user_subscription_creation, :retry_after)
expect(response.headers["Retry-After"]).to eq(expected_retry_after)
end
end
end

View file

@ -0,0 +1,20 @@
require "rails_helper"
RSpec.describe UserSubscriptions::SubscriptionCacheChecker, type: :service do
let(:user) { create(:user) }
let(:article) { create(:article) }
it "checks if subscribed to a thing and returns true if they are" do
create(:user_subscription,
subscriber_id: user.id,
subscriber_email: user.email,
author_id: article.user_id,
user_subscription_sourceable: article)
expect(described_class.call(user, article.class_name, article.id)).to eq(true)
end
it "checks if subscribed to a thing and returns false if they are not" do
expect(described_class.call(user, article.class_name, article.id)).to eq(false)
end
end