[IMPAC-745] Fix Cash Projection Crash and Refactor Highcharts by iseessel · Pull Request #480 · maestrano/impac-angular

iseessel · 2018-02-17T00:46:39Z

@xaun
Implemented what we discussed. Its a pretty big pull request, so let me know if you have any questions. Let me know when you want me to squash ( would rather squash when ready to merge, as keeping track of my commits helps me).

I think this is an improvement over what was there originally. Only thing is its a bit confusing when @options should be populated in constructor vs in setter. All the dynamic options under the template, I put into setter methods, because they seemed specific to line charts.

xaun

Hi @iseessel, thanks for these changes, I think we're getting close, I like the setters. I have made some suggestions which are mostly cleanups / formatting changes except for one potential issue with the _highChartOptions.

Once these have been made, we will need to stage this and do some extensive testing with the Cash Projection widget, as there is quite a lot of potential for regressions here.

xaun · 2018-02-19T10:57:40Z

  class Chart
-    constructor: (@id, @data = {}, @options = {})->
-      @_template = templates[@options.chartType]
+    constructor: (@id, @series = {}, @settings = {})->


@settings can be removed from the this scope, and just be settings right?

EDIT: We can't remove settings from this, as it is called in #onChartNotify in chart-threshold.component.coffee

Ah ok, do you think we should rename @options to @hcOptions or @highchartsOptions for clarity between options & settings?

Yeah probably a good idea.

xaun · 2018-02-19T11:00:08Z

-      @_template = templates[@options.chartType]
+    constructor: (@id, @series = {}, @settings = {})->
+      @_template = templates[@settings.chartType]
+      @populateChartOptions(@series, @settings)


Not a big fan of how this method is public, seems more like a private method, and I think everything that happens in that method could just be within the constructor block.

Sure thing, can just move code over.

xaun · 2018-02-19T11:01:13Z

+      template = @_template
+      formatters = @formatters(settings.currency)
+      todayMarker = @todayMarker(settings)
+      series = @populateSeries(series)


Prefer series = { series: series }, I dont think we need a populateSeries public instance method here

xaun · 2018-02-19T11:04:04Z

-        @hc.update(chartConfig)
+    populateChartOptions: (series, settings) ->
+      # Every chart will have these options. The rest of the options are created in the setter methods.
+      template = @_template


If you move the contents of this populateChartOptions method to just the constructor block, as @_template is not being accessed anywhere, we can remove it from the this scope, and just use

template = templates[@settings.chartType] angular.merge({}, templates ... ... . .)

xaun · 2018-02-19T11:06:52Z


-    todayMarker: ->
-      return {} unless @options.showToday
+    todayMarker: (settings) ->


todayMarker: (showToday, markerColor = 'rgba(0, 85, 255, 0.2)') ?

Yeah that works.

xaun · 2018-02-19T11:13:53Z

  w.initContext = ->
    $scope.isDataFound = angular.isDefined(w.content) && !_.isEmpty(w.content.summary) && !_.isEmpty(w.content.companies)
    if $scope.isDataFound
-      $scope.timePeriodInfoParams.histParams = w.metadata && w.metadata.hist_parameters


Why is this removed?

Oops most likely a rebasing problem.

xaun · 2018-02-19T11:17:21Z

+    highChartsFactory.addCustomLegend(legendFormatter)
+    highChartsFactory.addSeriesEvent('click', onClickBar)
+    highChartsFactory.addSeriesEvent('legendItemClick', onClickLegend)
+    highChartsFactory.addXAxisOptions(_highChartOptions.withZooming)


I like the setters, means widgets can really configure the charts with a nice api.

Thought It feels weird giving the factory the full _highChartOptions on init, then accessing one of the attributes for the addXAxisOptions method. I wouldn't shy away from using this syntax:

highChartsFactory.addXAxisOptions({ defaults: w.metadata.xAxis callback: onZoom })

It means I you dont have to go scrolling up and down the file looking for the options hash, keeping things together. I think we should move the whole options to this if possible, see my comment at the _highChartOptions declarations.

Good point. What exactly do you mean by "we should move the whole options to this if possible."

@iseessel never-mind about that comment actually

xaun · 2018-02-19T11:20:48Z

+  _highChartOptions = Object.freeze
+    chartType: 'line'
+    chartOnClickCallbacks: []
+    currency: w.metadata.currency


As the variable _highChartsOptions is being defined in the controller function level of this directive, it will be evaluation as soon as the function is invoked internally by angular. This means, I am quite confident, w.metadata will be undefined at that exact point in time.

This is essentially being decared as a constant, but as it has dynamic data inside it, it needs to either be $onInit (still flakey with asyc data), or better $onChanges, or even better (less code), re-evaluated when w.format is called.

Good catch. Will put back into w.format.

xaun · 2018-02-19T11:29:30Z

      }]
-      return unless ImpacKpisSvc.validateKpiTargets(params.targets)
+
+      if !ImpacKpisSvc.validateKpiTargets(params.targets)


prefer unless to if ! with coffee

xaun · 2018-02-19T11:30:02Z


+    handleInvalidAlertAmount = ->
+      toastr.error("Please choose a number one or greater.", 'Error')
+      ctrl.cancelCreateKpi()


