Most recent with excluded fields #477

Alig1493 · 2018-11-07T10:28:00Z

Description

Added changes in manager to compensate for excluded fields while feetching most recent. Updated test_model_with_excluded_fields test to verify.

Related Issue

Steps to reproduce:

Create a model with a historical records field
Add an excluded model field in historical records
Query for most_recent of the model's history field
most_recent cannot be combined with excluded_fields #333

Motivation and Context

How Has This Been Tested?

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:

My code follows the code style of this project.
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.

rossmechanic · 2018-11-08T19:55:09Z

simple_history/models.py

@@ -65,6 +65,9 @@ def __init__(self, verbose_name=None, bases=(models.Model,),
        except TypeError:
            raise TypeError("The `bases` option must be a list or a tuple.")

+    def _get_excluded_fields(self):


Is this being used? Can't see it being called anywhere else

It's not being used anywhere. My mistake since I hadn't spot the meta attribute containing the excluded fields in the first place.

kseever · 2018-11-08T19:56:46Z

simple_history/tests/tests/test_models.py

        self.assertIn('question', all_fields_names)
        self.assertNotIn('pub_date', all_fields_names)
+        self.assertEqual(most_recent.__class__, PollWithExcludeFields)



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

@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. =)

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!

rossmechanic · 2018-11-13T23:17:04Z

@Alig1493 not sure why this isn't building. Want to try fixing conflicts and pushing?

Alig1493 · 2018-11-22T14:31:50Z

I'll have the conflicts resolved. SOrry for the delayed response

rossmechanic · 2018-12-05T17:02:35Z

Want to run make format and fix the build? @Alig1493

Alig1493 · 2018-12-09T07:10:23Z

@rossmechanic sure I'll do it right away. sorry for the late reply.

codecov-io · 2018-12-09T09:52:45Z

Codecov Report

Merging #477 into master will increase coverage by 0.5%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #477     +/-   ##
=========================================
+ Coverage   97.23%   97.74%   +0.5%     
=========================================
  Files          16       17      +1     
  Lines         724      799     +75     
  Branches       95      111     +16     
=========================================
+ Hits          704      781     +77     
+ Misses         10        8      -2     
  Partials       10       10

Impacted Files	Coverage Δ
simple_history/manager.py	`94.11% <100%> (+0.46%)`	⬆️
...ory/management/commands/clean_duplicate_history.py	`100% <0%> (ø)`
...le_history/management/commands/populate_history.py	`96.03% <0%> (+2.22%)`	⬆️

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 cbb0ab4...aa63579. Read the comment docs.

Alig1493 · 2018-12-09T10:03:48Z

@rossmechanic any advice regarding solving cyclomatic complexity def most_recent(self)?

rossmechanic · 2018-12-11T22:49:22Z

@Alig1493 refactoring your new code into a get_excluded_fields function should do the trick

Alig1493 · 2018-12-12T08:48:32Z

@rossmechanic Is my latest change what you had in mind? Maybe I missed something?

rossmechanic · 2018-12-13T21:21:03Z

@Alig1493 yes! just rebase and the codeclimate issue should go away. However, I would like to see the code coverage increase

Alig1493 · 2018-12-16T20:18:12Z

I'll have it rebased soon. Sorry was on leave.

…ching most recent. Updated test_model_with_excluded_fields test to verify.

Alig1493 · 2018-12-17T07:34:39Z

@rossmechanic I have rebased and pushed but it seems the initial problem still persists. Anything I might have missed?

rossmechanic · 2018-12-17T16:50:22Z

simple_history/manager.py

@@ -34,6 +34,11 @@ 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):
+        if isinstance(self.instance._meta.model.history, self.__class__):


Don't want to hardcode history here. Instead, use utils.get_history_manager_for_model

What is this conditional checking for?

I was using this conditional check because I thought all the instances might not have a model history class.

They should all have a model history class. However, they might not have _history_excluded_fields so you should do:
excluded_fields = getattr(self.model, '_history_excluded_fields', [])

oh shoot you're right. Now that I'm looking at the print statements for debugging it's painfully obvious. Damn!

rossmechanic · 2018-12-17T16:55:09Z

simple_history/manager.py

@@ -34,6 +34,11 @@ 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):
+        if isinstance(self.instance._meta.model.history, self.__class__):
+            return self.instance._meta.model.history.model._history_excluded_fields


could do self.model._history_excluded_fields

rossmechanic · 2018-12-17T16:56:11Z

@Alig1493 don't worry about the codeclimate error, but I left a few other comments. Also want to get the codecov/patch up to 100%

Alig1493 · 2018-12-19T08:01:30Z

