maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

UserSubscription backend refactor (#9060)

Browse source 
* Refactor controller validations to model

* Update specs and move current email validation back

* Refactor #subscribed

* Cleanup UserSubscriptions::Create service

* Refactor service

* Move source inside of call for clarity

* Specs for UserSubscription::Create

* Cleanup spec

* Fix Article spec

* Updated for subscriber_email

* Add comment for explicitness of subscriber_email

* Fix reference in comment...oops!

* I know what I'm doing...sometimes...maybe

* Rename CachChecker - Subscription to IsSubscribed

* Use include vs eq in specs for error messages

* attr_accessor --> attr_reader

* Don't explicitly set nil

* Rename Create service to CreateFromService

* Use errors_as_sentence

* Rename service to CreateFromControllerParams

* Assign Struct to a new constant
This commit is contained in:
Alex 2020-07-02 12:08:37 -04:00 • committed by GitHub
parent 4f31a8ac46
commit 9ee024f9d0
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 247 additions and 108 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

69
app/controllers/user_subscriptions_controller.rb
View file

@ -5,10 +5,7 @@ class UserSubscriptionsController < ApplicationController
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)
is_subscribed = UserSubscriptions::IsSubscribedCacheChecker.call(current_user, params)
render json: { is_subscribed: is_subscribed, success: true }, status: :ok
end
@ -16,74 +13,16 @@ class UserSubscriptionsController < ApplicationController
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)
user_subscription = UserSubscriptions::CreateFromControllerParams.call(current_user, user_subscription_params)
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?
return error_response("cannot subscribe with Apple private relay email") if subscriber_authed_with_apple?
@user_subscription = user_subscription_source.build_user_subscription(current_user)
if @user_subscription.save
if user_subscription.success
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)
render json: { error: user_subscription.error, success: false }, status: :unprocessable_entity
end
end
private
def subscriber_authed_with_apple?
current_user.email.end_with?("@privaterelay.appleid.com")
end
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

48
app/models/user_subscription.rb
View file

@ -16,6 +16,10 @@ class UserSubscription < ApplicationRecord
validates :user_subscription_sourceable_id, presence: true
validates :user_subscription_sourceable_type, presence: true, inclusion: { in: ALLOWED_TYPES }
validate :tag_enabled
validate :non_apple_auth_subscriber
validate :active_user_subscription_source
def self.build(source:, subscriber:)
new(build_attributes(source, subscriber))
end
@ -32,4 +36,48 @@ class UserSubscription < ApplicationRecord
subscriber_email: subscriber&.email
}
end
private
def tag_enabled
return unless user_subscription_sourceable
liquid_tags =
case user_subscription_sourceable_type
when "Article"
user_subscription_sourceable.liquid_tags_used(:body)
else
user_subscription_sourceable.liquid_tags_used
end
return if liquid_tags.include?(UserSubscriptionTag)
errors.add(:base, "User subscriptions are not enabled for the source.")
end
def non_apple_auth_subscriber
return unless subscriber_email&.end_with?("@privaterelay.appleid.com")
errors.add(:subscriber_email, "Can't subscribe with an Apple private relay. Please update email.")
end
def active_user_subscription_source
return unless user_subscription_sourceable
source_active =
# Don't create new user subscriptions for inactive sources
# (i.e. unpublished Articles, deleted Comments, etc.)
case user_subscription_sourceable_type
when "Article"
user_subscription_sourceable.published?
when "Comment"
!user_subscription_sourceable.deleted?
else
false
end
return if source_active
errors.add(:base, "Source not found.")
end
end

71
app/services/user_subscriptions/create_from_controller_params.rb Normal file
View file

@ -0,0 +1,71 @@
module UserSubscriptions
# When creating a UserSubscription from a controller/user interation on the
# frontend, we need to do some extra validations and logic.
class CreateFromControllerParams
attr_reader :user, :source_type, :source_id, :subscriber_email, :response
Response = Struct.new(:success, :data, :error, keyword_init: true)
def self.call(*args)
new(*args).call
end
def initialize(user, user_subscription_params)
@user = user
@source_type = user_subscription_params[:source_type]
@source_id = user_subscription_params[:source_id]
@response = Response.new(success: false)
# TODO: [@thepracticaldev/delightful]: uncomment this once email confirmation is re-enabled
# @subscriber_email = user_subscription_params[:subscriber_email]
end
def call
return response if invalid_source_type?
source = source_type.constantize.find_by(id: source_id)
return response if invalid_source?(source)
# TODO: [@thepracticaldev/delightful]: uncomment this once email confirmation is re-enabled
# return response if subscriber_email_mismatch?
user_subscription = source.build_user_subscription(user)
if user_subscription.save
response.success = true
response.data = user_subscription
else
response.error = user_subscription.errors_as_sentence
end
response
end
private
def invalid_source_type?
return false if UserSubscription::ALLOWED_TYPES.include?(source_type)
response.error = "Invalid source_type."
true
end
def invalid_source?(source)
return false if source
response.error = "Source not found."
true
end
def subscriber_email_mismatch?
# 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.
return false if user.email == subscriber_email
response.error = "Subscriber email mismatch."
true
end
end
end

