Due to the complexities involved with introducing more permissions and new ways of filtering data (e.g. by location), we need to clean up the usage of StaffPerson so that it more closely aligns with Django Auth defaults.

In order to implement sane permissions for #167, we have to deal with unforeseen complications related to the pairing of Auth.User and Staff_Person.StaffPerson (and Auth.Group with Staff_Person.StaffJob). Work done so far has uncovered a series of interlocking problems with no one simple or clear solution.

So, to simplify the system, we'll now switch to extending Auth.User to include fields necessary for StaffPerson to operate (and StaffPerson may become a Proxy model in the meantime). StaffJob will be superseded by Auth.Group. Permissions will then be applied to Groups.

There's a few things to have to deal with here. So far they include:

Data migration process to be determined
- Need to create Users for StaffPersons (whilst addressing duplicates #180)
- Then change models that point at StaffPersons to point at Users? Or a Proxy
- Then migrate data to point to the new User Ids
Need to update fixtures to capture the new groups and their default permissions
Work out how django-revision will operate against User (bear in mind frequent updates with last_logged in), and how created_by will be stored (if still needed?)

This model change will then need to be tested against the person selection lists (i.e. Transplant Co-ord, Theatre Contact, etc)

Publish a new protocol for creating full users.

A new milestone (0.8.0) has been created to reflect this reboot, and a new branch has been taken from the 0.6.4 Testing branch (Dev-0.8.0). Will be implementing User changes first, and then bringing in additional work done in the other branches that is ready for deployment after this is working.

Turns out those notes were optimistic. Due to dependancies within our application, a "simple" move to a custom user model isn't an option... see combinations of the following errors as we try different approaches:

SystemCheckError: System check identified some issues:

ERRORS:
auth.User.groups: (fields.E304) Reverse accessor for 'User.groups' clashes with reverse accessor for 'Person.groups'.
        HINT: Add or change a related_name argument to the definition for 'User.groups' or 'Person.groups'.
auth.User.user_permissions: (fields.E304) Reverse accessor for 'User.user_permissions' clashes with reverse accessor for 'Person.user_permissions'.
        HINT: Add or change a related_name argument to the definition for 'User.user_permissions' or 'Person.user_permissions'.
staff_person.Person.groups: (fields.E304) Reverse accessor for 'Person.groups' clashes with reverse accessor for 'User.groups'.
        HINT: Add or change a related_name argument to the definition for 'Person.groups' or 'User.groups'.
staff_person.Person.user_permissions: (fields.E304) Reverse accessor for 'Person.user_permissions' clashes with reverse accessor for 'User.user_permissions'.
        HINT: Add or change a related_name argument to the definition for 'Person.user_permissions' or 'User.user_permissions'.

pm check
DEBUG: reading env data from /Users/carl/Projects/py3_cope/cope_repo/local.env
DEBUG: Loading settings from development
Traceback (most recent call last):
  File "manage.py", line 14, in <module>
    execute_from_command_line(sys.argv)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/core/management/__init__.py", line 367, in execute_from_command_line
    utility.execute()
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/core/management/__init__.py", line 341, in execute
    django.setup()
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/__init__.py", line 27, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/apps/registry.py", line 115, in populate
    app_config.ready()
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/contrib/admin/apps.py", line 23, in ready
    self.module.autodiscover()
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/contrib/admin/__init__.py", line 26, in autodiscover
    autodiscover_modules('admin', register_to=site)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/utils/module_loading.py", line 50, in autodiscover_modules
    import_module('%s.%s' % (app_config.name, module_to_search))
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 986, in _gcd_import
  File "<frozen importlib._bootstrap>", line 969, in _find_and_load
  File "<frozen importlib._bootstrap>", line 958, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 673, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 662, in exec_module
  File "<frozen importlib._bootstrap>", line 222, in _call_with_frames_removed
  File "/Users/carl/Projects/py3_cope/cope_repo/wp4/compare/admin.py", line 322, in <module>
    admin.site.register(Organ, OrganAdmin)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/contrib/admin/sites.py", line 108, in register
    admin_obj = admin_class(model, self)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/reversion/admin.py", line 147, in __init__
    inline_model, follow_field = self._reversion_introspect_inline_admin(inline)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/reversion/admin.py", line 131, in _reversion_introspect_inline_admin
    issubclass(self.model, remote_model(field))
TypeError: issubclass() arg 2 must be a class or tuple of classes

pm makemigrations staff_person
DEBUG: reading env data from /Users/carl/Projects/py3_cope/cope_repo/local.env
DEBUG: Loading settings from development
Traceback (most recent call last):
  File "manage.py", line 14, in <module>
    execute_from_command_line(sys.argv)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/core/management/__init__.py", line 367, in execute_from_command_line
    utility.execute()
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/core/management/__init__.py", line 359, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/core/management/base.py", line 294, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/core/management/base.py", line 345, in execute
    output = self.handle(*args, **options)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/core/management/commands/makemigrations.py", line 149, in handle
    loader.project_state(),
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/db/migrations/loader.py", line 317, in project_state
    return self.graph.make_state(nodes=nodes, at_end=at_end, real_apps=list(self.unmigrated_apps))
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/db/migrations/graph.py", line 402, in make_state
    for migration in self.forwards_plan(node):
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/db/migrations/graph.py", line 280, in forwards_plan
    self.ensure_not_cyclic(target, lambda x: (parent.key for parent in self.node_map[x].parents))
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/db/migrations/graph.py", line 370, in ensure_not_cyclic
    raise CircularDependencyError(", ".join("%s.%s" % n for n in cycle))
django.db.migrations.exceptions.CircularDependencyError: staff_person.0001_initial, locations.0001_initial

I very much want to avoid porting data to a clean installation of the application as that would involve likely losing the audit history for the data as doing a complete system playback isn't likely to work (complexity, plus buggy data).

So now trying to disconnect StaffPerson from any specific links to User, and then import a Custom User model. After that we can see about merging the StaffPerson data into the new CustomUser table.

... unlinking went well enough to allow for Person to be declared, and a migration (07) to be generated... but trying to apply that migration results in:

pm migrate
DEBUG: reading env data from /Users/carl/Projects/py3_cope/cope_repo/local.env
DEBUG: Loading settings from development
Operations to perform:
  Apply all migrations: admin, adverse_event, auth, compare, contenttypes, followups, health_economics, locations, perfusion_machine, reversion, samples, sessions, sites, staff_person
Traceback (most recent call last):
  File "manage.py", line 14, in <module>
    execute_from_command_line(sys.argv)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/core/management/__init__.py", line 367, in execute_from_command_line
    utility.execute()
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/core/management/__init__.py", line 359, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/core/management/base.py", line 294, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/core/management/base.py", line 345, in execute
    output = self.handle(*args, **options)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/core/management/commands/migrate.py", line 164, in handle
    pre_migrate_apps = pre_migrate_state.apps
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/utils/functional.py", line 35, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/db/migrations/state.py", line 176, in apps
    return StateApps(self.real_apps, self.models)
  File "/Users/carl/.virtualenvs/py3_cope/lib/python3.5/site-packages/django/db/migrations/state.py", line 249, in __init__
    raise ValueError("\n".join(error.msg for error in errors))
ValueError: The field admin.LogEntry.user was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field adverse_event.AdverseEvent.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field compare.Donor.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field compare.Organ.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field compare.OrganAllocation.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field compare.OrganPerson.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field compare.ProcurementResource.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field compare.Recipient.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field compare.RetrievalTeam.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field followups.FollowUp1Y.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field followups.FollowUp3M.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field followups.FollowUp6M.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field followups.FollowUpInitial.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field health_economics.QualityOfLife.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field health_economics.ResourceHospitalAdmission.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field health_economics.ResourceLog.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field health_economics.ResourceRehabilitation.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field health_economics.ResourceVisit.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field locations.Hospital.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field perfusion_machine.PerfusionFile.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.
The field perfusion_machine.PerfusionMachine.created_by was declared with a lazy reference to 'staff_person.person', but app 'staff_person' doesn't provide model 'person'.

"lazy reference" doesn't sound good

From the Django docs on Custom Users...

Warning

Changing AUTH_USER_MODEL has a big effect on your database structure. It changes the tables that are available, and it will affect the construction of foreign keys and many-to-many relationships. If you intend to set AUTH_USER_MODEL, you should set it before creating any migrations or running manage.py migrate for the first time.

Changing this setting after you have tables created is not supported by makemigrations and will result in you having to manually fix your schema, port your data from the old user table, and possibly manually reapply some migrations.

Warning

Due to limitations of Django’s dynamic dependency feature for swappable models, you must ensure that the model referenced by AUTH_USER_MODEL is created in the first migration of its app (usually called 0001_initial); otherwise, you will have dependency issues.

In addition, you may run into a CircularDependencyError when running your migrations as Django won’t be able to automatically break the dependency loop due to the dynamic dependency. If you see this error, you should break the loop by moving the models depended on by your User model into a second migration (you can try making two normal models that have a ForeignKey to each other and seeing how makemigrations resolves that circular dependency if you want to see how it’s usually done)

Consensus on those trying to do similar is Avoid At All Costs, or accept that you're going to have to manually port data.

Having already tried to explore the avoiding options originally, this path is still the sanest option left, so the question now becomes how much data porting do I have to do, and by what means.

This is looking like a process now of:

Create a new "Staff" app and put new functionality into there
Replace references to staff_person.StaffPerson with Person on a new attribute (this is to avoid issues with historical data colliding with the new structure) in all linked models
Remove the staff_person app
Clear all migrations on all apps so that we have only initial state for the 0.8.0 schema
Create new blank database from 0.8.0 that uses staff.Person for AUTH_USER_MODEL
SQL copy data from original database into new database:
1. Copy old auth.User to new staff.Person as the baseline. Check for user duplicates, remove and note them.
2. Copy table by table the original data, taking time to migrate staff_person.StaffPerson pointers to their equivalent staff.Person records. Where Person is missing, look for data in the original StaffPerson data. Match on names to help reduce the duplicates.
Work through the views, forms, admin and related that will be pointing at the wrong fields
Test existing functionality

Audit history is going to be a problem. Need to check now how django-reversion handles data model changes.

Then we can get back to adding in the new permissions, and writing object managers that use them.

Staff app done in draft form.
Staff_Person app removed (not all attributes have been changed yet)
Removed Samples.Worksheet (need to implement the rest of the fixes for Samples to work, but see issue #124 to capture that)
Migrations all cleared

Next will be to test if migrations can be created for the new data structure, and to investigate if they are as expected.

There have been a few aborted bits of progress in trying to require the system for new permissions, however we now have the following situation:

To ensure consistency, I'm adding automated tests as I go along to check basic functionality
- This means that the system needs to be able to be setup from scratch via automated routes such as migrations and fixtures
- Fixtures can't overwrite existing data put in place by migrations (so permissions, content_types, etc)
- Permissions use ContentTypes for references
- We need to have Groups created, and Permissions added to groups
- The groups are fairly constant (thus can load from fixture), but the permission and content_type ids change each time the system is built (such as on testing, or when creating a clean db for merging).
- This means we need to programmatically generate the Groups, and their Permissions.
- The advisable way to do this is via a custom migration (e.g. [http://stackoverflow.com/questions/25024795/django-1-7-where-to-put-the-code-to-add-groups-programatically]())
I've so far been trying to generate the Group, Permissions, and Content Type records as Fixtures, which I can load manually on setup - however that doesn't work for the automated testing.
The other problem is that DjangoReversion uses ContentType IDs to reference the models it stores as strings. It doesn't account for these content types changing, so the next migration challenge (in order to maintain audit history) will be to map the content type changes - but only once we have a final installation, because it's non-trivial, and this history should only need to change the once.

Thus I'm going to have to undo a large chunk of my work for the last couple of days and re-write as a migration script.

This doesn't address what happens if/when we add/change group permissions dynamically once the system is live. dumpdata is only any good as a json fixture output, but would need manually mapping into the migration files in the future if this gets redeployed, or needs the testing to be rerun.

On the plus side, I've some proof of concept code from the tests I've done so far to suggest I can do this...

Testing has been helpful in identifying sections of code I'd missed out editing for permissions (e.g. PerfusionMachine), or missing permissions (caused by not spotting the content type dependancy during the migration).

I will do a few more things as part of this re-write:

Rename PerfusionMachine and PerfusionFile to Machine and FileRecord
Remove organ.perfusion_file as this field has never been implemented or used
Move the permissions defined in #167 being global, to being specific to each model. This means:
- view_classname will be defined for view only privilege
- restrict_to_zzzzz will be defined only where is makes sense, and a note will be left to clarify what location will be used for that object
Review every app model again to ensure we're inheriting Meta and expected properties
Remove the created_by, created_on, version fields in preference for DjangoReversion handling this functionality.
- This will simplify a lot of the model saving functionality, so these methods will need reviewing on Models, Forms, and Admin definitions
Migrations will be flattened and rebuild once again
- Then I can create custom migrations in the staff app for groups and permissions, with dependancies being set for existing migrations that put these permissions in place

16 hours later... and I'm finally back where I was, only now I can run the test client and pass the first 16 basic tests. Saving progress now

RECAP

Replace StaffPerson.StaffPerson and StaffPerson.StaffJob with Staff.Person and Auth.Groups - DONE
1. Data cleanup of StaffPerson manually performed on live site
2. All Staff Persons linked to an Auth.User account, that then becomes a Staff.Person record
Data migration process established and cursorily tested with data up to 12 hours old (at time of writing).
1. Automated site testing process under construction.
2. Test Runner setup required redoing the fixtures and migrations to enable site setup to be automated.
3. Data migration has dropped the duplicate StaffPerson records, and has cleaned up the locations.Hospital records as well
Other bits of refactoring have taken place as part of this restructure and data porting, including:
- PerfusionMachine app being trimmed down to minimum (perfusion file field removed from donor)
- Duplicated audit information in created_by, created_on, version fields has been removed; save functions have been simplified or removed to reflect this too.
- Permissions have been applied to concrete models for Issue #167 purposes

Automated testing (what little there is) now runs successfully. Manual testing underway, but proceeding well so far. Looks like all aspects of this issue have been addressed now.

ouhft / COPE

Merge StaffPerson app functionality into Django.Auth #208

Warning

Warning

RECAP