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

GH-512: allow foreign key to reference self using self str #513

Merged
merged 5 commits into from
Jan 14, 2019

Conversation

rossmechanic
Copy link
Collaborator

…nKey to self

Description

This PR adds tests for models with foreign keys to themselves. It also adds functionality (which was accidentally removed in #462) that allows developers to use a 'self' string when creating a self-referential foreign key.

Related Issue

Fixes #512

Motivation and Context

Users that use the string 'self' when using self-referential ForeignKeys will likely see migrations when updating from django-simple-history 2.5.1 to 2.6.0. Those migrations will point the historical foreign keys to the historical models rather than the base models. This can be fixed locally by using the model's name rather than 'self', as shown below:

class SelfReferentialModel(models.Model):
    fk_to_self = models.ForeignKey('SelfReferentialModel')  # this still works in 2.6.0
    fk_to_self = models.ForeignKey('self') # this is broken is 2.6.0 but will be fixed with this PR 

How Has This Been Tested?

Added 2 tests:

  1. Checks that a self-referential foreign key using a string of the model's name results in the correct foreign key on the historical model (this worked before the change)
  2. Checks that a self-referential foreign key using a string of 'self' results in the correct foreign key on the historical model (this is fixed by this PR)

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.
  • I have added my name and/or github handle to AUTHORS.rst
  • I have added my change to CHANGES.rst
  • All new and existing tests passed.

@rossmechanic rossmechanic added bug Issues related to confirmed bugs accepted Issue accepted for completion labels Jan 14, 2019
@codecov-io
Copy link

codecov-io commented Jan 14, 2019

Codecov Report

Merging #513 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
+ Coverage   97.63%   97.63%   +<.01%     
==========================================
  Files          17       17              
  Lines         802      804       +2     
  Branches      109      110       +1     
==========================================
+ Hits          783      785       +2     
  Misses          8        8              
  Partials       11       11
Impacted Files Coverage Δ
simple_history/models.py 98.85% <100%> (ø) ⬆️

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 45d6f8e...db5b140. Read the comment docs.

@rossmechanic rossmechanic merged commit 2e89360 into master Jan 14, 2019
@rossmechanic rossmechanic deleted the GH-512-allow-foreign-key-to-self-using-self-str branch January 14, 2019 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Issue accepted for completion bug Issues related to confirmed bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

simple-history 2.6.0 generates a migration while 2.5.1 doesn't
3 participants