Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor PastIteration mounting process #379

Merged
merged 3 commits into from
Sep 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
# Changelog

## [Unreleased]
### Added
- New endpoint to update batch stories
- Replace polling with Pusher to fetch project updates

### Fixed
- StoryOperations reading refactored to optimize queries and performance
- Fixes the project export process, to properly generate the downloadable CSV file

## [1.19.0] 2018-05-25
### Added
- New way to load past stories
Expand Down
24 changes: 12 additions & 12 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ gem 'jquery-atwho-rails'
gem 'jquery-ui-rails'
gem 'kaminari'
gem 'material_icons'
gem 'newrelic_rpm'
gem 'pg'
gem 'pg_search'
gem 'platform-api'
Expand All @@ -66,35 +67,34 @@ end
group :production do
gem 'kgio'
gem 'letsencrypt-rails-heroku'
gem 'newrelic_rpm'
gem 'rack-cache'
gem 'rack-timeout'
gem 'rails_12factor'
end

group :test do
gem 'rspec-rails'
gem 'rspec-its'
gem 'rspec-activemodel-mocks'
gem 'shoulda-matchers'
gem 'capybara'
gem 'poltergeist'
gem 'capybara-screenshot'
gem 'codeclimate-test-reporter', require: nil
gem 'database_cleaner'
gem 'factory_girl_rails'
gem 'codeclimate-test-reporter', require: nil
gem 'poltergeist'
gem 'rspec-activemodel-mocks'
gem 'rspec-its'
gem 'rspec-rails'
gem 'shoulda-matchers'
gem 'simplecov'
gem 'timecop'
gem 'vcr'
gem 'webmock'
gem 'timecop'
gem 'simplecov'
end

group :development do
gem 'better_errors'
gem 'binding_of_caller'
gem 'bullet'
gem 'letter_opener'
gem 'letter_opener_web', '~> 1.3.4'
gem "better_errors"
gem "binding_of_caller"
gem "bullet"
gem 'rubocop', '0.49.1'
end

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/stories_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ def select_stories_by_params
elsif params[:label]
StorySearch.labels(policy_scope(Story), params[:label])
else
StoryOperations::ReadAll.call(project: @project)
@project.stories.with_dependencies
end
end

Expand Down
22 changes: 7 additions & 15 deletions app/models/iterations/past_iteration.rb
Original file line number Diff line number Diff line change
@@ -1,23 +1,15 @@
module Iterations
class PastIteration
attr_reader :start_date, :end_date, :points, :iteration_number
include Virtus.model

def initialize(start_date:, end_date:, project:, iteration_number: nil)
@start_date = start_date
@end_date = end_date
@project = project
@points = points
@iteration_number = iteration_number
end
attribute :start_date, Date
attribute :end_date, Date
attribute :iteration_number, Integer
attribute :stories, Array[Story]
attribute :points, Integer

def points
@points ||= stories.sum(:estimate)
end

def stories
@stories ||= @project.stories.where(
'accepted_at >= ? AND accepted_at <= ?', @start_date, @end_date
)
@points ||= stories.to_a.map(&:estimate).compact.sum
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not stories.sum(:estimate) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not every story has value in estimate, so I'm doing a map, compacting to remove nil values, and summing after.

I can change if you have a better approach...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for clarification:

to_a here is only to always ensure we are doing it in an array, never into database. I'm avoiding queries here...

end
end
end
45 changes: 35 additions & 10 deletions app/models/iterations/project_iterations.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
module Iterations
class ProjectIterations
DAYS_IN_A_WEEK = 7

def initialize(project:)
@project = project
end
Expand All @@ -10,35 +12,58 @@ def current_iteration_start

def past_iterations
(0...length).map do |iteration_number|
start_date = start_date(iteration_number)
end_date = end_date(start_date)
PastIteration.new(start_date: start_date,
end_date: end_date,
project: @project,
iteration_number: iteration_number + 1)
start_at = start_date(iteration_number)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodrigovirgilio apply beginning_of_day and end_of_day in those methods

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied a39d2c9

end_at = end_date(start_at)

PastIteration.new(
start_date: start_at,
end_date: end_at,
stories: stories_from(start_at, end_at),
iteration_number: iteration_number + 1
)
end
end

private

attr_reader :project

def stories_from(start_at, end_at)
stories.select do |story|
story.accepted_at.to_date >= start_at && story.accepted_at.to_date <= end_at
end
end

