Fix for issue #594, reifying sub-classed models that use STI #1108

lorint · 2018-06-07T02:19:02Z

This update provides a fix for Issue #594 -- Reify on associations fails if using Single Table Inheritance.

In order to properly reify a version of a model using STI, item_type can refer directly to the class name instead of being base_class. When making this change, the five reifiers in the gem paper_trail-association_tracking that refer to base_class can also be updated, which is covered in PT-AT PR #5. With these changes, the previously failing example in person_spec.rb will pass.

One key benefit from this change is to allow PT to represent STI model names. So if you have for instance the model "Person", there could be subclasses for "Doctor" and "Patient". The subclassed name gets put into item_type so that audit logs can be more clear.

…se STI

jaredbeck · 2018-06-07T15:44:33Z

Given that this changes what is stored in the database, do we need to provide a convenient way for people convert their existing database records? If they did not convert their records, then reification (without version_reification_class) would produce incorrect results, right? Is there any practical way to detect when someone has not converted their existing records, and warn them?

jaredbeck · 2018-06-07T16:14:18Z

spec/models/family/celebrity_family_spec.rb

+        it "uses the correct item_type in queries" do
+          parent = described_class.new(name: "Jermaine Jackson")
+          parent.path_to_stardom = "Emulating Motown greats such as the Temptations and "\
+                                   "The Supremes"


Ah yes -- I have a distinct interest in making a quasi-boring topic (auditing) into something enduring.

And actually I would hope that things like email notifications and cool reporting with pretty charts could piggy-back on top of what we store in versions!

...do we need to provide a convenient way for people convert their existing database records?

We could, and in our project I actually have a migration that does this. It's possible to include with this a generator that creates a migration to cycle through existing PaperTrail::Version entries and update item_type appropriately.

-= But =- wonderfully enough this is not entirely essential because ActiveRecord just does the right thing; reified objects get instantiated as their proper subclassed type. With the Person / Doctor / Patient example, if for a Doctor you have a combo of entries from the past (<= v9.1) where the item_type was stored as Person, and now there are other entries (>= v9.2) some changes stored as Doctor, the reification always brings back a Doctor.

The AT part currently does not rehydrate all the historic attributes properly when this update is applied, so that's the reason for the couple notes in spec/models/pet_spec.rb. During the coming week I have high hopes to solve that and then have the best of all worlds. The biggest "win" in my mind becomes more specific reporting of the item_type, and certainly having the example in spec/models/person.rb work out is a wonderful benefit as well.

jaredbeck · 2018-06-07T19:09:50Z

...do we need to provide a convenient way for people convert their existing database records?

We could, and .. I actually have a migration that does this .. But wonderfully enough this is not entirely essential because ActiveRecord just does the right thing; reified objects get instantiated as their proper subclassed type. With the Person / Doctor / Patient example, if for a Doctor you have a combo of entries from the past (<= v9.1) where the item_type was stored as Person, and now there are other entries (>= v9.2) some changes stored as Doctor, the reification always brings back a Doctor.

Great. If both item_types reify into a Doctor, then it sounds like we don't have to help people convert existing records, and I'm less inclined to treat this as a breaking change. We'll definitely need a test that demonstrates old-style records reifying correctly.

I don't think this PR will make it into 9.2. Maybe 9.3? It's hard to predict the contents of future releases.

The AT part currently does not rehydrate all the historic attributes properly when this update is applied, so that's the reason for the couple notes in spec/models/pet_spec.rb. During the coming week I have high hopes to solve that and then have the best of all worlds. The biggest "win" in my mind becomes more specific reporting of the item_type, and certainly having the example in spec/models/person.rb work out is a wonderful benefit as well.

Sweet! Thanks for your work on this tricky issue.

lorint · 2018-06-07T21:23:16Z

... it sounds like we don't have to help people convert existing records ... We'll definitely need a test that demonstrates old-style records reifying correctly.

Added an example to spec/models/pet_spec.rb to showcase this compatibility. It artificially marks a Cat to have the item_type of the base class Animal. It also tests an object made directly from the base class.

jaredbeck

First review, looks like a good start!

Did you want to wait for PT-AT #5 to be released (eg. as PT-AT 1.0.1) and then we update our gemspec dependency to eg. ~> 1.0.1?

jaredbeck · 2018-06-08T15:09:46Z

lib/paper_trail/has_paper_trail.rb

