Timezones reduxification #10934

retrofox · 2017-01-25T14:18:44Z

~~This PR depends on #10932.~~

This PR reduxifies the Timezone data adding actions, reducer, etc. into the state and, also, add the timezones to the data-layer library.

Testing

Run client tests

Redux implementation

> npm run test-client client/state/timezones/test/

Data-layer

> npm run test-client client/state/data-layer/wpcom/timezones/test/

Also, an easy way to inspect would be to type the following in the browser console:

dispatch( { type: 'TIMEZONES_REQUEST' } );

And you should see the receive dispatch in redux dev tools:

matticbot · 2017-01-25T14:18:45Z

Test live: https://calypso.live/?branch=update/timezones-state-tree

gwwar

This looks pretty good so far @retrofox 👍 I'll defer to @dmsnell for any data-layer suggestions

gwwar · 2017-01-25T22:57:10Z

client/state/data-layer/wpcom/timezones/index.js

+	timezonesReceiveAction,
+} from 'state/timezones/actions';
+
+const undocumented = wpcom.undocumented();


For example, the WordPress.com API middleware mirrors the WordPres.com API and its files should thus mirror the API's structure.

@dmsnell should the filename here be client/state/data-layer/wpcom/timezones/index.js or client/state/data-layer/wpcom/undocumented/timezones/index.js

gwwar · 2017-01-25T23:01:38Z

client/state/timezones/schema.js

