Allow filter model to be a subclass of the queryset one.

[charettes]

The original issubclass check seems to have been misused.

Also moved the FilterableItem model to the common module since the one defined in test_filters was actually a reference to the one defined in test_pagination because of the way BaseModel.__new__ works.

[tomchristie]

Does this need to move into models? We should try to keep models close to their corresponding test cases where possible. (Even if it means redefining in some places)

[charettes]

Since both models are defined in the tests package they both have the same app_label ('tests').

Because the test_pagination module was loaded before the test_pagination one and Django cache model definition by (app_label, model_name) to work around double imports test_filters.FilterableItem was actually test_pagination.FilterableItem. You can verify this by printing test_filters.FilterableItem.__module__.

I hit this issue while attempting to define a base of FilterableItem to add the test cases for this bugfix by attempting to define BaseFilterableItem in test_filters and realizing FilterableItem was not a subclass of it due to the Django model definition cache behavior described above.

Since both the test_filters and the test_pagination model had the same definition I thought moving them to the models module would be a great way to solve this issue.

I don't mind keeping models separate if you prefer. We'll just have to rename test_pagination.FilterableItem to another name.

[xordoquy]

@tomchristie what's the reason you try to keep models inside the tests ?

[tomchristie]

what's the reason you try to keep models inside the tests ?

• It's not clear when they can be deleted otherwise.
• Models end up getting reused between differing tests, pull requests then add in extra fields, options etc and otherwise making the tests less focused.
• The tests should be as self-contained as possible.

[charettes]

@tomchristie in this case do you want me to rename one of two conflicting models?

[xordoquy]

@charettes I'll most probably update this PR to go against 2.4.x branch which moved the test location. I'll deal with the model change by then.

[tomchristie]

What's the status on this? Just waiting for updating against latest master?

[xordoquy]

@tomchristie due to 1.7 support, the FilterableItem has been moved into the models application. This is because does not validate two models with the same name / application.
IMO this is sensible but if you'd rather have models in their test files we can move them back and rename them to avoid name collision. I opened #1777 to discuss further on the matter.

[xordoquy]

Closing this PR in favor of #1778 (same PR against 2.4 branch)

[xordoquy]

Thanks for the PR and sorry it took so long