[BUG] master branch time scale min/max handling broken

[andig]

Follow-up to #4493 (comment). http://jsfiddle.net/andig2/6raazty4/3/ has min/max set to 1.7.2016-1.8.2017. The fiddle shows different min/max values when displaying the chart.

Expected behaviour: min/max axis settings are honored.

[etimberg]

Did some debugging on this. It was introduced in #4267

I think the issue is coming from https://github.com/chartjs/Chart.js/blob/master/src/helpers/helpers.time.js#L58-L59 but I'm not sure exactly how yet.

CC: @benmccann @IlyaBeliaev

[etimberg]

The reason this works with round: true is due to https://github.com/chartjs/Chart.js/blob/master/src/helpers/helpers.time.js#L63-L68

I think a potential solution to this bug is to change line 63 to also include a condition of && !options.min

I also think that line 74 needs to change from ticks.push(cur.valueOf()); to ticks.push(realMax); because in the case of a max value being set it needs to push the max value and currently when max is set and is not a multiple of the unit, the last tick value will be pushed instead of the user defined maximum.

[etimberg]

@andig here's a fork of your fiddle with a custom build of my two proposed changes above: http://jsfiddle.net/dnr07vge/1/

@benmccann @IlyaBeliaev please verify that my potential solution is correct.

[benmccann]

Adding && !options.min seems like a reasonable solution

[andig]

!anything generally seems a bad idea for values. If axis min is 1.1.1970 I‘d pass 0 as min. Imho the tests should be for null like here https://github.com/chartjs/Chart.js/pull/4267/files#diff-e4b810d743d51c0b308df72d4788703aR58 Null test would also need to honor undefined?

[andig]

Also, regarding #4267 the min/max timestamps in the example already are exactly at the month boundary- shouldn't those remain untouched in any case?

[etimberg]

we have an isNullOrUndef helper explicitly for this so we should use that in any fix here and convert the old checks as well.

For 4267 I think it's that they were also at the year boundary too which is the major unit which is why it was not seen.

[andig]

It seems there is already an associated test but it succeeds: https://github.com/chartjs/Chart.js/blob/master/test/specs/scale.time.tests.js#L303. The min/max is already being modified from what is set to in the options and the test was there before #4267.

	it('should use the min option', function() {
		config.time.unit = 'day';
		config.time.min = '2014-12-29T04:00:00';

		var scale = createScale(mockData, config);
		expect(scale.ticks[0].value).toEqual('Dec 28');
	});

	it('should use the max option', function() {
		config.time.unit = 'day';
		config.time.max = '2015-01-05T06:00:00';

		var scale = createScale(mockData, config);
		expect(scale.ticks[scale.ticks.length - 1].value).toEqual('Jan 6');
	});

It seems unit may already be forcing ticks outside the range configured via min/max which would imho be unexpected behaviour.

So- what is the expected behaviour?

[etimberg]

Looks like the test changed in #4268 2d7c1f0#diff-32e22477b9f6458d20701ea5c8720ea4R300

I think the test needs to go back to the prior value and the code changed so that it passes