1245471 - Release Tracking Report should be able to have custom dates

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[Carsten Book [:Tomcat]]

the release tracking report https://bugzilla.mozilla.org/page.cgi?id=release_tracking_report.html have so far fixed dates - since other queries turned to be problematic due to target milestones we should have a option to set flexible dates or maybe aligned to merge days.

This way it would allow sheriff and relman to use the report as source nothing is missed for uplifts.

Sylvestre: what do you think re the dates alligned to merge days ?

Comment 1

[David Lawrence [:dkl]]

I feel the report needs this ability regardless so assigning to me.

dkl

Comment 2

[David Lawrence [:dkl]]

Attachment: 1245471_1.patch

Comment 3

[:glob ✱]

Comment on attachment 8716035 [details] [diff] [review]
1245471_1.patch

Review of attachment 8716035 [details] [diff] [review]:
-----------------------------------------------------------------

this looks good, but i think we should also include a 'presets' dropdown with a list of all date ranges from https://wiki.mozilla.org/RapidRelease/Calendar, with future dates filtered out.

Comment 4

[David Lawrence [:dkl]]

Attachment: 1245471_2.patch

Comment 5

[:glob ✱]

Comment on attachment 8716386 [details] [diff] [review]
1245471_2.patch

Review of attachment 8716386 [details] [diff] [review]:
-----------------------------------------------------------------

the ux of this looks pretty rough.

i'd suggest having the preset periods as per the calendar (including all dates for this year), with an "custom" or "other" option.
when "custom" isn't selected the from/to date pickers should not be visible.

::: extensions/BMO/lib/Reports/ReleaseTracking.pm
@@ +213,3 @@
>      $start_date = string_to_datetime('2013-01-08');
>      $end_date = $start_date->clone->add(weeks => 6)->add(days => -1);
> +    $now_date = string_to_datetime('2016-01-05');

just stopping at that date isn't very useful - it adds many new steps to the process, including a visit to an external reference to determine the right dates to use.  this is slow and prone to errors.

we have fixed dates, they just aren't all with the same period.
rather than trying to calculate the dates, we should just have a list with all the dates in them, from the existing list + the published release calendar.

@@ +375,5 @@
> +                                          format => 'YYYY-MM-DD' });
> +    trick_taint($from_date);
> +    trick_taint($to_date);
> +    $query->{start_date} = $from_date;
> +    $query->{end_date} = $to_date;

splitting the dates into two fields breaks existing bookmarks:
'20160105-20160215' is not a legal date.

::: extensions/BMO/template/en/default/pages/release_tracking_report.html.tmpl
@@ +47,4 @@
>        <option value="+">+</option>
>      </select>
>  
> +    during past branch dates

"during past branch dates" doesn't make any sense, because you'd never use this to search for future branch dates.

::: extensions/BMO/web/js/release_tracking_report.js
@@ +156,5 @@
>    selectValue(flagEl, parts[0]);
>    onFlagChange();
>    selectValue($('#flag_value')[0], parts[1]);
> +  $('#from').val(parts[2]);
> +  $('#to').val(parts[3]);

use selectValue() to match the surrounding code.

Comment 6

[David Lawrence [:dkl]]

Attachment: 1245471_3.patch

Thanks for the review:

Changes:
- Updated UI to hide the date fields when using the preselects.
- Proper handling of bookmarked links when using custom dates
- Added DATE_RANGES constant with all dates until end of 2016 instead of auto-generating them.

I had to edit some of the 2015 dates as they did not match the shared calendar dates. Let me know if I should have just left the old ones alone for some reason.

dkl

Comment 7

[:glob ✱]

Comment on attachment 8718106 [details] [diff] [review]
1245471_3.patch

Review of attachment 8718106 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

::: extensions/BMO/lib/Reports/ReleaseTracking.pm
@@ +473,4 @@
>      $vars->{products_json} = $json->encode(\@products_json);
>      $vars->{fields_json} = $json->encode(\@fields_json);
>      $vars->{flag_names} = \@flag_names;
> +    $vars->{ranges} = DATE_RANGES;

the list of dates shouldn't include those where the start date is in the future

@@ +477,2 @@
>      $vars->{default_query} = $input->{q};
> +    $vars->{range_type} = $input->{range_type};

for clarity 'range_type' should probably be called 'is_custom'

::: extensions/BMO/template/en/default/pages/release_tracking_report.html.tmpl
@@ +63,5 @@
> +      and
> +      <input class="date_field" name="to" id="to" onChange="serialiseForm()">
> +      <img class="date_field-img" id="to-img" src="extensions/BugModal/web/calendar.png" width="16" height="16">
> +    </span>
> +    <input type="checkbox" id="range_type" onchange="selectRangeType()" [% "checked=\"checked\"" IF range_type %]>

nit: you can avoid backslashes with [% "checked" IF range_type %] (because we're not xhtml)

::: extensions/BMO/web/js/release_tracking_report.js
@@ +115,5 @@
>  }
>  
>  function onFormSubmit() {
> +  if ($('#range_type').is(':checked')
> +      && (!$('#from').val() || !$('#to').val()))

extensions/BMO/web/js/release_tracking_report.js: line 119, col 7, Bad line breaking before '&&'.

@@ +117,5 @@
>  function onFormSubmit() {
> +  if ($('#range_type').is(':checked')
> +      && (!$('#from').val() || !$('#to').val()))
> +  {
> +    alert('You must enter a two values for the date range.');

'You must enter both the start and end dates'

Comment 8

[David Lawrence [:dkl]]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   4cdd5e5..d78003b  master -> master

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Merge day is perfect (the big one, not the m-b => m-r merge)

Comment 10

[David Lawrence [:dkl]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #9)
> Merge day is perfect (the big one, not the m-b => m-r merge)

Well the new form improvements are now live as of this morning so please let me know how the dates look.

https://bugzilla.mozilla.org/page.cgi?id=release_tracking_report.html

Thanks
dkl