@rossmechanic the codecov patch is upto a 100% just like you mentioned.

rossmechanic · 2018-12-19T15:33:19Z

@Alig1493 Great! Just left one more comment

Alig1493 · 2018-12-20T09:22:21Z

@rossmechanic I've made your requested changes.

rossmechanic · 2018-12-20T15:19:00Z

simple_history/manager.py

-                tmp.append(field.name + "_id")
-            elif field.name not in excluded_fields:
-                tmp.append(field.name)
+            if field.name not in excluded_fields:


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)

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

Yea can you do that? Then I’ll merge

Alig1493 · 2018-12-24T08:24:04Z

@rossmechanic please see if the changes are in order.

rossmechanic

Looks good to me! Thanks!

Alig1493 mentioned this pull request Nov 7, 2018

most_recent cannot be combined with excluded_fields #333

Closed

rossmechanic reviewed Nov 8, 2018

View reviewed changes

kseever reviewed Nov 8, 2018

View reviewed changes

Alig1493 force-pushed the most_recent_with_excluded branch from b71b1fb to 4f78744 Compare November 22, 2018 17:40

Alig1493 added 5 commits December 17, 2018 13:27

Added changes in manager to compensate for excluded fields while feet…

028ba91

…ching most recent. Updated test_model_with_excluded_fields test to verify.

Requirement changes.

829185d

hot fix

7564885

Formatted test_models file

fd1a30b

Refactored exclude fields to another method.

e28710a

Alig1493 force-pushed the most_recent_with_excluded branch from 61c8ab3 to e28710a Compare December 17, 2018 07:28

rossmechanic reviewed Dec 17, 2018

View reviewed changes

Requested changes for excluded fields

b7df921

Getting history excluded fields attribute if it exists

db8714f

rossmechanic reviewed Dec 20, 2018

View reviewed changes

changed nested ifs

aa63579

rossmechanic approved these changes Dec 24, 2018

View reviewed changes

rossmechanic merged commit 65785a0 into jazzband:master Dec 24, 2018

Alig1493 deleted the most_recent_with_excluded branch December 27, 2018 10:58

rossmechanic mentioned this pull request Jan 16, 2019

Remove get_excluded_fields function from manager – shouldn't be part … #515

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Most recent with excluded fields #477

Most recent with excluded fields #477

Alig1493 commented Nov 7, 2018

rossmechanic Nov 8, 2018

Alig1493 Nov 11, 2018

kseever Nov 8, 2018

Alig1493 Nov 11, 2018

kseever Nov 12, 2018

rossmechanic commented Nov 13, 2018

Alig1493 commented Nov 22, 2018

rossmechanic commented Dec 5, 2018

Alig1493 commented Dec 9, 2018

codecov-io commented Dec 9, 2018 •

edited

Alig1493 commented Dec 9, 2018

rossmechanic commented Dec 11, 2018

Alig1493 commented Dec 12, 2018

rossmechanic commented Dec 13, 2018

Alig1493 commented Dec 16, 2018

Alig1493 commented Dec 17, 2018

rossmechanic Dec 17, 2018

rossmechanic Dec 17, 2018

Alig1493 Dec 19, 2018

rossmechanic Dec 19, 2018

Alig1493 Dec 20, 2018

rossmechanic Dec 17, 2018

rossmechanic commented Dec 17, 2018

Alig1493 commented Dec 19, 2018

rossmechanic commented Dec 19, 2018

Alig1493 commented Dec 20, 2018

rossmechanic Dec 20, 2018

Alig1493 Dec 23, 2018

rossmechanic Dec 23, 2018

Alig1493 commented Dec 24, 2018

rossmechanic left a comment

Most recent with excluded fields #477

Most recent with excluded fields #477

Conversation

Alig1493 commented Nov 7, 2018

Description

Related Issue

Steps to reproduce:

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossmechanic commented Nov 13, 2018

Alig1493 commented Nov 22, 2018

rossmechanic commented Dec 5, 2018

Alig1493 commented Dec 9, 2018

codecov-io commented Dec 9, 2018 • edited

Codecov Report

Alig1493 commented Dec 9, 2018

rossmechanic commented Dec 11, 2018

Alig1493 commented Dec 12, 2018

rossmechanic commented Dec 13, 2018

Alig1493 commented Dec 16, 2018

Alig1493 commented Dec 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossmechanic commented Dec 17, 2018

Alig1493 commented Dec 19, 2018

rossmechanic commented Dec 19, 2018

Alig1493 commented Dec 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alig1493 commented Dec 24, 2018

rossmechanic left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 9, 2018 •

edited