Added Model Delta and HistoricalChanges #277

leportella · 2017-05-27T20:13:56Z

This is related to the issue #244 that I have opened earlier.

Hope it is helpful :)

Closes #244

codecov-io · 2017-05-27T20:19:13Z

Codecov Report

Merging #277 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #277     +/-   ##
=========================================
+ Coverage   97.48%   97.59%   +0.1%     
=========================================
  Files          12       12             
  Lines         557      582     +25     
  Branches       66       70      +4     
=========================================
+ Hits          543      568     +25     
  Misses          7        7             
  Partials        7        7

Impacted Files	Coverage Δ
simple_history/models.py	`97.08% <100%> (+0.4%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d76e446...64ac78d. Read the comment docs.

treyhunner

I really like this. I'm a little hesitant to add a new feature like this because whatever API we choose initially will be something we'll need to build up from (assuming we decide not to break backwards compatibility).

I don't want to bike shed too much on this one, but I do want to give it a little extra care since we're adding a new feature. So the feedback I've added is mostly of the "I'd like to hear the justification for this design decision" variety.

Linking to the ModelTracker discussion for django-model-utils (jazzband/django-model-utils#26) because we went back and forth quite a bit before merging. It's a similar feature, but still fairly different so I'm not sure how much insight we could borrow from it.

I'm looking forward to seeing this one merged!

treyhunner · 2017-06-04T21:47:26Z

simple_history/models.py

+                    changed_fields.append(field.name)
+        return model_delta_class(changes,
+                                 changed_fields=changed_fields,
+                                 date=self.history_date,


Should date by self.history_date or old_history.history_date? Is one date more significant than the other or should we represent them both? Or maybe represent neither since self and old_history can be found via the new_history and old_history attributes?

I haven't given this much thought, but I'd like to hear an argument for one of these variations.

Well, on the case I applied here in our company we used this subtractions to build timelines. Since the history is stored as soon as the model changes, I added the user and date from the last historical record. But indeed this was a very specific case. Do you think we should add both?

Maybe we should add neither for now since new_history and old_history allow fetching each individually.

We can always add them later if we decide there's a good use case for copying them over to make them more easily accessible.

Random idea from a simple_history user: What about returning a timedelta?

treyhunner · 2017-06-04T21:50:17Z

simple_history/models.py

@@ -311,3 +311,46 @@ def __get__(self, instance, owner):
        values = (getattr(instance, f.attname)
                  for f in self.model._meta.fields)
        return self.model(*values)
+
+
+class HistoricalChanges(models.Model):


I think tuple(bases) will already inherit from models.Model, so maybe this should be more of a mixin that just inherits from object. I'm thinking something like this:

class HistoricalChangesMixin(object):

Then the class Meta: abstract = True wouldn't be needed.

Nice! I think this will be better as well

treyhunner · 2017-06-04T21:56:10Z

simple_history/models.py

+                old_value = getattr(old_history, field.name, '')
+                new_value = getattr(self, field.name)
+                if old_value != new_value:
+                    changes[field.name] = {'old_value': old_value,


I think the API might be a little easier to use if this were a namedtuple instead of a dictionary, so changes could be unpacked like this:

for old, new in changes: pass # something

Not sure what the name and attributes should be. The name could be long but the attributes might be nice to keep short. Maybe namedtuple('HistoricalChange', ['old', 'new']). Or ModelChange.

Don't know what nametuple are. I will look into this and get back at you!

Great!

namedtuple allows making a tuple-like class on the fly, like this:

>>> from collections import namedtuple >>> HistoricalChange = namedtuple('HistoricalChange', ['old', 'new']) >>> change = HistoricalChange(old=5, new=6) >>> change.old 5 >>> change.new 6 >>> old, new = change >>> old 5 >>> new 6

(I'm not saying we should necessarily use those names as the name/attributes... just an example)

treyhunner · 2017-06-04T21:57:55Z

simple_history/models.py

+        return model_delta_class(changes,
+                                 changed_fields=changed_fields,
+                                 date=self.history_date,
+                                 user=self.history_user,


I'm not sure whether this should be the old or new user either (or whether one is more important than the other). Maybe this attribute isn't needed? I'm up for hearing arguments either way.

Well, as I said on the other comments, we used this to build timelines, so the user and date that the instance changed was very relevant. Maybe we should keep both.

I think maybe we should do neither here as well. If it becomes burdensome to type my_delta.new_record.user and my_delta.old_record.user we can make shortcuts later.

I'm open to re-considering and adding this and/or the historical date feature, but I'd like to not add them unless we're sure they're particularly helpful. One less feature to maintain? 😄

treyhunner · 2017-06-04T22:03:25Z

simple_history/models.py

+                                 changed_fields=changed_fields,
+                                 date=self.history_date,
+                                 user=self.history_user,
+                                 old_history=old_history,


I'm not sure what we should call these (old_history/new_history, old_record/new_record, old/new, etc.). I'd prefer to be consistent, but I'm not sure whether we've made variable names referring to these objects yet.

I prefer old_record/newrecord because we are considering each instance and just history is not detailed enough indeed. I think that just old/new can be quite confusing.

That makes sense to me. Let's go with old_record and new_record then.

cloudsurf-digital · 2018-05-11T10:33:06Z

sadly no progress since long.. Can we still expect a merge?

rossmechanic · 2018-05-11T14:53:18Z

@johnny-die-tulpe Going to be working on getting this merged in the next few months

kseever · 2018-06-01T14:06:23Z

simple_history/models.py

+    class Meta:
+        abstract = True
+
+    def __sub__(self, old_history):


My C++ background left me with a bad taste in my mouth for operator overloads. I tend to favor explicit method calls.

delta = current.changesSince(prior)

atodorov · 2018-07-05T07:56:17Z

@kseever, @rossmechanic any update on this ? It looks like this is exactly what I need so is there anything I can do to help/speed-up the inclusion of this PR ?

rossmechanic · 2018-07-10T22:43:01Z

Closing this with #416

Added Model Delta and HistoricalChanges

64ac78d

treyhunner suggested changes Jun 4, 2017

View reviewed changes

treyhunner mentioned this pull request Jun 4, 2017

Compare history versions #103

Closed

kseever reviewed Jun 1, 2018

View reviewed changes

rossmechanic closed this Jul 10, 2018

Added Model Delta and HistoricalChanges #277

Added Model Delta and HistoricalChanges #277

Uh oh!

Conversation

leportella commented May 27, 2017

Uh oh!

codecov-io commented May 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

treyhunner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merwok Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

treyhunner Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloudsurf-digital commented May 11, 2018

Uh oh!

rossmechanic commented May 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atodorov commented Jul 5, 2018

Uh oh!

rossmechanic commented Jul 10, 2018

Uh oh!

Uh oh!

codecov-io commented May 27, 2017 •

edited

Loading

treyhunner left a comment •

edited

Loading

merwok Nov 7, 2017 •

edited

Loading

treyhunner Jun 13, 2017 •

edited

Loading