Projects to relay #1460

mattkrick · 2017-11-01T03:39:47Z

it's time to bite the bullet. this will fix up faulty cache invalidations, speed up dashboard loading by a bunch, and enable us to point to a specific card on the dashboard, which is necessary when a notification says "you've got a new card, click to see it".

this refactor is really painful because Relay makes you use a "Global ID" for entities, which is base64(type, id). That means if a function can get called from something populated by both cashay & relay, then we have to handle both. It's also a challenge because foreign keys need to be converted to Global IDs, too. For example, if a team has an activeFacilitator then team.activeFacilitator must get matched with a TeamMember.id. A project has a teamMemberId, too. So, now that it comes from Relay we have to make sure the IDs are correct there, too.
... All of that to say, despite being 200 changed files, I'm really surprised it isn't bigger.

tests:

UI should be exactly the same, except load no less than 400% faster (also implements optimistic UI on just about everything)
meeting facilitator shouldn't lose their facilitator role if they lose connectivity for a second (we now check every 5 seconds, instead of immediately)
fix Notification bell count only updates on refresh #1536 where notification subscriptions don't work right

things out of scope, but still need fixing (for another, smaller, PR):

a teamMemberRemoved subscription. without it, if a teamMember is removed, lists won't update in real time.
colocate notification data needs to speicific components. without it, it'll get a little unruly
implement dataLoader throughout. this will reduce the number of lookups for all hits to graphql.

finish createProject mutation

dan-f · 2017-12-08T17:28:10Z

Upon completion of a meeting with just myself in which I added a project during the updates phase and also reordered a few projects during the updates phase.

Reloading the page results in the same error.

dan-f · 2017-12-08T17:36:49Z

Not seeing my name during the agenda phase of the meeting

dan-f · 2017-12-08T17:37:40Z

I wrapped up the meeting as the facilitator, but my other attendee ends up stuck in the meeting still.

mattkrick · 2017-12-08T18:56:55Z

NICE!

