diff --git a/app/models/subscription.rb b/app/models/subscription.rb index a04a9d7ccb..c092a75d72 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -37,13 +37,25 @@ def name end end + def creator_no_longer_associated_with?(creation) + # If we're subscribed to a work or series, it doesn't matter who the creator is, we should send the notification + return false unless subscribable_type == "User" + + # If any of the creation's pseud's users matches the subscribable, then the + # creator still matches, so we return "true" + return false if creation.pseuds.any? { |p| p.user == subscribable } + + # We reach this case if e.g. someone is subscribed to a user, but they + # orphan the work before the subscription notification would be sent + true + end + # Guard against scenarios that may break anonymity or other things. # Emails should only contain works or chapters. # Emails should only contain posted works or chapters. # Emails should never contain chapters of draft works. # Emails should never contain hidden works or chapters of hidden works. - # Emails should never contain orphaned works or chapters. - # TODO: AO3-3620 & AO3-5696: Allow subscriptions to orphan_account to receive notifications. + # Emails should not contain orphaned works or chapters if the subscription is to a creator no longer associated with the work # Emails for user subs should never contain anon works or chapters. # Emails for work subs should never contain anything but chapters. # TODO: AO3-1250: Anon series subscription improvements @@ -52,7 +64,7 @@ def valid_notification_entry?(creation) return false unless creation.try(:posted) return false if creation.is_a?(Chapter) && !creation.work.try(:posted) return false if creation.try(:hidden_by_admin) || (creation.is_a?(Chapter) && creation.work.try(:hidden_by_admin)) - return false if creation.pseuds.any? { |p| p.user == User.orphan_account } + return false if creator_no_longer_associated_with?(creation) return false if subscribable_type == "User" && creation.anonymous? return false if subscribable_type == "Work" && !creation.is_a?(Chapter) diff --git a/features/other_a/orphan_work.feature b/features/other_a/orphan_work.feature index f8d67087d4..6081995e46 100644 --- a/features/other_a/orphan_work.feature +++ b/features/other_a/orphan_work.feature @@ -8,9 +8,9 @@ Feature: Orphan work Background: Given I have an orphan account And the following activated users exists - | login | password | email | - | orphaneer | password | orphaneer@foo.com | - | author_subscriber | password | author_subscriber@foo.com | + | login | password | email | + | orphaneer | password | orphaneer@foo.com | + | author_subscriber | password | author_subscriber@foo.com | And "author_subscriber" subscribes to author "orphaneer" And all emails have been delivered And I am logged in as "orphaneer" @@ -53,7 +53,7 @@ Feature: Orphan work When I follow "Orphan Work" Then I should see "Read More About The Orphaning Process" When I choose "Leave a copy of my pseud on" - And I press "Yes, I'm sure" + And I press "Yes, I'm sure" Then I should see "Orphaning was successful." When I go to orphaneer's works page Then I should not see "Shenanigans2" @@ -84,68 +84,72 @@ Feature: Orphan work Then 0 emails should be delivered - Scenario: Orphan a work (leave pseud) and don't notify subscribers to the work + Scenario: Orphan a work (leave pseud) but do notify subscribers to the work Given the following activated user exists - | login | password | email | - | work_subscriber | password | work_subscriber@foo.com | + | login | password | email | + | work_subscriber | password | work_subscriber@foo.com | And I post the work "Torrid Idfic" And I am logged in as "work_subscriber" And I view the work "Torrid Idfic" And I press "Subscribe" + And I am logged in as "orphaneer" And a chapter is added to "Torrid Idfic" And I follow "Edit" And I follow "Orphan Work" And I choose "Leave a copy of my pseud on" And I press "Yes, I'm sure" When subscription notifications are sent - Then 0 emails should be delivered + Then 1 email should be delivered - - Scenario: Orphan a work (leave pseud) and don't notify subscribers to the work's series + Scenario: Orphan a work (leave pseud) but do notify subscribers to the work's series Given the following activated user exists - | login | password | email | - | series_subscriber | password | series_subscriber@foo.com | - And I add the work "Lazy Purple Sausage" to series "Shame Series" + | login | password | email | + | series_subscriber | password | series_subscriber@foo.com | + # Create the series, so we can subscribe to it before we add a work-to-be-orphaned + And I add the work "Work to create the series" to series "Shame Series" + # Subscribe And I am logged in as "series_subscriber" And I view the series "Shame Series" And I press "Subscribe" + # Add a new work to the series, which we then immediately orphan And I am logged in as "orphaneer" + And I add the work "Lazy Purple Sausage" to series "Shame Series" And I view the work "Lazy Purple Sausage" And I follow "Edit" And I follow "Orphan Work" And I choose "Leave a copy of my pseud on" And I press "Yes, I'm sure" When subscription notifications are sent - Then 0 emails should be delivered + Then 1 email should be delivered Scenario: I can orphan multiple works at once - Given I am logged in as "author" - And I post the work "Glorious" with fandom "SGA" - And I post the work "Excellent" with fandom "Star Trek" - And I post the work "Lovely" with fandom "Steven Universe" - And I go to author's works page - When I follow "Edit Works" - Then I should see "Edit Multiple Works" - When I select "Glorious" for editing - And I select "Excellent" for editing - And I press "Delete" - Then I should see "Are you sure you want to delete these works PERMANENTLY?" - And I should see "Glorious" - And I should see "Excellent" - And I should not see "Lovely" - # Delay before orphaning to make sure the cache is expired - And it is currently 1 second from now - When I follow "Orphan Works Instead" - Then I should see "Orphaning a work removes it from your account and re-attaches it to the specially created orphan_account." - When I press "Yes, I'm sure" - And all indexing jobs have been run - Then I should see "Orphaning was successful." - When I go to author's works page - Then I should not see "Glorious" - And I should not see "Excellent" - And I should see "Lovely" + Given I am logged in as "author" + And I post the work "Glorious" with fandom "SGA" + And I post the work "Excellent" with fandom "Star Trek" + And I post the work "Lovely" with fandom "Steven Universe" + And I go to author's works page + When I follow "Edit Works" + Then I should see "Edit Multiple Works" + When I select "Glorious" for editing + And I select "Excellent" for editing + And I press "Delete" + Then I should see "Are you sure you want to delete these works PERMANENTLY?" + And I should see "Glorious" + And I should see "Excellent" + And I should not see "Lovely" + # Delay before orphaning to make sure the cache is expired + And it is currently 1 second from now + When I follow "Orphan Works Instead" + Then I should see "Orphaning a work removes it from your account and re-attaches it to the specially created orphan_account." + When I press "Yes, I'm sure" + And all indexing jobs have been run + Then I should see "Orphaning was successful." + When I go to author's works page + Then I should not see "Glorious" + And I should not see "Excellent" + And I should see "Lovely" Scenario: Orphaning a shared work should not affect chapters created solely by the other creator diff --git a/spec/mailers/user_mailer_spec.rb b/spec/mailers/user_mailer_spec.rb index 27a57650a6..6885e197e0 100644 --- a/spec/mailers/user_mailer_spec.rb +++ b/spec/mailers/user_mailer_spec.rb @@ -740,6 +740,7 @@ def queue_adapter_for_test let(:creator2) { create(:user).default_pseud } let(:work) { create(:work, authors: [creator1, creator2]) } let(:chapter) { create(:chapter, work: work, authors: [creator1]) } + let(:subscription) { create(:subscription, subscribable: creator1.user) } it "has the correct subject line" do subject = "[#{ArchiveConfig.APP_SHORT_NAME}] #{creator1.byline} posted #{chapter.chapter_header} of #{work.title}" diff --git a/spec/models/subscription_spec.rb b/spec/models/subscription_spec.rb index 0c2865d87c..ccc6344d47 100644 --- a/spec/models/subscription_spec.rb +++ b/spec/models/subscription_spec.rb @@ -1,4 +1,4 @@ -require 'spec_helper' +require "spec_helper" describe Subscription do let(:subscription) { build(:subscription) } @@ -22,7 +22,8 @@ end it "should be destroyed" do - expect { subscription.reload }.to raise_error ActiveRecord::RecordNotFound + expect { subscription.reload } + .to raise_error ActiveRecord::RecordNotFound end end end @@ -43,7 +44,8 @@ end it "should be destroyed" do - expect { subscription.reload }.to raise_error ActiveRecord::RecordNotFound + expect { subscription.reload } + .to raise_error ActiveRecord::RecordNotFound end end end @@ -64,7 +66,8 @@ end it "should be destroyed" do - expect { subscription.reload }.to raise_error ActiveRecord::RecordNotFound + expect { subscription.reload } + .to raise_error ActiveRecord::RecordNotFound end end end @@ -111,12 +114,13 @@ describe "#valid_notification_entry?" do let(:subscription) { build(:subscription) } let(:series) { build(:series) } - let(:work) { build(:work) } + let(:author_pseud) { create(:user).default_pseud } + let(:work) { create(:work, authors: [author_pseud]) } let(:draft) { build(:draft) } - let(:chapter) { build(:chapter) } - let(:anon_work) { build(:work, collections: [build(:anonymous_collection)]) } + let(:chapter) { create(:chapter, authors: [author_pseud]) } + let(:anon_work) { create(:work, authors: [author_pseud], collections: [build(:anonymous_collection)]) } let(:anon_series) { build(:series, works: [anon_work]) } - let(:anon_chapter) { build(:chapter, work: anon_work) } + let(:anon_chapter) { create(:chapter, authors: [author_pseud], work: anon_work) } let(:orphan_pseud) { create(:user, login: "orphan_account").default_pseud } it "returns false when the creation is nil" do @@ -132,11 +136,6 @@ expect(subscription.valid_notification_entry?(draft)).to be_falsey end - # TODO: AO3-3620 & AO3-5696: Allow subscriptions to orphan_account to receive notifications - it "returns false when the creation is by orphan_account" do - expect(subscription.valid_notification_entry?(create(:work, authors: [orphan_pseud]))).to be_falsey - end - it "returns false when the creation is hidden_by_admin" do expect(subscription.valid_notification_entry?(build(:work, hidden_by_admin: true))).to be_falsey end @@ -151,11 +150,6 @@ expect(subscription.valid_notification_entry?(build(:chapter, work: draft))).to be_falsey end - # TODO: AO3-3620 & AO3-5696: Allow subscriptions to orphan_account to receive notifications - it "returns false when the creation is by orphan_account" do - expect(subscription.valid_notification_entry?(create(:chapter, authors: [orphan_pseud]))).to be_falsey - end - it "returns false when the chapter is on a hidden work" do expect(subscription.valid_notification_entry?(build(:chapter, work: build(:work, hidden_by_admin: true)))).to be_falsey end @@ -205,7 +199,7 @@ end context "when subscribable is a user" do - let(:subscription) { build(:subscription, subscribable: create(:user)) } + let(:subscription) { build(:subscription, subscribable: author_pseud.user) } it "returns true for a non-anonymous work" do expect(subscription.valid_notification_entry?(work)).to be_truthy @@ -222,6 +216,31 @@ it "returns false for an anonymous chapter" do expect(subscription.valid_notification_entry?(anon_chapter)).to be_falsey end + + it "returns false for a work by a different user (orphan_account)" do + expect(subscription.valid_notification_entry?(create(:work, authors: [orphan_pseud]))).to be_falsey + end + + it "returns false for a chapter by a different user (orphan_account)" do + expect(subscription.valid_notification_entry?(create(:chapter, authors: [orphan_pseud]))).to be_falsey + end + + it "returns false for a work by a different user" do + expect(subscription.valid_notification_entry?(create(:work, authors: [create(:user).default_pseud]))).to be_falsey + end + + it "returns false for a chapter by a different user, even if the subscribed-to user is one of the work creators" do + # subscription is for author_pseud + author2 = create(:user).default_pseud + work = create(:work, authors: [author_pseud, author2]) + # chapter is by author2 only + chapter = create(:chapter, authors: [author2], work: work, position: 2) + expect(subscription.valid_notification_entry?(chapter)).to be_falsey + end + + it "returns false for a chapter by a different user, even if the user is one of the work creators" do + expect(subscription.valid_notification_entry?(create(:chapter, authors: [create(:user).default_pseud]))).to be_falsey + end end context "when subscribable is a work" do @@ -232,6 +251,11 @@ it "returns true for a non-anonymous chapter" do expect(subscription.valid_notification_entry?(chapter)).to be_truthy end + + # if the subscribable type is work, creation must be a chapter + it "returns false when the creation is a work" do + expect(subscription.valid_notification_entry?(work)).to be_falsey + end end context "when work is anonymous" do