Clean up mixins #148

New Issue

mpeltriaux · 2022-04-14T09:19:23+02:00

mpeltriaux commented

2022-04-14 09:19:23 +02:00

Status quo

Mixins are used to encapsule methods and attributes for all major datatypes, which are used in different models, e.g. RecordableObjectMixin provides the attributes and methods used specifically for all recording related issues.

There are some code fragments inside these mixins, which assert the mixin to be inherited by another mixin or another regular class, e.g. BaseObject, which provides the attribute identifier:

class RecordableObjectMixin:
    recorded = models.OneToOneField(
        "user.UserActionLogEntry",
        on_delete=models.SET_NULL,
        null=True,
        blank=True,
        help_text="Holds data on user and timestamp of this action",
        related_name="+"
    )
    class Meta:
        abstract = True
        
    ...
    
    def set_recorded(self, user):
        """ Perform recording

        Args:
            user (User): Performing user

        Returns:

        """
        from user.models import UserActionLogEntry
        if self.recorded:
            return None

        self.unshare_with_default_users()
        action = UserActionLogEntry.get_recorded_action(user)
        self.recorded = action
        self.save()
        self.log.add(action)

        shared_users = self.shared_users.values_list("id", flat=True)
        shared_teams = self.shared_teams.values_list("id", flat=True)

        for user_id in shared_users:
            celery_send_mail_shared_data_recorded.delay(self.identifier, self.title, user_id)

        for team_id in shared_teams:
            celery_send_mail_shared_data_recorded_team.delay(self.identifier, self.title, team_id)

        return action

In here the attributes identifier and title are being used, despite the fact they do not even exist on the mixin but only on BaseObject. Therefore we assert silently that this mixin can only be used properly if combined with BaseObject.

For our current purpose, this is a handy solution, since all needed logic can be wrapped entirely in the mixins and we can focus on model specific methods on the main models, which inherits the mixins.

Why toDo?

However, what if we will extend our model base (someday in the future) by another model, which does not follow our current model architecture, based on BaseObject and proper mixins? The mixin methods could possibly fail, if a required attribute in the method does not exist on this new model, since it's structured differently.

Solution

We would need to restructre the code a little bit to be safe for such an occassion:

Identify all code fragments inside the mixins, which rely on non-mixin attributes or even non-mixin methods
Inherit the mixin methods into the inheriting model class and move the non-mixin attributes or non-mixin method calls into this inherited methods. Start each of these methods with a super call to the inhertied method, to make sure the mixin logic will be performed before the inherited model logic

Using this approach we will have a lot cleaner mixins, since they only focus on their existing attributes. On the other hand, of course, we will move all these different code fragments into the model classes, which will grow in size (a little). Furthermore there will be redundancy, since formerly in a mixin placed logic will now need to live in Intervention, Compensation, EMA and EcoAccount.

# Status quo Mixins are used to encapsule methods and attributes for all major datatypes, which are used in different models, e.g. `RecordableObjectMixin` provides the attributes and methods used specifically for all recording related issues. There are some code fragments inside these mixins, which assert the mixin to be inherited by another mixin or another regular class, e.g. `BaseObject`, which provides the attribute `identifier`: ```python class RecordableObjectMixin: recorded = models.OneToOneField( "user.UserActionLogEntry", on_delete=models.SET_NULL, null=True, blank=True, help_text="Holds data on user and timestamp of this action", related_name="+" ) class Meta: abstract = True ... def set_recorded(self, user): """ Perform recording Args: user (User): Performing user Returns: """ from user.models import UserActionLogEntry if self.recorded: return None self.unshare_with_default_users() action = UserActionLogEntry.get_recorded_action(user) self.recorded = action self.save() self.log.add(action) shared_users = self.shared_users.values_list("id", flat=True) shared_teams = self.shared_teams.values_list("id", flat=True) for user_id in shared_users: celery_send_mail_shared_data_recorded.delay(self.identifier, self.title, user_id) for team_id in shared_teams: celery_send_mail_shared_data_recorded_team.delay(self.identifier, self.title, team_id) return action ``` In here the attributes `identifier` and `title` are being used, despite the fact they do not even exist on the mixin but only on `BaseObject`. Therefore we assert silently that this mixin can only be used properly if combined with `BaseObject`. For our current purpose, this is a handy solution, since all needed logic can be wrapped entirely in the mixins and we can focus on model specific methods on the main models, which inherits the mixins. # Why toDo? However, what if we will extend our model base (someday in the future) by another model, which does not follow our current model architecture, based on `BaseObject` and proper mixins? The mixin methods could possibly fail, if a required attribute in the method does not exist on this new model, since it's structured differently. ## Solution We would need to restructre the code a little bit to be safe for such an occassion: 1. Identify all code fragments inside the mixins, which rely on non-mixin attributes or even non-mixin methods 1. Inherit the mixin methods into the inheriting model class and move the non-mixin attributes or non-mixin method calls into this inherited methods. Start each of these methods with a super call to the inhertied method, to make sure the mixin logic will be performed before the inherited model logic Using this approach we will have a lot cleaner mixins, since they only focus on their existing attributes. On the other hand, of course, we will move all these different code fragments into the model classes, which will grow in size (a little). Furthermore there will be redundancy, since formerly in a mixin placed logic will now need to live in `Intervention`, `Compensation`, `EMA` and `EcoAccount`.

mpeltriaux added the

backlog

label 2022-04-14 09:19:27 +02:00

mpeltriaux self-assigned this 2022-04-14 09:19:30 +02:00

mpeltriaux commented

2022-04-25 08:26:43 +02:00

After further research regarding best practices, it looks like Mixins are expected to behave like this. Mixins are generally seen as primary holding methods for other classes, which attributes are in most cases unknown to them.

After further research regarding best practices, it looks like Mixins **are expected to behave like this**. Mixins are generally seen as primary holding methods for other classes, which attributes are in most cases unknown to them.

mpeltriaux closed this issue

2022-04-25 08:26:43 +02:00

Sign in to join this conversation.