+
+export const timezonesSchema = {
+	type: 'object',
+	additionalProperties: false,


Generally we only need this when using a patternProperty. It'd allow anything outside of the regex otherwise.

gwwar · 2017-01-25T23:02:28Z

client/state/timezones/schema.js

+		},
+
+		timezones_by_continent: {
+			type: 'object',


Let's add additionalProperties: false, here

gwwar · 2017-01-25T23:04:29Z

client/state/timezones/test/actions.js

+describe( 'actions', () => {
+	describe( 'creators functions', () => {
+		it( '#timezonesRequestAction()', () => {
+			expect( timezonesRequestAction() ).to.eql( ACTION_TIMEZONES_REQUEST );


It doesn't look like we're reusing the action values. It'd be a bit easier to read if the data is inlined here then.

gwwar · 2017-01-25T23:07:52Z

client/state/timezones/test/reducer.js

+			const action = timezonesReceiveAction( timezones );
+
+			const expectedState = timezones;
+			deepFreeze( expectedState );


We generally only need to deep freeze initialState unless you think the tests might manipulate the fixture data. If you think that might be the case for the fixture, maybe consider moving that test setup out of these it blocks.

dmsnell

Yay more time functions 🙃

I've left some comments related to general patterns and idioms in Redux and the middleware. Please don't hesitate to ask for any help or clarification.

dmsnell · 2017-01-25T22:59:23Z

client/state/data-layer/wpcom/timezones/index.js

+		.timezones()
+		.then( zones => {
+			dispatch( timezonesRequestSuccessAction() );
+			dispatch( timezonesReceiveAction( zones ) );


the zones object here is completely opaque to us in Calypso and makes no guarantees on what we import from the API.

one very critical piece of the API middleware is that we provide a strong interface to translate data in its API form into the shape of data that Calypso wants to use it.

for example, the snake-case naming of the variable is entirely un-idiomatic for JavaScript code. this is the perfect place to use fromApi() to normalize the names

const fromApi = data => ( { manualUtcOffset: data.manual_utc_offsets, timezonesByContinent: data.timezones_by_continent, } )

just with this one simple step we have both made the names more natural to Calypso and we have also given a declarative shape to the data. we can do more though. is this the way we most want our data to look?

const fromApi = data => ( { ...data.timezones_by_continent, rawOffsets: data.manual_utc_offsets, } );

in this case we've made a single big object and merging the offsets with the continent values. the point isn't that this is the right way to shape the data, but rather that this is the gateway and here we have the chance to robustly conform and validate the data from the API into Calypso-native format.

it will be significantly easier to test/mock/dispatch form the console if we can clearly see in the code how the data should look instead of needing to visit the PHP code or testing the API.

the zones object here is completely opaque to us in Calypso and makes no guarantees on what we import from the API.

one very critical piece of the API middleware is that we provide a strong interface to translate data in its API form into the shape of data that Calypso wants to use it.

Make sense and sound great.

this is the perfect place to use fromApi() to normalize the names

👍 We should pay attention to those redux implementations which are already assembling their data gotten from the REST API though.

yeah I've had this concern for a while but unlike scattered redux-thunk action creators the middleware gives us one centralized and clear place to put them. when I first started doing this with the importer it ended up saving me considerable headaches because as the API changed (it did) and as we better understood the process (we learned) there was a single spot to update the code and nothing broke.

this is the power of abstracting at the data object level which Calypso expects rather than at the API level.

dmsnell · 2017-01-25T23:01:43Z

client/state/data-layer/wpcom/timezones/index.js

+ */
+export const requestTimezones = ( { dispatch } ) => (
+	undocumented
+		.timezones()


Is this an existing function inside of undocumented()?

With the data middleware I'm strongly encouraging people to rely more on wpcom.req.get() than writing code inside of wpcom.js and then calling it.

As we move (hopefully very soon) to using the HTTP middleware this will be necessary in order to gain the benefits of things like error/retry policies and offline queuing.

That is to say that if we can centralize the API-specific code in the API-specific folder/sub-system and then do the same for HTTP things then we will win big.

Is this an existing function inside of undocumented()?

it is.

dmsnell · 2017-01-25T23:10:22Z

client/state/timezones/reducer.js

+		manual_utc_offsets,
+		timezones_by_continent,
+	} ) => ( { manual_utc_offsets, timezones_by_continent } ),
+}, timezonesSchema );


Please consider the structure of this reducer. In fact we are combining two different data properties into one reducer and as a result we have a complicated and unspecific initial state. If instead we use the idiomatic Redux composition we end up with separate reducers which can compose and automatically create a valid initial state.

const offsets = ( state = [], action ) => TIMEZONES_RECEIVE === action.type ? action.manual_utc_offsets : state; const timezones = ( state = [], action ) => TIMEZONES_RECEIVE === action.type ? action.timezones_by_continent : state; const items = combineReducers( { offsets, timezones, } )

of course, if you read my earlier comments I have some suggestions for restructuring the data which would have implications here.

please also consider the difference between replacing operations and merging operations. when we replace we eliminate history and wipe out any changes. might it be possible that for testing or development purposes we might want to inject a specific timezone? if we merge the arrays instead of replace them we can easily do this from the console

dispatch( { type: actionTypes.TIMEZONES_RECEIVE, manual_utc_offset: [ { value: 'UTC-7', label: 'Dinner Time' } ] } )

and ideally this just pops into the list without breaking anything else. didn't even need to add all the continents in the action to make it work.

dmsnell · 2017-01-25T23:11:19Z

client/state/timezones/reducer.js

+	[ TIMEZONES_REQUEST ]: () => true,
+	[ TIMEZONES_REQUEST_FAILURE ]: () => false,
+	[ TIMEZONES_REQUEST_SUCCESS ]: () => false,
+} );


For simple reducers like this one we may want to consider writing them as simpler functions. These run on every action and adding the extra overhead from createReducer() is just one more thing to run hundreds of times over and over.

dmsnell · 2017-01-25T23:11:50Z

client/state/timezones/schema.js

+			}
+		}
+	}
+};


Thanks for writing up the schema!

Let's keep this in mind as we decide how best we want to represent our data.

dmsnell · 2017-01-27T18:24:32Z

client/state/timezones/actions.js

