added 284 commits

• f9fe4593...e34923aa - 272 commits from branch master
• 101a3b66 - Merge branch '23674-simplify-milestone-summary-ee' into 91-milestone-burndown-charts
• b3d1dd1e - create burndown chart template and js bundle
• 974e2fa7 - implement burndown chart hint message
• 89b80350 - move burndown chart before user action hints
• 6e57e5a9 - implement burndown chart basic outline
• bfe0a623 - correctly handle empty array passed to setData
• ae6ce624 - render burndown chart lines
• 9a877250 - allow burndown chart to resize during animations
• 4a372b5a - defer render call to the next animation frame to avoid layout thrashing
• 9c078713 - fix axis font size in burndown chart
• 85e68eba - add overflow to the x-axis domain
• 6b5fdb52 - create x-axis line in constructor instead of regenerating on each render

Compare with previous version

added 15 commits

• 6b5fdb52...2d9fbfdc - 4 commits from branch master
• dfe504ea - create burndown chart template and js bundle
• 43e08f9a - implement burndown chart hint message
• 48cbca5c - move burndown chart before user action hints
• 20247f10 - implement burndown chart basic outline
• 51fd4c93 - correctly handle empty array passed to setData
• 78571b3d - render burndown chart lines
• 25007294 - allow burndown chart to resize during animations
• 54df6a4d - defer render call to the next animation frame to avoid layout thrashing
• 0df65beb - fix axis font size in burndown chart
• c736d11d - add overflow to the x-axis domain
• 42a8387a - create x-axis line in constructor instead of regenerating on each render

Compare with previous version

added 1 commit

• 73ea691d - add label to the y-axis

Compare with previous version

added 1 commit

• 5e77568d - ideal line should end at dueDate not maxDate in event that milestone goes overdue

Compare with previous version

added 2 commits

• a4d995c0 - create burndown chart legend
• 43efea41 - add header to burndown chart and adjust chart size breakpoints

Compare with previous version

added 1 commit

• cba3a5f7 - add 800ms animation to the line path whenever setData is called

Compare with previous version

added 1 commit

• 47a5821c - implement first pass at tooltips feature

Compare with previous version

marked the task add header as completed

marked the task add chart legend as completed

marked the task add y-axis label as completed

added 1 commit

• 9a894109 - improve tooltip design and placement

Compare with previous version

added 1 commit

• a53bd8c8 - improve tooltip design and placement

Compare with previous version

added 1 commit

• 839aaa36 - hide tooltips during chart animation

Compare with previous version

added 2 commits

• e827923e - rename resize and animate methods for clarity
• 0578ee29 - remove posessives in comment blocks

Compare with previous version

marked the task add tooltips as completed

added 3 commits

• 4293a046 - rename and reorder additional methods for clarity
• 4f187dbd - update tooltip label through setData options
• 6f13b46d - fix phantomjs mishandling of svgs with implicit dimensions

Compare with previous version

added 239 commits

• 6f13b46d...cf91b76a - 215 commits from branch master
• 78fd5405 - create burndown chart template and js bundle
• 6e4361c4 - implement burndown chart hint message
• b3a0db53 - move burndown chart before user action hints
• bd08a573 - implement burndown chart basic outline
• c6084c94 - correctly handle empty array passed to setData
• 57e38c36 - render burndown chart lines
• 88ced2e6 - allow burndown chart to resize during animations
• 3940508a - defer render call to the next animation frame to avoid layout thrashing
• 2feaf499 - fix axis font size in burndown chart
• dc8e620c - add overflow to the x-axis domain
• f1363e33 - create x-axis line in constructor instead of regenerating on each render
• c3fd9c7e - add label to the y-axis
• f01c25e0 - ideal line should end at dueDate not maxDate in event that milestone goes overdue
• c7ade5b9 - create burndown chart legend
• 8fbda9f9 - add header to burndown chart and adjust chart size breakpoints
• ee9318d5 - add 800ms animation to the line path whenever setData is called
• 3750a477 - implement first pass at tooltips feature
• 733521b2 - improve tooltip design and placement
• f8057eb9 - hide tooltips during chart animation
• aab01927 - rename resize and animate methods for clarity
• a19c99aa - remove posessives in comment blocks
• 3b6478fd - rename and reorder additional methods for clarity
• d9093464 - update tooltip label through setData options
• 601142f9 - fix phantomjs mishandling of svgs with implicit dimensions

