Allow historical tracking to be set on abstract bases

[macro1]

Addresses #63.

@treyhunner, thoughts? It doesn't seem as bad as I was expecting.

[treyhunner]

The solution looks good to me.

We should probably add some more tests for other inheritance structures. For example a test that non-abstract inheritance shouldn't track history in the child. Maybe even a test for a weird case like this:

class TrackedAbstractBase(models.Model):
    history = HistoricalRecords()

    class Meta:
        abstract = True


class InBetween(TrackedAbstractBase):
    pass

class Tracked(InBetween):
    pass

[macro1]

I would actually expect all children of TrackedAbstractBase to have the history attribute. Where you expecting tracking to only be inherited by the first concrete child in that branch of the inheritance tree? I'm only preventing inheritance when the original tracked model is concrete currently.

Also, @paulcollinsiii mentioned in the original issue that tracking through inheritance will allow attempted multiple tracking models for the same model. We probably want a meaningful error raised when that happens so the issue and resolution are clear.

Do we want models registered through register() to have the same inheritable quality? This could cause problems since the children could already be prepared before register() is called.

Maybe we should change this feature to be opt-in? Something like history = HistoricalRecords(inheritable=True) If that's the case, allow non-abstract models to carry the same inheritable property?

[naegelyd]

Really looking forward to this feature. Anything I can do to help get this PR finished and ready to merge?

[rocktavious]

+1 i would really love this feature too, let me know how i can help.

[ccarlos]

+1, Would like to see this as well. I can provide a hand too. thx

[macro1]

@naegelyd / @rocktavious / @ccarlos:

Thanks for your interest! Do any of your have some specifics of how you expect the tracking to propegate? Would you expect only direct concrete subclasses to be tracked, as @treyhunner mentions above?

If we looked at inheritance, I think Django makes no restriction on the model parent combinations, so we need to consider all of these:

• Single abstract parent with tracking
• Multiple abstract parents with at least one having tracking
• Multiple parents, abstract and concrete, abstract has tracking
• Indirect through an abstract parent
• Indirect through a concrete parent

Probably the easiest to add and support is just the first, a concrete model with a single abstract parent that defines a history manager.

[naegelyd]

My current use case is single abstract parent that defines a history manager so, if we started with support for that, it would solve my issue but might cause more unexpected behavior. Being new to this project, I would personally assume that all five of these cases would include history tracking on my model.

• Single abstract parent with tracking
• Multiple abstract parents with at least one having tracking
• Multiple parents, abstract and concrete, abstract has tracking
• Indirect through an abstract parent
• Indirect through a concrete parent

I would expect the child model to include history tracking on all these cases. While I know it is the more complicated route, it seems the one that makes most sense to me. All that said, I'm not going to argue much if only the single abstract parent with child model is the only case supported originally.

[jgr3go]

Are there any plans to merge this in? I agree with @naegelyd that if you're inheriting, regardless of whether it's an abstract parent or not, you get the history records. I see no reason why it would be anything but that. Either way, it's been over a year since this was requested and this would really help clean my code up.

[macro1]

@jgr3go: I know the changes above look simple, but Django model inheritance is surprisingly flexible. Without tests for each case we can't get it merged in. I will probably return to work on this when I have time, but if you could look at it as well feel free to branch off of it and either make a PR back to this branch or to master.

[macro1]

Alright, to try it out... Install this branch and when using HistoricalRecords or register() pass in inherit=True:

class TrackedParent(models.Model):
    history = HistoricalRecords(inherit=True)


class TrackedChild(models.Model):
    pass

I think we can easily support history tracking on any model that is registered once (through inheritance, the HistoricalRecords descriptor, or the register() function). Referencing the inheritance examples above, that would mean at least partial support for all of them.

In the cases where behavior is ambiguous (the same model registered multiple times to have history tracking) I'm currently opting to raise an exception, this will cause this only breaking change for current behavior. Previously calling register() multiple times on the same function would do nothing, with these changes it will raise an exception. I think that's acceptable.

I did not touch the handling of inherited fields, so models inheriting from concrete parents will track the reference to the parent, but none of the fields on the parent model. The exact same behavior you'll see currently if a child of a concrete model is registered directly.

I'll work on getting additions to the documentation before merging this in... If anyone interested in this feature could try it out, it'd be appreciated.

