Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set app label via instantiation of historical records #486

Merged
merged 6 commits into from
Nov 28, 2018
Merged

Set app label via instantiation of historical records #486

merged 6 commits into from
Nov 28, 2018

Conversation

uhurusurfa
Copy link
Contributor

@uhurusurfa uhurusurfa commented Nov 27, 2018

Description

Provide the ability to override the app_label Meta attribute on the generated history model.
This functionality alreay exists using the register method. The mechanism for this feature is identical
to the way it is implemented in the register method by allowing passing an app name in the instantiation.
The code change adds an additional instantiation parameter to the init method for the HistoricalRecords class
and then uses this parameter if provided in the get_meta_data method.

Related Issue

Fixes #485

Motivation and Context

It is often desirable to have the app_label different to the base model so that the history models can be directed to a different database.
This enhances security of the audit trail and can be used in situations where there is a need ot reduce the load on the main application database.
This also provides the same functionality between the "register()" and "HistoricalRecords()" mechanism for initiating audit trails on base models.

How Has This Been Tested?

A unit test was included to test this change. It only affects the setting of a Meta class attribute and does not functionally change the app code.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run the make format command to format my code
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
    NOTE: There is 1 test that failed but failed even without the code change

@codecov-io
Copy link

codecov-io commented Nov 27, 2018

Codecov Report

Merging #486 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #486      +/-   ##
==========================================
+ Coverage   97.19%   97.22%   +0.02%     
==========================================
  Files          16       16              
  Lines         713      720       +7     
  Branches       94       95       +1     
==========================================
+ Hits          693      700       +7     
  Misses         10       10              
  Partials       10       10
Impacted Files Coverage Δ
simple_history/models.py 99.21% <100%> (ø) ⬆️
simple_history/registry_tests/tests.py 95.87% <100%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53421d9...488e673. Read the comment docs.

rossmechanic
rossmechanic previously approved these changes Nov 28, 2018
Copy link
Collaborator

@rossmechanic rossmechanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Want to add this change to CHANGES.rst and add your name to AUTHORS.rst and then I'll merge? @uhurusurfa

@rossmechanic
Copy link
Collaborator

Also @uhurusurfa what test failed?

@uhurusurfa
Copy link
Contributor Author

Also @uhurusurfa what test failed?

Here is the fail trace extract minus the middle bits that do not help much:

`ERROR: test_transaction_rolls_back_if_bulk_history_create_fails (simple_history.tests.tests.test_utils.BulkCreateWithHistoryTransactionTestCase)

Traceback (most recent call last):
File "/Users/chris/git/django-simple-history/.tox/py36-djangotrunk/lib/python3.6/site-packages/django/db/backends/utils.py", line 84, in _execute
return self.cursor.execute(sql, params)
File "/Users/chris/git/django-simple-history/.tox/py36-djangotrunk/lib/python3.6/site-packages/django/db/backends/sqlite3/base.py", line 328, in execute
return Database.Cursor.execute(self, query, params)
sqlite3.IntegrityError: UNIQUE constraint failed: django_content_type.app_label, django_content_type.model

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
File "/Users/chris/git/django-simple-history/.tox/py36-djangotrunk/lib/python3.6/site-packages/django/test/testcases.py", line 211, in call
self._post_teardown()
File "/Users/chris/git/django-simple-history/.tox/py36-djangotrunk/lib/python3.6/site-packages/django/test/testcases.py", line 926, in _post_teardown
self._fixture_teardown()
File "/Users/chris/git/django-simple-history/.tox/py36-djangotrunk/lib/python3.6/site-packages/django/test/testcases.py", line 967, in _fixture_teardown
connections[db_name]._test_serialized_contents
File "/Users/chris/git/django-simple-history/.tox/py36-djangotrunk/lib/python3.6/site-packages/django/db/backends/base/creation.py", line 133, in deserialize_db_from_string
obj.save()
File "/Users/chris/git/django-simple-history/.tox/py36-djangotrunk/lib/python3.6/site-packages/django/core/serializers/base.py", line 223, in save
....
....
....
File "/Users/chris/git/django-simple-history/.tox/py36-djangotrunk/lib/python3.6/site-packages/django/db/backends/sqlite3/base.py", line 328, in execute
return Database.Cursor.execute(self, query, params)
django.db.utils.IntegrityError: UNIQUE constraint failed: django_content_type.app_label, django_content_type.model
`

@uhurusurfa
Copy link
Contributor Author

@rossmechanic - have etched my name in the anals of simple history and updted the changes file

Copy link
Collaborator

@rossmechanic rossmechanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird that you saw that error locally. Since CI passed, I'm happy to merge. Thanks for this change!

@rossmechanic rossmechanic merged commit 9e95fd8 into jazzband:master Nov 28, 2018
@uhurusurfa uhurusurfa deleted the set_app_label_via_instantiation_of_HistoricalRecords branch November 29, 2018 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting app_label different to model class
3 participants