Public recordings: Add foundation and structural changes. (#5227)

* Added recordings model visibility attribute checks.

* Reducing visibilities literals.

* Added expected scenarios to RecordingsController#update_visibility spec.

* Met expected scenarios.

* DRY improvements.

app/controllers/api/v1/recordings_controller.rb

@@ -65,17 +65,16 @@ module Api
       # POST /api/v1/recordings/update_visibility.json
       # Update's a recordings visibility by setting publish/unpublish and protected/unprotected
       def update_visibility
-        new_visibility = params[:visibility]
-        old_visibility = @recording.visibility
-        bbb_api = BigBlueButtonApi.new(provider: current_provider)
+        new_visibility = params[:visibility].to_s
 
-        bbb_api.publish_recordings(record_ids: @recording.record_id, publish: true) if old_visibility == 'Unpublished'
+        new_visibility_params = visibility_params_of(new_visibility)
 
-        bbb_api.publish_recordings(record_ids: @recording.record_id, publish: false) if new_visibility == 'Unpublished'
+        return render_error status: :bad_request if new_visibility_params.nil?
 
-        bbb_api.update_recordings(record_id: @recording.record_id, meta_hash: { protect: true }) if new_visibility == 'Protected'
+        bbb_api = BigBlueButtonApi.new(provider: current_provider)
 
-        bbb_api.update_recordings(record_id: @recording.record_id, meta_hash: { protect: false }) if old_visibility == 'Protected'
+        bbb_api.publish_recordings(record_ids: @recording.record_id, publish: new_visibility_params[:publish])
+        bbb_api.update_recordings(record_id: @recording.record_id, meta_hash: new_visibility_params[:meta_hash])
 
         @recording.update!(visibility: new_visibility)
 
@@ -114,6 +113,18 @@ module Api
       def find_recording
         @recording = Recording.find_by! record_id: params[:id]
       end
+
+      def visibility_params_of(visibility)
+        params_of = {
+          Recording::VISIBILITIES[:unpublished] => { publish: false, meta_hash: { protect: false, 'meta_gl-listed': false } },
+          Recording::VISIBILITIES[:published] => { publish: true, meta_hash: { protect: false, 'meta_gl-listed': false } },
+          Recording::VISIBILITIES[:public] => { publish: true, meta_hash: { protect: false, 'meta_gl-listed': true } },
+          Recording::VISIBILITIES[:protected] => { publish: true, meta_hash: { protect: true, 'meta_gl-listed': false } },
+          Recording::VISIBILITIES[:public_protected] => { publish: true, meta_hash: { protect: true, 'meta_gl-listed': true } }
+        }
+
+        params_of[visibility.to_s]
+      end
     end
   end
 end

app/models/recording.rb

@@ -17,6 +17,14 @@
 # frozen_string_literal: true
 
 class Recording < ApplicationRecord
+  VISIBILITIES = {
+    published: 'Published',
+    unpublished: 'Unpublished',
+    protected: 'Protected',
+    public: 'Public',
+    public_protected: 'Public/Protected'
+  }.freeze
+
   belongs_to :room
   has_one :user, through: :room
   has_many :formats, dependent: :destroy
@@ -26,6 +34,7 @@ class Recording < ApplicationRecord
   validates :visibility, presence: true
   validates :length, presence: true
   validates :participants, presence: true
+  validates :visibility, inclusion: VISIBILITIES.values
 
   scope :with_provider, ->(current_provider) { where(user: { provider: current_provider }) }
 

app/services/recording_creator.rb

@@ -49,11 +49,11 @@ class RecordingCreator
 
   # Returns the visibility of the recording (published, unpublished or protected)
   def get_recording_visibility(recording:)
-    return 'Protected' if recording[:protected].to_s == 'true'
+    return Recording::VISIBILITIES[:protected] if recording[:protected].to_s == 'true'
 
-    return 'Published' if recording[:published].to_s == 'true'
+    return Recording::VISIBILITIES[:published] if recording[:published].to_s == 'true'
 
-    'Unpublished'
+    Recording::VISIBILITIES[:unpublished]
   end
 
   # Returns the length of presentation recording for the recording given

spec/controllers/recordings_controller_spec.rb

@@ -194,66 +194,88 @@ RSpec.describe Api::V1::RecordingsController, type: :controller do
 
   describe '#update_visibility' do
     let(:room) { create(:room, user:) }
-    let(:published_recording) { create(:recording, room:, visibility: 'Published') }
-    let(:unpublished_recording) { create(:recording, room:, visibility: 'Unpublished') }
-    let(:protected_recording) { create(:recording, room:, visibility: 'Protected') }
+    let(:recording) { create(:recording, room:) }
 
-    it 'changes the recording from Published to Unpublished on the bbb server' do
-      expect_any_instance_of(BigBlueButtonApi).to receive(:publish_recordings).with(record_ids: published_recording.record_id, publish: false)
-      expect_any_instance_of(BigBlueButtonApi).not_to receive(:update_recordings)
-      post :update_visibility, params: { visibility: 'Unpublished', id: published_recording.record_id }
+    def expect_to_update_recording_props_to(publish:, protect:, list:, visibility:)
+      expect_any_instance_of(BigBlueButtonApi).to receive(:publish_recordings).with(record_ids: recording.record_id, publish:)
+      expect_any_instance_of(BigBlueButtonApi).to receive(:update_recordings).with(record_id: recording.record_id,
+                                                                                   meta_hash: {
+                                                                                     protect:, 'meta_gl-listed': list
+                                                                                   })
+
+      post :update_visibility, params: { visibility:, id: recording.record_id }
+
+      expect(recording.reload.visibility).to eq(visibility)
+      expect(response).to have_http_status(:ok)
     end
 
-    it 'changes the recording from Published to Protected on the bbb server' do
-      expect_any_instance_of(BigBlueButtonApi).to receive(:update_recordings).with(record_id: published_recording.record_id,
-                                                                                   meta_hash: { protect: true })
-      expect_any_instance_of(BigBlueButtonApi).not_to receive(:publish_recordings)
-      post :update_visibility, params: { visibility: 'Protected', id: published_recording.record_id }
+    it 'changes the recording visibility to "Published"' do
+      expect_to_update_recording_props_to(publish: true, protect: false, list: false, visibility: Recording::VISIBILITIES[:published])
     end
 
-    it 'changes the recording from Unpublished to Protected on the bbb server' do
-      expect_any_instance_of(BigBlueButtonApi).to receive(:publish_recordings).with(record_ids: unpublished_recording.record_id, publish: true)
-      expect_any_instance_of(BigBlueButtonApi).to receive(:update_recordings).with(record_id: unpublished_recording.record_id,
-                                                                                   meta_hash: { protect: true })
-      post :update_visibility, params: { visibility: 'Protected', id: unpublished_recording.record_id }
+    it 'changes the recording visibility to "Unpublished"' do
+      expect_to_update_recording_props_to(publish: false, protect: false, list: false, visibility: Recording::VISIBILITIES[:unpublished])
     end
 
-    it 'changes the recording from Unpublished to Published on the bbb server' do
-      expect_any_instance_of(BigBlueButtonApi).to receive(:publish_recordings).with(record_ids: unpublished_recording.record_id, publish: true)
-      expect_any_instance_of(BigBlueButtonApi).not_to receive(:update_recordings)
-      post :update_visibility, params: { visibility: 'Published', id: unpublished_recording.record_id }
+    it 'changes the recording visibility to "Protected"' do
+      expect_to_update_recording_props_to(publish: true, protect: true, list: false, visibility: Recording::VISIBILITIES[:protected])
     end
 
-    it 'changes the recording from Protected to Published on the bbb server' do
-      expect_any_instance_of(BigBlueButtonApi).not_to receive(:publish_recordings)
-      expect_any_instance_of(BigBlueButtonApi).to receive(:update_recordings).with(record_id: protected_recording.record_id,
-                                                                                   meta_hash: { protect: false })
-      post :update_visibility, params: { visibility: 'Published', id: protected_recording.record_id }
+    it 'changes the recording visibility to "Public"' do
+      expect_to_update_recording_props_to(publish: true, protect: false, list: true, visibility: Recording::VISIBILITIES[:public])
     end
 
-    it 'changes the recording from Protected to Unpublished on the bbb server' do
-      expect_any_instance_of(BigBlueButtonApi).to receive(:publish_recordings).with(record_ids: protected_recording.record_id, publish: false)
-      expect_any_instance_of(BigBlueButtonApi).to receive(:update_recordings).with(record_id: protected_recording.record_id,
-                                                                                   meta_hash: { protect: false })
-      post :update_visibility, params: { visibility: 'Unpublished', id: protected_recording.record_id }
+    it 'changes the recording visibility to "Public/Protected"' do
+      expect_to_update_recording_props_to(publish: true, protect: true, list: true, visibility: Recording::VISIBILITIES[:public_protected])
     end
 
-    it 'changes the local recording visibility' do
-      allow_any_instance_of(BigBlueButtonApi).to receive(:publish_recordings)
-      post :update_visibility, params: { visibility: 'Unpublished', id: unpublished_recording.record_id }
-      expect(unpublished_recording.reload.visibility).to eq('Unpublished')
+    context 'Unkown visibility' do
+      it 'returns :bad_request and does not update the recording' do
+        expect_any_instance_of(BigBlueButtonApi).not_to receive(:publish_recordings)
+        expect_any_instance_of(BigBlueButtonApi).not_to receive(:update_recordings)
+
+        expect do
+          post :update_visibility, params: { visibility: '404', id: recording.record_id }
+        end.not_to(change { recording.reload.visibility })
+
+        expect(response).to have_http_status(:bad_request)
+      end
+    end
+
+    context 'shared access' do
+      let(:signed_in_user) { create(:user) }
+      let(:recording) { create(:recording, room:, visibility: Recording::VISIBILITIES[:published]) }
+
+      before do
+        sign_in_user(signed_in_user)
       end
 
       it 'allows a shared user to update a recording visibility' do
-      shared_user = create(:user)
-      create(:shared_access, user_id: shared_user.id, room_id: room.id)
-      sign_in_user(shared_user)
+        create(:shared_access, user_id: signed_in_user.id, room_id: room.id)
+
+        expect_any_instance_of(BigBlueButtonApi).to receive(:publish_recordings).with(record_ids: recording.record_id, publish: false)
+        expect_any_instance_of(BigBlueButtonApi).to receive(:update_recordings).with(record_id: recording.record_id,
+                                                                                     meta_hash: {
+                                                                                       protect: false, 'meta_gl-listed': false
+                                                                                     })
+
+        expect do
+          post :update_visibility, params: { visibility: Recording::VISIBILITIES[:unpublished], id: recording.record_id }
+        end.to(change { recording.reload.visibility })
 
-      expect_any_instance_of(BigBlueButtonApi).to receive(:publish_recordings).with(record_ids: published_recording.record_id, publish: false)
+        expect(response).to have_http_status(:ok)
+      end
+
+      it 'disallows a none shared user to update a recording visibility' do
+        expect_any_instance_of(BigBlueButtonApi).not_to receive(:publish_recordings)
         expect_any_instance_of(BigBlueButtonApi).not_to receive(:update_recordings)
+
         expect do
-        post :update_visibility, params: { visibility: 'Unpublished', id: published_recording.record_id }
-      end.to(change { published_recording.reload.visibility })
+          post :update_visibility, params: { visibility: Recording::VISIBILITIES[:unpublished], id: recording.record_id }
+        end.not_to(change { recording.reload.visibility })
+
+        expect(response).to have_http_status(:forbidden)
+      end
     end
 
     # TODO: samuel - add tests for user_with_manage_recordings_permission

spec/factories/recording_factory.rb

@@ -21,7 +21,7 @@ FactoryBot.define do
     room
     name { Faker::Educator.course_name }
     record_id { Faker::Internet.uuid }
-    visibility { 'Unpublished' }
+    visibility { Recording::VISIBILITIES[:unpublished] }
     length { Faker::Number.within(range: 1..60) }
     participants { Faker::Number.within(range: 1..100) }
     recorded_at { Faker::Time.between(from: 2.days.ago, to: Time.zone.now) }

spec/models/recording_spec.rb

@@ -19,6 +19,20 @@
 require 'rails_helper'
 
 RSpec.describe Recording, type: :model do
+  describe 'Constants' do
+    context 'VISIBILITES' do
+      it 'matches certain map' do
+        expect(Recording::VISIBILITIES).to eq({
+                                                published: 'Published',
+                                                unpublished: 'Unpublished',
+                                                protected: 'Protected',
+                                                public: 'Public',
+                                                public_protected: 'Public/Protected'
+                                              })
+      end
+    end
+  end
+
   describe 'validations' do
     subject { create(:recording) }
 
@@ -29,6 +43,7 @@ RSpec.describe Recording, type: :model do
     it { is_expected.to validate_presence_of(:visibility) }
     it { is_expected.to validate_presence_of(:length) }
     it { is_expected.to validate_presence_of(:participants) }
+    it { is_expected.to validate_inclusion_of(:visibility).in_array(Recording::VISIBILITIES.values) }
   end
 
   describe 'scopes' do