Don't raise internal error when handling non-user or request rules

Handle attempts to edit or destroy global or public body censor rules
with a notice and a redirect.

Closes #2009

2 files changed, 116 insertions, 52 deletions

diff --git a/app/controllers/admin_censor_rule_controller.rb b/app/controllers/admin_censor_rule_controller.rb
index d1a65ac09..3387fd832 100644
--- a/app/controllers/admin_censor_rule_controller.rb
+++ b/app/controllers/admin_censor_rule_controller.rb
@@ -7,6 +7,7 @@
 class AdminCensorRuleController < AdminController
 
     before_filter :set_editor, :only => [:create, :update]
+    before_filter :find_and_check_rule, :only => [:edit, :update, :destroy]
 
     def new
         if params[:request_id]
@@ -52,12 +53,9 @@ class AdminCensorRuleController < AdminController
     end
 
     def edit
-        @censor_rule = CensorRule.find(params[:id])
     end
 
     def update
-        @censor_rule = CensorRule.find(params[:id])
-
         if @censor_rule.update_attributes(params[:censor_rule])
 
             flash[:notice] = 'CensorRule was successfully updated.'
@@ -68,8 +66,6 @@ class AdminCensorRuleController < AdminController
             elsif @censor_rule.user
                 expire_requests_for_user(@censor_rule.user)
                 redirect_to admin_user_url(@censor_rule.user)
-            else
-                raise "internal error"
             end
 
         else
@@ -78,10 +74,8 @@ class AdminCensorRuleController < AdminController
     end
 
     def destroy
-        @censor_rule = CensorRule.find(params[:id])
         info_request = @censor_rule.info_request
         user = @censor_rule.user
-
         @censor_rule.destroy
 
         flash[:notice] = "CensorRule was successfully destroyed."
@@ -92,8 +86,6 @@ class AdminCensorRuleController < AdminController
         elsif user
             expire_requests_for_user(user) if user
             redirect_to admin_user_url(user)
-        else
-            raise "internal error"
         end
 
      end
@@ -103,5 +95,13 @@ class AdminCensorRuleController < AdminController
     def set_editor
         params[:censor_rule][:last_edit_editor] = admin_current_user
     end
+
+    def find_and_check_rule
+        @censor_rule = CensorRule.find(params[:id])
+        unless (@censor_rule.user || @censor_rule.info_request)
+            flash[:notice] = 'Only user and request censor rules can be edited'
+            redirect_to admin_general_index_path
+        end
+    end
 end
 
diff --git a/spec/controllers/admin_censor_rule_controller_spec.rb b/spec/controllers/admin_censor_rule_controller_spec.rb
index 3087af34a..68eaecd6a 100644
--- a/spec/controllers/admin_censor_rule_controller_spec.rb
+++ b/spec/controllers/admin_censor_rule_controller_spec.rb
@@ -241,81 +241,126 @@ describe AdminCensorRuleController do
 
     describe 'GET edit' do
 
-        before(:each) do
-            @censor_rule = FactoryGirl.create(:global_censor_rule)
-        end
+        context 'a CensorRule with an associated InfoRequest' do
+
+            before(:each) do
+                @censor_rule = FactoryGirl.create(:info_request_censor_rule)
+            end
+
+            it 'returns a successful response' do
+                get :edit, :id => @censor_rule.id
+                expect(response).to be_success
+            end
+
+            it 'renders the correct template' do
+                get :edit, :id => @censor_rule.id
+                expect(response).to render_template('edit')
+            end
+
+            it 'finds the correct censor rule to edit' do
+                get :edit, :id => @censor_rule.id
+                expect(assigns[:censor_rule]).to eq(@censor_rule)
+            end
 
-        it 'returns a successful response' do
-            get :edit, :id => @censor_rule.id
-            expect(response).to be_success
         end
 
-        it 'renders the correct template' do
-            get :edit, :id => @censor_rule.id
-            expect(response).to render_template('edit')
+        context 'a CensorRule with an associated User' do
+
+            before(:each) do
+                @censor_rule = FactoryGirl.create(:user_censor_rule)
+            end
+
+            it 'returns a successful response' do
+                get :edit, :id => @censor_rule.id
+                expect(response).to be_success
+            end
+
+            it 'renders the correct template' do
+                get :edit, :id => @censor_rule.id
+                expect(response).to render_template('edit')
+            end
+
+            it 'finds the correct censor rule to edit' do
+                get :edit, :id => @censor_rule.id
+                expect(assigns[:censor_rule]).to eq(@censor_rule)
+            end
+
         end
 
