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

Use backup sensor values #22

Merged
merged 1 commit into from
Jun 29, 2017
Merged

Use backup sensor values #22

merged 1 commit into from
Jun 29, 2017

Conversation

kellyi
Copy link
Contributor

@kellyi kellyi commented Jun 28, 2017

Overview

This PR adapts the fetchDataForMetric function to try getting data from a backup sensor on the Delaware River if the primary sensor's url is unavailable. If both requests fail, we then use the fallback values.

This works by giving the xhr object a different error listener depending on whether the url is the primary or backup sensor: if it fails for the primary url, we call the same function again for the backup url. If it fails there, we call the error handler implemented in #21.

I made a few related adjustments here, too: the change updates the renderHeader & renderSubtext methods in order to return text for the Delaware gauge if necessary, and reducing the logic of the urlForMetric method while turning the base urls for the primary and backup sensors into longer string constants.

Connects #18

Screenshot

screen shot 2017-06-28 at 4 55 34 pm

Notes

  • I slimmed down the text here from what was suggested in Fallback values: use different gauge #18 so that it would roughly match the length of the other headers & subscripts. Tagging @ajrobbins for feedback on that.

  • Only the last commit is part of this PR. Once the others have gone in I can rebase this on develop.

Testing

  • get this branch, then server
  • load the spokes without making any changes and verify that you see data for the Schulykill and appropriate headers and subscripts
  • drop .usgs from the PRIMARY_SENSOR_URL constant, then refresh the app. visit the spokes and verify that you see the values, headers, and subscripts from the Delaware River.
  • drop .usgs from the BACKUP_SENSOR_URL const, refresh the app, then visit the spokes again. Verify that you see the fallback values and text.

@ajrobbins
Copy link
Contributor

Text looks fine for now; we'll address it with design in a separate card.

Copy link
Collaborator

@rajadain rajadain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 tested. Works well. Code could use some polish.

function renderHeader(metric, isFallback, isBackupSensor) {
if (isFallback) {
return 'Typical ' + metric.display + ' in the Schuylkill River:';
} else if (isBackupSensor) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the body of the if is a simple return, prefer an independent if to an else if.

if (isFallback) {
  return ...;
}

if (isBackupSensor) {
  return ...;
}

return ...;

function renderSubtext(isFallback, isBackupSensor) {
if (isFallback) {
return 'A common sensor reading for this month.';
} else if (isBackupSensor) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

function renderHeader(metric, isFallback) {
var firstWord = isFallback ? 'Typical ' : 'Current ';
return firstWord + metric.display + ' in the Schuylkill River:';
function renderHeader(metric, isFallback, isBackupSensor) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's starting to feel like we're passing too many parameters too often. Since isFallback and isBackupSensor are essentially global state values, consider making them so, setting them once, and using them directly, instead of passing in each call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I can see the logic in this but I think I'm inclined to leave these arguments lists as is for now. Aside from string constants and the metrics object, I don't think we've set any other global variables. If we end up doing #23, we should consider creating some app state there.

@kellyi kellyi mentioned this pull request Jun 29, 2017
- make primary & backup sensor url constants & adjust urlForMetric fn
- adjust renderHeader & renderSubtext methods to return
backup-sensor-specific values for the Delaware gage
- update fetchDataForMetric fn to try the backup sensor if the primary
sensor request fails
@kellyi kellyi force-pushed the ki/use-backup-sensor-values branch from dc1e67f to 864fa1a Compare June 29, 2017 14:18
@kellyi
Copy link
Contributor Author

kellyi commented Jun 29, 2017

Rebased on develop and tested again. Everything's still working, so I'm going to merge this in.

@kellyi kellyi merged commit 42e950c into develop Jun 29, 2017
@kellyi kellyi deleted the ki/use-backup-sensor-values branch June 29, 2017 14:33
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.

3 participants