Implementing Abstract Models

[nemesifier]

Followup of #118.

Consider this a work in progress. I think it will take a few weeks to complete, but I managed to get something working early, so I can try to integrate it in the project I'm working on (named OpenWISP2 btw) before trying to merging it here.

I will also write some tests for this new use case, modeling them against the existing ones.

• working rough implementation
• improve naming of leading underscore classes
• decide if the leading underscore classes should be kept private or made public
• decide if the new abstract classes should be prefixed with Abstract (would need to modify OrgMeta)
• add tests for new use case
• update documentation

[bennylope]

Just reading this at a glance the one thing that sticks out is naming with leading underscores. This makes sense for 'quasi-protected' methods/attributes, but should be avoided for class names. Better to replace with something more descriptive.

[bennylope]

Tests pass! I'm going to plan on merging this into a new branch instead of dev for the time being until it's a bit more solidified with doc updates as necessary.

Regarding the naming issue above, that is not urgent.

[bennylope]

I created a new branch better-abstract-models from your PR.

[nemesifier]

👍

[nemesifier]

We'll speak again on tuesday. I'm going to be busy because going to FOSDEM, so if you don't hear from me next week, you will surely hear me again the week later.

[nemesifier]

@bennylope the classes with the leading underscores contain the base logic, just without the metaclass machinery. That base logic is then extended by the new abstract classes with the metaclass machinery on top of it. The same base logic is also used on the old base classes to maintain backward compatibility.

I think there's 3 things that need to be discussed and decided:

• we should find a good name for the classes that now have leading underscores
• decide if we want to keep the classes with leading underscores private (maybe avoid documenting them?) or explicitly make them public (and document them)
• it would be better if the (new) abstract classes could be prefixed with something like Abstract, eg: AbstractOrganization in order to differentiate them from the current Base classes, but to do this I suppose OrgMeta has to be modified and I would need a tiny bit of guidance to do so

[bennylope]

In brief, I think it's totally fine if the internal meta classes are not externally documented (although should be in docstrings!). The important thing is that the names make sense and everything's backwards compatible (breaking backwards compatibility for massive improvements isn't out of the question if it's necessary and/or significantly reduces a lot of cruft, and is well documented for end users). These underscore classes are what, mediating classes? Maybe something that indicates the role of the class would be good for a name. I'm less concerned about the specific name and more that it makes sense to someone reading the code for the first time.

What does a Django dev expect a base class to be named? What kind of conventions does the name imply?

Is there any reason that someone using the existing base classes, e.g. OrganizationBase would be negatively affected if this were replaced by what now constitutes your proposed AbstractBase? Just a thought - if not - since any methods they've added would override the ones you've added - then perhaps your new classes should be the 'base' classes in the base module and the meta classes should be moved to a distinct module.

[nemesifier]

Inside django, there is AbstractBaseUser and AbstractUser.
AbstractBaseUser is equivalent to the new classes with leading underscores while AbstractUser is equivalent to the new abstract classes. Both are documented.

If we want to follow this convention, we should name the leading underscore classes as AbstractBaseOrganization, AbstractBaseOrganizationOwner, AbstractBaseOrganizationUser or something similar, and the new classes should be named AbstractOrganization, AbstractOrganizationOwner, AbstractOrganizationUser.

The current implementation is backward compatible, I did not change the behaviour of the currently documented base classes.

I think changing the existing base classes can affect negatively apps already extending django-organizations. The safest option is to add the new classes, document them, deprecate the old ones, remove their mention from the docs and consider removing them if everything goes fine and no one complains after a year or two.

[nemesifier]

@bennylope the commit db6a295 contains an implementation of the new naming proposal that follows Django's AbstractUser and AbstractBaseUser naming convention.

I propose to:

• add tests for this new use case
• document both class groups (Abstract and AbstractBase)
• deprecate the old base classes on the docs

I can do this work if you let me know what you think.

Federico Capoano

[nemesifier]

PS: I'm reading the code but I haven't understood how to add tests for this new use case. I have taken a look at the existing dir test_vendors, but noticed the model logic (test_vendors/models.py) is duplicated in example/vendors/models.py.

[bennylope]

model logic (test_vendors/models.py) is duplicated in example/vendors/models.py.

Yeah, the example probably needs to go at some point (not now). I'd create another test_ app to add testable functionality, following the convention of test_custom, test_accounts; perhaps test_abstract defining any new usage to be tested. Unless you can think of a better way that is.

[nemesifier]

@bennylope I did create test_abstract in my local repo (haven't pushed it yet) but did not understand how to test it in practice. The amount of tests performed looks always the same.

How can I, for example, test only the new test_abstract or perform tests only for test_vendors?

[nemesifier]

@bennylope I figured out how the previous tests for custom organization apps were added and followed those examples. I also have updated the docs.
I'm using this development version to organizations to OpenWISP2 and it's going pretty well.
Backward compatibility is maintained and I realised it's probably good to keep the old base classes as well, some people may need a more minimalistic approach (the updated documentation explain this).

Looking forward to hear your feedback.

PS: i've rebased, modified and renamed a few commits in order to cleanup the pull request

[bennylope]

I'm hoping (!!) to have some time in the next day or two to look over, but my intent is to merge this into dev now.

[nemesifier]

@bennylope ok great. I had to update the PR because I spotted a little but considerable mistake in the docs, I was referring to the old base models as "prefixed" with Base while in reality Base is their suffix.

[icyc9]

LGTM. Excited to see this merged.