Compare with previous version

no weight on test data?

I'll add that in once we've merged the two branches. It wasn't needed to test the frontend chart code because it'll work the same in either case (just changes the labels).

added 3 commits

• 028be8b7 - Burndown Chart backend implementation
• 226202e0 - Improve algorithm to avoid double queries on postgres and mysql
• c82ac9bb - Merge branch 'issue_91' into '91-milestone-burndown-charts'

Compare with previous version

added 1 commit

• 88475528 - remove test_data and rely on data passed from rails

Compare with previous version

added 1 commit

• cd11348a - add toggle between issue count and issue weight

Compare with previous version

marked the task update to use database-fed data instead of hard coded test data (once BE component is finished) as completed

marked the task add toggle to view issue weight as completed

resolved all discussions

added 1 commit

• b1164e1a - Show chart only until current date when milestone due date is bigger

Compare with previous version

added 1 commit

• 7e658aa6 - adjust burndown chart data toggle style

Compare with previous version

unmarked as a Work In Progress

changed title from WIP Burndown charts (frontend) to Burndown Charts

added 1 commit

• 1864d0de - fix end date not matching due date

Compare with previous version

assigned to @jschatz1

This GIF does not do justice. I scrolled down and got a seizure from some sort of scroll detection and I don't see a burndown chart. I prob don't see it because there isn't any data but you should check it out sans data.

@mikegreiling I had to take an actual video of my screen to prove it.

VID_20170331_202134

@mikegreiling Actually I just noticed the burndown doesn't show up unless you are signed in. And that jitter happens when you aren't signed it because the burndown chart isn't there.

I would just check a few things with:

1. No data.
2. No user logged in.

Might have overlooked a few things.

@jschatz1 that bug is actually unrelated to the burndown chart. it's the sidebar logic. @annabeldunstone already merged a fix in CE, but it hasn't made its way to EE yet

Here's the MR: gitlab-ce!10262 (merged)

We need a focus state for keyboard users

This color already exists in variables.scss

@pedroms do we need to add specific color variables that are only going to be used for burndown charts? Could we instead make these less specific and reuse them elsewhere in the app?

This color already exists in variables.scss

@dbalexandre Can you review backend please?

added 100 commits

• 1864d0de...50fd7e13 - 70 commits from branch master
• 8825a65a - create burndown chart template and js bundle
• 1ea45f81 - implement burndown chart hint message
• 767d9a79 - move burndown chart before user action hints
• 1a230346 - implement burndown chart basic outline
• dab26c55 - correctly handle empty array passed to setData
• e2c39ec9 - render burndown chart lines
• c2257bdf - allow burndown chart to resize during animations
• 12e24118 - defer render call to the next animation frame to avoid layout thrashing
• 36027a8a - fix axis font size in burndown chart
• c44a9d22 - add overflow to the x-axis domain
• 63fa244e - create x-axis line in constructor instead of regenerating on each render
• 40e55bd0 - add label to the y-axis
• 22284ec3 - ideal line should end at dueDate not maxDate in event that milestone goes overdue
• d1190d62 - create burndown chart legend
• 4a17b833 - add header to burndown chart and adjust chart size breakpoints
• 7d76ba45 - add 800ms animation to the line path whenever setData is called
• 21b19c03 - implement first pass at tooltips feature
• 9847f60e - improve tooltip design and placement
• 2e40f0bd - hide tooltips during chart animation
• 090c9405 - rename resize and animate methods for clarity
• 47a6524d - remove posessives in comment blocks
• 5c9778d1 - rename and reorder additional methods for clarity
• 1292fb03 - update tooltip label through setData options
• c88b3a8f - fix phantomjs mishandling of svgs with implicit dimensions
• f446ebc8 - Merge branch 'issue_91' into '91-milestone-burndown-charts'
• c2d6dd32 - remove test_data and rely on data passed from rails
• d36d04f4 - add toggle between issue count and issue weight
• 8595ff79 - Show chart only until current date when milestone due date is bigger
• 5ae8f237 - adjust burndown chart data toggle style
• 0d7671e7 - fix end date not matching due date

Compare with previous version

@felipe_artur Note: I just rebased on master to pick up some changes from CE that were in the last CE-to-EE merge. Make sure you reset your local branch if you need to commit anything new.

