Skip to content
This repository has been archived by the owner on Mar 7, 2018. It is now read-only.

Fix edges queries for cluster cassandra #99

Merged
merged 6 commits into from Aug 16, 2017
Merged

Fix edges queries for cluster cassandra #99

merged 6 commits into from Aug 16, 2017

Conversation

c-w
Copy link
Contributor

@c-w c-w commented Aug 15, 2017

Depends on this schema change: CatalystCode/project-fortis-pipeline#110


const query = `
SELECT placename, placeid, mentioncount
SELECT placeid, mentioncount, centroidlat, centroidlon
Copy link
Collaborator

Choose a reason for hiding this comment

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

This query should factor in the bbox coordinates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added new feature in 4db00c6. Will also require client-side change since bbox was not considered in Fortis-v1.

Copy link
Contributor

@Smarker Smarker left a comment

Choose a reason for hiding this comment

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

LGTM - with comments

function formatIdsUri(ids) {
return `${apiUrlBase}/features/id/${ids.map(encodeURIComponent).join(',')}?include=bbox`;
function formatIdsUri(ids, include) {
let uri = `${apiUrlBase}/features/id/${ids.map(encodeURIComponent).join(',')}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using const for uri? (there's a couple of other instances where there's let uri)

Copy link
Contributor Author

@c-w c-w Aug 16, 2017

Choose a reason for hiding this comment

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

The uri is being mutated just below so it can't be const :)

return `${apiUrlBase}/features/id/${ids.map(encodeURIComponent).join(',')}?include=bbox`;
function formatIdsUri(ids, include) {
let uri = `${apiUrlBase}/features/id/${ids.map(encodeURIComponent).join(',')}`;
if (include) uri += `?include=${include}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really sure what include is from the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed in 436c114.

@@ -6,20 +6,28 @@ const trackDependency = require('../appinsights/AppInsightsClient').trackDepende

const apiUrlBase = process.env.FORTIS_FEATURE_SERVICE_HOST;

function formatIdsUri(ids) {
return `${apiUrlBase}/features/id/${ids.map(encodeURIComponent).join(',')}?include=bbox`;
function formatIdsUri(ids, include) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does include have to be a parameter passed in to all of these methods? Could it be a member of the FeatureServiceClient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FeatureServiceClient is not an object, these are all just functions. What we want to include depends on the context of the request (e.g. some callers need the bbox, others don't), so this really should be configurable on a per-call level.

@c-w c-w merged commit 0b6c61d into master Aug 16, 2017
@c-w c-w deleted the fix-edges-queries branch August 16, 2017 20:40
@c-w c-w removed the in progress label Aug 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants