Download as
Browse Code »

Commit 62c67ea635f1cc467c3b3189b5c9aaff33d15480

Authored by Rafael Manzo
1 parent 792ed99d
Exists in colab and in 4 other branches master, spb-master, spb-stable, stable

Refactored ReadingsGroupsController 

set_reading_group on a before_action
ownership checkage semantically correct
Began to test concerns

Signed off by: Guilherme Rojas V. de Lima <guilhermehrojas@gmail.com>
Showing 7 changed files with 57 additions and 36 deletions   Show diff stats
app/controllers/concerns/ownership_authentication.rb
  Diff comments   View file @62c67ea
@@ -10,7 +10,15 @@ module OwnershipAuthentication @@ -10,7 +10,15 @@ module OwnershipAuthentication
10 end 10 end
11 11
12 def reading_group_owner? 12 def reading_group_owner?
13 - check_reading_group_ownership(params[:id]) 13 + if self.kind_of?(ReadingGroupsController)
  14 + id = params[:id]
  15 + elsif self.kind_of?(ReadingsController)
  16 + id = params[:reading_group_id]
  17 + else
  18 + raise "Not supported"
  19 + end
  20 +
  21 + check_reading_group_ownership(id)
14 end 22 end
15 23
16 def reading_owner? 24 def reading_owner?
app/controllers/reading_groups_controller.rb
  Diff comments   View file @62c67ea
1 include OwnershipAuthentication 1 include OwnershipAuthentication
2 2
3 class ReadingGroupsController < ApplicationController 3 class ReadingGroupsController < ApplicationController
4 - before_action :authenticate_user!,  
5 - except: [:index, :show] 4 + before_action :authenticate_user!, except: [:index, :show]
6 before_action :reading_group_owner?, only: [:edit, :update, :destroy] 5 before_action :reading_group_owner?, only: [:edit, :update, :destroy]
  6 + before_action :set_reading_group, only: [:show, :edit, :update, :destroy]
7 7
8 # GET /reading_groups/new 8 # GET /reading_groups/new
9 def new 9 def new
@@ -27,19 +27,13 @@ class ReadingGroupsController &lt; ApplicationController @@ -27,19 +27,13 @@ class ReadingGroupsController &lt; ApplicationController
27 27
28 # GET /reading_group/1 28 # GET /reading_group/1
29 # GET /reading_group/1.json 29 # GET /reading_group/1.json
30 - def show  
31 - set_reading_group  
32 - @reading_group_readings = @reading_group.readings  
33 - end 30 + def show; end
34 31
35 # GET /reading_groups/1/edit 32 # GET /reading_groups/1/edit
36 # GET /reading_groups/1/edit.json 33 # GET /reading_groups/1/edit.json
37 - def edit  
38 - set_reading_group  
39 - end 34 + def edit; end
40 35
41 def update 36 def update
42 - set_reading_group  
43 if @reading_group.update(reading_group_params) 37 if @reading_group.update(reading_group_params)
44 redirect_to(reading_group_path(@reading_group.id)) 38 redirect_to(reading_group_path(@reading_group.id))
45 else 39 else
@@ -50,7 +44,6 @@ class ReadingGroupsController &lt; ApplicationController @@ -50,7 +44,6 @@ class ReadingGroupsController &lt; ApplicationController
50 # DELETE /reading_group/1 44 # DELETE /reading_group/1
51 # DELETE /reading_group/1.json 45 # DELETE /reading_group/1.json
52 def destroy 46 def destroy
53 - set_reading_group  
54 current_user.reading_group_ownerships.find_by_reading_group_id(@reading_group.id).destroy 47 current_user.reading_group_ownerships.find_by_reading_group_id(@reading_group.id).destroy
55 @reading_group.destroy 48 @reading_group.destroy
56 respond_to do |format| 49 respond_to do |format|
@@ -60,26 +53,27 @@ class ReadingGroupsController &lt; ApplicationController @@ -60,26 +53,27 @@ class ReadingGroupsController &lt; ApplicationController
60 end 53 end
61 54
62 private 55 private
63 - # Use callbacks to share common setup or constraints between actions.  
64 - def set_reading_group  
65 - @reading_group = ReadingGroup.find(params[:id])  
66 - end 56 +
  57 + # Use callbacks to share common setup or constraints between actions.
  58 + def set_reading_group
  59 + @reading_group = ReadingGroup.find(params[:id])
  60 + end
67 61
68 - # Never trust parameters from the scary internet, only allow the white list through.  
69 - def reading_group_params  
70 - params[:reading_group]  
71 - end 62 + # Never trust parameters from the scary internet, only allow the white list through.
  63 + def reading_group_params
  64 + params[:reading_group]
  65 + end
72 66
73 - # Extracted code from create action  
74 - def create_and_redir(format)  
75 - if @reading_group.save  
76 - current_user.reading_group_ownerships.create reading_group_id: @reading_group.id 67 + # Extracted code from create action
  68 + def create_and_redir(format)
  69 + if @reading_group.save
  70 + current_user.reading_group_ownerships.create reading_group_id: @reading_group.id
