diff --git a/app/controllers/subscriptions_controller.rb b/app/controllers/subscriptions_controller.rb index 0e4ddc2253a..6729df1328f 100644 --- a/app/controllers/subscriptions_controller.rb +++ b/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 diff --git a/app/models/subscription.rb b/app/models/subscription.rb index c3b077cffbc..8ad19d7ff2d 100644 --- a/app/models/subscription.rb +++ b/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 diff --git a/app/views/subscriptions/index.html.erb b/app/views/subscriptions/index.html.erb index b30076f5be2..df2152cf598 100644 --- a/app/views/subscriptions/index.html.erb +++ b/app/views/subscriptions/index.html.erb @@ -1,32 +1,35 @@

- <%= 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 %>

-

<%= ts('Navigation') %>

+

<%= t("a11y.navigation") %>

@@ -34,28 +37,31 @@ <%= will_paginate @subscriptions %> -

<%= ts('List of Subscriptions') %>

+

<%= t(".heading.landmark.list") %>

<% @subscriptions.each do |subscription| %>
- <%= 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 %>
<%= 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 %>
<% end %>
<%= will_paginate @subscriptions %> - \ No newline at end of file + diff --git a/config/locales/models/en.yml b/config/locales/models/en.yml index 49f2435dcb3..417c374fed6 100644 --- a/config/locales/models/en.yml +++ b/config/locales/models/en.yml @@ -235,3 +235,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 diff --git a/config/locales/views/en.yml b/config/locales/views/en.yml index 406a1112496..852cd9f97ab 100644 --- a/config/locales/views/en.yml +++ b/config/locales/views/en.yml @@ -780,6 +780,25 @@ en: skins: confirm_delete: confirm_html: Are you sure you want to delete 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}." diff --git a/spec/controllers/subscriptions_controller_spec.rb b/spec/controllers/subscriptions_controller_spec.rb new file mode 100644 index 00000000000..9d1cdae2750 --- /dev/null +++ b/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 diff --git a/spec/models/subscription_spec.rb b/spec/models/subscription_spec.rb index f2b001be161..e2ee6ef2e79 100644 --- a/spec/models/subscription_spec.rb +++ b/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