Skip to content

Custom sort value field #9

Closed
wants to merge 2 commits into from

2 participants

@treyhunner

I prefer using a different field name for the sort field. I actually migrated from a different sortable app and didn't want to change the field name.

This adds a SORTEDM2M_FIELD_NAME setting which allows the field name to be overridden. I also added an escape for the field name when querying so reserved names like "order" can be used.

I intended to add tests for this feature before creating a pull request but I never got around to it. I'm creating this pull request to at least start the discussion about this feature.

@gregmuellegger
Owner

Ok, I think it makes sense - at least in your case - to change the field name.
But I would prefer to not add this as a global configuration, but to use an attribute you can pass into the Field constructor.

I hope to implement and test this later this week/next week. Feel free to nudge me if it's not done by then.
Another pull request is welcome as well ofcourse!

@treyhunner

The field name isn't actually changed so much as it is escaped to avoid an accidental SQL injection. The escaping surrounds the field name by backticks (`) which doesn't actually affect the field name, but allows special words/characters in it. It is the SQL equivalent of surrounding a command line parameter in quotes. So if "sort_value" was specified in the setting it would work as expected.

I do think the method of escaping I used may be a little primitive though. Maybe the function(s) Django uses to escape field names should be used instead of surrounding the name by backticks.

@gregmuellegger gregmuellegger added a commit that closed this pull request Jan 29, 2013
@gregmuellegger Fixes #9 -- Allow the user to customize the name of the sort-by field…
… of the intermediate m2m relation table.
2aaaf03
@gregmuellegger
Owner

Thanks for the feature request! This is implemented in the new 0.5.0 release. Have a look at the readme file for basic documentation.

@treyhunner

Awesome. Thanks for the merge. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.