Hopefully this fixes the spinach test failure. It seems unrelated to our branch.

added 1 commit

• b622f464 - adjust color variables and remove focus state css

Compare with previous version

alright, I've removed this line

I just replaced all of these with references to already-defined color variables... the only one we don't really have an analog for is the burndown-chart-axis-label-color. I had to settle for a color that wasn't quite as dark, but I don't think it is very noticeable. @pedroms what do you think?

@mikegreiling yup, sounds good. I trust your color choices 😉

Can we use the start_date, and end_date over the instance variables?

Can we create a method Burndown#valid? and use it here and here !1540 (diffs)?

This seems a bit weird to me since you are selecting only closed issues here !1540 (diffs). Should not be closed_count?

Derp, I notice that you are assigning this value in the initializer. Can we rename then to open_issues_count and open_issues_weight and use the accessor methods?

Can we rename this to closed_issues?

these are not closed issues, they can be opened.
We should rename to issues_with_closed_at

Should not be @milestone.issues.only_opened?

No. We also need the closed ones here.
We calculate the All issues - closed issues per day

Can we rename this to make its intent more clear?

Can we rename the method to make sense with the order of the return values?

I think that count_and_weight_of is doing too much here. Wdyt?

closed_issues_count = closed_issues.count
closed_issues_weight = sum_issues_weight(closed_issues)

I don't have a strong opinion against it. We can change anyway...

Same here:

reopened_issues_count = reopened_issues.count
reopened_issues_weight = sum_issues_weight(reopened_issues)

See !1540 (diffs, comment 26750997)

Can we use issues.sum(:weight)?

No. This is not a relation.

Can we rename this to reopened_and_closed_issues_by_data(date)?

renamed to closed_and_reopened_issues_by(date) i think it is less idiomatic to repeat date eg:by_date(date) in the parameter.

I think the closed_and_reopened_issues_by raises the question "by what".

Can we use closed.only_reopened?

No because it is not a relation.

I think to do like this we would need to use #where instead of #select and that will require to trunc the date, which are two different methods for Postgres and Mysql.

Why did you remove this?

To follow a condition of burndown:

If d > i.closed_at && i.state(now)==open, then assume i was opened on i.closed_at + 1 day. So do count/weight it.

if we set closed_at to nil when reopening an issue we won't be able to count as reopened on burndown.

Thanks for explaining why 👍

Do we need to remove this in CE too?

Yes, this should be backported to CE

likewise with the changes in spec/models/issue_spec.rb

Yes, if a CE user upgrades to EE the data for burndown will be more accurate.

@smcgivern The backport MR gitlab-ce!10453 (merged)

See !1540 (diffs, comment 26751346)

Can we use create_list(:issues, count, ...)?

Can we use closed.each(&:closed)?

Can we use closed.slice(0..count / 4).each(&: reopen)?

is there supposed to be a space between &: and the method name?

nope, probably a typo...

Thanks @felipe_artur! I left some comments.

Thanks for your help @dbalexandre!!!

@smcgivern Can you review please?

added 1 commit

• 31ca765f - Small changes to burndown model and specs

Compare with previous version

Wdyt issues.map(&:weight).compact.reduce(:+)?

added 1 commit

• f87c9f6a - Small changes to burndown model and specs

Compare with previous version

added 1 commit

• 2a048b17 - Small changes to burndown model and specs

Compare with previous version

added 1 commit

• 1bd74265 - Small changes to burndown model and specs

Compare with previous version

we could change this one and eliminate can_generate_chart variable

- if @burndown.valid?  
# Generate chart
- elsif can?(current_user, :admin_milestone, @project) && cookies['hide_burndown_message'].nil?
# show zero state panel

@mikegreiling

👍

added 1 commit

• c48f972b - fix simulate_drag issues

Compare with previous version

added 1 commit

• cfeadfb2 - clean up burndown empty state logic

Compare with previous version

mentioned in merge request gitlab-ce!10437 (merged)

reduce(:+) -> sum!

We also don't need the || 0 if we use sum 🙂

Don't need return!

is an explicit return needed when you are returning a tuple like this, maybe?

Yes, it is needed when returning multiple values.

Or it could just be [closed, reopened] 🙂 Sorry, that was very terse of me - would you mind changing it to that?