+
+/*
+ * The data given in as the function parameter has already been checked
+ * from the `timezones` data-layer.


aha! here lies some beauty - this data doesn't have to come from the data-layer (and actually then needs no reference here)

validation in the reducers is different than validation from the API

the data-layer makes sure that the information returned from the API fits the right shape and structure of the data that Calypso expects

the Redux system makes sure that the information it receives makes sense and is valid in the terms of things like acceptable values

we could just as easily send this request from the dev tools or over a web socket or have them included in a JSON blob at the top of the original HTML

dmsnell · 2017-01-27T18:25:25Z

client/state/timezones/actions.js

+ * from the `timezones` data-layer.
+ * Take a look to requestTimezones function for more information.
+ */
+export const timezonesReceiveAction = ( data ) => ( {


related to the note above, shall we think about this action truly as "we received timezone data" or something more akin to "we want to add this timezone data into the app"

the name may be relevant

Do you mind suggest a name for this action please? I'll be more confident with your criteria that with mine.

How about something like TIMEZONE_ADD? That name isn't tied to the API and it still conveys what we want. Note also that it also seems to imply Calypso-native data. This is the kind of thing we could easily imagine being fired off by some user input as well

dmsnell · 2017-01-27T18:27:52Z

client/state/timezones/reducer.js

+}, timezonesSchema );
+
+/**
+ * Track the timezones requesting process


❓ - are we actually concerned with knowing if a timezone request succeeded or failed or is in-transit as part of the visually renderable app state?

the middleware layer has all the power it needs to perform actions like HTTP retries and reporting (it can dispatch a notice for example). could we just as easily not track this value in Redux?

How could this affect to a data query component? The logic of this one is avoiding make two similar requests at the same time.

cc @gwwar

https://github.com/Automattic/wp-calypso/pull/10788/files#diff-29383f70732462ef1c3adc509aaa5b6aR21

is this contemplated when you say perform actions like HTTP retries ?

The main difference here is that timezonesRequestAction returns a simple object action, rather than a function thunk. This means that the data-layer hander can intercept the action and then decide on what to do. It isn't guaranteed that dispatching that action will trigger an API request.

Depending on our needs we might always want to fire an API request in the data-layer handler when we see an action, or throttle the same requests to every X seconds, or ignore them ( say if we're offline ).

Unless I'm misreading the wiring forrequestEligibility @dmsnell it doesn't look like we're checking for in-flight requests in the data-layer. Did you or @aduth have any plans for utilities there? I assume we'll want to have common request strategies.

Unless I'm misreading the wiring forrequestEligibility @dmsnell it doesn't look like we're checking for in-flight requests in the data-layer. Did you or @aduth have any plans for utilities there?

It's not there yet and it's waiting for the HTTP layer to merge in because that's honestly the place to perform network retries. For now though in the meantime if we are going to use the data layer I think we should be putting that logic into the API endpoint files themselves instead of in the components and reducers.

I suggest go ahead with the current approach. It means to keep the requesting state in the state-tree and work in a better approach to another PR and I think it's out of the scope of this one.
Also, keep in mind that there are following PRs waiting for this to continue ahead.

Sure, but why move to the data layer and not adopt the programming model?

The point of my comment was this: if Calypso doesn't have to explicitly show that some data is being refreshed from the server then this is the place to hold the "isRequesting" state and prevent sending out duplicate requests. It's kind of a big deal here (to me at least) that we not pile up work for ourselves in the future on this when we could more easily handle it now.

Since the HTTP layer has been merged (#10327) could I just remove the isRequesting reducer?

Since the HTTP layer has been merged (#10327) could I just remove the isRequesting reducer?

Sounds like a great plan. We're going to make sure that each individual API endpoint handler need not worry about this trivial case of "make sure I don't send out a request while one is still in transit"

dmsnell · 2017-01-27T18:28:32Z

Looking good @retrofox! you must be a glutton for punishment as much as you tackle hard things like dates and times 😉

obenland · 2017-01-30T10:12:17Z

client/state/timezones/reducer.js

+
+const offsets = createReducer( [], {
+	[ TIMEZONES_RECEIVE ]: ( state, { rawOffsets = [] } ) => rawOffsets.slice( 0 )
+}, rawOffsetsSchema	);


Looks like a tab vs space issue here?

obenland · 2017-01-30T10:13:39Z

client/state/timezones/test/actions.js

+ */
+/**
+ * Internal dependencies
+ */


