On Tue, Nov 25, 2008 at 8:31 PM, LAN-SUN-LUK Benjamin
<Benjamin.LAN-SUN-LUK(a)supinfo.com> wrote:
Signed-off-by: Benjamin LAN-SUN-LUK <benjamin.lan-sun-luk(a)supinfo.com>
This patch won't apply. Can you rebase and resend it? But I do have
some comments for it before you resend.
---
app/controllers/sprints_controller.rb | 26 ++++++++++++++++++--------
1 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/app/controllers/sprints_controller.rb
b/app/controllers/sprints_controller.rb
index 09ebe5b..29e2d49 100644
--- a/app/controllers/sprints_controller.rb
+++ b/app/controllers/sprints_controller.rb
@@ -87,15 +87,25 @@ class SprintsController < ApplicationController
def update
respond_to do |format|
if @sprint.can_edit?(@user)
- @sprint.update_attributes(params[:sprint])
-
- if @sprint.save
- flash[:message] = "Sprint updated successfully."
- format.html { redirect_to params[:url] ? params[:url] :
product_sprints_path(@product) }
+ if params[:sprint][:status].to_i == Sprint::STATUS_PLANNED
What happens if the status param isn't present? Won't this result in a
nil pointer?
+ @sprint.backlog_items.each do |backlog_item|
+ unless backlog_item.tasks.empty?
+ flash[:error] = "You cannot change the status because you got
tasks against the sprint"
You can check this value more directly by checking
@sprint.actual_hours instead. If it's anything but zero then the
sprint has tasks against it. Since this is business logic, it should
be moved into the Sprint class itself. Maybe something more like:
def update
...
if @sprint.can_edit?(@user)
if @sprint.allowed_status?(params[:sprint][:status])
...
else
flash[:error] = "You cannot move the sprint into the selected status."
format.html { render :action => :edit }
end
end
...
end
And the Sprint::allowed_status? method would look like:
def allowed_status?(new_status)
# if the new status is not valid, then fail outright
return false unless (STATUS_PLANNED..STATUS_CANCELED).include? new_status
result = true
case new_status
when STATUS_PLANNED
result = false if actual_hours != 0
end
return result
end
+ format.html { redirect_to :action => :show }
+ end
+ end
else
- @title = "Sprint #{(a)sprint.id} (Edit)"
- @sprint.valid?
- format.html { render :action => :edit }
+
+ @sprint.update_attributes(params[:sprint])
+
+ if @sprint.save
+ flash[:message] = "Sprint updated successfully."
+ format.html { redirect_to params[:url] ? params[:url] :
product_sprints_path(@product) }
+ else
+ @title = "Sprint #{(a)sprint.id} (Edit)"
+ @sprint.valid?
+ format.html { render :action => :edit }
+ end
end
else
flash[:error] = "You are now allowed to edit sprints for
#{(a)product.name}."
On the sprint view page we should also exclude the Planned option if
the sprint has tasks:
<%= select :sprint, :status, Sprint::STATUS_TEXT -
(@sprint.has_activity? ? [Sprint::STATUS_PLANNED] : ['']) %>
--
Darryl L. Pierce <mcpierce(a)gmail.com>
Visit the Infobahn Offramp: <
http://mcpierce.multiply.com>
"Bury me next to my wife. Nothing too fancy..." - Ulysses S. Grant