+          end
+        end
+        @paper_trail_type_name
+      end


Can this method be moved into RecordTrail?

Why use an instance variable? I'd prefer a local variable if possible.

This caches @paper_trail_type_name -- although perhaps people would not often call .versions more than once on a versioned object.

If it moves anywhere then I'd prefer to have it back in model_config where the has_many is established.

(TBH this started because the Rubocop ABC is set at 22, and I couldn't get under 22.69 after breaking out the has_many stuff in model_config, and with type_name only being a simple string it seemed like a fairly lightweight addition to each object.)

jaredbeck · 2018-06-08T15:13:18Z

lib/paper_trail/model_config.rb

@@ -168,6 +168,31 @@ def cannot_record_after_destroy?
        ::ActiveRecord::Base.belongs_to_required_by_default
    end

+    def type_aware_has_many(klass)


In keeping with the name of the method setup_associations, I would call this setup_versions_association.

Good suggestion!

jaredbeck · 2018-06-08T15:16:42Z

lib/paper_trail/model_config.rb

+          # custom inheritance column, `species`. If `attrs["species"]` is "Dog",
+          # type_name is set to `Dog`. If `attrs["species"]` is blank, type_name
+          # is set to `Animal`. You can see this particular example in action in
+          # `spec/models/animal_spec.rb`.


Assuming paper_trail_type_name can be moved into RecordTrail, and renamed to something like versions_association_item_type, this would be object.paper_trail.versions_association_item_type.

Extract a local variable item_type = object.paper_trail.versions_association_item_type and use in the two places below to avoid calculating it twice.

OK all set!

jaredbeck · 2018-06-08T15:17:42Z

lib/paper_trail/record_trail.rb

@@ -237,7 +237,10 @@ def record_create
      @in_after_callback = true
      return unless enabled?
      versions_assoc = @record.send(@record.class.versions_association_name)
-      versions_assoc.create! data_for_create
+      version = versions_assoc.new data_for_create
+      version.item_type = @record.class.name


Can item_type be moved into data_for_create?

Unfortunately when initializing the new Version, ActiveRecord uses the polymorphic association to override whatever you specify for item_type, so we need to new it and then reset the item_type on our own.

jaredbeck · 2018-06-08T15:31:39Z

CHANGELOG.md

@@ -24,7 +24,8 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/).

 ### Fixed

