Opened 10 years ago

Closed 10 years ago

#24479 closed New feature (fixed)

Add a system check to prevent defining both ordering and order_with_respect_to

Reported by: Daniel Hahler Owned by: Tim Graham <timograham@…>
Component: Core (System checks) Version: dev
Severity: Normal Keywords:
Cc: jon.dufresne@…, me@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

(In django/db/models/options.py)

  1. When order_with_respect_to is being used, and value in ordering gets silently overwritten:

self.ordering = ('_order',)

This should be documented.

  1. Additionally, I am wondering if there should be a warning / hint when this happens, i.e. when self.ordering is non-empty. It might be possible to keep any existing values, and prepend/append the _order value?!
  1. With that code (models.Options._prepare), ordering gets set to ('_order',) always (see point 1), but the proxy "field" gets only added if there's no OrderWrt field yet. This could lead to having an invalid setting for ordering, in case there's a OrderWrt field that's not named _order?!

I think point 1 should be addressed anyway (apart from what gets done considering 2. and 3.).

Change History (9)

comment:1 by Tim Graham, 10 years ago

Component: Uncategorized → Core (System checks)
Summary: models: please document that order_with_respect_to overwrites ordering / add a warning → Add a system check to prevent defining both ordering and order_with_respect_to
Triage Stage: Unreviewed → Accepted
Type: Bug → New feature

comment:2 by Andriy Sokolovskiy, 10 years ago

Owner: changed from nobody to Andriy Sokolovskiy
Status: new → assigned

comment:3 by Jon Dufresne, 10 years ago

Cc: jon.dufresne@… added
Has patch: set

​https://github.com/django/django/pull/4343

comment:4 by Andriy Sokolovskiy, 10 years ago

Owner: Andriy Sokolovskiy removed
Status: assigned → new

@jdufresne, if you decided to work on ticket, you must mark it is as assigned to yourself, so people will see that someone is working on it now. I spent some time on it today, and, likely, in vain.

comment:5 by Jon Dufresne, 10 years ago

Sorry about that. I meant no disrespect. I will do so next time.

comment:6 by Andriy Sokolovskiy, 10 years ago

Cc: me@… added

comment:7 by Tim Graham, 10 years ago

Patch needs improvement: set

comment:8 by Jon Dufresne, 10 years ago

Patch needs improvement: unset

Updated patch based on feedback from reviews. Added additional tests and docs. All tests are passing.

comment:9 by Tim Graham <timograham@…>, 10 years ago

Owner: set to Tim Graham <timograham@…>
Resolution: → fixed
Status: new → closed

In 966a29c:

Fixed #24479 -- Added system check to prevent both ordering and order_wrt.

Note: See TracTickets for help on using tickets.
Back to Top