Is this new functionality? Do we want to cancelCreateKpi if I enter a wrong number?

This is new functionality. Currently when we enter a wrong number, the update box will keep spinning -- this prevents that.

@iseessel I had a quick look, the problem is the ctrl.saveKpi method sets ctrl.loading = true, but then returns without setting the ctrl.loading back to false unless ImpacKpisSvc.validateKpiTargets(params.targets) returns true.

The quick solution to use a toastr to notify the user is fine I guess, though ngMessages txt would probs be better. But instead of calling ctrl.cancelCreateKpi() in your handleInvalidAlertAmount method, can we just try setting ctrl.loading to false? I think it would be a better ux.

xaun

@iseessel just if you could fix the indentation inconsistency I mentioned then should be good to go.

@cesar-tonnoir could you do a final quick review?

xaun · 2018-02-21T09:58:54Z

-            min: _.get(zoomingOptions.defaults, 'min')
-          }
-
        chart:


@iseessel there is inconsistent indentation here. The line object properties are indented 4 spaces, and the rest of the file is 2 spaces.

xaun · 2018-02-21T10:07:56Z

@iseessel if you could squash all commits like "refactoring", "debugging", "fix rebase problem", "comments", "take out debugging". Commits like "Separated multiple update calls into one create call" can remain. Not sure why you need to keep "take out debuggers" commits during your I development process? I suggest always using git add -p <files> to stage your changes in hunks, giving you the chance to audit your work in pieces before committing, therefor not needing to remove debuggers later, and squash "remove debuggers" commits.

iseessel · 2018-02-21T21:30:39Z

@xaun Found a bug where an onclick callback did not work. Was quite a doozy to find the problem.

It boiled down to a pointer program. You need the chartOnClickCallbacks to point to settings.onClickCallbacks, because in `chart-threshold-component.coffee' you push another callback into this array. Let me know when if have any questions.

iseessel · 2018-02-21T21:44:24Z

@xaun So add manual transaction does not work in the cash projection widget, getting a 400 error from the server -- this is an existing error (tested it pre changes as well) and I suggest putting it in a new issue, as this PR is getting a bit beefy.

xaun · 2018-02-22T10:11:47Z

@iseessel I think there may be a better way that follows the new concept of setting options and rendering / re-rendering.

If you look here you can see this is where the chart-threshold.component is calling onChartNotify.

ctrl.chartPromise is a component binding given by the parent widget.

If we take a look here at the cash-projection.directive, ($scope.chartDeferred is bound to chart-threshold.component as chartPromise), the notify is being invoked after render. I think if we simply invoke $scope.chartDeferred.notify($scope.chart) before calling $scope.chart.render(), it will work.

xaun suggested changes Feb 19, 2018

View reviewed changes

iseessel force-pushed the bug/745-cash-projection-crash branch from 6c9bf95 to 415acc2 Compare February 20, 2018 20:05

xaun approved these changes Feb 21, 2018

View reviewed changes

xaun requested a review from cesar-tonnoir February 21, 2018 10:11

xaun mentioned this pull request Feb 21, 2018

[IMPAC-745] Fixed Cash Projection Crash #463

Closed

iseessel force-pushed the bug/745-cash-projection-crash branch 2 times, most recently from 692348b to 042e906 Compare February 21, 2018 16:15

iseessel added 3 commits February 21, 2018 11:17

[IMPAC-745] Separated multiple update calls into one create call

ef87684

[IMPAC-745] Refactored Highcharts Factory to use setter methods

70b9847

[IMPAC-745] Changed ui for failed update

fb0bbd7

iseessel force-pushed the bug/745-cash-projection-crash branch from 042e906 to fb0bbd7 Compare February 21, 2018 16:17

[Impac-745] Fixed #chartOnClickCallbacks

d66bf5f

xaun approved these changes Feb 23, 2018

View reviewed changes

iseessel force-pushed the bug/745-cash-projection-crash branch 2 times, most recently from 21e1da9 to 02ea906 Compare February 26, 2018 16:57

[IMPAC-745] Refactor #addOnClickCallback API

dae9eb7

iseessel force-pushed the bug/745-cash-projection-crash branch from 02ea906 to dae9eb7 Compare February 26, 2018 17:05

xaun merged commit c9051f2 into maestrano:release/v1.6.9 Feb 26, 2018

xaun mentioned this pull request Mar 19, 2018

Feature/lmi insights #487

Open

Conversation

iseessel commented Feb 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xaun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iseessel Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xaun Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iseessel Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xaun Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xaun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xaun commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iseessel commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iseessel commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xaun commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

iseessel commented Feb 17, 2018 •

edited

Loading

iseessel Feb 20, 2018 •

edited

Loading

xaun Feb 19, 2018 •

edited

Loading

iseessel Feb 20, 2018 •

edited

Loading

xaun Feb 20, 2018 •

edited

Loading

xaun commented Feb 21, 2018 •

edited

Loading

iseessel commented Feb 21, 2018 •

edited

Loading

iseessel commented Feb 21, 2018 •

edited

Loading

xaun commented Feb 22, 2018 •

edited

Loading