def stories
@stories ||= begin
project
.stories
.with_dependencies
.where(state: 'accepted')
.where.not(accepted_at: nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that chaining where calls will create an AND instead of an OR, right? If so, does it make sense to have stories that are accepted but have no accepted_at value? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, makes sense, but I would keep it to prevent any possible inconsistence. It reflects exactly the kind of stories I'm expecting in the filter method above.

.order(:accepted_at)
end
end

def length
(days_since_project_start / iteration_length_in_days).floor
end

def iteration_length_in_days
@project.iteration_length * 7
project.iteration_length * DAYS_IN_A_WEEK
end

def project_start_date
@project.start_date
project.start_date
end

def start_date(iteration_number)
project_start_date + (iteration_number * iteration_length_in_days)
iteration_days = (iteration_number * iteration_length_in_days)
(project_start_date + iteration_days)
end

def end_date(start_date)
start_date + iteration_length_in_days - 1
(start_date + (iteration_length_in_days - 1))
end

def days_since_project_start
Expand Down
25 changes: 21 additions & 4 deletions app/operations/iteration_operations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,33 @@ def initialize(start_date:, end_date:, project:)

def run
{
stories: stories
stories: past_iteration.stories
}
end

private

attr_reader :project, :start_date, :end_date

def past_iteration
Iterations::PastIteration.new(
start_date: start_date,
end_date: end_date,
stories: stories
)
end

def stories
Iterations::PastIteration.new(start_date: @start_date,
end_date: @end_date,
project: @project).stories
@stories ||= begin
project
.stories
.with_dependencies
.where(
'accepted_at >= ? AND accepted_at <= ?',
start_date.beginning_of_day,
end_date.end_of_day
)
end
end
end
end
23 changes: 14 additions & 9 deletions app/operations/story_operations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,34 +67,39 @@ def after_save
class DestroyAll < BaseOperations::DestroyAll; end

class ReadAll
delegate :past_iterations, :current_iteration_start, to: :iterations

def self.call(*args)
new(*args).run
end

def initialize(project:)
@story_scope = project.stories.with_dependencies
@project = project
@iterations = Iterations::ProjectIterations.new(project: project)
end

def run
{
active_stories: active_stories,
past_iterations: @iterations.past_iterations
past_iterations: past_iterations
}
end

private

attr_reader :project

def active_stories
order(@story_scope.where("state != 'accepted' OR
accepted_at >= ?", @iterations.current_iteration_start))
@active_stories ||= begin
project
.stories
.with_dependencies
.where("state != 'accepted' OR accepted_at >= ?", current_iteration_start)
.order('updated_at DESC')
end
end

def order(query)
query.order('updated_at DESC').tap do |relation|
relation.limit(ENV['STORIES_CEILING']) if ENV['STORIES_CEILING']
end
def iterations
@iterations ||= Iterations::ProjectIterations.new(project: project)
end
end
end
6 changes: 3 additions & 3 deletions spec/controllers/stories_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,16 +77,16 @@
end

before do
create_list(:story, 3, :done, project: project, requested_by: user)

sign_in user
end

describe '#index' do
let(:stories) { { active_stories: project.stories, past_iterations: [] } }

specify do
xhr :get, :index, project_id: project.id
expect(response).to be_success
expect(response.body).to eq(stories.to_json)
expect(response.body).to eq(project.stories.to_json)
end
end

Expand Down
19 changes: 10 additions & 9 deletions spec/models/iterations/past_iteration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,29 @@
module Iterations
describe PastIteration do
let(:user) { create(:user, :with_team) }

let(:project) do
create(:project, :with_past_iteration, users: [user], teams: [user.teams.first])
end

let(:stories) do
create_list(:story, 3, :done, project: project, requested_by: user)
end

let(:past_iteration_params) do
{ start_date: project.created_at, end_date: project.created_at + 7.days, project: project }
{
start_date: project.created_at,
end_date: project.created_at + 7.days,
stories: stories
}
end

subject { described_class.new(past_iteration_params) }

let!(:stories) { create_list(:story, 3, :done, project: project, requested_by: user) }

describe '#points' do
it 'sums the story estimates in this iteration' do
expect(subject.points).to eq(24)
end
end

describe '#stories' do
it 'returns the stories in the iteration' do
expect(subject.stories).to eq(stories)
end
end
end
end
2 changes: 1 addition & 1 deletion spec/operations/iteration_operations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
create(:project, :with_past_iteration, users: [user], teams: [user.teams.first])
end
let(:user) { create(:user, :with_team) }
let(:stories) { create_list(:story, 3, :done, project: project, requested_by: user) }
let(:start_date) { project.created_at.to_date }
let(:end_date) { ((project.created_at + project.iteration_length * 7.days) - 1.day).to_date }
let!(:stories) { create_list(:story, 3, :done, project: project, requested_by: user) }

subject { IterationOperations::Read }
let(:result) { subject.call(start_date: start_date, end_date: end_date, project: project) }
Expand Down
3 changes: 2 additions & 1 deletion spec/operations/story_operations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@ def expect_past_iteration_attrs(subject_past_iteration, past_iteration)
expect(subject_past_iteration.start_date).to eq(past_iteration.start_date)
expect(subject_past_iteration.end_date).to eq(past_iteration.end_date)
expect(subject_past_iteration.points).to eq(past_iteration.points)
expect(subject_past_iteration.stories).to eq(past_iteration.stories)
expect(subject_past_iteration.iteration_number).to eq(1)
end

Expand All @@ -416,7 +417,7 @@ def expect_past_iteration_attrs(subject_past_iteration, past_iteration)
iteration_end = ((project.created_at + project.iteration_length * 7.days) - 1.day).to_date
Iterations::PastIteration.new(start_date: iteration_start,
end_date: iteration_end,
project: project)
stories: [done_story])
end

subject { StoryOperations::ReadAll }
Expand Down