[BUGFIX] keep local state after a deletion #5058

runspired · 2017-07-10T22:26:05Z

If this approach is deemed good, this should be backported to beta and release.

This addresses an issue uncovered by the work to improve relationships: deleting a relationship results in a call to flushCanonical which erases any other local changes made. We can prevent local edits being removed for the common case (delete returning no additional data).

cc @igorT @hjdivad @stefanpenner for review

stefanpenner · 2017-07-11T03:49:30Z

addon/-private/system/model/internal-model.js

@@ -885,17 +885,28 @@ export default class InternalModel {
    @method clearRelationships
    @private
  */
-  clearRelationships() {
+  clearRelationships(resetAll = false) {


I would prefer two methods then a single method filled with if statements. Maybe clearRelationships and unloadRelationship or maybe, I think the following are more clear, rollbackRelationships and unloadRelationships?

I agree, will refactor it tomorrow

we may not need to split the methods, depending on the outcome of our discussion -> #5058 (comment)

stefanpenner · 2017-07-11T03:50:42Z

tests/unit/model/relationships/has-many-test.js

+  assert.equal(get(pets, 'length'), 3, 'precond2 - relationship now has three pets');
+
+  run(() => {
+    shen.destroyRecord({})


stefanpenner · 2017-07-11T03:51:41Z

tests/unit/model/relationships/has-many-test.js

+    const person = store.peekRecord('person', '1');
+    const petsProxy = person.get('pets');
+
+    petsProxy.then((pets) => {


stefanpenner · 2017-07-11T04:20:10Z

I'll take some time tonight or tomorrow to review.

I'm not sure the current semantics for unload purging inverse makes sense in the first place, as it seems at odds with resurrection...

I'll compare your tests to another approach I was playing with (one that I think corrects the above resurrection vs unload issue) and see what I missed.

runspired · 2017-07-11T04:35:56Z

So the naming here could be better but the idea is that the delete has already occurred and been committed and ergo the method uses the term "unload" because the relationship itself will be in the correct state afterwards, no additional signaling needed.

Overall it turns out that clear is called well before unloadRecord (is called during the delete), and our issue with the clearing is/was not due to the dematerialization change itself but other relationship bug fixes.

We need not fear resurrection here because these methods are called to remove a persisted deletion from the relationship.

stefanpenner

Left some notes as I reviewed and tested locally.

stefanpenner · 2017-07-11T04:49:26Z

addon/-private/system/model/internal-model.js

@@ -715,7 +715,7 @@ export default class InternalModel {
    heimdall.increment(send);
    let currentState = this.currentState;

-    if (!currentState[name]) {
+    if (typeof currentState[name] !== 'function') {


seems unrelated, can this be a different PR?

It can be, just something I noticed as I was stepping through.

stefanpenner · 2017-07-11T05:04:37Z

addon/-private/system/relationships/state/has-many.js

+        currentState.splice(idx, 1);
+
+        manyArray.set('length', currentState.length);
+        manyArray.notifyPropertyChange('length');


why is the above set not sufficient to notify?

for async relationships an issue was surfaced in which set was not enough to notify the PromiseProxy that length had changed. This particular notify was a shot in the dark at fixing it that did not work and should be removed. See comment in model.js for more on the issue surfaced, you may have an idea as to what isn't wired correctly.

removing this notifyPropertyChange appears to have no affect on the tests. it seems safe to remove. Or we need another test case surfacing this issue

if https://github.com/emberjs/data/pull/5058/files#r126607364 is addressed, most of this goes away.

stefanpenner · 2017-07-11T05:05:08Z

addon/-private/system/relationships/state/has-many.js

+        manyArray.set('length', currentState.length);
+        manyArray.notifyPropertyChange('length');
+
+        this.internalModel.notifyHasManyRemoved(this.key, internalModel, idx);


if the proxy is just wrapping the hasMany, why wouldn't the above setpropagate?

unknown, see comments left in model.js

~~emberjs/ember.js#15487 may fix, and if so the following would polyfill it.~~
[edit: removed code example, comment bellow is a less hacky fix]

scratch that.

The issue is, ManyArray emulates Enumerable, and thus must provide it's own arrayContentWillChange and arrayContentDidChange events, these events are how ArrayProxies work, and length (amongst other things) update based on that.

. It's internal methods do this correctly, but your manual and external currentState.splice + manyArray.set('length does not. Rather then reaching into the internal state, you may want to use manyArray.internalReplace manyArray._removeInternalModels or add your own manyArray.splice.

splice(idx, count) { this.arrayContentWillChange(idx, count, 0); this.currentState.splice(idx, count); this.set('length', this.currentState.length); this.arrayContentDidChange(idx, count, 0); }

doh, that makes some sense of why this seemed so odd.

I'm still a bit confused why this (1) didn't seem to be an issue unless there was the extra proxy from the async wrapper involved and (2) the Enumerable docs suggest just set is fine (https://www.emberjs.com/api/classes/Ember.Enumerable.html) and (3) just for learning why is this.arrayContentWillChange absolutely necessary to trigger change events here?

the Enumerable docs suggest just ...

Enumerable docs appear in-complete arrayContent{Will,Did}Change stuff seems to be missing. Should b

I'm still a bit confused why this (1) didn't seem to be an issue unless there was the extra proxy from the async wrapper

As soon as there is an ArrayProxy (in this case: PromiseManyArray) at-play, this issue would arise.

stefanpenner · 2017-07-11T05:19:33Z

addon/-private/system/model/internal-model.js

+
+        if (resetAll === true) {
+          rel.clear();
+          rel.removeInverseRelationships();


curious, do we actually want to remove the entire inverse here, or just remove our own internalModel form the inverse? If its just remove ourselves, then the clear flag feels less bad and the above recommendation to two methods can be disregarded.

Specifically:

if (resetAll === true) { rel.clear(); } rel.unloadInverseInternalModel(this);

Also, would the following name change improve clarity?

rel.unloadInverseInternalModel(this); -> rel.unloadInternalModelFromInverse(this);

--

This would seem to tidy up how we deal with inverses, spuriously destroying them must lead to other issues...

We still end up with the name spaghetti with unloadInternalModelFromInverseFromOwn and unloadInternalModelFromInverseFromInverses

This would seem to tidy up how we deal with inverses, spuriously destroying them must lead to other issues...

Yes this seems to cause data-loss in more scenarios (like #5052)

But, good news (after testing locally) I believe always doing unloadInverseInternalModel(this) will nearly fix #5052 the remaining assertion failures will be based on what we decide resurrection + unload + retained by inverse relationship actually means. The code-base is currently slightly confused... specifically:

the following assertion https://github.com/emberjs/data/pull/5052/files#diff-f5099af3b5cad3fc343f3f4b73eb3c97R208 will be true if resurrection does apply in this case (reviving 2 + 6) OR resurrection does not apply and the contacts list will become ['3','4','5','7']

I think @hjdivad will need to chime in re: the resurrection details. If we can decide on those, we can likely fix both issues in one swoop.

stefanpenner · 2017-07-11T05:28:07Z

addon/-private/system/model/internal-model.js

+
+      if (resetAll === true) {
+        rel.clear();
+        rel.removeInverseRelationships();


stefanpenner · 2017-07-11T06:50:49Z

addon/-private/system/model/model.js

@@ -1115,6 +1115,14 @@ const Model = Ember.Object.extend(Ember.Evented, {
    this.notifyPropertyChange(key);
  },

+  notifyHasManyRemoved(key) {


once https://github.com/emberjs/data/pull/5058/files#r126607364 is addressed, this goes away.

stefanpenner · 2017-07-11T06:51:16Z

addon/-private/system/relationships/state/has-many.js

+        currentState.splice(idx, 1);
+
+        manyArray.set('length', currentState.length);
+        manyArray.notifyPropertyChange('length');


if https://github.com/emberjs/data/pull/5058/files#r126607364 is addressed, most of this goes away.

stefanpenner · 2017-07-11T06:53:12Z

addon/-private/system/relationships/state/has-many.js

+    const manyArray = this._manyArray;
+
+    if (manyArray) {
+      const currentState = manyArray.currentState;


this block can likely be replaced with: manyArray.removeInternalModels([internalModel])

stefanpenner · 2017-07-11T07:06:27Z

addon/-private/system/relationships/state/relationship.js

+    let allMembers =
+      // we actually want a union of members and canonicalMembers
+      // they should be disjoint but currently are not due to a bug
+      this.members.toArray().concat(this.canonicalMembers.toArray());


rather then making 3 arrays only to iterate them, maybe we should do something like:

const unload = inverseInternalModel => { let relationship = inverseInternalModel._relationships.get(this.inverseKey); relationship.unloadInverseInternalModelFromOwn(internalModel); } this.members.forEach(unload); this.canonicalMembers(unload);

runspired · 2017-07-11T14:29:20Z

addon/-private/system/model/internal-model.js

@@ -885,14 +885,14 @@ export default class InternalModel {
        let rel = this._relationships.get(name);

        rel.clear();
-        // rel.removeInverseRelationships();
+        rel.removeInverseRelationships();


@hjdivad commenting these out didn't make any tests fail o.O

hmm yeah it looks like the unload tests assert against the id map and the relationship payloads, but not against relationships (either our side or inverse).

hjdivad · 2017-07-11T18:01:09Z

addon/-private/system/model/internal-model.js

+  /*
+    @method unloadDeletedRecordFromRelationships
+   */
+  unloadDeletedRecordFromRelationships() {


per our chat out of band, let's rename this to not conflate this case with unloadRecord

Also let's add some comments here as breadcrumbs to our future selves who will clean this up.

This does fix an important bug, but currently there are two codepaths whose different semantics are not obvious within the same level of abstraction, ie that rel.clear() causes a flushCanonical but unloadDeletedInverseInternalModel does not is not clear, and that this is the entire point is also not clear.

hjdivad · 2017-07-11T18:06:14Z

addon/-private/system/relationships/state/relationship.js

@@ -266,6 +269,33 @@ export default class Relationship {
    this.flushCanonicalLater();
  }

+  unloadDeletedInverseInternalModel(internalModel) {


what's the case where internalModel != this.internalModel?

Per our discussion out of band, let's fix the inconsistency in naming here. unloadDeletedInverseInternalModel and unloadDeletedInverseInternalModelFromOwn both mention an "inverse internal model" but are referring to different sides of the relationship.

hjdivad · 2017-07-11T18:09:45Z

tests/unit/model/relationships/has-many-test.js

+  const rebel = store.peekRecord('pet', '3');
+
+  assert.equal(get(shen, 'name'), 'Shenanigans', 'precond - relationships work');
+  assert.equal(get(pets, 'length'), 1, 'precond - relationship has only one pet to start');


let's change these assertions to deepEqual an array of ids rather than just checking the length

hjdivad · 2017-07-11T18:09:51Z

tests/unit/model/relationships/has-many-test.js

+    pets.pushObjects([rambo, rebel]);
+  });
+
+  assert.equal(get(pets, 'length'), 3, 'precond2 - relationship now has three pets');


let's change these assertions to deepEqual an array of ids rather than just checking the length

hjdivad · 2017-07-11T18:14:27Z

tests/unit/model/relationships/has-many-test.js

+      });
+  });
+
+  assert.equal(get(pets, 'length'), 2, 'relationship now has two pets');


let's change these assertions to deepEqual an array of ids rather than just checking the length

hjdivad · 2017-07-11T18:18:56Z

tests/unit/model/relationships/has-many-test.js

+          shen.unloadRecord();
+
+          assert.equal(get(pets, 'length'), 2, 'relationship now has two pets');
+          assert.equal(get(petsProxy, 'length'), 2, 'proxy now reflects two pets');


let's change these assertions to deepEqual an array of ids rather than just checking the length

hjdivad · 2017-07-11T18:45:00Z

addon/-private/system/relationships/state/belongs-to.js

@@ -63,6 +63,25 @@ export default class BelongsToRelationship extends Relationship {
    this.notifyBelongsToChanged();
  }

+  unloadDeletedInverseInternalModelFromOwn(internalModel) {


foo.set(belongsTo, new); new.destroyRecord(); foo.get(belongsTo) === null; `` We should not restore canonical when deleting new; we should let belongsto be null. That's consistent with [current behaviour](https://ember-twiddle.com/7f94a0e81ea38d9022a5007ef8be15c8?openFiles=adapters.application.js%2C) and in any case, restoring here is a bit wat

hjdivad · 2017-07-11T18:46:43Z

tests/unit/model/relationships/has-many-test.js

+  });
+});
+
+test('deleting an item that is the current state of a belongsTo restores canonicalState', function(assert) {


why?

this is a change in semantics (see this twiddle)

this is surprising behaviour

hjdivad · 2017-07-11T18:48:13Z

tests/unit/model/relationships/has-many-test.js

+  the parent record's hasMany is a situation in which this limitation will be encountered should other
+  local changes to the relationship still exist.
+ */
+test('returning new hasMany relationship info from a delete supplements local state', function(assert) {


s/supplements/supersedes

Supplements is exactly what it does not do ^_^

wrote the test for what we want to happen, and it should fail once what we want does happen, should fix that test description to account for this

hjdivad · 2017-07-11T18:48:29Z

tests/unit/model/relationships/has-many-test.js

+  const rebel = store.peekRecord('pet', '3');
+
+  assert.equal(get(shen, 'name'), 'Shenanigans', 'precond - relationships work');
+  assert.equal(get(pets, 'length'), 2, 'precond - relationship has only one pet to start');


as before, let's deepEqual ids instead of just checking length in this test

hjdivad · 2017-07-11T18:52:26Z

addon/-private/system/relationships/state/relationship.js

+    @method removeDeletedInternalModelFromInverse
+    @private
+   */
+  removeDeletedInternalModelFromInverse(internalModel) {


what's the case where internalModel != this.internalModel?

there isn't one afaik, just seemed better to pass this in with the original implementation. using this.internalModel is probably a bit safer

hjdivad

pending rebase &c.

Although As discussed out of band we should clean up the semantics around this, but that cleanup should be a 2.15 effort and here we should focus on the bugfix

igorT · 2017-07-12T17:53:29Z

addon/-private/system/model/internal-model.js

+    @method unloadDeletedRecordFromRelationships
+    @private
+   */
+  unloadDeletedRecordFromRelationships() {


We use unload as the term for unloading records, maybe we should call this method something else, like remove?

Absolutely.

@runspired I guess we missed an instance of the renaming

confirm, will rename

How about: removeFromInverseRelationships, example:

internalModel.removeFromInverseRelationships();

The method would now describes concisely what is does not when to use it. The when and why should be provided via docs, or derived contextually from where it is used.

igorT · 2017-07-12T17:55:48Z

addon/-private/system/relationships/state/belongs-to.js

@@ -63,6 +63,19 @@ export default class BelongsToRelationship extends Relationship {
    this.notifyBelongsToChanged();
  }

+  removeDeletedInternalModelFromOwn(internalModel) {
+    super.removeDeletedInternalModelFromOwn(internalModel);


I thought we moved away from calling super in relationships?

We never landed that

igorT · 2017-07-12T17:58:29Z

addon/-private/system/model/internal-model.js

    @private
  */
-  clearRelationships() {
+  _resetAllRelationshipsToEmpty() {


What is the difference in behavior between the isNew and the deletion case?

stefanpenner · 2017-07-13T16:16:47Z

addon/-private/system/relationships/state/relationship.js

+    this.canonicalMembers.forEach(unload);
+  }
+
+  removeDeletedInternalModelFromOwn(internalModel) {


I believe Deleted should be removed from this record name. In general a method name should describe what it does, not why or when it should be invoked.

Will follow guidance on your other comment for renaming

@runspired this function is quite similar to removeInternalModelFromOwn what is the divergence?

The divergence (but feel free to confirm):

removeInternalModelFromOwn: is removing from current state (awaiting future canonical flush)

removeDeletedInternalModelFromOwn: is removing from both current state (members) and canonical state (canonicalMembers)

The naming of these methods sorta blows, but naming is hard. So at the very least we may need to document them somewhat.

Some ideas:

Removes a given internal model from the relationships currentState, but not from that relationships canonicalState. This results in an optimistic delete, and subsequent confirmation (typically server side) will update the canonicalState. @method removeInternalModelFromOwn @param internalModel

Removes a given internal model from both a relationships currentState and canonical state. This typically occurs when an internalRecord's deletion is persisted. @method removeDeletedInternalModelFromOwn @param internalModel

stefanpenner · 2017-07-13T16:20:03Z

addon/-private/system/relationships/state/relationship.js

+  removeDeletedInternalModelFromOwn(internalModel) {
+    this.canonicalMembers.delete(internalModel);
+    this.members.delete(internalModel);
+    this.notifyRecordRelationshipRemoved(internalModel);


does this method do anything? I only see two implementations and they appear to be noops.

https://github.com/emberjs/data/search?utf8=%E2%9C%93&q=notifyRecordRelationshipRemoved&

I believe this was put here for something with dematerialization that was never completed maybe?

stefanpenner · 2017-07-13T16:22:57Z

addon/-private/system/model/internal-model.js

+    @method unloadDeletedRecordFromRelationships
+    @private
+   */
+  unloadDeletedRecordFromRelationships() {


How about: removeFromInverseRelationships, example:

internalModel.removeFromInverseRelationships();

The method would now describes concisely what is does not when to use it. The when and why should be provided via docs, or derived contextually from where it is used.

stefanpenner · 2017-07-13T16:23:47Z

addon/-private/system/model/states.js

@@ -671,7 +671,7 @@ const RootState = {
      isDirty: false,

      setup(internalModel) {
-        internalModel.clearRelationships();
+        internalModel.unloadDeletedRecordFromRelationships();


internalModel.removeFromInverseRelationships()

stefanpenner · 2017-07-13T16:36:11Z

addon/-private/system/model/internal-model.js

+      if (this._relationships.has(name)) {
+        let rel = this._relationships.get(name);
+
+        rel.removeDeletedInternalModelFromInverse();


should destroyRelationships also get this treatment?

hjdivad · 2017-07-13T17:45:44Z

addon/-private/system/relationships/state/has-many.js

@@ -111,6 +111,26 @@ export default class ManyRelationship extends Relationship {
    super.removeCanonicalInternalModelFromOwn(internalModel, idx);
  }

+  removeDeletedInternalModelFromOwn(internalModel) {


seems related to removeInternalModelFromOwn, why diverge?

i believe the divergence is: https://github.com/emberjs/data/pull/5058/files#r127379185

removeInternalModelFromOwn removes only current state while removeDeletedInternalModelFromOwn removes from current and canonical state.

hjdivad · 2017-07-13T18:21:41Z

Summary of issues

lose local changes in hasMany on a delete of a canonical record and the server returns no new canonical data.
lose local changes in hasMany more generally during flushCanonical as flushCanonical does not diff
unload clears inverse relationships eagerly, preventing intra-loop rematerialization
unload perf issue possibly overwalking during gc

This pr addresses 1.

cc @runspired @igorT @stefanpenner

stefanpenner · 2017-07-13T21:03:12Z

addon/-private/system/relationships/state/relationship.js

+    this.canonicalMembers.forEach(unload);
+  }
+
+  removeDeletedInternalModelFromOwn(internalModel) {


@runspired this function is quite similar to removeInternalModelFromOwn what is the divergence?

stefanpenner · 2017-07-14T03:36:27Z

addon/-private/system/relationships/state/relationship.js

+    this.canonicalMembers.forEach(unload);
+  }
+
+  removeDeletedInternalModelFromOwn(internalModel) {


The divergence (but feel free to confirm):

removeInternalModelFromOwn: is removing from current state (awaiting future canonical flush)

removeDeletedInternalModelFromOwn: is removing from both current state (members) and canonical state (canonicalMembers)

The naming of these methods sorta blows, but naming is hard. So at the very least we may need to document them somewhat.

Some ideas:

Removes a given internal model from the relationships currentState, but not from that relationships canonicalState. This results in an optimistic delete, and subsequent confirmation (typically server side) will update the canonicalState. @method removeInternalModelFromOwn @param internalModel

Removes a given internal model from both a relationships currentState and canonical state. This typically occurs when an internalRecord's deletion is persisted. @method removeDeletedInternalModelFromOwn @param internalModel

runspired added 4 commits July 10, 2017 15:25

fix failing tests

879a814

fix async relationships

269b1b4

expand comment

b972555

add additional test coverage

4c3d594

runspired changed the title ~~[WIP] keep local state after a deletion~~ [BUGFIX] keep local state after a deletion Jul 10, 2017

stefanpenner suggested changes Jul 11, 2017

View reviewed changes

stefanpenner mentioned this pull request Jul 11, 2017

ArrayProxy.length YUNO DK? emberjs/ember.js#15487

Closed

2 tasks

stefanpenner suggested changes Jul 11, 2017

View reviewed changes

runspired added 4 commits July 11, 2017 07:17

address feedback from @stefanpenner

e9acc1f

fix perf of unloading inverses

941ca58

fix typos

9a72fb5

revert accidental code commenting left behind

cee07bd

runspired commented Jul 11, 2017

View reviewed changes

code cleanup to address @dhamilton review

94503e6

hjdivad suggested changes Jul 11, 2017

View reviewed changes

hjdivad reviewed Jul 11, 2017

View reviewed changes

runspired added 2 commits July 11, 2017 12:15

address remaining feedback from @hjdivad

4d7f4e8

fix type

5830358

hjdivad approved these changes Jul 11, 2017

View reviewed changes

igorT reviewed Jul 12, 2017

View reviewed changes

stefanpenner suggested changes Jul 13, 2017

View reviewed changes

hjdivad suggested changes Jul 13, 2017

View reviewed changes

fixup function naming

e5cdfe4

stefanpenner suggested changes Jul 13, 2017

View reviewed changes

runspired added 2 commits July 13, 2017 18:51

un-diverge isNew and deletion scenarios

b538bbf

improve code comment

348b72f

emberjs deleted a comment from runspired Jul 14, 2017

remove unused code

02a5a46

stefanpenner suggested changes Jul 14, 2017

View reviewed changes

emberjs deleted a comment from runspired Jul 14, 2017

stefanpenner approved these changes Jul 14, 2017

View reviewed changes

final cleanup

8c3f77d

stefanpenner merged commit 00334e9 into emberjs:master Jul 14, 2017

stefanpenner deleted the fix-delete-with-local-state branch July 14, 2017 19:39

stefanpenner mentioned this pull request Jul 14, 2017

Revert "[BUGFIX] keep local state after a deletion" #5066

Merged

hjdivad mentioned this pull request Nov 2, 2017

Fix client-side delete + resurrection #5249

Merged

runspired mentioned this pull request Apr 18, 2018

[BUGFIX] ensure destroy-sync cleanup is correct #5438

Merged

[BUGFIX] keep local state after a deletion #5058

[BUGFIX] keep local state after a deletion #5058

Conversation

runspired commented Jul 10, 2017 • edited Loading

stefanpenner Jul 11, 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

stefanpenner commented Jul 11, 2017

runspired commented Jul 11, 2017 • edited Loading

stefanpenner 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

stefanpenner Jul 11, 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

stefanpenner Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

stefanpenner Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

stefanpenner Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner Jul 11, 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

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

hjdivad left a comment • 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

stefanpenner Jul 14, 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

runspired commented Jul 10, 2017 •

edited

Loading

stefanpenner Jul 11, 2017 •

edited

Loading

runspired commented Jul 11, 2017 •

edited

Loading

stefanpenner Jul 11, 2017 •

edited

Loading

stefanpenner Jul 11, 2017 •

edited

Loading

stefanpenner Jul 11, 2017 •

edited

Loading

stefanpenner Jul 11, 2017 •

edited

Loading

stefanpenner Jul 11, 2017 •

edited

Loading

stefanpenner Jul 11, 2017 •

edited

Loading

hjdivad left a comment •

edited

Loading

stefanpenner Jul 14, 2017 •

edited

Loading

stefanpenner Jul 14, 2017 •

edited

Loading