Re: [PROJXP] [PATCH] If a sprint is active and has no tasks against it, then it can be moved back to the planned state. #71

[Darryl L. Pierce]

On Tue, Nov 25, 2008 at 8:31 PM, LAN-SUN-LUK Benjamin
<Benjamin.LAN-SUN-LUK(a)supinfo.com&gt; wrote:
>  Signed-off-by: Benjamin LAN-SUN-LUK <benjamin.lan-sun-luk(a)supinfo.com&gt; 
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}.&quot; 
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&gt;
Visit the Infobahn Offramp: <http://mcpierce.multiply.com>
"Bury me next to my wife. Nothing too fancy..." - Ulysses S. Grant