Skip to content
This repository has been archived by the owner. It is now read-only.

Remove query on placeid #94

Merged
merged 1 commit into from Aug 14, 2017

Conversation

Projects
None yet
3 participants

@c-w c-w requested a review from erikschlegel Aug 10, 2017

c-w added a commit to CatalystCode/project-fortis-pipeline that referenced this pull request Aug 10, 2017

Remove eventids collection
This is the schema change for the following service-side changes:
CatalystCode/project-fortis-services#92
CatalystCode/project-fortis-services#94
@Smarker
Copy link
Contributor

left a comment

LGTM

@@ -49,64 +48,61 @@ function byBbox(args, res) { // eslint-disable-line no-unused-vars
if (!args.bbox || args.bbox.length !== 4) return reject('Invalid bbox specified');

const { fromDate, toDate } = parseFromToDate(args.fromDate, args.toDate);
const limit = args.limit || 15;

This comment has been minimized.

Copy link
@Smarker

Smarker Aug 14, 2017

Contributor

I'd maybe use a name more specific than limit to make it more clear why it's needed.

@c-w c-w merged commit 6bac34c into master Aug 14, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@c-w c-w deleted the remove-placeid-query branch Aug 14, 2017

@erikschlegel
Copy link
Collaborator

left a comment

Looks good. Just one posted question below.

const [north, west, south, east] = args.bbox;

const tagsQuery = `
SELECT eventid

This comment has been minimized.

Copy link
@erikschlegel

erikschlegel Aug 14, 2017

Collaborator

would distinct eventid work for this query? Trying to mitigate the duplicate record issue.

This comment has been minimized.

Copy link
@c-w

c-w Aug 14, 2017

Author Member

@erikschlegel How do you propose to address this?

image

This comment has been minimized.

Copy link
@erikschlegel

erikschlegel Aug 14, 2017

Collaborator

it was more of a question which you just answered.

@erikschlegel
Copy link
Collaborator

left a comment

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.