One of these comments can probably be removed

dmsnell · 2017-02-03T21:34:19Z

client/state/data-layer/wpcom/timezones/index.js

+const valueLabelToObject = data => (
+	fromPairs( map( data, ( { label, value } ) => ( [ value, label ] ) ) )
+);
+


Not sure if it seems clear to me whether or not this function is very generalizable or more specific. In the general sense it's valueLabelToObject but in the specific case inside of this module its feels more like it could be timezoneRecordsToMap or apiTimezonesToObject

Please don't change it just because I wrote this. I'm just making an observation and raising a question.

Honestly I think you're right.

dmsnell · 2017-02-03T21:36:03Z

client/state/timezones/reducer.js

+} from './schema';
+
+export const rawOffsetReducer = createReducer( {}, {
+	[ TIMEZONES_RECEIVE ]: ( state, { rawOffsets = [] } ) => rawOffsets


this is a set-and-replace. there might be utility in folding in the new results with the old ones.

set-and-replace by the moment. The main purpose of this whole implementation is to be consistent with the wp-admin. For this reason, we did the ./timezones endpoints, etc. Let's avoid the chance of not to be, at least for now.

dmsnell · 2017-02-03T21:40:45Z

client/state/timezones/test/reducer.js

+			const expectedState = initialState;
+			const newState = rawOffsetReducer( initialState, action );
+			expect( newState ).to.eql( expectedState );
+		} );


wow. this makes me nervous here. I haven't seen us testing that something gets persisted before. it seems like the responsibility of the persistence layer…

We can consolidate persistence tests elsewhere, but I think it's important for folks that choose to persist state, to consider if they need to persist, when they do, and when it should fail.

these aren't really unit tests then. maybe we need to start discussing how to add integration tests which run separately from the unit tests.

I personally feel nervous because these tests are testing things that are supposedly already tested (if a subtree has a schema attached and has a SERIALIZE reducer then it should persist).

If we tested this on everything we'd have to introduce a huge redundant test suite that increases maintenance burden but wouldn't add much testing value.

Why would it be an integration test at that point? It's testing the JSON schema. Even if we throw away the tests afterward I think it has some value for the author in the first implementation, esp considering how many schema mistakes I usually find while testing.

Until we auto-generate the schema from the data transform layer, I still see value in keeping these.

the integration part is that we have made a promise in the persistence layer: "if you provide a schema and use something like createReducer() or withSchema() then we will handle the persistence"

if we're going to provide duplicate tests that that layer does its job everywhere we rely on it then we'll be adding tons and tons of redundant code to maintain as well as spread out the focus from that layer. that is to say, if we kept these as unit tests and trusted each unit to do its job, then we could focus our efforts on the persistence layer to find and fix bugs in one place instead of spread ourselves out on each different piece that depends on it.

tests aren't free; tests can have their own bugs; tests divert our efforts. that's the solutions that unit tests solve: laser-focused tests on individual units to validate that the guarantees they provide hold up. if we can trust the unit, then we can trust that the units fit together.

if the schema itself defines the structure then we don't need to test it because JSON schema has its own testing and validation.

It does have it's own testing and validation, but in this case we're not testing that the JSON schema spec works in general. We're testing that we wrote down the schema we wanted, that the schema actually represents the data contract we want to have. Our mock data in those tests become the source of truth, and our tests will be green or red in a deterministic way. So really this is still trying to catch a programming error (misconfiguring a schema), rather than say detect when our api changes unexpectedly or when systems goes down.

Can the mocks get out of date? Yes, but if we update what we store locally, we also need to update our schema and tests. That's our only safeguard that we aren't using old data that doesn't match the shape we expect.

I think there is value to writing these tests, even if the author throws them away later. Most folks are rather unfamiliar with the JSON schema spec, and tests provide a quick feedback loop to see that they did this correctly.

we don't have an integration suite to my knowledge.

