diff --git a/app/controllers/user_subscriptions_controller.rb b/app/controllers/user_subscriptions_controller.rb index 754ae3dc2..fb3b9b2d3 100644 --- a/app/controllers/user_subscriptions_controller.rb +++ b/app/controllers/user_subscriptions_controller.rb @@ -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 diff --git a/app/models/user_subscription.rb b/app/models/user_subscription.rb index 269f860e2..ced4a14ce 100644 --- a/app/models/user_subscription.rb +++ b/app/models/user_subscription.rb @@ -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 diff --git a/app/services/user_subscriptions/create_from_controller_params.rb b/app/services/user_subscriptions/create_from_controller_params.rb new file mode 100644 index 000000000..2e7834c83 --- /dev/null +++ b/app/services/user_subscriptions/create_from_controller_params.rb @@ -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 diff --git a/app/services/user_subscriptions/subscription_cache_checker.rb b/app/services/user_subscriptions/is_subscribed_cache_checker.rb similarity index 71% rename from app/services/user_subscriptions/subscription_cache_checker.rb rename to app/services/user_subscriptions/is_subscribed_cache_checker.rb index aaa220331..632ba3c99 100644 --- a/app/services/user_subscriptions/subscription_cache_checker.rb +++ b/app/services/user_subscriptions/is_subscribed_cache_checker.rb @@ -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 diff --git a/spec/factories/articles.rb b/spec/factories/articles.rb index d2e469675..a9630c635 100644 --- a/spec/factories/articles.rb +++ b/spec/factories/articles.rb @@ -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 diff --git a/spec/factories/user_subscription.rb b/spec/factories/user_subscription.rb index 47889c971..8f0219739 100644 --- a/spec/factories/user_subscription.rb +++ b/spec/factories/user_subscription.rb @@ -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 } diff --git a/spec/models/shared_examples/user_subscription_sourceable_spec.rb b/spec/models/shared_examples/user_subscription_sourceable_spec.rb index bede2d6ef..47ef5c950 100644 --- a/spec/models/shared_examples/user_subscription_sourceable_spec.rb +++ b/spec/models/shared_examples/user_subscription_sourceable_spec.rb @@ -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 diff --git a/spec/models/user_subscription_spec.rb b/spec/models/user_subscription_spec.rb index 5569684e1..f19da930c 100644 --- a/spec/models/user_subscription_spec.rb +++ b/spec/models/user_subscription_spec.rb @@ -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 diff --git a/spec/requests/user_subscriptions_spec.rb b/spec/requests/user_subscriptions_spec.rb index af4292717..8d4a36e00 100644 --- a/spec/requests/user_subscriptions_spec.rb +++ b/spec/requests/user_subscriptions_spec.rb @@ -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) } diff --git a/spec/services/user_subscriptions/create_from_controller_params_spec.rb b/spec/services/user_subscriptions/create_from_controller_params_spec.rb new file mode 100644 index 000000000..800184c3c --- /dev/null +++ b/spec/services/user_subscriptions/create_from_controller_params_spec.rb @@ -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 diff --git a/spec/services/user_subscriptions/subscription_cache_checker_spec.rb b/spec/services/user_subscriptions/is_subscribed_cache_checker_spec.rb similarity index 53% rename from spec/services/user_subscriptions/subscription_cache_checker_spec.rb rename to spec/services/user_subscriptions/is_subscribed_cache_checker_spec.rb index 7458f04c6..d0d2274bd 100644 --- a/spec/services/user_subscriptions/subscription_cache_checker_spec.rb +++ b/spec/services/user_subscriptions/is_subscribed_cache_checker_spec.rb @@ -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