-        it 'finds the correct censor rule to edit' do
-            get :edit, :id => @censor_rule.id
-            expect(assigns[:censor_rule]).to eq(@censor_rule)
+        context 'when editing a global rule' do
+
+            before(:each) do
+                @censor_rule = FactoryGirl.create(:global_censor_rule)
+            end
+
+            it 'shows an error notice' do
+                get :edit, :id => @censor_rule.id
+                flash[:notice].should == 'Only user and request censor rules can be edited'
+            end
+
+            it 'redirects to the admin index' do
+                get :edit, :id => @censor_rule.id
+                expect(response).to redirect_to(admin_general_index_path)
+            end
+
         end
 
     end
 
     describe 'PUT update' do
 
-        before(:each) do
-            @censor_rule = FactoryGirl.create(:global_censor_rule)
-        end
+        context 'a global CensorRule' do
 
-        it 'finds the correct censor rule to edit' do
-            put :update, :id => @censor_rule.id,
-                         :censor_rule => { :text => 'different text' }
+            before(:each) do
+                @censor_rule = FactoryGirl.create(:global_censor_rule)
+            end
 
-            expect(assigns[:censor_rule]).to eq(@censor_rule)
-        end
+            it 'shows an error notice' do
+                get :edit, :id => @censor_rule.id
+                flash[:notice].should == 'Only user and request censor rules can be edited'
+            end
 
-        it 'sets the last_edit_editor to the current admin' do
-            put :update, :id => @censor_rule.id,
-                         :censor_rule => { :text => 'different text' }
+            it 'redirects to the admin index' do
+                get :edit, :id => @censor_rule.id
+                expect(response).to redirect_to(admin_general_index_path)
+            end
 
-            expect(assigns[:censor_rule].last_edit_editor).to eq('*unknown*')
         end
 
-        context 'successfully saving the censor rule' do
+        context 'a CensorRule with an associated InfoRequest' do
 
             before(:each) do
-                CensorRule.any_instance.stub(:save).and_return(true)
+                @censor_rule = FactoryGirl.create(:info_request_censor_rule)
             end
 
-            it 'updates the censor rule' do
-                pending("This raises an internal error in most cases")
+            it 'finds the correct censor rule to edit' do
                 put :update, :id => @censor_rule.id,
                              :censor_rule => { :text => 'different text' }
-                @censor_rule.reload
-                expect(@censor_rule.text).to eq('different text')
+
+                expect(assigns[:censor_rule]).to eq(@censor_rule)
             end
 
-            it 'confirms the censor rule is updated' do
-                pending("This raises an internal error in most cases")
+            it 'sets the last_edit_editor to the current admin' do
                 put :update, :id => @censor_rule.id,
                              :censor_rule => { :text => 'different text' }
 
-                msg = 'CensorRule was successfully updated.'
-                expect(flash[:notice]).to eq(msg)
+                expect(assigns[:censor_rule].last_edit_editor).to eq('*unknown*')
             end
 
-            it 'raises an error after updating the rule' do
-                expect {
+            context 'successfully saving the censor rule' do
+
+                it 'updates the censor rule' do
                     put :update, :id => @censor_rule.id,
                                  :censor_rule => { :text => 'different text' }
-                }.to raise_error 'internal error'
-            end
-
-            context 'a CensorRule with an associated InfoRequest' do
+                    @censor_rule.reload
+                    expect(@censor_rule.text).to eq('different text')
+                end
 
-                before(:each) do
-                    @censor_rule = FactoryGirl.create(:info_request_censor_rule)
+                it 'confirms the censor rule is updated' do
+                    put :update, :id => @censor_rule.id,
+                                 :censor_rule => { :text => 'different text' }
+                    msg = 'CensorRule was successfully updated.'
+                    expect(flash[:notice]).to eq(msg)
                 end
 
                  it 'purges the cache for the info request' do
@@ -380,6 +425,7 @@ describe AdminCensorRuleController do
                              :censor_rule => { :text => 'different text' }
 
                 expect(assigns[:censor_rule].last_edit_editor).to eq('*unknown*')
+
             end
 
 
@@ -438,12 +484,31 @@ describe AdminCensorRuleController do
                 end
 
              end
+
         end
 
     end
 
     describe 'DELETE destroy' do
 
+        context 'a global CensorRule' do
+
+            before(:each) do
+                @censor_rule = FactoryGirl.create(:global_censor_rule)
+            end
+
+            it 'shows an error notice' do
+                get :edit, :id => @censor_rule.id
+                flash[:notice].should == 'Only user and request censor rules can be edited'
+            end
+
+            it 'redirects to the admin index' do
+                get :edit, :id => @censor_rule.id
+                expect(response).to redirect_to(admin_general_index_path)
+            end
+
+        end
+
         context 'a CensorRule with an associated InfoRequest' do
 
             before(:each) do
@@ -522,5 +587,4 @@ describe AdminCensorRuleController, "when making censor rules from the admin int
         PurgeRequest.all().first.model_id.should == ir.id
     end
 
-
 end