10
app/services/user_subscriptions/subscription_cache_checker.rb → app/services/user_subscriptions/is_subscribed_cache_checker.rb
View file

@ -1,15 +1,17 @@
module UserSubscriptions
class SubscriptionCacheChecker
# This checks if the provided user is subscribed to the provided source
# (returns boolean).
class IsSubscribedCacheChecker
attr_accessor :user, :source_type, :source_id
def self.call(*args)
new(*args).call
end
def initialize(user, source_type, source_id)
def initialize(user, params)
@user = user
@source_type = source_type
@source_id = source_id
@source_type = params[:source_type]
@source_id = params[:source_id]
end
def call

7
spec/factories/articles.rb
View file

@ -14,6 +14,7 @@ FactoryBot.define do
with_tags { true }
with_hr_issue { false }
with_tweet_tag { false }
with_user_subscription_tag { false }
with_title { true }
with_collection { nil }
end
@ -35,6 +36,7 @@ FactoryBot.define do
#{Faker::Hipster.paragraph(sentence_count: 2)}
#{'{% tweet 1018911886862057472 %}' if with_tweet_tag}
#{'{% user_subscription CTA text %}' if with_user_subscription_tag}
#{Faker::Hipster.paragraph(sentence_count: 1)}
#{"\n\n---\n\n something \n\n---\n funky in the code? \n---\n That's nice" if with_hr_issue}
HEREDOC
@ -53,4 +55,9 @@ FactoryBot.define do
create(:notification_subscription, user_id: article.user_id, notifiable: article)
end
end
# TODO: (Alex Smith) - update roles before release
trait :with_user_subscription_tag_role_user do
after(:build) { |article| article.user.add_role(:super_admin) }
end
end

2
spec/factories/user_subscription.rb
View file

@ -1,7 +1,7 @@
FactoryBot.define do
factory :user_subscription do
association :subscriber, factory: :user, strategy: :create
association :user_subscription_sourceable, factory: :article
association :user_subscription_sourceable, factory: %i[article with_user_subscription_tag_role_user], with_user_subscription_tag: true
author { user_subscription_sourceable.user }
subscriber_email { subscriber.email }

2
spec/models/shared_examples/user_subscription_sourceable_spec.rb
View file

@ -1,6 +1,6 @@
RSpec.shared_examples "UserSubscriptionSourceable" do
let(:model) { described_class }
let(:source) { create(model.to_s.underscore.to_sym) }
let(:source) { create(model.to_s.underscore.to_sym, :with_user_subscription_tag_role_user, with_user_subscription_tag: true) }
let(:subscriber) { create(:user) }
describe "#build_user_subscription" do

38
spec/models/user_subscription_spec.rb
View file

