Skip to content
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

Fix undefined Handler Type in current master branch #73

Merged
merged 1 commit into from
Dec 4, 2017

Conversation

PeteBa
Copy link
Contributor

@PeteBa PeteBa commented Sep 1, 2017

In Release 1.0.4, I noticed that the default Handler Type is not selected for new series as seen in the screenshot below:

grafana

I have only recently noticed this and suspect it could actually be due to new behaviour in Grafana version 4.4.1.

The issue results from upgradeOldVersion() being called multiple times on a new series when still on the Metrics tab and before the Options tab has a chance to set the default. This results in valueHandler being incorrectly set to "Regular" rather than the default of"Threshold" (i.e. valueHandlers[0]).

Looking at the code in status_ctrl.js, this occurs as follows:

  1. when a series is first created in the Metrics tab then valueHandler is null
  2. a first call to upgradeOldVersion() will set the displayType to "Regular" (i.e. displayType[0]) on line 236
  3. a second call to upgradeOldVersion() will then copy "Regular" into the valueHandler on line 232
  4. when you then go to the Options tab then the Handler Type is blank

I think this is new behaviour in the latest Grafana release as I cannot recall seeing this problem before. The fix uses (target.displayType != null) to limit the upgrade checks to only legacy series and not new ones.

@PeteBa PeteBa changed the title Fix legacy defect when upgrading from old version Fix undefined Handler Type in current master branch Sep 1, 2017
@PeteBa PeteBa force-pushed the bugfix-upgrade-old-version branch from 8e0aca2 to 133388d Compare September 3, 2017 17:08
@PeteBa
Copy link
Contributor Author

PeteBa commented Sep 3, 2017

I have rebased this to the latest merge.

@tomer-amir-vonage
Copy link
Contributor

Thanks!
I'll take a look in the weekend and get back to you

@PeteBa
Copy link
Contributor Author

PeteBa commented Nov 6, 2017

Rebased to merge with current master.

@valeriu-a
Copy link

Any updates on this merge?

@YehonatanVonage YehonatanVonage merged commit f97d0d7 into Vonage:master Dec 4, 2017
@PeteBa PeteBa deleted the bugfix-upgrade-old-version branch December 4, 2017 10:03
@PeteBa
Copy link
Contributor Author

PeteBa commented Dec 4, 2017

Thanks for the merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants