Adds location field to Group and SchoolActionStat types #263

aaronschachter · 2020-07-31T17:30:05Z

What's this PR do?

This pull request adds support for the location fields and filters added to the Group and ActionStat resources in DoSomething/rogue#1082.

Example request:

{
  schoolActionStats(location: "US-TX") {
    actionId
    location
    schoolId
    impact
  }
}

Example response:

{
  "data": {
    "schoolActionStats": [
      {
        "actionId": 11,
        "location": "US-TX",
        "schoolId": "4809500",
        "impact": 8
      }
    ]
  },
  ...

How should this be reviewed?

👀

Any background context you want to provide?

We'll eventually drop the state column from the groups table once Rogue and Phoenix are updated to query location per the discussions in DoSomething/rogue#1079 (comment)

Relevant tickets

References Pivotal #174021616.

Checklist

This PR has been added to the relevant Pivotal card.
Added appropriate feature/unit tests.


        Adds location field to ActionStat and Group types


        Adds location arg to schoolActionStats query


        Fix lint errors

DFurnes

Looks good! 🌐

DFurnes · 2020-08-03T17:47:11Z

src/schema/rogue.js

@@ -214,8 +214,10 @@ const typeDefs = gql`
    goal: Int
    "The group city."
    city: String
+    "The group location."
+    location(format: LocationFormat = ISO_FORMAT): String


Right now, we handle this format argument on posts by deciding which Rogue property to return for this field. Since we don't have a locationName on stats (computed here for posts), we should probably omit this format argument.

If we are planning to add that on the ActionStat resource in Rogue though, I'm fine to keep! (And, longer term, I wonder if this ISO-to-human conversion is something we can just handle in GraphQL for simplicity...)

Thanks for catching this -- I'll remove. Didn't realize there was a location_name property at all, and then 👀 DoSomething/rogue#842 when digging around..

ISO to human in GraphQL sounds like the way to go -- I figured that adding ISO support to schools would just be done within GraphQL so we don't have to alter our Great Schools import (which is simply just importing their raw data)

src/schema/rogue.js


        Update src/schema/rogue.js

Co-authored-by: David Furnes <[email protected]>


        Cleanup per #263 review

aaronschachter added 3 commits Jul 31, 2020

Adds location field to ActionStat and Group types

Loading status checks…

309c6a9

Adds location arg to schoolActionStats query

Loading status checks…

cd78ca1

Fix lint errors

Loading status checks…

12da4c0

aaronschachter requested a review from DoSomething/applications as a code owner Jul 31, 2020

aaronschachter requested a review from Vmack Jul 31, 2020

DFurnes approved these changes Aug 3, 2020

View changes

aaronschachter and others added 2 commits Aug 3, 2020

Update src/schema/rogue.js

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

5ba8054

Co-authored-by: David Furnes <[email protected]>

Cleanup per #263 review

Loading status checks…

806d378

aaronschachter merged commit 54a41e3 into master Aug 3, 2020
1 check passed

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details

aaronschachter deleted the groups-location branch Aug 3, 2020

This was referenced Aug 11, 2020

Query groups by location instead of state DoSomething/phoenix-next#2324

Merged

Adds School location field/filter #268

Merged

DoSomething / graphql

Adds location field to Group and SchoolActionStat types #263

Adds location field to Group and SchoolActionStat types #263

aaronschachter commented Jul 31, 2020 •

edited

DFurnes left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

DoSomething / graphql

Join GitHub today

Adds location field to Group and SchoolActionStat types #263

Adds location field to Group and SchoolActionStat types #263

Conversation

aaronschachter commented Jul 31, 2020 • edited

What's this PR do?

How should this be reviewed?

Any background context you want to provide?

Relevant tickets

Checklist

DFurnes left a comment

This comment has been minimized.

DFurnes Aug 3, 2020

This comment has been minimized.

DFurnes Aug 3, 2020

This comment has been minimized.

aaronschachter Aug 3, 2020

aaronschachter commented Jul 31, 2020 •

edited