fixed the dates in the first one (relay doesn't like the Date type, so everything needs to support ISO8601 strings)
added picture & preferredName to the MeetingAgendaItems fragment
that 3rd one is gonna be trickier, still working on repro

mattkrick · 2017-12-08T19:00:07Z

MOAR BUGZ:

optimistic greeting is a string of draftJS json
callingMember is sometimes undefined when on the dev server

mattkrick · 2017-12-10T21:06:53Z

okie dokie, all ready!

mattkrick · 2017-12-11T03:27:17Z

@jordanh @ackernaut if you guys have the time, i'd love to get some more fingers on this! it's currently up on the dev server. especially try to break the meeting.

dan-f

Hey Matt!

You've definitely kept yourself busy here! I'm still doing my review, but figured I'd share some of my comments so far. Some caveats:

Due to the enormous scope of this work, I haven't been able to go into too much detail, though there are some lines of code that sort of arbitrarily caught my eye to poke at ¯_(ツ)_/¯
Though I'm mostly done looking at the backend changes, I have some reservations that I'm going to try to put to words in a bit around the concept of the shared data loader
I'm wondering about when you decide to put data resolution logic in a GraphQL type definition vs in the query modules.

That's it for now! More on data loaders and the relay / cache layer later today.

dan-f · 2017-12-11T17:52:18Z

src/server/utils/RethinkDataLoader.js

+};
+
+
+export default class RethinkDataLoader {


My understanding of DataLoader based on the README was that the API client is intended to have one unique loader per request, per data object. So callng this thing "RethinkDataLoader" is a bit misleading. It's more of a grab-bag of data loaders. I'd be tempted to simply export the various loaders separately.

yeah, the readme isn't great about realworld examples, however there is one example here: https://github.com/facebook/dataloader#caching-per-request

this is the standard practice in other open source projects, although we all suffer with what to call it! There's a dataloader, then a dataloader bag, then a shared data loader is a bag of dataloader bags.

there's no value in exporting the separately because you don't know what you'll need at the time of creation, unless in misunderstood?

dan-f · 2017-12-11T18:02:34Z

src/server/graphql/makeSubscribeIter.js

    if (filterFn(value)) {
+      if (value.operationId) {
+        if (!dataLoader) console.log('NO DL', channelName);


if !dataLoader, we still hit L18.

ah good call, i'll just remove this & the else clause, they were just to help in dev.

dan-f · 2017-12-11T18:40:06Z

src/server/graphql/mutations/__tests__/endMeeting.test.js

@@ -38,7 +38,7 @@ describe('endMeeting', () => {
      teamMember: r.table('TeamMember').getAll(teamId, {index: 'teamId'}).orderBy('preferredName')
    }, dynamicSerializer);
    expect(db).toMatchSnapshot();
-    expect(mockFn).toBeCalledWith(teamId);
+    expect(dataLoader.isShared()).toEqual(true);


This feels a bit like testing the implementation. As long as we get the data we asked for, we're good, right?

right you are!
the point of sharing the dataloader is to make sure we don't hit the database n+1 times (n subscribers + 1 mutation). we'll still get the right result if we don't share, but it'll be much more expensive & virtually impossible to pinpoint the source. If you want i can remove

dan-f · 2017-12-11T18:52:41Z

src/server/graphql/mutations/addAgendaItem.js

+
+    // RESOLUTION
+    const now = new Date();
+    const agendaItem = await r.table('AgendaItem').insert({


Just giving a heads-up: I've been asynchronously working on adding data constructors and functions for operating on well-defined data so that we can reduce the usage of ad-hoc object literals and ID generation: https://github.com/ParabolInc/action/tree/data-constructors-and-accessors/src/universal/data

dan-f · 2017-12-11T19:19:25Z

src/server/graphql/rootQuery.js

      })
    },
-    ...agenda,
-    ...meeting,


Are these definitely unused?

yep! everything with a spread operator is the old format. as i switch stuff to relay, I remove the legacy stuff. The relay convention is to store this stuff on the "viewer" (read User). so it'd be like:

viewer { team(teamId: "123") { agendaItems { id content } } }

it's actually pretty great, relations are a lot easier to see in code & since everything is stored under the viewer you never need to check for the userId authorization because that's baked in.

dan-f · 2017-12-11T20:12:41Z

src/server/graphql/types/TeamMember.js

+          type: GraphQLISO8601Type,
+          description: 'the datetime cursor'
+        }
+        // private: {


Code comment reminder

dan-f · 2017-12-11T20:13:55Z

src/server/graphql/types/User.js

@@ -103,6 +107,12 @@ const User = new GraphQLObjectType({
      type: GraphQLString,
      description: 'The application-specific name, defaults to nickname'
    },
+    // presence: {


Code comment reminder

removed in a subsequent PR (don't wanna cause too many conflicts)

dan-f · 2017-12-11T20:33:04Z

src/server/graphql/types/User.js

@@ -111,6 +121,31 @@ const User = new GraphQLObjectType({
        return (userId === source.id) ? source.tms : undefined;
      }
    },
+    teams: {


At this point, I'm confused by when we decide to handle field resolution in server/graphql/queries modules vs. in server/graphql/types modules.

Type vs. Field. let's chat it up if it's still a little foggy, once you get it down it'll make GraphQL seem a lot cooler!

dan-f · 2017-12-11T20:42:30Z

src/server/safeMutations/reactivateTeamMembersAndMakeNotifications.js

@@ -37,6 +37,10 @@ const reactivateTeamMembersAndMakeNotifications = async (invitees, inviter, team
  reactivatedUsers.forEach(({id: userId, tms}) => {
    getPubSub().publish(`${NEW_AUTH_TOKEN}.${userId}`, {newAuthToken: tmsSignToken({sub: userId}, tms)});
  });
+  reactivatedTeamMembers.forEach((teamMember) => {
+    const teamMemberAdded = {teamMember};
+    getPubSub().publish(`${TEAM_MEMBER_ADDED}.${teamId}`, {teamMemberAdded, ...subOptions});


Are inactive team members actually removed from teams?

we set an isNotRemoved: false flag on it. That way, archived projects still have an owner even when the person is long gone.

dan-f · 2017-12-11T20:43:35Z

src/server/socketHandlers/scConnectionHandler.js

@@ -21,16 +22,16 @@ const isTmsValid = (tmsFromDB = [], tmsFromToken = []) => {
  return true;
 };

-export default function scConnectionHandler(exchange) {
+export default function scConnectionHandler(exchange, sharedDataLoader) {
  return async function connectionHandler(socket) {
    // socket.on('message', message => {
    //   if (message === '#2') return;
    //   console.log('SOCKET SAYS:', message);


oh yeah, good for debuggin, but this'll be gone soon.

dan-f

Alright, I think I've sorted out my remaining thoughts on the backend changes. It's getting late here, so I'll finish up with the front-end/relay part of the review tomorrow morning.

The dataloader concept was new to me, so I spent a bit reading up on their docs. Seems like a really reasonable concept, and a simple caching layer. Thanks for adding it!

I have some suggestions on changing how we integrate dataloader into our architecture. In general terms, I think that the current approach in this PR creates our dataloader instances at an event in the application lifecycle which is a bit too early and could result in a shared memory bugs for us down the road.

From the dataloader readme:

DataLoader caching does not replace Redis, Memcache, or any other shared application-level cache. DataLoader is first and foremost a data loading mechanism, and its cache only serves the purpose of not repeatedly loading the same data in the context of a single request to your Application.

Avoid multiple requests from different users using the DataLoader instance, which could result in cached data incorrectly appearing in each request. Typically, DataLoader instances are created when a Request begins, and are not used once the Request ends.

So the authors recommend to manage dataloader instances at the request lifecycle level, meaning each request should get its own instance of dataloader.

However, based on my understanding of SocketCluster, we're currently creating one SharedDataLoader per worker at the moment the worker is born (worker.js L41). That piece of memory is then read and written to by every HTTP and websocket connection which is handed off to that worker.

I suggest we do the following:

For HTTP requests:
- Create our set of dataloaders at the moment that the request is born. I'm no expert in express, so perhaps this means adding some sort of middleware such that we can pass the dataloaders through the middleware stack, or simply creating the dataloaders in the per-request httpGraphQLHandler.
For WebSocket connections:
- My understanding was that the requests to subscribe/unsubscribe don't do much data fetching on their own, so let's not worry about caching data in these cases yet.
- For the procedure which handles GraphQL requests over WebSockets (scGraphQLHandler) we're essentially imposing a request/response model on top of websockets. So at the moment the "request" (e.g. "graphql" message) comes in, we should create our dataloaders, and those loaders should then be garbage-collected when that procedure returns.
I took a look at the 'shared-dataloader' library, and I can't quite tell what the main sell is. Since the recommended usage of 'dataloder' is to not state across request lifetime boundaries, I'd say we should probably just use plain ol' dataloader.

dan-f · 2017-12-12T00:36:50Z

src/server/graphql/types/Project.js

+    editors: {
+      type: new GraphQLList(ProjectEditorDetails),
+      description: 'a list of users currently editing the project (fed by a subscription, so queries return null)',
+      resolve: ({editors = []}) => {


Alternatively, in the future, we could keep this data in an in-memory cache such as redis

dan-f · 2017-12-12T00:37:13Z

src/server/graphql/types/Project.js

@@ -38,7 +44,11 @@ const Project = new GraphQLObjectType({
      type: GraphQLFloat,
      description: 'the shared sort order for projects on the team dash & user dash'
    },
-    status: {type: new GraphQLNonNull(ProjectStatusEnum), description: 'The status of the project'},
+    // TODO make this nonnull again


What's keeping it from being non-null?

a bug in the relay compiler that makes fragments order dependent :-(.
The more i work in GraphQL, the more i dislike nonnulls, so i may just keep it out.

mattkrick · 2017-12-12T05:21:37Z

I'm writing a medium article on what shared-dataloader does, so i'll be brief:
In short,

yes, dataloaders are usually per-request, but not always. it was built before graphql supported subscriptions.
no, the shared dataloader does NOT create dataloaders at the worker level. it just creates the bag. dataloaders are still created per-request. if they aren't shared, they're destroyed on that same request. If they get shared, they get destroyed in 5 seconds. the bag simply allows 2 subscribers on the same worker that want the same document to share database results. to understand why this is harder than sending a fixed payload to all subscribers, i can talk your ear off or you can read about data transform pipelines in the GraphQL RFC here http://github.com/facebook/graphql/blob/master/rfcs/Subscriptions.md. To see how we implement this, see https://github.com/ParabolInc/action/blob/master/src/server/graphql/makeSubscribeIter.js
the problem it solves is here: Context lifecycle in subscriptions graphql/graphql-js#894

Hope that clears some stuff up! Let's chat about dataloaders & fields vs. types tomorrow

dan-f · 2017-12-12T15:12:01Z

no, the shared dataloader does NOT create dataloaders at the worker level. it just creates the bag.

Duly noted! I think the singular naming was confusing me a bit. So we have a SharedDataLoader which has many DataLoaders, and it just so happens that our RethinkDataLoader itself is a bag of many data loaders. Perhaps we can iterate a bit on the naming? RethinkDataLoaders with an s?

I'm still not convinced that we should be implementing a global database cache in the application itself. Let's talk more about this today! Ping me when you wanna chat, and until then I'll be continuing with the review.

dan-f

Alright! I think I've done what I can with the front-end review. This wraps up my feedback! Here goes:

Patterns:

Can we define our naming conventions for fragment containers, query roots, redux containers, etc.? They're all sort of "container" components, but I don't know if I've seen a consistent pattern yet.
With the relay mutations, there's a pattern of using the optimisticUpdater rather than the more declarative optimisticResponse. Is there something keeping us from using optimisticResponse?

Documentation:

Being newer to this codebase, it would be really useful for me if our modules, functions, classes, components, etc. could have some sort of minimal documentation. I realize that code changes over time and docs get stale, and I'm not asking for JSDoc level documentation. But I think something along the lines of, e.g., // this container component renders a component during the error, loading, and ready states of it's data lifecycle would be really useful for speeding up these reviews and also for just remembering when to use which components.
- While the source code tells me how we've implemented something, a good name and a quick comment tells me why we're implementing it, what it does, and when I should use it.
- This would be especially useful for cases in which we're overriding a framework module.
Let's avoid using PropTypes.any. If we know that we're expecting a field with some set of values, then we know its type. If the type of a field is some ad-hoc class instance, we can always use PropTypes.instanceof. At the very least a comment accompanying a PropTypes.any declaration would be much appreciated.

dan-f · 2017-12-12T15:33:03Z

src/universal/components/AsyncEditorSuggestions.js

+      {suggestions && suggestions.map((suggestion, idx) => {
+        return (
+          // eslint-disable-next-line
+          <div key={idx} onMouseDown={dontTellDraft} onClick={handleSelect(idx)}>


Rather than adding click handlers to divs, let's use the PlainButton component.

the plain button still has some styles that i don't want since here, since the child is non deterministic. I should make an PlainListItem component!

dan-f · 2017-12-12T15:46:14Z

src/universal/components/DashboardWrapper/DashboardWrapper.js

+  NotificationsClearedSubscription
+];
+
+const DashboardWrapper = ({atmosphere, dispatch, history, location}) => {


Now that this Component is more of a container component why don't we call this something like DashboardContainer and move it to the containers directory?

I agree, & it's changed a lot since this PR! Captured in #1559 so i don't bork the future changes

dan-f · 2017-12-12T15:47:46Z

src/universal/components/EditingStatus/EditingStatus.js

@@ -61,4 +65,13 @@ const styleThunk = (custom, {isEditing}) => ({
  }
 });

-export default withStyles(styleThunk)(EditingStatus);
+export default createFragmentContainer(


Time to move to containers?

So hear me out, how I think of this is there is an EditingStatus component that is wrapped in a container, and that container is nothing more than the thing the HOC provides. This is the relay-approved way of doing things because the fragment name must match the component name that it's tied to, so if we tied it to EditingStatusContainer, we'd be breaking that convention.

dan-f · 2017-12-12T15:51:15Z

src/universal/components/EditorInputWrapper.js

+import entitizeText from 'universal/utils/draftjs/entitizeText';
+
+
+class EditorInputWrapper extends Component {


Pretty sure this can be written as a functional component or extend PureComponent.

Also, regarding naming, if this is the component we should be using when we want to edit DraftJS content, let's just call it something like Editor, since the views it's wrapping are opaque to the caller.

Editor is the name of the component that Draft-js uses.
ProjectEditor is what we use when we wanna use draft-js inside a Project
This is an editor that I envision we'll use whenever we wanna swap out an input box. So I should probably call in InputEditor? but then we have an Editable that is an input. I'll rename & add comments in #1559

dan-f · 2017-12-12T18:14:02Z

src/universal/components/PlainButton/PlainButton.js

@@ -11,7 +11,7 @@ const PlainButton = ({styles, ...props}) => (
 );

 PlainButton.propTypes = {
-  children: PropTypes.arrayOf(PropTypes.element),
+  children: PropTypes.any,


Hm, looks like this should be PropTypes.node!

dan-f · 2017-12-12T18:47:23Z

src/universal/modules/invoice/containers/InvoiceRoot.js

@@ -24,7 +24,7 @@ const InvoiceRoot = ({atmosphere, match: {params: {invoiceId}}}) => {
      variables={{invoiceId}}
      render={({error, props: queryProps}) => {
        return (
-          <TransitionGroup appear style={{overflow: 'hidden'}}>
+          <TransitionGroup appear component={null}>


component={null}?

the things i do for backwards compatibility... reactjs/react-transition-group#242

dan-f · 2017-12-12T18:49:35Z

src/universal/modules/meeting/components/MeetingAgendaItems/MeetingAgendaItems.js

-    team
-  } = props;
+class MeetingAgendaItems extends Component {
+  state = {};


state = { agendaProjects: [] }

dan-f · 2017-12-12T19:02:27Z

src/universal/modules/meeting/components/MeetingRoot.js

+  match: PropTypes.object.isRequired
+};
+
+export default connect()(withRouter(withAtmosphere(MeetingRoot)));


Rather than calling react-redux's connect on a parent which itself doesn't need dispatch, can we let the children that need access to dispatch connect themselves?

that's a good practice! In this case, MeetingRoot does need dispatch to feed to the subParams because a certain subscription needs it (NotificationsAdded). May be a code smell to do it like this, but I don't know how else to couple the subscription to the query (and unfortunately they need coupling since we need to flush the query if the sub dies).

dan-f · 2017-12-12T19:03:20Z

src/universal/modules/meeting/components/MeetingUpdates/MeetingUpdates.js

-  const myUserId = myTeamMemberId && myTeamMemberId.split('::')[0];
-  const isMyMeetingSection = myTeamMemberId === currentTeamMember.id;
+class MeetingUpdates extends Component {
+  state = {};


We should be declaring the shape of state, even if it's initially empty.

dan-f · 2017-12-12T19:24:50Z

src/universal/mutations/DeleteProjectMutation.js

+  if (isTempId(projectId)) return undefined;
+  const {viewerId} = environment;
+  const updater = (store) => {
+    removeFromProjectConnections(store, viewerId, projectId, teamId);


Rather than imperatively working with the store, can we not just use the NODE_DELETE config for this CRUD operation?

oh such a good question.
so those configs are a carry over from relay classic-- back then, they didn't have the imperative API or the proxy stores. you did everything with these configs. and. people. hated. it.

let's say Matt joins the team. do you append him or prepend him to the team members array? Neither, he is in the freakin middle. where's that option?

let's say you use a GraphQLList instead of a Connection. You have to use the imperative approach, anyways.

let's say you're doing something aside from the most basic add/delete, like maybe you need some arguments in your pathToConnection, but it's not working. How do you trace your steps?

IIRC RANGE_DELETE removes the node from the array, but doesn't GC the node, yet. NODE_DELETE deletes the node, and so now your array has an undefined in it, which propagates to your view layer & now you're wondering why a card is undefined... it's no fun.

FWIW, the todo-modern example uses the imperative approach while the classic todo uses the config approach, so i take that as a "yeah, we like this way better, but there's no reason to have to rewrite your old code if it's working". Personally, I don't really care that much, but I'd prefer to write everything the same way instead of starting out using a config, finding it breaks or the business logic changes, then using the imperative approach anyways.

mattkrick added 30 commits October 30, 2017 16:00

initial dataloader implementation for orgUserCount

1ae2cc8

remove archived projects subscription

9a7f465

move /me to relay

4a7e78e

handle serialized timestamps in the projects

1f751d4

wire up team dash for relay

d410239

set up server sub events for projects

4402dd1

add concept of loading to QueryRenderer. handle project connections

75d1082

support dataloader with graphql subscriptions

96b4918

fix mem leak in new dataloader

0b394ff

move updateProject to relay

d83bcb0

move RelayProject to Project

32cd327

finish createProject mutation

add projectCreated subscription

923edb9

add deleteProject mutation

6689952

add projectDeleted subscription

4f4de3c

add MeetingRoot

dbeab32

add meetingUpdated subscription

c8cff0a

add startMeeting mutation

9a2c57b

add optimisticUpdater to startMeeting

78726e1

add moveMeeting mutation

50db310

add killMeeting mutation

138c832

add endMeeting mutation

80103c8

move checkIn to relay

8aeb9bc

add teamMemberUpdated subscription

15affeb

wire up updateTeamCheckInQuestion

516fd09

allow facilitators to refresh without a new election

86d620b

fix promote to facilitator from toast

17f9261

begin refactoring meeting updates

150f47d

factored out operationId of sharedDataLoader

5ac70f4

break projects out by team member

1b43e8b

fix race condition in new shareddataloader

45ba6ac

mattkrick requested a review from dan-f December 8, 2017 06:00

mattkrick mentioned this pull request Dec 8, 2017

Notification bell count only updates on refresh #1536

Closed

mattkrick changed the title ~~Epic 8 - Projects to relay [WIP]~~ Epic 8 - Projects to relay Dec 8, 2017

mattkrick changed the title ~~Epic 8 - Projects to relay~~ Projects to relay Dec 8, 2017

mattkrick mentioned this pull request Dec 8, 2017

[object Object] is now facilitator bug #1420

Closed

fix dates from relay and add team member fields to agenda items

9d228d0

mattkrick added 7 commits December 8, 2017 14:16

fix optimistic greeting, self-connected presence

07d663e

fix editProject payload, double send of updateProject

5474e8c

new dataloader API to prepare for graphql microservice

29ec485

fix loading & opId warning

a434f30

lint

9f81964

fix tests

9089057

fix "off of undefined" sentry bug

15b8bc1

move updateAgendaItem to relay

8b308cf

dan-f reviewed Dec 11, 2017

View reviewed changes

dan-f suggested changes Dec 12, 2017

View reviewed changes

clean up comments & console logs

3e5862a

dan-f suggested changes Dec 12, 2017

View reviewed changes

add initial state & small fixes

7c50df4

mattkrick merged commit 88b6715 into epic8.5 Dec 12, 2017

mattkrick deleted the projects-to-relay branch January 19, 2018 15:23

		import entitizeText from 'universal/utils/draftjs/entitizeText';


		class EditorInputWrapper extends Component {

Projects to relay #1460

Projects to relay #1460

Conversation

mattkrick commented Nov 1, 2017 • edited Loading

dan-f commented Dec 8, 2017 • edited Loading

dan-f commented Dec 8, 2017

dan-f commented Dec 8, 2017

mattkrick commented Dec 8, 2017 • edited Loading

mattkrick commented Dec 8, 2017 • edited Loading

mattkrick commented Dec 10, 2017

mattkrick commented Dec 11, 2017

dan-f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkrick commented Dec 12, 2017 • edited Loading

dan-f commented Dec 12, 2017

dan-f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkrick Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkrick Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

mattkrick commented Nov 1, 2017 •

edited

Loading

dan-f commented Dec 8, 2017 •

edited

Loading

mattkrick commented Dec 8, 2017 •

edited

Loading

mattkrick commented Dec 8, 2017 •

edited

Loading

mattkrick commented Dec 12, 2017 •

edited

Loading

mattkrick Dec 12, 2017 •

edited

Loading

mattkrick Dec 12, 2017 •

edited

Loading