-
Notifications
You must be signed in to change notification settings - Fork 74
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
Conversation
2f2a0a6
to
fa9f799
Compare
fa9f799
to
58fe723
Compare
58fe723
to
f6c8dbe
Compare
f6c8dbe
to
35727a0
Compare
35727a0
to
094dee0
Compare
094dee0
to
b624717
Compare
@akitaonrails @talyssonoc @adbatista There's a Review App up to tests. Please note, since it's rebased with master, it is running with all those dependencies updated. Lets try to merge it on monday, please. |
b624717
to
8b38fbb
Compare
|
||
def stories_from(start_at, end_at) | ||
stories.select do |story| | ||
story.accepted_at >= start_at && story.accepted_at <= end_at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it better to do these conditionals with a query instead of directly in Ruby side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dunno, I moved from a query because it was doing a lot of queries to get all this...
Only if we have a query to bring grouped those stories by start/end_at...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
def length | ||
(days_since_project_start / iteration_length_in_days).floor | ||
end | ||
|
||
def iteration_length_in_days | ||
@project.iteration_length * 7 | ||
project.iteration_length * 7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're already here, what do you think about extracting the 7
to a DAYS_IN_A_WEEK
constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
project | ||
.stories | ||
.with_dependencies | ||
.where('accepted_at >= ? AND accepted_at <= ?', start_date, end_date) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the same thing as https://github.com/Codeminer42/cm42-central/pull/379/files#diff-0648f597f84a74e685e5eabc0855e13cR31 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost, but for just one past_iteration...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app/operations/story_operations.rb
Outdated
.with_dependencies | ||
.where("state != 'accepted' OR accepted_at >= ?", current_iteration_start) | ||
.order('updated_at DESC') | ||
.limit(ENV['STORIES_CEILING'].presence&.to_i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about extracting the ENV['STORIES_CEILING'].presence&.to_i
to a method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to understand why this ceiling is there?
Active stories need to be limited?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm removing this. We are lazy loading done stories nowadays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As spoke offline, we won't need this limit
anymore.
.stories | ||
.with_dependencies | ||
.where(state: 'accepted') | ||
.where.not(accepted_at: nil) |
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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.
@stories ||= @project.stories.where( | ||
'accepted_at >= ? AND accepted_at <= ?', @start_date, @end_date | ||
) | ||
@points ||= stories.to_a.map(&:estimate).compact.sum |
There was a problem hiding this comment.
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)
?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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...
It was full of N+1 queries, and getting the things very slow
ef27aa5
to
ec30e09
Compare
end_date: end_date, | ||
project: @project, | ||
iteration_number: iteration_number + 1) | ||
start_at = start_date(iteration_number) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied a39d2c9
a39d2c9
to
cf3b208
Compare
It was full of N+1 queries, and letting the things very slow