Skip to content

Most recent with excluded fields #477

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Dec 24, 2018
Merged

Most recent with excluded fields #477

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
028ba91
Added changes in manager to compensate for excluded fields while feet…
Alig1493 Nov 7, 2018
829185d
Requirement changes.
Alig1493 Nov 11, 2018
7564885
hot fix
Alig1493 Nov 11, 2018
fd1a30b
Formatted test_models file
Alig1493 Dec 9, 2018
e28710a
Refactored exclude fields to another method.
Alig1493 Dec 12, 2018
b7df921
Requested changes for excluded fields
Alig1493 Dec 19, 2018
db8714f
Getting history excluded fields attribute if it exists
Alig1493 Dec 20, 2018
aa63579
changed nested ifs
Alig1493 Dec 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions simple_history/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ def get_queryset(self):
key_name = self.instance._meta.pk.name
return self.get_super_queryset().filter(**{key_name: self.instance.pk})

def get_excluded_fields(self):
return getattr(self.model, "_history_excluded_fields", [])

def most_recent(self):
"""
Returns the most recent copy of the instance available in the history.
Expand All @@ -45,11 +48,14 @@ def most_recent(self):
)
)
tmp = []
excluded_fields = self.get_excluded_fields()

for field in self.instance._meta.fields:
if isinstance(field, models.ForeignKey):
tmp.append(field.name + "_id")
else:
tmp.append(field.name)
if field.name not in excluded_fields:
Copy link

@rossmechanic rossmechanic Dec 20, 2018

Choose a reason for hiding this comment

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

I might just change this so you don't nest the if statement.

Try:

if field.name in excluded_fields:
    continue
elif isinstance(field, models.ForeignKey):
    tmp.append(field.name + "_id")
else:
    tmp.append(field.name)

Copy link
Contributor Author

@Alig1493 Alig1493 Dec 23, 2018

Choose a reason for hiding this comment

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

Have you made the changes? If not maybe I can just do them and push it for you and it stays there?

Copy link

@rossmechanic rossmechanic Dec 23, 2018

Choose a reason for hiding this comment

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

Yea can you do that? Then I’ll merge

if isinstance(field, models.ForeignKey):
tmp.append(field.name + "_id")
else:
tmp.append(field.name)
fields = tuple(tmp)
try:
values = self.get_queryset().values_list(*fields)[0]
Expand Down
6 changes: 6 additions & 0 deletions simple_history/tests/tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,12 @@ def test_model_with_excluded_fields(self):
self.assertIn("question", all_fields_names)
self.assertNotIn("pub_date", all_fields_names)

most_recent = p.history.most_recent()
self.assertIn("question", all_fields_names)
self.assertNotIn("pub_date", all_fields_names)
self.assertEqual(most_recent.__class__, PollWithExcludeFields)
self.assertIn("pub_date", history._history_excluded_fields)

Copy link
Contributor

@kseever kseever Nov 8, 2018

Choose a reason for hiding this comment

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

Could we also get a test case that ensures that the excluded fields are indeed excluded?

Copy link
Contributor Author

@Alig1493 Alig1493 Nov 11, 2018

Choose a reason for hiding this comment

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

@kseever the test here is already checking whether the field is present in the fields of the history model so I think it's already being covered. I have added another statement checking if the field is present in the excluded fields of the history model. It might be that I may have failed to understand your requirement so please feel free to elaborate. =)

Copy link
Contributor

@kseever kseever Nov 12, 2018

Choose a reason for hiding this comment

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

Ah, I just noticed that the assertion I was looking for already exists on line 403 - confirming that pub_date isn't in the list of returned fields. Nevermind me!

def test_user_model_override(self):
user1 = User.objects.create_user("user1", "[email protected]")
user2 = User.objects.create_user("user2", "[email protected]")
Expand Down