- None
+- [#594](https://github.com/paper-trail-gem/paper_trail/issues/594) -
+  In order to properly reify a version of a model using STI, item_type can refer to the class instead of base_class. When making this change, the five reifiers in the gem [paper_trail-association_tracking] that refer to base_class can also be updated, which is covered in [PT-AT PR #5](https://github.com/westonganger/paper_trail-association_tracking/pull/5). With these changes, the previously failing example in person_spec.rb passes.


Can you boil this down a bit, maybe into a single sentence?

Should we link to Reify on associations fails if using Single Table Inheritance #594 or this PR (Fix for issue #594, reifying sub-classed models that use STI #1108) or both?

Perhaps linking to this PR is best, and I'll make reference to #594 at the top.

lorint · 2018-06-08T22:36:03Z

Did you want to wait for PT-AT #5 to be released ... and then we update our gemspec dependency?

I believe the other way around might be best; I figure that more people use the core PaperTrail features than the association tracking. If Weston were to update his side now then it would break two examples in pet_spec.rb. Once this change is put into place then those 5 reifiers in PT-AT can be updated from referencing class.base_class back to simplyclass, and then things can progress on his side, allowing all tests to pass.

Certainly both halves of PaperTrail are important, at least to me, and it's tough to enact this kind of fix without having both sides move in tandem.

jaredbeck · 2018-06-10T04:18:53Z

What if we leave item_type as-is and add another column called e.g. item_reification_type?

Now that I understand this proposal better, I'm uncomfortable about taking over the item_type column. As long as we're going to use a polymorphic association (as: :item), I think rails should control that column.

jaredbeck · 2018-06-10T16:58:40Z

Now that I understand this proposal better, I'm uncomfortable about taking over the item_type column. As long as we're going to use a polymorphic association (as: :item), I think rails should control that column.

To make my concerns a bit more concrete, here are two possible future scenarios:

rails decides to add some validation to polymorphic-STI associations, starts raising error when we call item
rails decides to build new feature X, based on polymorphic-STI associations, which requires item_type to contain base class. PaperTrail gets bug reports saying "X doesn't work".

Sorry for not raising this concern earlier, it did not occur to me.

What if we leave item_type as-is and add another column called e.g. item_reification_type?

What do you think?

lorint · 2018-06-15T09:13:17Z

Have had some free cycles again to dig back in on this, and I think we're finally out of the woods; some VERY good things to report! We don't have to force ActiveRecord's hand in the least.

Remarkably when calling .create!() as we had been doing previously the result is different than if we call .new() and then .save(). Thankfully the latter causes item_type to receive the subclassed model name! I would not anticipate this to cause any kind of breaking change in the future, especially in light of how Inheritance#find_sti_class works:

https://apidock.com/rails/ActiveRecord/Inheritance/ClassMethods/find_sti_class

Putting together a commit now for you to see this in action.

jaredbeck · 2018-06-15T15:41:52Z

Remarkably when calling .create!() as we had been doing previously the result is different than if we call .new() and then .save(). Thankfully the latter causes item_type to receive the subclassed model name!

I cannot confirm this. Here is what I tried:

  # paper_trail/spec/models/animal_spec.rb
  it "saves the expected item_type" do
    cat = Cat.create(name: "Buddy")
    expect(cat.versions.first.item_type).to eq('Animal')
    cat2 = Cat.new(name: "Buddy")
    cat2.save
    expect(cat2.versions.first.item_type).to eq('Cat') # => fails: Expected Cat, got Animal
  end

The above demonstrates that create (or create!) produces the same item_type as new/save.

lorint · 2018-06-15T17:43:46Z

I cannot confirm this

Ah -- the trick is not in how the Cat object gets built out, but how RecordTrail builds out the associated Version object:

it "saves the expected item_type" do
  buddy_cat = Cat.create(name: "Buddy")

  # Let's make a couple versions in two possible ways that RecordTrail could:
  version1 = buddy_cat.versions.create!(buddy_cat.paper_trail.data_for_create)
  version2 = buddy_cat.versions.new(buddy_cat.paper_trail.data_for_create)
  version2.save
  expect(version1.item_type).to eq("Animal")
  expect(version2.item_type).to eq("Cat")
end

jaredbeck · 2018-06-15T18:08:34Z

Ah -- the trick is not in how the Cat object gets built out, but how RecordTrail builds out the associated Version object:

Ah ha! Because of the lambda this PR would add to the has_many?

lorint · 2018-06-15T18:21:36Z

Because of the lambda this PR would add to the has_many?

That is a necessary additional piece. First the three places with .new() .save() in RecordTrail are able to record the real class name. Then when referencing versions, the original lambda had just order(model.timestamp_sort_order). In this updated has_many this behaviour is preserved for non-STI tables, and for STI tables there's the .unscope() and .where() to always find based on their real class name, the one that RecordTrail now properly records.

One big win is that this now alleviates the trickery that version_reification_class() had to do in the reifier, and as well it paves the way for PT-AT to more easily work with STI.

jaredbeck · 2018-06-15T18:40:43Z

I raised some concerns about us tampering with the item_type column.

Now that I understand this proposal better, I'm uncomfortable about taking over the item_type column. As long as we're going to use a polymorphic association (as: :item), I think rails should control that column.

To make my concerns a bit more concrete, here are two possible future scenarios:

rails decides to add some validation to polymorphic-STI associations, starts raising error when we call item

rails decides to build new feature X, based on polymorphic-STI associations, which requires item_type to contain base class. PaperTrail gets bug reports saying "X doesn't work".

Can you address these concerns, please? Do you not share them? Thanks.

lorint · 2018-06-15T20:36:19Z

Can you address these ... concerns about us tampering with the item_type column. Do you not share them?

I fail to see what kind of concern we can have. I mean, things operate just as before such that when we build a Version for a create or update it fully relies on the polymorphic association as: :item to establish item_type and item_id. The nice benefit of this PR is that it cleans up other tampering that we previously had to do, digging into base_class in order to reify. I think all of these changes can now be considered win-win.

The three places in RecordTrail that use ActiveRecord code to make everything work each go a little something like this:

version = versions_assoc.new data_for_update_columns(changes)
version.save

It is curious that calling versions.create() through the has_many only gets the base class, and instead doing versions.new() + version.save() arrives upon the real class, but that's not something that gives me concern. I figure the battle to try to get the kind of update to fix .create() rolled into Rails would be a significant uphill battle, especially since many people may have coded around this nuance, and rely on this behaviour to remain unmodified.

I'm very happy with how it all works, and feel that further refinement may only be to improve performance slightly by caching versions_association_item_type.

I dislike optional arguments in general. I think they introduce more complexity than they are worth, in most cases.

@migrator

We sleep until the next whole second because that is the precision of the timestamp that rails puts in generator filenames. If we didn't sleep, there's a good chance two tests would run within the same second and generate the same exact migration filename. Then, even though we delete the generated migrations after running them, some form of caching (perhaps filesystem, perhaps rails) will run the cached migration file. This implementation, with sleep, replaces a previous implementation using a "dummy migration" that forced rails to use the next timestamp. This was a very clever solution, and in the best case was N seconds faster than the sleep solution, where N is the number of times we run `generate_and_migrate` in the test suite. Currently N is 3, and I'm willing to live with 3s of sleep, and I prefer its simplicity. Perhaps most importantly, this implementation does not require any state to be persisted across tests (ie. the @migrator ivar in the previous implementation. Tests should be completely independent of eachother.

[ci skip]

jaredbeck · 2018-07-27T05:24:47Z

I had a few hours tonight to review the changes to the PaperTrailSpecMigrator that had been bothering me. Your solution for the migration name timestamp conflict was very clever, but I'd like to replace it with (I can't believe I'm saying this) a sleep. It'll add a few seconds to the test suite (at most three) but it's so much simpler, and it removes the persistent state we were sharing between tests (the persistent @migrator instance) If you'd like to review my changes, which i would appreciate, please read my detailed comments in 2952579 first.

I'm busy all day Friday and Saturday, but I'm hoping to finish my review on Sunday.

Dismissing first review. Final review pending.

- Avoid repeating order clause - Method comments

Some people don't read changelogs.

jaredbeck · 2018-07-29T04:52:51Z

In addition to the changes to PaperTrailSpecMigrator I described above, I've added a few tests, made minor changes to the docs, and added a warning for the people who don't read changelogs.

I'm ready to merge this, but I'd welcome a review of my recent changes. Thanks.

lorint

Gosh we're happy to see all of this come together!

One note is that in RecordTrail#versions_association_item_type it previously returned a blank string if the class type was the same as the base_class. This avoided the overhead of always having to .unscope() / .where(). I wonder about bringing that part back, but must admit that I haven't actually benchmarked how much extra CPU time and memory overhead this may or may not cause.

lorint · 2018-07-30T08:35:57Z

lib/paper_trail/model_config.rb

          item_type = object.paper_trail.versions_association_item_type
-          if item_type.blank?


As a note, I had been doing this test for blank because when versions_association_item_type referred to the base_class then I had returned blank so that it wouldn't have to go through the unscope / where changes to the relation. If this overhead is acceptable then no worries.

lorint · 2018-07-30T09:09:50Z

lib/paper_trail/record_trail.rb

-      if type_name == @record.class.base_class.name
-        ""
+      if item_type == @record.class.base_class.name
+        nil


(In connection with the test for .blank? from 184)
Not sure how much performance loss might be experienced, but the blank was an attempt to avoid the unscope / scope when item_type referred to the base_class. For many STI implementations perhaps it doesn't matter because the majority of things might be subclassed, but in some cases it could provide a benefit. (Family / CelebrityFamily kind of examples.)

lorint · 2018-07-30T09:11:35Z

lib/paper_trail/record_trail.rb

+      ::PaperTrail.config.classes_warned_about_sti_item_types ||= []
+      return if ::PaperTrail.config.classes_warned_about_sti_item_types.include?(record_class)
+
+      ::Kernel.warn(format(E_STI_ITEM_TYPES_NOT_UPDATED, record_class.name))


I love this message! Thanks for putting this part together.

lorint · 2018-07-30T09:57:52Z

Your solution for the migration name timestamp conflict was very clever, but I'd like to replace it with ...
(hehe, will not mention our silliness)

Once I better understood that caching issue, you might have seen that I had put in a sleep(2). For a good day I had toyed around with LOTS of different options, even a temporary monkey patching of Rails just for that test! Boy was that messy.

After thinking more about it, another option we might consider is to use TimeCop. But I do like the approach you've arrived upon.

jaredbeck · 2018-07-30T14:48:11Z

One note is that in RecordTrail#versions_association_item_type it previously returned a blank string if the class type was the same as the base_class. This avoided the overhead of always having to .unscope() / .where(). I wonder about bringing that part back, but must admit that I haven't actually benchmarked how much extra CPU time and memory overhead this may or may not cause.

The unless item_type.nil? I have written should be exactly the same as the if item_type.blank? you had.

I'll go ahead and merge this, and I hope you can help test it by pointing your apps at master and running their test suites.

Thanks for your hard work and patience on this.

seanlinsley · 2018-08-14T19:55:13Z

In my application, I have a Company model which via STI can become Management. There are no extra attributes that exist for management companies; it's purely a distinction that exists for the sake of ActiveRecord associations (a company can either be an owner, a manager, or both).

While this PR fixes issues for those using more complex STI features, it breaks my basic use-case.

A distinguishing factor is that I don't have a type column on the table. Perhaps we could continue to use the base class if the table doesn't have a type column?

lorint · 2018-08-15T16:50:58Z

I don't have a type column on the table. Perhaps we could continue to use the base class if the table doesn't have a type column

First let me say that I like the minimalist approach that is a hallmark of your suggestions -- we have a large-scale app that will benefit from your involvement around object / object_changes. In this case about using STI without type I catch a sense of, "Why have a type column if it's not absolutely necessary?"

So that I can be certain about the goal -- here's my attempt at a failing test. In the dummy app in the test suite, Customer only has :id and :name columns. Let's say that this STI-ish model were added:

class Management < Customer
end

Then perhaps this would this describe your expected behaviour:

it "utilises base_class for STI classes having no type column" do
  expect(Management.inheritance_column).to eq("type")
  expect(Management.columns.map(&:name)).not_to include("type")

  customer1 = Customer.create(name: "Cust 1")
  customer2 = Management.create(name: "Cust 2")
  cust_versions = PaperTrail::Version.where(item_type: "Customer")
  mgmt_versions = PaperTrail::Version.where(item_type: "Management")
  expect(cust_versions.count).to eq(2)
  expect(mgmt_versions.count).to eq(0)
end

(And if so then I would certainly like to add in examples for update and destroy as well.)

To enact a fix then perhaps in record_trail.rb there could be this:

def versions_association_item_type
  type_column = @record.class.inheritance_column
  base_class_name = @record.class.base_class.name
  item_type = (@record.respond_to?(type_column) ? @record.send(type_column) : base_class_name) ||
    @record.class.name
  if item_type == base_class_name
    nil
  else
    item_type
  end
end

Whadya think?

seanlinsley · 2018-08-15T16:55:43Z

In #1137 I ended up fixing the bug (I think), but haven't added a test for it yet. I'd appreciate if you could review that PR 😄

This partially reverts commit 58369e1. I have kept the specs, skipped. Per the following, this approach does not seem to be working: - #1135 - #1137 - seanlinsley#1

…se STI (paper-trail-gem#1108) See the changes to the changelog and readme for details.

This partially reverts commit 58369e1. I have kept the specs, skipped. Per the following, this approach does not seem to be working: - paper-trail-gem#1135 - paper-trail-gem#1137 - seanlinsley#1

v10.0.0 * tag 'v10.0.0': (40 commits) Release 10.0.0 Testing the :skip option allow `object_changes_adapter` to use the default behavior Lint: Improve Metrics/AbcSize from 22 to 21 Docs: which class attributes are public/private Docs: installation Docs: Update changelog and readme re: paper-trail-gem#1143 Generator to update historic item_subtype entries (paper-trail-gem#1144) Testing joins, as recommended by Sean Add optional column: item_subtype Revert paper-trail-gem#1108 (lorint's STI fix) Lint: RSpec/EmptyLineAfterExampleGroup Update development dependencies Lint: RSpec/InstanceVariable in model_spec, ctn'd Code style re: errors Docs: Fix link to bug report Add association tracking removal exception Readme fix: caller.find {} rather than caller.first {} Do not require PT-AT Docs: Organizing the changelog for 10.0.0 ...

lorint added 3 commits June 7, 2018 03:06

Fix for issue paper-trail-gem#594, reifying sub-classed models that u…

f2ff35f

…se STI

Describe the fix for issue paper-trail-gem#594

1d5f91c

Rubocop niceties

113aa4b

lorint mentioned this pull request Jun 7, 2018

Override item_type in Versions table #1107

Closed

jaredbeck reviewed Jun 7, 2018

View reviewed changes

lorint added 6 commits June 7, 2018 20:49

Caching the calculated type_name for subclassed STI objects

9a6670f

Merge branch 'master' of paper_trail

f7c07a8

Out of the woods with Rubocop

b00d8dc

Test to confirm old STI version info will be compatible

d39d91d

Confirm old STI version info will be compatible

59c7b47

Verify old STI version info will be compatible

770813f

Strike assertions dependent upon PT-AT PR#5

ee3679d

lorint mentioned this pull request Jun 7, 2018

Fix for PT issue #594 westonganger/paper_trail-association_tracking#5

Closed

jaredbeck reviewed Jun 8, 2018

View reviewed changes

Small naming updates

14aa5df

Refactored versions_association_item_type

744a5d8

Simpler way to get correct subclassed item_type

1fdfcba

jaredbeck added 4 commits July 26, 2018 23:13

Remove unused MUTEX_FILE

63469c8

Replace optional constructor arg with private method

66cd7fb

I dislike optional arguments in general. I think they introduce more complexity than they are worth, in most cases.

Docs: Treat this as a breaking change

5550d48

[ci skip]

jaredbeck added 5 commits July 28, 2018 23:24

Basic item_type tests

d7a8027

setup_versions_association: minor cleanup, comments

33eecce

- Avoid repeating order clause - Method comments

Docs re: update_sti

bb1151b

Warn STI users about breaking change

a14a27d

Some people don't read changelogs.

Fix regression re: exception raised by record_create

3b9366f

jaredbeck mentioned this pull request Jul 29, 2018

Make the object column optional #1122

Merged

lorint commented Jul 30, 2018

View reviewed changes

jaredbeck merged commit 58369e1 into paper-trail-gem:master Jul 30, 2018

This was referenced Aug 15, 2018

Unable to join versions association #1135

Closed

Fix joins(:versions) #1137

Closed

jaredbeck added a commit that referenced this pull request Aug 22, 2018

Revert #1108 (lorint's STI fix)

9f004a6

This partially reverts commit 58369e1. I have kept the specs, skipped. Per the following, this approach does not seem to be working: - #1135 - #1137 - seanlinsley#1

jaredbeck mentioned this pull request Aug 22, 2018

New optional column: item_subtype #1143

Merged

lorint mentioned this pull request Aug 26, 2018

Causes PaperTrail "destroy" versions to not be found appfolio/store_base_sti_class#37

Open

aried3r pushed a commit to aried3r/paper_trail that referenced this pull request Dec 14, 2020

Fix for issue paper-trail-gem#594, reifying sub-classed models that u…

55dfc56

…se STI (paper-trail-gem#1108) See the changes to the changelog and readme for details.

aried3r mentioned this pull request Jul 21, 2021

Improve fetching reified class names for STI models #1332

Closed

		item_type = object.paper_trail.versions_association_item_type
		if item_type.blank?

Fix for issue #594, reifying sub-classed models that use STI #1108

Fix for issue #594, reifying sub-classed models that use STI #1108

Conversation

lorint commented Jun 7, 2018 • edited Loading

jaredbeck commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredbeck commented Jun 7, 2018

lorint commented Jun 7, 2018

jaredbeck 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

lorint Jun 8, 2018 • 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

lorint commented Jun 8, 2018 • edited Loading

jaredbeck commented Jun 10, 2018

jaredbeck commented Jun 10, 2018

lorint commented Jun 15, 2018

jaredbeck commented Jun 15, 2018 • edited Loading

lorint commented Jun 15, 2018

jaredbeck commented Jun 15, 2018

lorint commented Jun 15, 2018 • edited Loading

jaredbeck commented Jun 15, 2018

lorint commented Jun 15, 2018

jaredbeck commented Jul 27, 2018

jaredbeck commented Jul 29, 2018

lorint 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

lorint commented Jul 30, 2018

jaredbeck commented Jul 30, 2018

seanlinsley commented Aug 14, 2018

lorint commented Aug 15, 2018

seanlinsley commented Aug 15, 2018

lorint commented Jun 7, 2018 •

edited

Loading

lorint Jun 8, 2018 •

edited

Loading

lorint commented Jun 8, 2018 •

edited

Loading

jaredbeck commented Jun 15, 2018 •

edited

Loading

lorint commented Jun 15, 2018 •

edited

Loading