@ -3,8 +3,8 @@ require "rails_helper"
RSpec.describe UserSubscription, type: :model do
subject { build(:user_subscription) }
let(:source) { create(:article) }
let(:subscriber) { create(:user) }
let(:source) { create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true) }
describe "validations" do
it { is_expected.to validate_presence_of(:user_subscription_sourceable_id) }
@ -14,6 +14,27 @@ RSpec.describe UserSubscription, type: :model do
it { is_expected.to validate_presence_of(:author_id) }
it { is_expected.to validate_inclusion_of(:user_subscription_sourceable_type).in_array(%w[Article]) }
it { is_expected.to validate_uniqueness_of(:subscriber_id).scoped_to(%i[subscriber_email user_subscription_sourceable_type user_subscription_sourceable_id]) }
it "validates the source is active" do
unpublished_source = create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true, published: false)
user_subscription = described_class.build(source: unpublished_source, subscriber: subscriber)
expect(user_subscription).not_to be_valid
expect(user_subscription.errors[:base]).to include "Source not found."
end
it "validates the tag is enabled in the source" do
source_without_tag = create(:article, :with_user_subscription_tag_role_user)
user_subscription = described_class.build(source: source_without_tag, subscriber: subscriber)
expect(user_subscription).not_to be_valid
expect(user_subscription.errors[:base]).to include "User subscriptions are not enabled for the source."
end
it "validates the subscriber isn't using an Apple private relay" do
subscriber_with_apple_relay = create(:user, email: "test@privaterelay.appleid.com")
user_subscription = described_class.build(source: source, subscriber: subscriber_with_apple_relay)
expect(user_subscription).not_to be_valid
expect(user_subscription.errors[:subscriber_email]).to include "Can't subscribe with an Apple private relay. Please update email."
end
end
describe "#build" do
@ -35,19 +56,12 @@ RSpec.describe UserSubscription, type: :model do
describe "#make" do
it "returns a created UserSubcription with the correct attributes" do
user_subscription_fields = %w[author_id subsciber_id subscriber_email user_subscription_sourceable_id user_susbcription_sourceable_type]
user_subscription = create(:user_subscription,
user_subscription_sourceable: source,
author_id: source.user_id,
subscriber_id: subscriber.id,
subscriber_email: subscriber.email)
factory_user_subscription = described_class.make(source: source, subscriber: subscriber)
user_subscription_fields.each do |field|
expect(factory_user_subscription[field]).to eq user_subscription[field]
end
expect(factory_user_subscription.user_subscription_sourceable).to eq source
expect(factory_user_subscription.author_id).to eq source.user_id
expect(factory_user_subscription.subscriber_id).to eq subscriber.id
expect(factory_user_subscription.subscriber_email).to eq subscriber.email
end
end

38
spec/requests/user_subscriptions_spec.rb
View file

@ -1,8 +1,6 @@
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 }
@ -13,7 +11,7 @@ RSpec.describe "UserSubscriptions", type: :request do
end
it "returns true if a user is already subscribed" do
article = create(:article)
article = create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true)
create(:user_subscription,
subscriber_id: user.id,
@ -39,7 +37,7 @@ RSpec.describe "UserSubscriptions", type: :request do
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' %}")
article = create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true)
valid_attributes = { source_type: article.class_name, source_id: article.id, subscriber_email: user.email }
expect do
post user_subscriptions_path,
@ -63,7 +61,7 @@ RSpec.describe "UserSubscriptions", type: :request do
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")
expect(response.parsed_body["error"]).to include("Invalid source_type.")
end
it "returns an error for a source that can't be found" do
@ -75,11 +73,11 @@ RSpec.describe "UserSubscriptions", type: :request do
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")
expect(response.parsed_body["error"]).to include("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' %}")
unpublished_article = create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true, published: false)
invalid_source_attributes = { source_type: unpublished_article.class_name, source_id: unpublished_article.id, subscriber_email: user.email }
expect do
post user_subscriptions_path,
@ -88,11 +86,11 @@ RSpec.describe "UserSubscriptions", type: :request do
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")
expect(response.parsed_body["error"]).to include("Source not found.")
end
it "returns an error for a source that doesn't have the UserSubscription liquid tag enabled" do
article = create(:article)
article = create(:article, :with_user_subscription_tag_role_user)
invalid_source_attributes = { source_type: article.class_name, source_id: article.id, subscriber_email: user.email }
expect do
post user_subscriptions_path,
@ -101,12 +99,11 @@ RSpec.describe "UserSubscriptions", type: :request do
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")
expect(response.parsed_body["error"]).to include("User subscriptions are not enabled for the 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' %}")
article = create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true)
# 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.
@ -125,12 +122,12 @@ RSpec.describe "UserSubscriptions", type: :request do
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")
expect(response.parsed_body["error"]).to include("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' %}")
# TODO: [@thepracticaldev/delightful]: re-enable this once email confirmation is re-enabled
xit "returns an error for an email mismatch" do
article = create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true)
invalid_source_attributes = { source_type: article.class_name, source_id: article.id, subscriber_email: "old_email@test.com" }
expect do
@ -140,13 +137,12 @@ RSpec.describe "UserSubscriptions", type: :request do
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")
expect(response.parsed_body["error"]).to include("Subscriber email mismatch.")
end
it "returns an error for a subscriber that signed up with Apple" do
allow(user).to receive(:email).and_return("test@privaterelay.appleid.com")
article = create(:article, user: super_admin_user, body_markdown: "---\ntitle: User Subscription#{rand(1000)}\npublished: true\n---\n\n{% user_subscription 'CTA text' %}")
article = create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true)
valid_source_attributes = { source_type: article.class_name, source_id: article.id, subscriber_email: user.email }
expect do
@ -156,13 +152,13 @@ RSpec.describe "UserSubscriptions", type: :request do
end.to change(UserSubscription, :count).by(0)
expect(response).to have_http_status(:unprocessable_entity)
expect(response.parsed_body["error"]).to eq("cannot subscribe with Apple private relay email")
expect(response.parsed_body["error"]).to include("Subscriber email Can't subscribe with an Apple private relay. Please update email.")
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(:article) { create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true) }
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) }

