-
Notifications
You must be signed in to change notification settings - Fork 2
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
Added node status by period in filter #120
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems also poorly tested so would be good to have more complex test.
src/Services/NodeUptimeService.ts
Outdated
return await database.runQuery<NodeUptime>( | ||
`select "isWorking",date_trunc('day', "updatedAt") "timePeriod", ( | ||
case when "isWorking" = false | ||
then 'Node is down' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is an API, it's better and expected to use boolean (true/false) so frontend can parse it easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was that the response also has a message that can be displayed, like { "isWorking": false, "timePeriod": "2020-03-17T00:00:00.000Z", "nodeStatus": "Node is down" }
Should I just leave the isWorking with timePeriod in the response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, please don't break current implementation by renaming variable, response should have the same structure
src/Services/NodeUptimeService.ts
Outdated
else 'Node is up' | ||
end) as "nodeStatus" | ||
from "NodeUptime" | ||
where "nodeId" = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1?
src/Services/NodeUptimeService.ts
Outdated
order: [['updatedAt', 'DESC']] | ||
}); | ||
return await database.runQuery<NodeUptime>( | ||
`select "isWorking",date_trunc('day', "updatedAt") "timePeriod", ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
day hardcoded
src/Services/NodeUptimeService.ts
Outdated
end) as "nodeStatus" | ||
from "NodeUptime" | ||
where "nodeId" = 1 | ||
and "updatedAt" >= now() - interval '1 year' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 year?filter?
Partially covers #57 Return node status in period by filter.