Does this run two queries? We could do milestone.issues.reorder(nil).pluck('COUNT(*), SUM(weight)'), but I'm not sure if it's worth it.

milestone.issues.reorder(nil).pluck('COUNT(*), COALESCE(SUM(weight), 0)') does the job.

I think it is better to have just one query.

Please indent this two spaces more, as it's a continuation!

it is idented two spaces more, do you mean four spaces?

I think we should memoize to an instance variable that matches the method name.

Why do we need the to_i on both sides? Could we just use to_date instead of .beginning_of_day.to_i?

Can we pull the assignments out of the array? It's quite confusing otherwise 🙂

Do we actually need the ordering?

Since #select iterates over the array if the dates come first it will probably be faster, at least for small arrays.
But i don't know if using order will take that advantage off.

               static VALUE
rb_ary_select_bang(VALUE ary)
{
    long i1, i2;

    RETURN_SIZED_ENUMERATOR(ary, 0, 0, ary_enum_length);
    rb_ary_modify(ary);
    for (i1 = i2 = 0; i1 < RARRAY_LEN(ary); i1++) {
        VALUE v = RARRAY_AREF(ary, i1);
        if (!RTEST(rb_yield(v))) continue;
        if (i1 != i2) {
            rb_ary_store(ary, i2, v);
        }
        i2++;
    }

    if (i1 == i2) return Qnil;
    if (i2 < i1)
        ARY_SET_LEN(ary, i2);
    return ary;
}

OK, it's not super-important anyway 🙂

can_generate_chart!

oops, forgot to remove that line

Could these start and end dates not be part of burndown.chart_data?

The chart_data is a simple array of values... to make it a part of that would require turning it into a hash and then adopting a schema of some sort. I think it is simpler to just do it this way.

An array with items in a specific order also has a schema, though! Really, we should use whatever data structure makes most sense. Here it seems weird that we're passing this data from the milestone rather than the burndown, because the burndown should have all the information we need.

the due_date cannot be introspected from the array of points that chart_data gives us because it may be a milestone in-progress. we'd still need a way to specify the start and end dates separately...

we could change chart_data to output a hash like:

{
  start_date: "2017-03-01",
  due_date: "2017-04-01",
  data: [ ... ]
}

but that's not really much different from what we're doing here and I'd argue it would be a bit more arcane...

as for using burndown.due_date instead of milestone.due_date yeah I think that would be a good idea.

@mikegreiling right, that's what I mean. I don't see why the object is worse, and then the backend can return a single object which contains all the information we need, instead of having most of the information from one method, and the rest from another.

Burndown has these dates accessible already.

If we made this the as_json method, we could just do burndown.to_json in the view. Up to you 🙂

It was on my initial plans, forgot about it. Thx!

@felipe_artur @mikegreiling thanks! Just out of interest, I ran this on staging for 8.17, using the updated_at column instead of closed_at. Here's what I got 🙂