61
spec/services/user_subscriptions/create_from_controller_params_spec.rb Normal file
View file

@ -0,0 +1,61 @@
require "rails_helper"
RSpec.describe UserSubscriptions::CreateFromControllerParams, type: :service do
let(:subscriber) { create(:user) }
it "returns an error for an invalid source type" do
source = create(:comment)
user_subscription_params = { source_type: source.class.name, source_id: source.id, subscriber_email: subscriber.email }
user_subscription = described_class.call(subscriber, user_subscription_params)
expect(user_subscription.data).to be_nil
expect(user_subscription.success).to be false
expect(user_subscription.error).to eq "Invalid source_type."
end
it "returns an error for an invalid source" do
source = create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true)
user_subscription_params = { source_type: source.class.name, source_id: source.id + 999, subscriber_email: subscriber.email }
user_subscription = described_class.call(subscriber, user_subscription_params)
expect(user_subscription.data).to be_nil
expect(user_subscription.success).to be false
expect(user_subscription.error).to eq "Source not found."
end
# TODO: [@thepracticaldev/delightful]: re-enable this once email confirmation is re-enabled
xit "returns an error for an email mismatch" do
source = create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true)
user_subscription_params = { source_type: source.class.name, source_id: source.id, subscriber_email: "old@email.com" }
user_subscription = described_class.call(subscriber, user_subscription_params)
expect(user_subscription.data).to be_nil
expect(user_subscription.success).to be false
expect(user_subscription.error).to eq "Subscriber email mismatch."
end
it "returns an error if a UserSubscription can't be created" do
source = create(:article, :with_user_subscription_tag_role_user)
user_subscription_params = { source_type: source.class.name, source_id: source.id, subscriber_email: subscriber.email }
user_subscription = described_class.call(subscriber, user_subscription_params)
expect(user_subscription.data).to be_nil
expect(user_subscription.success).to be false
expect(user_subscription.error).to eq "User subscriptions are not enabled for the source."
end
it "creates a UserSubscription" do
source = create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true)
user_subscription_params = { source_type: source.class.name, source_id: source.id, subscriber_email: subscriber.email }
user_subscription = described_class.call(subscriber, user_subscription_params)
expect(user_subscription.data).to be_an_instance_of UserSubscription
expect(user_subscription.data.user_subscription_sourceable_type).to eq source.class.name
expect(user_subscription.data.user_subscription_sourceable_id).to eq source.id
expect(user_subscription.data.subscriber_email).to eq subscriber.email
expect(user_subscription.data.subscriber_id).to eq subscriber.id
expect(user_subscription.data.author_id).to eq source.user.id
expect(user_subscription.success).to be true
expect(user_subscription.error).to be_nil
end
end

9
spec/services/user_subscriptions/subscription_cache_checker_spec.rb → spec/services/user_subscriptions/is_subscribed_cache_checker_spec.rb
View file

@ -1,8 +1,9 @@
require "rails_helper"
RSpec.describe UserSubscriptions::SubscriptionCacheChecker, type: :service do
RSpec.describe UserSubscriptions::IsSubscribedCacheChecker, type: :service do
let(:user) { create(:user) }
let(:article) { create(:article) }
let(:article) { create(:article, :with_user_subscription_tag_role_user, with_user_subscription_tag: true) }
let(:params) { { source_type: article.class_name, source_id: article.id } }
it "checks if subscribed to a thing and returns true if they are" do
create(:user_subscription,
@ -11,10 +12,10 @@ RSpec.describe UserSubscriptions::SubscriptionCacheChecker, type: :service do
author_id: article.user_id,
user_subscription_sourceable: article)
expect(described_class.call(user, article.class_name, article.id)).to eq(true)
expect(described_class.call(user, params)).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)
expect(described_class.call(user, params)).to eq(false)
end
end