We have a selenium test runner that performs a number of smoke tests at different viewport sizes for common flows. The tests currently run during every master branch merge. It's quite handy as a sanity check, and is very useful to test components that regress often, even the ones covered by a number of unit tests.

The e2e integration tests are also non-trivial to maintain and are by their nature much more fragile. I would really recommend looking into the project if you're interested in contributing or need to add a smoke test. Any flow patrol folks will be happy to help you troubleshoot setup.

That said I agree that it may be useful to have the e2e tests complain when we see a console warning from our persistence layer.

That said I agree that it may be useful to have the e2e tests complain when we see a console warning from our persistence layer.

Added an enhancement item to our e2e test repository so this idea doesn't get lost

We're testing that we wrote down the schema we wanted, that the schema actually represents the data contract we want to have

exactly 😄 we could also consider creating some property-based testing which would use the JSON schema to generate random test data. at some point we'll always hit the fact that we're manually encoding some structure. I'm not sure where the line is on testing that structure. I'm pretty sure it's drawn before we introduce 500 tests to make sure that the schema persists for various sub-stress

We have a selenium test runner that performs a number of smoke tests at different viewport sizes for common flows. The tests currently run during every master branch merge.

This is cool. I would like to learn more about this and maybe communicate more about it generally.

exactly 😄 we could also consider creating some property-based testing which would use the JSON schema to generate random test data.

I would totally be okay with this, though we'd add a burden for folks to create a schema even if they don't persist. Packages like https://www.npmjs.com/package/json-schema-faker can generate mock data. We'd basically be creating test factories for use in other areas.

I would totally be okay with this, though we'd add a burden for folks to create a schema even if they don't persist.

Not a bad thing. We absolutely have to create a schema. We either do it upfront and a way that guides the code we write and prevents a bunch of bugs or we do it post-facto in distributed and ad-hoc ways after bugs make known the fact that we never really understood the schema we assumed existed 😉

dmsnell · 2017-02-03T21:42:19Z

@dmsnell I'm thinking we can group any http layer changes in with the QueryComponent PR, since this is a good size chunk of code already.

not sure if some comment I made triggered this comment @gwwar but as I was looking through this it did seem possible that using the HTTP layer here could significantly shrink the size of this PR. I would be happy to work with you @retrofox if you wanted to explore that but by no means would I demand it.

dmsnell · 2017-02-03T21:42:45Z

@retrofox I didn't see anything handling errors from the API requests. if we ignore them, why bother creating actions and dispatching actions for them?

gwwar · 2017-02-03T23:52:50Z

not sure if some comment I made triggered this comment

Nothing in particular. I'd just like to see some momentum on this PR, and the http layer changes/discussion would be interesting on its own.

retrofox · 2017-02-06T19:23:37Z

@retrofox I didn't see anything handling errors from the API requests. if we ignore them, why bother creating actions and dispatching actions for them?

Should we add them just in case? If I'm not wrong, we've been adding actions and dispatching them beyond they are being used. In fact, we aren't using anything of this implementation yet.
After this we will have to work on the data component, <Timezone /> component, etc.

retrofox · 2017-02-06T19:24:22Z

Thanks @gwwar @dmsnell. I've updated and rebased the code according to our conversations.

gwwar · 2017-02-07T18:25:47Z

client/state/timezones/reducer.js