irb(main):103:0> Burndown.new(milestone).chart_data
D, [2017-04-04T10:27:21.847368 #42571] DEBUG -- :    (1.4ms)  SELECT COUNT(*) FROM "issues" WHERE "issues"."deleted_at" IS NULL AND "issues"."milestone_id" = $1  [["milestone_id", 134231]]
D, [2017-04-04T10:27:21.848826 #42571] DEBUG -- :    (0.9ms)  SELECT SUM("issues"."weight") FROM "issues" WHERE "issues"."deleted_at" IS NULL AND "issues"."milestone_id" = $1  [["milestone_id", 134231]]
D, [2017-04-04T10:27:21.851861 #42571] DEBUG -- :   Issue Load (1.6ms)  SELECT updated_at AS closed_at, weight, state FROM "issues" WHERE "issues"."deleted_at" IS NULL AND "issues"."milestone_id" = $1 AND (updated_at IS NOT NULL)  ORDER BY "issues"."id" DESC, updated_at ASC  [["milestone_id", 134231]]
=> [["2017-01-22", 165, 7], ["2017-01-23", 160, 7], ["2017-01-24", 152, 7], ["2017-01-25", 146, 7], ["2017-01-26", 139, 7], ["2017-01-27", 133, 7], ["2017-01-28", 133, 7], ["2017-01-29", 130, 7], ["2017-01-30", 125, 7], ["2017-01-31", 121, 7], ["2017-02-01", 117, 7], ["2017-02-02", 108, 7], ["2017-02-03", 98, 7], ["2017-02-04", 97, 7], ["2017-02-05", 97, 7], ["2017-02-06", 90, 7], ["2017-02-07", 73, 3], ["2017-02-08", 40, 2], ["2017-02-09", 28, 2], ["2017-02-10", 17, 1], ["2017-02-11", 15, 1], ["2017-02-12", 15, 1], ["2017-02-13", 15, 1], ["2017-02-14", 15, 1], ["2017-02-15", 15, 1], ["2017-02-16", 15, 1], ["2017-02-17", 15, 1], ["2017-02-18", 15, 1], ["2017-02-19", 15, 1], ["2017-02-20", 15, 1], ["2017-02-21", 15, 1], ["2017-02-22", 15, 1]]

added 3 commits

• bc7d2588 - clean up burndown empty state logic
• 3cd662fc - Port 'refactor_simulate_drag' to EE
• 0f946245 - convert capitalization scheme to sentence case

Compare with previous version

added 1 commit

• 5757480c - fix unused variable

Compare with previous version

added 4 commits

• 6056cddb - Port 'refactor_simulate_drag' to EE
• d0d9980c - convert capitalization scheme to sentence case
• d9c7034c - fix unused variable
• 47ce88a3 - Merge branch 'refactor_simulate_drag-ee' into 91-milestone-burndown-charts

Compare with previous version

added 1 commit

• aa04757a - Code improvements on burndown model and view partial

Compare with previous version

mentioned in merge request gitlab-ce!10453 (merged)

@smcgivern Done. Can you take a look again please?

I removed the Closes #91 reference since we still need docs for this :)

added 2 commits

• db6fb490 - use a less memory-intensive sourcemap when running in CI
• 0cf67a93 - Merge branch 'refactor_simulate_drag-ee' into 91-milestone-burndown-charts

Compare with previous version

As I mentioned in our call and as you discovered, I believe the dotted line needs to go to the end of the milestone not to the end of today.

added 1 commit

• ae8ef069 - don't confuse the due date and the last data entry date

Compare with previous version

added 1 commit

• 38a666e6 - fix "Undefined method `+' for nil:NilClass" error

Compare with previous version

Looks fixed @mikegreiling

added 158 commits

• 38a666e6...2019c376 - 157 commits from branch master
• 7f577bda - Merge branch 'master' into 91-milestone-burndown-charts

Compare with previous version

Generally like to name jquery things to $nameOfConst

done

I am fine with this.

👍

I was pretty careful to read-only on these resize callbacks and defer DOM manipulation to requestAnimationFrame callbacks so as to not cause any layout thrashing.

Should we debounce?

I think it should be fine as it is. animateResize is a very lightweight function that works similarly to a debounce, but is a bit better suited for a resize callback. It won't wait for the debounce function to stop being called, but it does prevent the resize callback from causing any more than 1 re-render per 50ms.

So if a resize event calls animateResize 10 times in 50ms, it will only trigger resize one time, and resize will only schedule a render if the size has changed. It should be pretty performant.

I can alter the 50ms threshold to something higher if it becomes necessary.

is this specific enough?

I'm adding a .js-burndown-data-selector class to make this more clear.

@mikegreiling I am good with this minus 1 or 2 comments

added 2 commits

• 796158c3 - follow jQuery variable $someConst convention
• 163a87a7 - make a custom class for the burndown data toggle

Compare with previous version

@axil thank you, that's my mistake. Who is writing the docs for this @felipe_artur @mikegreiling? Can you create an empty follow-up MR so we don't lose track of this?

@felipe_artur I've merged the backport MR, one last request!

assigned to @felipe_artur

added 1 commit

• 065fa720 - Remove useless return burndown.rb

Compare with previous version

resolved all discussions

@jschatz1 I'm happy with this, can you do the honours and merge please?

assigned to @jschatz1

enabled an automatic merge when the pipeline for 065fa720 succeeds

mentioned in issue #2092 (closed)

canceled the automatic merge

enabled an automatic merge when the pipeline for 065fa720 succeeds

mentioned in commit 644abaf2

merged

yay 🎉

Thanks to everyone involved, this is an awesome feature! 👏

mentioned in issue #2578 (closed)