AO3-4728 Fix Error 500 on subscriptions page for invalid items (#4816)

* AO3-4728: Don't 500 on invalid subscribable

* AO3-4728: PR feedback

* AO3-4728: lint

* AO3-4728: fix test expectation

* AO3-4728: adding controller tests

* AO3-4728: lint

* AO3-4728 Tidy and apply review feedback

* AO3-4728 Tidy and fix subtle logic mistake

* AO3-4728 Try to fix type params bug

* AO3-4728 Feedback

---------

Co-authored-by: tlee911 <tlee911@users.noreply.github.com>

app/controllers/subscriptions_controller.rb

@@ -15,9 +15,12 @@ def load_user
   # GET /subscriptions.xml
   def index
     @subscriptions = @user.subscriptions.includes(:subscribable)
-    if params[:type].present?
-      @subscriptions = @subscriptions.where(subscribable_type: params[:type].classify)
+
+    if params[:type] && Subscription::VALID_SUBSCRIBABLES.include?(params[:type].singularize.titleize)
+      @subscribable_type = params[:type]
+      @subscriptions = @subscriptions.where(subscribable_type: @subscribable_type.classify)
     end
+
     @subscriptions = @subscriptions.to_a.sort { |a,b| a.name.downcase <=> b.name.downcase }
     @subscriptions = @subscriptions.paginate page: params[:page], per_page: ArchiveConfig.ITEMS_PER_PAGE
   end

app/models/subscription.rb

@@ -11,7 +11,7 @@ class Subscription < ApplicationRecord
   # if there's an invalid subscribable type
   validates :subscribable, presence: true,
                            if: proc { |s| VALID_SUBSCRIBABLES.include?(s.subscribable_type) }
-  
+
   # Get the subscriptions associated with this work
   # currently: users subscribed to work, users subscribed to creator of work
   scope :for_work, lambda {|work|
@@ -32,6 +32,8 @@ def name
       subscribable.name
     elsif subscribable.respond_to?(:title)
       subscribable.title
+    else
+      I18n.t("subscriptions.deleted")
     end
   end
 

app/views/subscriptions/index.html.erb

@@ -1,61 +1,67 @@
 <!--Descriptive page name, messages and instructions-->
 <h2 class="heading">
-  <%= params[:type].blank? ? ts('My Subscriptions') : 
-  ts('My %{type} Subscriptions', type: params[:type].to_s.singularize.titleize)
-  %>
+  <% if @subscribable_type %>
+    <%= t(".page_heading.#{@subscribable_type}") %>
+  <% else %>
+    <%= t(".page_heading.all") %>
+  <% end %>
 </h2>
 <!--/descriptions-->
 
 <!--subnav-->
-<h3 class="landmark heading"><%= ts('Navigation') %></h3>
+<h3 class="landmark heading"><%= t("a11y.navigation") %></h3>
 <ul class="navigation actions" role="navigation">
   <li>
-    <%= span_if_current ts('All Subscriptions'),
-    user_subscriptions_path(@user), params[:type].blank? %>
+    <%= span_if_current(t(".navigation.all"),
+          user_subscriptions_path(@user),
+          @subscribable_type.blank?) %>
   </li>
   <li>
-    <%= span_if_current ts('Series Subscriptions'),
-    user_subscriptions_path(@user, :type => "series"), 
-    params[:type] == "series" %>
+    <%= span_if_current(t(".navigation.series"),
+          user_subscriptions_path(@user, type: "series"),
+          @subscribable_type == "series") %>
   </li>
   <li>
-    <%= span_if_current ts('User Subscriptions'),
-    user_subscriptions_path(@user, :type => "users"), 
-    params[:type] == "users" %>
+    <%= span_if_current(t(".navigation.user"),
+          user_subscriptions_path(@user, type: "users"),
+          @subscribable_type == "users") %>
   </li>
   <li>
-    <%= span_if_current ts('Work Subscriptions'),
-    user_subscriptions_path(@user, :type => "works"), 
-    params[:type] == "works" %>
+    <%= span_if_current(t(".navigation.work"),
+          user_subscriptions_path(@user, type: "works"),
+          @subscribable_type == "works") %>
   </li>
 </ul>
 <!--/subnav-->
 
 <!--main content-->
 <%= will_paginate @subscriptions %>
 
-<h3 class="landmark heading"><%= ts('List of Subscriptions') %></h3>
+<h3 class="landmark heading"><%= t(".heading.landmark.list") %></h3>
 <dl class="subscription index group">
   <% @subscriptions.each do |subscription| %>
     <dt>
-      <%= link_to(subscription.name, subscription.subscribable) %>
-      <% if params[:type].blank? && subscription.subscribable_type == "Work" %>
-        <%= ts('(Work)') %>
-      <% end %>
-      <% if params[:type].blank? && subscription.subscribable_type == "Series" %>
-        <%= ts('(Series)') %>
-      <% end %>
-      <% unless subscription.subscribable_type == "User" %>
-        <%= ts('by %{creators}', :creators => byline(subscription.subscribable)).html_safe %>
+      <% if subscription.subscribable %>
+        <%= link_to(subscription.name, subscription.subscribable) %>
+        <% case subscription.subscribable_type %>
+        <% when "Work" %>
+          <%= t(".work") if @subscribable_type.blank? %>
+          <%= t(".byline", creators: byline(subscription.subscribable)) %>
+        <% when "Series" %>
+          <%= t(".series") if @subscribable_type.blank? %>
+          <%= t(".byline", creators: byline(subscription.subscribable)) %>
+        <% end %>
+      <% else %>
+        <%= subscription.name %>
       <% end %>
     </dt>
     <dd>
       <%= form_for [current_user, subscription], :html => {:method => :delete} do |f| %>
-        <%= f.submit ts('Unsubscribe from %{name}', :name => subscription.name).html_safe %>
+        <%= f.submit t(".button_html", name: subscription.name) %>
       <% end %>
     </dd>
   <% end %>
 </dl>
 
 <%= will_paginate @subscriptions %>
-<!--/content-->
+<!--/content-->

config/locales/models/en.yml

@@ -236,3 +236,5 @@ en:
         closed_ticket: must not be closed.
         invalid_department: must be in your department.
         required: must exist and not be spam.
+  subscriptions:
+    deleted: Deleted item

config/locales/views/en.yml

@@ -852,6 +852,25 @@ en:
   skins:
     confirm_delete:
       confirm_html: Are you sure you want to <strong><em>delete</em></strong> the skin "%{skin_title}"?
+  subscriptions:
+    index:
+      button_html: Unsubscribe from %{name}
+      byline: by %{creators}
+      heading:
+        landmark:
+          list: List of Subscriptions
+      navigation:
+        all: All Subscriptions
+        series: Series Subscriptions
+        user: User Subscriptions
+        work: Work Subscriptions
+      page_heading:
+        all: My Subscriptions
+        series: My Series Subscriptions
+        users: My User Subscriptions
+        works: My Work Subscriptions
+      series: "(Series)"
+      work: "(Work)"
   tag_wranglers:
     show:
       last_wrangled_html: "%{wrangler_login} last wrangled at %{time}."

spec/controllers/subscriptions_controller_spec.rb

@@ -0,0 +1,79 @@
+require "spec_helper"
+
+describe SubscriptionsController do
+  include LoginMacros
+  include RedirectExpectationHelper
+
+  let(:user) { create(:user) }
+
+  describe "GET #index" do
+    let(:author) { create(:user) }
+    let(:work) { create(:work) }
+    let(:series) { create(:series) }
+    let!(:sub_series) { create(:subscription, user: user, subscribable: series) }
+    let!(:sub_work) { create(:subscription, user: user, subscribable_type: "Work", subscribable_id: work.id) }
+    let!(:sub_user) { create(:subscription, user: user, subscribable_type: "User", subscribable_id: author.id) }
+
+    it "redirects to login when not logged in" do
+      get :index, params: { user_id: user.login }
+      it_redirects_to_with_error(new_user_session_path,
+                                 "Sorry, you don't have permission to access the page you were trying to reach. Please log in.")
+    end
+
+    context "when logged in" do
+      before { fake_login_known_user(user) }
+
+      it "renders all subscriptions" do
+        get :index, params: { user_id: user.login }
+        expect(response).to render_template("index")
+        expect(assigns(:subscriptions)).to contain_exactly(sub_series, sub_work, sub_user)
+      end
+
+      context "with invalid subscriptions" do
+        let!(:subscription) { create(:subscription, user: user) }
+
+        before do
+          Subscription.update_all(subscribable_id: -1)
+        end
+
+        it "renders all subscriptions" do
+          get :index, params: { user_id: user.login }
+          expect(response).to render_template("index")
+
+          bad_sub = assigns(:subscriptions)[0]
+          expect(bad_sub.subscribable_id).to eq(-1)
+          expect(bad_sub.name).to eq("Deleted item")
+        end
+
+        it "allows deletion of invalid subscriptions" do
+          get :index, params: { user_id: user.login }
+          sub_id = assigns(:subscriptions)[0].id
+          delete :destroy, params: { user_id: user.login, id: sub_id }
+          it_redirects_to_with_notice(user_subscriptions_path(user), "You have successfully unsubscribed from Deleted item.")
+        end
+      end
+
+      context "with valid subscription types in params" do
+        %w[series works users].each do |sub_type|
+          it "renders #{sub_type} subscriptions only" do
+            get :index, params: { user_id: user.login, type: sub_type }
+
+            expect(response).to render_template("index")
+            expect(assigns(:subscribable_type)).to eq(sub_type)
+            expect(assigns(:subscriptions).count).to eq(1)
+          end
+        end
+      end
+
+      context "with invalid subscription type in params" do
+        it "renders all subscriptions" do
+          get :index, params: { user_id: user.login, type: "Invalid" }
+
+          expect(response).to render_template("index")
+          expect(assigns(:subscribable_type)).to be_nil
+          expect(assigns(:subscriptions)).to contain_exactly(sub_series, sub_work, sub_user)
+        end
+      end
+    end
+  end
+end

spec/models/subscription_spec.rb

@@ -2,13 +2,20 @@
 
 describe Subscription do
   let(:subscription) { build(:subscription) }
+  let(:work) { create(:work) }
+  let(:series) { create(:series) }
+  let(:user) { create(:user) }
 
   context "to a work" do
     before do
-      subscription.subscribable = create(:work)
+      subscription.subscribable = work
       subscription.save!
     end
 
+    it "has a name" do
+      expect(subscription.name).to eq(work.title)
+    end
+
     describe "when the work is destroyed" do
       before do
         subscription.subscribable.destroy
@@ -22,10 +29,14 @@
 
   context "to a series" do
     before do
-      subscription.subscribable = create(:series)
+      subscription.subscribable = series
       subscription.save!
     end
 
+    it "has a name" do
+      expect(subscription.name).to eq(series.title)
+    end
+
     describe "when the series is destroyed" do
       before do
         subscription.subscribable.destroy
@@ -39,10 +50,14 @@
 
   context "to a user" do
     before do
-      subscription.subscribable = create(:user)
+      subscription.subscribable = user
       subscription.save!
     end
 
+    it "has a name" do
+      expect(subscription.name).to eq(user.login)
+    end
+
     describe "when the user is destroyed" do
       before do
         subscription.subscribable.destroy
@@ -65,6 +80,10 @@
     it "should be invalid" do
       expect(subscription.valid?).to be_falsey
     end
+
+    it "has a name" do
+      expect(subscription.name).to eq("Deleted item")
+    end
   end
 
   context "when subscribable is not a valid object to subscribe to" do