+} from './schema';
+
+export const rawOffsets = createReducer( {}, {
+	[ TIMEZONES_RECEIVE ]: ( state, actions ) => ( { rawOffsets: actions.rawOffsets } )


It looks like we're doubling up on labels. We should be able to just return the actions.rawOffsets instead. The same goes for the other reducers below

state.timezones.rawOffsets.rawOffsets.

Fixed and rebased. thanks Kerry.

gwwar

Thanks @retrofox, changes here look good. Let's 🚢 this PR, and continue with http-layer changes in the QueryComponent PR.

retrofox mentioned this pull request Jan 25, 2017

[WIP] Update timezones stuff #10788

Closed

5 tasks

retrofox added the [Status] In Progress label Jan 25, 2017

retrofox force-pushed the update/timezones-state-tree branch from c2497fd to 744d752 Compare January 25, 2017 17:34

retrofox mentioned this pull request Jan 25, 2017

State: Add timezones selectors functions #10932

Merged

retrofox force-pushed the update/timezones-state-tree branch from 744d752 to f0124f8 Compare January 25, 2017 18:01

retrofox added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. State and removed [Status] In Progress labels Jan 25, 2017

retrofox requested review from gwwar and obenland January 25, 2017 18:03

retrofox force-pushed the update/timezones-state-tree branch 2 times, most recently from 8d03908 to 5308478 Compare January 25, 2017 18:09

retrofox changed the title ~~[WIP] Timezones reduxification~~ Timezones reduxification Jan 25, 2017

retrofox force-pushed the update/timezones-state-tree branch 2 times, most recently from 66de20e to f74bdb7 Compare January 25, 2017 18:59

gwwar requested a review from dmsnell January 25, 2017 22:50

gwwar reviewed Jan 25, 2017

View reviewed changes

dmsnell requested changes Jan 25, 2017

View reviewed changes

retrofox force-pushed the update/timezones-state-tree branch 4 times, most recently from b59afb8 to 478bde3 Compare January 27, 2017 16:38

dmsnell reviewed Jan 27, 2017

View reviewed changes

retrofox force-pushed the update/timezones-state-tree branch from 478bde3 to a63dccc Compare January 27, 2017 20:57

obenland reviewed Jan 30, 2017

View reviewed changes

dmsnell reviewed Feb 3, 2017

View reviewed changes

retrofox force-pushed the update/timezones-state-tree branch 4 times, most recently from 6ec031d to 3f177a6 Compare February 6, 2017 18:58

alisterscott mentioned this pull request Feb 6, 2017

Make e2e tests complain on console warning from persistance layer Automattic/wp-e2e-tests#383

Closed

gwwar reviewed Feb 7, 2017

View reviewed changes

retrofox added 2 commits February 7, 2017 18:41

state: add timezones constants

fba7500

state-tree: add timezones actions

24d60ca

retrofox force-pushed the update/timezones-state-tree branch from 3f177a6 to 4a5317d Compare February 7, 2017 22:41

retrofox added 5 commits February 7, 2017 20:02

state-tree: add timezones reducer

76c2a47

data-layer: add timezones communcation layer

d4b719c

data-layer: add timezone tests

1da95dc

state-tree: add timezones actions tests

cc90bdc

state-tree: add timezones reducer tests

d884e3a

retrofox force-pushed the update/timezones-state-tree branch from 4a5317d to d884e3a Compare February 7, 2017 23:02

gwwar approved these changes Feb 7, 2017

View reviewed changes

gwwar added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 7, 2017

retrofox merged commit 79e5914 into master Feb 7, 2017

matticbot removed the [Status] Ready to Merge label Feb 7, 2017

retrofox moved this from Under Review to Done in Post Date / Post Schedule Feb 7, 2017

alisterscott deleted the update/timezones-state-tree branch October 16, 2017 03:35

Timezones reduxification #10934

Timezones reduxification #10934

Conversation

retrofox commented Jan 25, 2017 • edited

Testing

Redux implementation

Data-layer

matticbot commented Jan 25, 2017

gwwar 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

dmsnell 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

retrofox Jan 27, 2017 • edited

Choose a reason for hiding this comment

retrofox Jan 27, 2017 • edited

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

dmsnell commented Jan 27, 2017

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

dmsnell Feb 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisterscott Feb 6, 2017 • edited

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

dmsnell commented Feb 3, 2017

dmsnell commented Feb 3, 2017

gwwar commented Feb 3, 2017

retrofox commented Feb 6, 2017

retrofox commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwwar left a comment

Choose a reason for hiding this comment

retrofox commented Jan 25, 2017 •

edited

retrofox Jan 27, 2017 •

edited

retrofox Jan 27, 2017 •

edited

dmsnell Feb 6, 2017 •

edited

alisterscott Feb 6, 2017 •

edited