[mikeengland]

@macro1 I'd be really interested in using this feature. Is there an ETA for a merge?

[macro1]

@mikeengland, no hard timeline. I was kind of hoping a few people would try it out before it gets merged in, but maybe it'll be fine.

[macro1]

Released as 1.8.0. Thanks everyone for your interest and input!

[bringfido-adams]

This may or may not be useful, but we implemented a Model abstract class that dynamically adds HistoricalRecord to subclasses with track_history = True. The class also has a name mangled class variable that allows all the subclasses to implement different __history_tracked_fields for each subclass. The HistoricalRecord that gets created gets its excluded_fields set in a way based on the accumulation of all the __history_tracked_fields of the entire parent tree. This way one class can specify a set of fields to track, and a subclass doesn't need to re-list those fields in its tracked list.

def get_all_parent_classes(classes):
    """Return a list of parents+classes from a list of classes."""
    all_classes = []
    for this_class in classes:
        new_classes = [
            this_class,
            *get_all_parent_classes(this_class.__bases__),
        ]
        all_classes.extend(new_classes)
    return all_classes


class HistoryTrackableMetaclass(ModelBase):
    """Dynamically creates the HistoricalRecords object for a class.

    Will add a HistoricalRecords to all Models with this metaclass that have
    track_history set to True. It is used by HistoryTrackable which can be
    inherited from to make a model use simple_history.
    """

    def __new__(cls, class_name, class_bases, class_dict):
        """Override default ModelBase metaclass __new__.

        This function creates the class object for HistoryTrackable (not an
        instance object).
        """
        # Call ModelBase's __new__ to generate the full field list
        class_obj = super().__new__(cls, class_name, class_bases, class_dict)

        # If track_history is true on this class, create a HistoricalRecords on
        # this class definition so simple_history registers it.
        if class_dict.get('track_history', False):
            class_lineage = (
                class_name,
                *list({this_class.__name__ for this_class in get_all_parent_classes(class_bases)}),
            )

            # Union all parent classes' and this class' tracked fields
            tracked_fields = set()
            for class_name in class_lineage:
                mangled_var_name = f'_{class_name}__history_tracked_fields'
                mangled_var_value = getattr(class_obj, mangled_var_name, [])
                tracked_fields |= set(mangled_var_value)

            # Create a set of all this class's fields including parents'
            all_fields = set()
            for class_var_name, class_var in class_obj.__dict__.items():
                # var is a DeferredAttribute after class definition if it is a
                # Field normally.
                if isinstance(class_var, DeferredAttribute) and isinstance(
                    class_var.field,
                    models.Field,
                ):
                    all_fields.add(class_var_name)

            # Create the HistoricalRecords object and attach it to the class
            excluded_fields = list(all_fields - tracked_fields)
            class_obj.history = HistoricalRecords(excluded_fields=excluded_fields)
            class_obj.full_tracked_fields = tracked_fields

            # The HistoricalRecords object needs some setup that is usually
            # performed by ModelBase.
            class_obj.history.contribute_to_class(class_obj, 'history')
            class_obj.history.finalize(class_obj)
        else:
            class_obj.full_tracked_fields = []

        return class_obj


class HistoryTrackable(models.Model, metaclass=HistoryTrackableMetaclass):
    """Model mixin for simple_history.

    Adds inheritable opt-in field tracking.

    Name mangled class variables:
    https://docs.python.org/3/tutorial/classes.html#private-variables
    """

    # Define in each class to accumulate inherited opt-in field tracking.
    # If not defined, no fields of that class are tracked.
    __history_tracked_fields = ['id']  # noqa: WPS112 (Name mangled class variable)

    # Set history_tracking to True on any child Model that should track
    # history. All the child's parents' __history_tracked_fields will be
    # accumulated to create the list of fields to track.
    history_tracking = False

    class Meta:
        abstract = True

    def save(self, track_history=True, *args, **kwargs):
        """Save that adds an option to skip creating a historical record.

        Pass in track_history=False for cases where automated saves would
        create clutter in the history.
        """
        if not track_history:
            self.skip_history_when_saving = True

        self._change_reason = self.get_changes()

        model_object = super().save(*args, **kwargs)

        if not track_history:
            del self.skip_history_when_saving
        return model_object