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

Fix: Error when setting table name with inherit #1196

Conversation

muneeb706
Copy link
Member

@muneeb706 muneeb706 commented Jun 24, 2023

Error was thrown when table_name is set with inherit=True in base model.
inherited check added in the condition where table_name is checked for updating db table name.

Description

inherited flag argument in create_history_model method is used in the condition where table_name is checked. Condition at line 301 of simple_history.models now look like this:
if not inherited and self.table_name is not None:

Related Issue

Closes #1195.

Motivation and Context

I found this issue while looking for the solution of #1174.
In documentation it is not mentioned that table_name can't be used with inherit. It is implied that this should work, this inconsistency was the motivation.

How Has This Been Tested?

Ran tests locally using runtests.py with python (3.8,3.11) and django 4.1

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 pre-commit run command to format and lint.
  • 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.

Error was thrown when table_name is set with inherit=True in base model.

- add not inherited check

jazzband#1195
- authors and changes docs updated
@muneeb706 muneeb706 marked this pull request as draft June 24, 2023 07:36
@muneeb706 muneeb706 marked this pull request as ready for review June 24, 2023 23:48
@muneeb706 muneeb706 changed the title 1195 error when setting table name with inherit Fix: Error when setting table name with inherit Jul 4, 2023
@muneeb706 muneeb706 marked this pull request as draft July 22, 2023 21:46
@codecov
Copy link

codecov bot commented Jul 22, 2023

Codecov Report

Merging #1196 (c9b2a11) into master (f2e028c) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1196   +/-   ##
=======================================
  Coverage   97.08%   97.08%           
=======================================
  Files          23       23           
  Lines        1271     1271           
  Branches      208      208           
=======================================
  Hits         1234     1234           
  Misses         19       19           
  Partials       18       18           
Files Changed Coverage Δ
simple_history/models.py 96.71% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@muneeb706 muneeb706 marked this pull request as ready for review July 22, 2023 22:09
Copy link
Member

@theryanwalker theryanwalker 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! It would be great to have a test for this fix.

@muneeb706
Copy link
Member Author

Looks good to me! It would be great to have a test for this fix.

Thank you for reviewing, I have added a test.

theryanwalker
theryanwalker previously approved these changes Aug 15, 2023
Copy link
Member

@theryanwalker theryanwalker left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the test @muneeb706!

Now also compares the table name of the inherited (base) model, so that
it's easier to see that the history table name is not inherited.
Copy link
Member

@ddabble ddabble 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! Thank you, @muneeb706, and also @theryanwalker for reviewing :)

@ddabble ddabble merged commit a75ec47 into jazzband:master Aug 15, 2023
19 checks passed
@ddabble ddabble mentioned this pull request Aug 15, 2023
11 tasks
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.

Error when setting table_name with inherit=True
3 participants