From 6e80b256f18fa5b87856c213fd71cb2f419e16c0 Mon Sep 17 00:00:00 2001 From: Mason Ballengee Date: Wed, 16 Aug 2023 14:58:35 -0400 Subject: [PATCH 1/4] Use blank canvas to represent inaccesible playlist items --- app/controllers/playlists_controller.rb | 9 +- app/models/iiif_playlist_canvas_presenter.rb | 35 ++++- .../iiif_playlist_canvas_presenter_spec.rb | 122 +++++++++++++++++- 3 files changed, 158 insertions(+), 8 deletions(-) diff --git a/app/controllers/playlists_controller.rb b/app/controllers/playlists_controller.rb index e93ed6a690..654e18cf7f 100644 --- a/app/controllers/playlists_controller.rb +++ b/app/controllers/playlists_controller.rb @@ -247,10 +247,13 @@ def manifest authorize! :read, @playlist canvas_presenters = @playlist.items.collect do |item| - next if item.clip.master_file.nil? @master_file = item.clip.master_file - stream_info = secure_streams(@master_file.stream_details, @master_file.media_object_id) - IiifPlaylistCanvasPresenter.new(playlist_item: item, stream_info: stream_info) + if @master_file.nil? + stream_info = nil + else + stream_info = secure_streams(@master_file.stream_details, @master_file.media_object_id) + end + IiifPlaylistCanvasPresenter.new(playlist_item: item, stream_info: stream_info, user: current_user) end presenter = IiifPlaylistManifestPresenter.new(playlist: @playlist, items: canvas_presenters) diff --git a/app/models/iiif_playlist_canvas_presenter.rb b/app/models/iiif_playlist_canvas_presenter.rb index c3a2ec53b9..4f694a48cb 100644 --- a/app/models/iiif_playlist_canvas_presenter.rb +++ b/app/models/iiif_playlist_canvas_presenter.rb @@ -13,19 +13,26 @@ # --- END LICENSE_HEADER BLOCK --- class IiifPlaylistCanvasPresenter - attr_reader :playlist_item, :stream_info + attr_reader :playlist_item, :stream_info, :user attr_accessor :media_fragment - def initialize(playlist_item:, stream_info:, media_fragment: nil) + def initialize(playlist_item:, stream_info:, user: nil, media_fragment: nil) @playlist_item = playlist_item @stream_info = stream_info + @user = user @media_fragment = media_fragment end delegate :id, to: :playlist_item def to_s - playlist_item.title + if restricted? + "Restricted item" + elsif master_file.nil? + "Deleted item" + else + playlist_item.title + end end def master_file @@ -45,15 +52,37 @@ def range # @return [IIIFManifest::V3::DisplayContent] the display content required by the manifest builder. def display_content + return if restricted? || master_file.nil? master_file.is_video? ? video_content : audio_content end def annotation_content + return if restricted? || master_file.nil? playlist_item.marker.collect { |m| marker_content(m) } end + def placeholder_content + if restricted? + IIIFManifest::V3::DisplayContent.new( nil, + label: 'You do not have permission to playback this item.', + type: 'Text', + format: 'text/plain') + elsif master_file.nil? + IIIFManifest::V3::DisplayContent.new( nil, + label: 'The source for this playlist item has been deleted.', + type: 'Text', + format: 'text/plain') + end + end + private + def restricted? + if user.present? + Ability.new( User.find user.id ).cannot? :read, master_file + end + end + def video_content # @see https://github.com/samvera-labs/iiif_manifest stream_urls.collect { |quality, _url| video_display_content(quality) } diff --git a/spec/models/iiif_playlist_canvas_presenter_spec.rb b/spec/models/iiif_playlist_canvas_presenter_spec.rb index e336e86e3f..0b0b5f56ec 100644 --- a/spec/models/iiif_playlist_canvas_presenter_spec.rb +++ b/spec/models/iiif_playlist_canvas_presenter_spec.rb @@ -23,7 +23,33 @@ let(:stream_info) { master_file.stream_details } let(:presenter) { described_class.new(playlist_item: playlist_item, stream_info: stream_info) } - context 'auth_service' do + describe '#to_s' do + it 'returns the playlist_item label' do + expect(presenter.to_s).to eq playlist_item.title + end + + context 'when a user does not have access to a restricted item' do + let(:owner) { FactoryBot.create(:user) } + let(:user) { FactoryBot.create(:user) } + let(:media_object) { FactoryBot.create(:media_object, creator: owner, visibility: 'private') } + let(:presenter) { described_class.new(playlist_item: playlist_item, stream_info: stream_info, user: user) } + + it 'return "Restricted item"' do + expect(presenter.to_s).to eq "Restricted item" + end + end + + context 'when an item has been deleted' do + let(:master_file) { nil } + let(:stream_info) { nil } + + it 'returns "Deleted item"' do + expect(presenter.to_s).to eq "Deleted item" + end + end + end + + describe '#auth_service' do subject { presenter.display_content.first.auth_service } it 'provides a cookie auth service' do @@ -73,11 +99,31 @@ expect(subject.url).to include("#t=#{playlist_item.start_time / 1000},#{playlist_item.end_time / 1000}") end end + + context 'when a user does not have access to a restricted item' do + let(:owner) { FactoryBot.create(:user) } + let(:user) { FactoryBot.create(:user) } + let(:media_object) { FactoryBot.create(:media_object, creator: owner, visibility: 'private') } + let(:presenter) { described_class.new(playlist_item: playlist_item, stream_info: stream_info, user: user) } + + it 'does not serialize the content' do + expect(presenter.display_content).to be_nil + end + end + + context 'when an item has been deleted' do + let(:master_file) { nil } + let(:stream_info) { nil } + + it 'does not serialize the content' do + expect(presenter.display_content).to be_nil + end + end end describe '#annotation_content' do let(:marker) { FactoryBot.create(:avalon_marker) } - let(:playlist_item) { FactoryBot.create(:playlist_item, marker: [marker]) } + let(:playlist_item) { FactoryBot.build(:playlist_item, clip: playlist_clip, marker: [marker]) } subject { presenter.annotation_content } it "serializes playlist markers as iiif annotations" do @@ -95,6 +141,26 @@ it "includes 'highlighting' motivation" do expect(subject.first.motivation).to eq 'highlighting' end + + context 'when a user does not have access to a restricted item' do + let(:owner) { FactoryBot.create(:user) } + let(:user) { FactoryBot.create(:user) } + let(:media_object) { FactoryBot.create(:media_object, creator: owner, visibility: 'private') } + let(:presenter) { described_class.new(playlist_item: playlist_item, stream_info: stream_info, user: user) } + + it 'does not serialize the content' do + expect(presenter.annotation_content).to be_nil + end + end + + context 'when an item has been deleted' do + let(:master_file) { nil } + let(:stream_info) { nil } + + it 'does not serialize the content' do + expect(presenter.annotation_content).to be_nil + end + end end describe '#range' do @@ -115,4 +181,56 @@ expect(subject.any? { |po| po["@id"] =~ /media_objects\/#{media_object.id}\/manifest/ }).to eq true end end + + describe '#placeholder_contetnt' do + subject { presenter.placeholder_content } + + it 'does not generate a placeholder for non-restricted items' do + expect(subject).to be_nil + end + + context 'when a user does not have access to a restricted item' do + let(:owner) { FactoryBot.create(:user) } + let(:user) { FactoryBot.create(:user) } + let(:media_object) { FactoryBot.create(:media_object, creator: owner, visibility: 'private') } + let(:presenter) { described_class.new(playlist_item: playlist_item, stream_info: stream_info, user: user) } + + it 'generates placeholder canvas' do + expect(subject).to be_present + end + + it 'has format' do + expect(subject.format).to eq "text/plain" + end + + it 'has type' do + expect(subject.type).to eq "Text" + end + + it 'has label' do + expect(subject.label).to eq "You do not have permission to playback this item." + end + end + + context 'when an item has been deleted' do + let(:master_file) { nil } + let(:stream_info) { nil } + + it 'generated placeholder canvas' do + expect(subject).to be_present + end + + it 'has format' do + expect(subject.format).to eq "text/plain" + end + + it 'has type' do + expect(subject.type).to eq "Text" + end + + it 'has label' do + expect(subject.label).to eq "The source for this playlist item has been deleted." + end + end + end end From 000935d5f8f85960c51f54f3ec3fc535588a30b7 Mon Sep 17 00:00:00 2001 From: Mason Ballengee Date: Wed, 16 Aug 2023 15:23:08 -0400 Subject: [PATCH 2/4] Fixes for codeclimate --- app/controllers/playlists_controller.rb | 10 +++++----- app/models/iiif_playlist_canvas_presenter.rb | 21 ++++++++++---------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/app/controllers/playlists_controller.rb b/app/controllers/playlists_controller.rb index 654e18cf7f..d9da5bc68e 100644 --- a/app/controllers/playlists_controller.rb +++ b/app/controllers/playlists_controller.rb @@ -248,11 +248,11 @@ def manifest canvas_presenters = @playlist.items.collect do |item| @master_file = item.clip.master_file - if @master_file.nil? - stream_info = nil - else - stream_info = secure_streams(@master_file.stream_details, @master_file.media_object_id) - end + stream_info = if @master_file.nil? + nil + else + secure_streams(@master_file.stream_details, @master_file.media_object_id) + end IiifPlaylistCanvasPresenter.new(playlist_item: item, stream_info: stream_info, user: current_user) end presenter = IiifPlaylistManifestPresenter.new(playlist: @playlist, items: canvas_presenters) diff --git a/app/models/iiif_playlist_canvas_presenter.rb b/app/models/iiif_playlist_canvas_presenter.rb index 4f694a48cb..16b9eb44c7 100644 --- a/app/models/iiif_playlist_canvas_presenter.rb +++ b/app/models/iiif_playlist_canvas_presenter.rb @@ -63,24 +63,23 @@ def annotation_content def placeholder_content if restricted? - IIIFManifest::V3::DisplayContent.new( nil, - label: 'You do not have permission to playback this item.', - type: 'Text', - format: 'text/plain') + IIIFManifest::V3::DisplayContent.new(nil, + label: 'You do not have permission to playback this item.', + type: 'Text', + format: 'text/plain') elsif master_file.nil? - IIIFManifest::V3::DisplayContent.new( nil, - label: 'The source for this playlist item has been deleted.', - type: 'Text', - format: 'text/plain') + IIIFManifest::V3::DisplayContent.new(nil, + label: 'The source for this playlist item has been deleted.', + type: 'Text', + format: 'text/plain') end end private def restricted? - if user.present? - Ability.new( User.find user.id ).cannot? :read, master_file - end + return unless user.present? + Ability.new(User.find(user.id)).cannot? :read, master_file end def video_content From 2afc316ce25b122b42d187d22ca498b1aa6f7ba8 Mon Sep 17 00:00:00 2001 From: Mason Ballengee Date: Wed, 16 Aug 2023 17:01:42 -0400 Subject: [PATCH 3/4] Pass into presenter the result of ability check This commit also includes a small efficiencty improvement in the test suite. --- app/controllers/playlists_controller.rb | 3 ++- app/models/iiif_playlist_canvas_presenter.rb | 21 +++++++------------ .../iiif_playlist_canvas_presenter_spec.rb | 20 ++++-------------- 3 files changed, 14 insertions(+), 30 deletions(-) diff --git a/app/controllers/playlists_controller.rb b/app/controllers/playlists_controller.rb index d9da5bc68e..e7dd2786d4 100644 --- a/app/controllers/playlists_controller.rb +++ b/app/controllers/playlists_controller.rb @@ -247,13 +247,14 @@ def manifest authorize! :read, @playlist canvas_presenters = @playlist.items.collect do |item| + cannot_read_item = cannot? :read, @master_file @master_file = item.clip.master_file stream_info = if @master_file.nil? nil else secure_streams(@master_file.stream_details, @master_file.media_object_id) end - IiifPlaylistCanvasPresenter.new(playlist_item: item, stream_info: stream_info, user: current_user) + IiifPlaylistCanvasPresenter.new(playlist_item: item, stream_info: stream_info, cannot_read_item: cannot_read_item) end presenter = IiifPlaylistManifestPresenter.new(playlist: @playlist, items: canvas_presenters) diff --git a/app/models/iiif_playlist_canvas_presenter.rb b/app/models/iiif_playlist_canvas_presenter.rb index 16b9eb44c7..35afd1aee8 100644 --- a/app/models/iiif_playlist_canvas_presenter.rb +++ b/app/models/iiif_playlist_canvas_presenter.rb @@ -13,20 +13,20 @@ # --- END LICENSE_HEADER BLOCK --- class IiifPlaylistCanvasPresenter - attr_reader :playlist_item, :stream_info, :user + attr_reader :playlist_item, :stream_info, :cannot_read_item attr_accessor :media_fragment - def initialize(playlist_item:, stream_info:, user: nil, media_fragment: nil) + def initialize(playlist_item:, stream_info:, cannot_read_item: nil, media_fragment: nil) @playlist_item = playlist_item @stream_info = stream_info - @user = user + @cannot_read_item = cannot_read_item @media_fragment = media_fragment end delegate :id, to: :playlist_item def to_s - if restricted? + if cannot_read_item "Restricted item" elsif master_file.nil? "Deleted item" @@ -52,17 +52,17 @@ def range # @return [IIIFManifest::V3::DisplayContent] the display content required by the manifest builder. def display_content - return if restricted? || master_file.nil? + return if cannot_read_item || master_file.nil? master_file.is_video? ? video_content : audio_content end def annotation_content - return if restricted? || master_file.nil? + return if cannot_read_item || master_file.nil? playlist_item.marker.collect { |m| marker_content(m) } end def placeholder_content - if restricted? + if cannot_read_item IIIFManifest::V3::DisplayContent.new(nil, label: 'You do not have permission to playback this item.', type: 'Text', @@ -76,12 +76,7 @@ def placeholder_content end private - - def restricted? - return unless user.present? - Ability.new(User.find(user.id)).cannot? :read, master_file - end - + def video_content # @see https://github.com/samvera-labs/iiif_manifest stream_urls.collect { |quality, _url| video_display_content(quality) } diff --git a/spec/models/iiif_playlist_canvas_presenter_spec.rb b/spec/models/iiif_playlist_canvas_presenter_spec.rb index 0b0b5f56ec..371d46bbda 100644 --- a/spec/models/iiif_playlist_canvas_presenter_spec.rb +++ b/spec/models/iiif_playlist_canvas_presenter_spec.rb @@ -29,10 +29,7 @@ end context 'when a user does not have access to a restricted item' do - let(:owner) { FactoryBot.create(:user) } - let(:user) { FactoryBot.create(:user) } - let(:media_object) { FactoryBot.create(:media_object, creator: owner, visibility: 'private') } - let(:presenter) { described_class.new(playlist_item: playlist_item, stream_info: stream_info, user: user) } + let(:presenter) { described_class.new(playlist_item: playlist_item, stream_info: stream_info, cannot_read_item: true) } it 'return "Restricted item"' do expect(presenter.to_s).to eq "Restricted item" @@ -101,10 +98,7 @@ end context 'when a user does not have access to a restricted item' do - let(:owner) { FactoryBot.create(:user) } - let(:user) { FactoryBot.create(:user) } - let(:media_object) { FactoryBot.create(:media_object, creator: owner, visibility: 'private') } - let(:presenter) { described_class.new(playlist_item: playlist_item, stream_info: stream_info, user: user) } + let(:presenter) { described_class.new(playlist_item: playlist_item, stream_info: stream_info, cannot_read_item: true) } it 'does not serialize the content' do expect(presenter.display_content).to be_nil @@ -143,10 +137,7 @@ end context 'when a user does not have access to a restricted item' do - let(:owner) { FactoryBot.create(:user) } - let(:user) { FactoryBot.create(:user) } - let(:media_object) { FactoryBot.create(:media_object, creator: owner, visibility: 'private') } - let(:presenter) { described_class.new(playlist_item: playlist_item, stream_info: stream_info, user: user) } + let(:presenter) { described_class.new(playlist_item: playlist_item, stream_info: stream_info, cannot_read_item: true) } it 'does not serialize the content' do expect(presenter.annotation_content).to be_nil @@ -190,10 +181,7 @@ end context 'when a user does not have access to a restricted item' do - let(:owner) { FactoryBot.create(:user) } - let(:user) { FactoryBot.create(:user) } - let(:media_object) { FactoryBot.create(:media_object, creator: owner, visibility: 'private') } - let(:presenter) { described_class.new(playlist_item: playlist_item, stream_info: stream_info, user: user) } + let(:presenter) { described_class.new(playlist_item: playlist_item, stream_info: stream_info, cannot_read_item: true) } it 'generates placeholder canvas' do expect(subject).to be_present From be0545855921ff66976a03d1726a25d71a3a24f3 Mon Sep 17 00:00:00 2001 From: Mason Ballengee <68433277+masaball@users.noreply.github.com> Date: Fri, 18 Aug 2023 09:57:36 -0400 Subject: [PATCH 4/4] Change kwarg default to reflect boolean expectation Co-authored-by: Chris Colvard --- app/models/iiif_playlist_canvas_presenter.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/iiif_playlist_canvas_presenter.rb b/app/models/iiif_playlist_canvas_presenter.rb index 35afd1aee8..852107ffd6 100644 --- a/app/models/iiif_playlist_canvas_presenter.rb +++ b/app/models/iiif_playlist_canvas_presenter.rb @@ -16,7 +16,7 @@ class IiifPlaylistCanvasPresenter attr_reader :playlist_item, :stream_info, :cannot_read_item attr_accessor :media_fragment - def initialize(playlist_item:, stream_info:, cannot_read_item: nil, media_fragment: nil) + def initialize(playlist_item:, stream_info:, cannot_read_item: false, media_fragment: nil) @playlist_item = playlist_item @stream_info = stream_info @cannot_read_item = cannot_read_item