77 71
78 - format.html { redirect_to reading_group_path(@reading_group.id), notice: 'Reading Group was successfully created.' }  
79 - format.json { render action: 'show', status: :created, location: @reading_group }  
80 - else  
81 - format.html { render action: 'new' }  
82 - format.json { render json: @reading_group.errors, status: :unprocessable_entity }  
83 - end 72 + format.html { redirect_to reading_group_path(@reading_group.id), notice: 'Reading Group was successfully created.' }
  73 + format.json { render action: 'show', status: :created, location: @reading_group }
  74 + else
  75 + format.html { render action: 'new' }
  76 + format.json { render json: @reading_group.errors, status: :unprocessable_entity }
84 end 77 end
  78 + end
85 end 79 end
app/controllers/readings_controller.rb
  Diff comments   View file @62c67ea
@@ -3,7 +3,8 @@ include OwnershipAuthentication @@ -3,7 +3,8 @@ include OwnershipAuthentication
3 class ReadingsController < ApplicationController 3 class ReadingsController < ApplicationController
4 before_action :authenticate_user!, except: [:index] 4 before_action :authenticate_user!, except: [:index]
5 before_action :set_reading, only: [:edit, :update, :destroy] 5 before_action :set_reading, only: [:edit, :update, :destroy]
6 - before_action :reading_owner?, except: [:new] 6 + before_action :reading_owner?, except: [:new, :create]
  7 + before_action :reading_group_owner?, only: [:new, :create]
7 8
8 def new 9 def new
9 @reading_group_id = params[:reading_group_id] 10 @reading_group_id = params[:reading_group_id]
app/views/reading_groups/show.html.erb
  Diff comments   View file @62c67ea
@@ -23,13 +23,14 @@ @@ -23,13 +23,14 @@
23 </thead> 23 </thead>
24 24
25 <tbody> 25 <tbody>
26 - <% if @reading_group_readings.size == 0 %> 26 + <% reading_group_readings = @reading_group.readings %>
  27 + <% if reading_group_readings.size == 0 %>
27 <tr> 28 <tr>
28 <% col_number = reading_groups_owner?(@reading_group.id) ? 5 : 3 %> 29 <% col_number = reading_groups_owner?(@reading_group.id) ? 5 : 3 %>
29 <td colspan="<%= col_number %>">There are no readings yet!</td> 30 <td colspan="<%= col_number %>">There are no readings yet!</td>
30 </tr> 31 </tr>
31 <% end %> 32 <% end %>
32 - <% @reading_group_readings.each do |reading| %> 33 + <% reading_group_readings.each do |reading| %>
33 <tr> 34 <tr>
34 <td><%= reading.label %></td> 35 <td><%= reading.label %></td>
35 <td><%= reading.grade %></td> 36 <td><%= reading.grade %></td>
spec/controllers/concerns/ownership_authentication_spec.rb 0 → 100644
  Diff comments   View file @62c67ea
@@ -0,0 +1,18 @@ @@ -0,0 +1,18 @@
  1 +require 'spec_helper'
  2 +
  3 +describe OwnershipAuthentication do
  4 + #TODO: test other methods
  5 +
  6 + describe 'reading_group_owner?' do
  7 + context 'Not ReadingGroupsController nor ReadingsController' do
  8 + before do
  9 + @projects_controller = ProjectsController.new # let doesn't work in here
  10 + @projects_controller.extend(OwnershipAuthentication)
  11 + end
  12 +
  13 + it 'should raise an exception' do
  14 + expect { @projects_controller.reading_group_owner? }.to raise_error("Not supported")
  15 + end
  16 + end
  17 + end
  18 +end
0 \ No newline at end of file 19 \ No newline at end of file
spec/controllers/reading_groups_controller_spec.rb
  Diff comments   View file @62c67ea
@@ -65,7 +65,6 @@ describe ReadingGroupsController do @@ -65,7 +65,6 @@ describe ReadingGroupsController do
65 let(:reading) { FactoryGirl.build(:reading) } 65 let(:reading) { FactoryGirl.build(:reading) }
66 before :each do 66 before :each do
67 ReadingGroup.expects(:find).with(subject.id.to_s).returns(subject) 67 ReadingGroup.expects(:find).with(subject.id.to_s).returns(subject)
68 - subject.expects(:readings).returns(reading)  
69 get :show, :id => subject.id 68 get :show, :id => subject.id
70 end 69 end
71 70
spec/controllers/readings_controller_spec.rb
  Diff comments   View file @62c67ea
@@ -10,7 +10,7 @@ describe ReadingsController do @@ -10,7 +10,7 @@ describe ReadingsController do
10 10
11 context 'when the current user owns the reading group' do 11 context 'when the current user owns the reading group' do
12 before :each do 12 before :each do
13 - subject.expects(:reading_owner?).returns true 13 + subject.expects(:reading_group_owner?).returns true
14 get :new, reading_group_id: reading_group.id 14 get :new, reading_group_id: reading_group.id
15 end 15 end
16 16
@@ -38,7 +38,7 @@ describe ReadingsController do @@ -38,7 +38,7 @@ describe ReadingsController do
38 38
39 context 'when the current user owns the reading group' do 39 context 'when the current user owns the reading group' do
40 before :each do 40 before :each do
41 - subject.expects(:reading_owner?).returns true 41 + subject.expects(:reading_group_owner?).returns true
42 end 42 end
43 43
44 context 'with valid fields' do 44 context 'with valid fields' do