1415732 - Refactor base/models.py into smaller files

Status: RESOLVED MOVED

(Webtools Graveyard :: Pontoon, enhancement, P4)

Description

[Jarek Śmiejczak [:jotes]]

This bug is a follow-up of our discussion from the irc channel.

Currently, base/models.py contains nearly 2k lines of code related to the models. At some point, It's hard to maintain such huge file.

I decided to suggest possible approaches:
1. Split "base" app into smaller apps if that's possible.
2. Change the structure of base app. Move all classes into separate files and put them in subdirectory called models.

I feel that 1. would be hard in the current design of Pontoon almost all classes in the base module are essential and they're really hard to split.
I would leave that refactoring for Pontoon.Next because there's a huge chance that structure of Database will change then.

Also, there's no point to create small modules that would contain only one class without any views, forms etc.

In case of 2. solution, I think we could start with the following structure of new files:

pontoon/base/models/user.py:

* All extensions related to django.contrib.auth.models.User
* class UserTranslationsManager(UserManager):
* class UserCustomManager(UserManager):
* class UserQuerySet(models.QuerySet):
* class UserProfile(models.Model):

pontoon/base/models/stat.py

* class AggregatedStats(models.Model):

pontoon/base/models/locale.py
* class LocaleQuerySet(models.QuerySet):
* class Locale(AggregatedStats):

pontoon/base/models/project.py

* class ProjectQuerySet(models.QuerySet):
* class Project(AggregatedStats):
* class ProjectLocaleQuerySet(models.QuerySet):
* class ProjectLocale(AggregatedStats):

pontoon/base/models/repository.py

* class Repository(models.Model):

pontoon/base/models/resource.py

* class ResourceQuerySet(models.QuerySet):
* class Resource(models.Model):
* class ExternalResource(models.Model):

pontoon/base/models/subpage.py

* class Subpage(models.Model):

pontoon/base/models/entity.py

* class EntityQuerySet(models.QuerySet):
* class Entity(DirtyFieldsMixin, models.Model):
* class ChangedEntityLocale(models.Model):

pontoon/base/models/translation.py

* class TranslationNotAllowed(Exception):
* class TranslationQuerySet(models.QuerySet):
* class Translation(DirtyFieldsMixin, models.Model):
* class TranslationMemoryEntryManager(models.Manager):
* class TranslationMemoryEntry(models.Model):
* class TranslatedResourceQuerySet(models.QuerySet):
* class TranslatedResource(AggregatedStats):

I don't feel that the whole issue is urgent but this particular improvement feels relatively low hanging (correct me if I'm wrong :).
Anyway, feel free to bash my structure and suggest better ideas.

Comment 1

[Axel Hecht [:Pike]]

3) Move business logic out of the models.

I've come across a couple of "maybe not" on fat django models over the twitter lately, so maybe that's an interesting alternative.

One elmo, I move towards class-based views, with the methods on the views being good for actual unit testing, and just keep my models slim.

Comment 2

[Jarek Śmiejczak [:jotes]]

:Pike

That's a good point - I feel that We should end up with the solution that would of contains parts of 2. and 3.
Some parts of the logic are stored in managers - maybe that code also should land in class-based views? Some of the methods are strongly related to The Model/Manager pattern and they may be hard to extract.

Also, I wonder if we should split that refactoring into phases to avoid too many changes at once. Maybe it's better to leave it for Pontoon.Next (?)

I'll NI Adrian and Mathjazz for their thoughts.

Comment 3

[Matjaz Horvat [:mathjazz]]

I'm sure you all have more experience with Django models best practices, and I fully trust you.

That said, I have concerns about proceeding with this refactor at this point in time. For two reasons:
1. It will cause conflicts in several open PRs.
2. Pontoon.NEXT.

Comment 4

[[DEACTIVATED] Adrian Gaudebert]

Pontoon.Next should not be a cause of concern for this. On the contrary, I think it would benefit from it. Well actually, I think the entire world would benefit from this, because splitting that file will be a great thing, and I think it will allow us to make more improvements in the future. But I digress. Pontoon.Next is, in the foreseeable future, only going to impact front-end code, and I want to avoid touching the django side as much as possible as part of it. Changes there should thus not be a problem. 

The question of conflicts is indeed an issue. But the actual issue, in my opinion, is that we have so many open pull requests at the moment. I believe we should close PRs we are not actively working on, and decide that it's the developer's responsibility to resolve conflicts. I dislike the idea that we refrain ourselves from doing good things because of a dormant PR. 

I think what you propose jotes is a very good first step, as it doesn't require any algorithmic change and will likely help surface other means of improving the overall models structure. My opinion is, go for it whenever you want to! 

Just one slight proposal: 

pontoon/base/models/locale.py
* class LocaleQuerySet(models.QuerySet):
* class Locale(AggregatedStats):

pontoon/base/models/project.py

* class ProjectQuerySet(models.QuerySet):
* class Project(AggregatedStats):

pontoon/base/models/projectlocale.py

* class ProjectLocaleQuerySet(models.QuerySet):
* class ProjectLocale(AggregatedStats):

:)

Comment 5

[BMO Automation]

*This bug has been moved to GitHub.*

*Please check it out on https://github.com/mozilla/pontoon/issues.*