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

Remove usage of MessagesSchema byLocations endpoint #10

Merged
merged 3 commits into from Aug 9, 2017
Merged

Conversation

@c-w
Copy link
Contributor

@c-w c-w commented Jul 28, 2017

In Fortis-V1, when the user selects the filter terms, we have a distinction between keywords (like "Isis" or "Bomb") and locations (like "Syria" or "Benghazi"): if we query for all events that contain a keyword, we'll match the keyword in the text, if we query for all events that contain a location, we instead match for the location's coordinates in the event's metadata.

In Fortis-V2, we no longer support this distinction as it's much easier to just treat keywords and locations the same by just using the locations' name as an additional keyword for which to query.

As a result, we need to remove the usage of the MessagesSchema byLocations endpoint in the GraphGL client. Note that the change of deprecating this endpoint has already been made on the GraphQL service, in the branch https://github.com/CatalystCode/project-fortis-services/tree/implement-tiles-resolver

In Fortis-V1, when the user selects the filter terms, we have a
distinction between keywords (like "Isis" or "Bomb") and locations (like
"Syria" or "Benghazi"): if we query for all events that contain a
keyword, we'll match the keyword in the text, if we query for all events
that contain a location, we instead match for the location's coordinates
in the event's metadata.

In Fortis-V2, we no longer support this distinction as it's much easier
to just treat keywords and locations the same by just using the
locations' name as an additional keyword for which to query.

As a result, we need to remove the usage of the MessagesSchema
byLocations endpoint in the GraphGL client. Note that the change of
deprecating this endpoint has already been made on the GraphQL service,
in the branch:
https://github.com/CatalystCode/project-fortis-services/tree/implement-tiles-resolver
@c-w c-w requested a review from erikschlegel Jul 28, 2017
@c-w c-w changed the title Remove usage of byLocations endpoint Remove usage of MessagesSchema byLocations endpoint Jul 28, 2017
c-w added a commit that referenced this issue Jul 28, 2017
This is a sister-change to pull request
#10, see
the the description of that change for further information.
Smarker
Smarker approved these changes Aug 7, 2017
Copy link
Contributor

@Smarker Smarker left a comment

LGTM - with comments


SERVICES.FetchMessageSentences(siteKey, originalSource, bbox, datetimeSelection, timespanType,
limit, offset, edges, DEFAULT_LANGUAGE, Actions.DataSources(filteredSource),
categoryValue?categoryValue.name.toLowerCase():categoryValue, searchValue, location, callback);
categoryValue?categoryValue.name.toLowerCase():categoryValue, searchValue, undefined, callback);
Copy link
Contributor

@Smarker Smarker Aug 7, 2017

I'm hesitant to pass in undefined as a parameter, since this might introduce new errors. Would there be a way to change the signature of this so it doesn't take a location parameter?

Copy link
Contributor Author

@c-w c-w Aug 7, 2017

I usually like keeping changes minimal to keep risk low, but sure. Done in 0f94c6b.

Copy link
Contributor

@Smarker Smarker Aug 7, 2017

yea makes sense

Smarker
Smarker approved these changes Aug 7, 2017
Copy link
Contributor

@Smarker Smarker left a comment

LGTM

Copy link
Collaborator

@erikschlegel erikschlegel left a comment

LGTM with one minor question below.

@@ -704,23 +704,13 @@ export const SERVICES = {

let query, variables;

if (coordinates && coordinates.length === 2) {
query = ` ${fragmentView}
query ByLocation($site: String!, $originalSource: String, $coordinates: [Float]!, $filteredEdges: [String]!, $langCode: String!, $limit: Int!, $offset: Int!, $fromDate: String!, $toDate: String!, $sourceFilter: [String], $fulltextTerm: String) {
Copy link
Collaborator

@erikschlegel erikschlegel Aug 9, 2017

why are we getting rid of ByLocation? This is used when you select a place from the popular locations donut chart.

Copy link
Contributor Author

@c-w c-w Aug 9, 2017

Let me merge this and then I'll have to revert this and #11 which already got merged.

@c-w c-w merged commit 77f16ee into master Aug 9, 2017
@c-w c-w deleted the remove-bylocations branch Aug 9, 2017
c-w added a commit that referenced this issue Aug 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants