Skip to content
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.

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

Posts and PrivatePosts should be ordered by created_at not by id #360

Closed
5 tasks done
timdiggins opened this issue Aug 7, 2016 · 3 comments · Fixed by #369
Closed
5 tasks done

Posts and PrivatePosts should be ordered by created_at not by id #360

timdiggins opened this issue Aug 7, 2016 · 3 comments · Fixed by #369
Assignees
Labels
Milestone

Comments

@timdiggins
Copy link
Collaborator

timdiggins commented Aug 7, 2016

Currently posts & private posts when order_oldest_first / order_newest_first are actually (under the hood) ordered by id.

This seems appealing in some ways, but it is a bit of a nightmare. In particular if the creation order is screwed up (e.g. by importing) then you have problems. It also just feels wrong to me -- and it was deeply surprising when I found out.

I have started changing this behaviour to be ordered by created_at

Of course changing this is quite a bit of work, as much test data currently relies on this, and some untested behaviours (e.g. Post / PrivatePost #page())

However, I've also discovered that there are one or two places where some desired behaviours rely on a natural id-ordering (rather than explicit call to order_oldest_first) which is not dependable (at least in postgres).

  • change ordering
  • deal with spec failures (including the read-state spec mentioned in Sporadic spec failure in travis ci #352)
  • add in default order_oldest_first for the associations in Topic and PrivateTopic
  • write spec for and change #page for both Post and PrivatePost
  • dry up #page by extracting to either PostCommon or PageCalculable
@timdiggins timdiggins self-assigned this Aug 7, 2016
@timdiggins timdiggins changed the title [Private]Posts should be ordered by created_at not by id Posts and PrivatePosts should be ordered by created_at not by id Aug 7, 2016
@timdiggins
Copy link
Collaborator Author

FYI: Having looked through the usages of .posts, none are actually reordering the posts, and many are relying on the id-order. I think we should change the default order of the associations (not the model) to be order_oldest_first. this can be overridden with .reorder(). This may be a controversial choice!

@glebm
Copy link
Collaborator

glebm commented Aug 7, 2016

👍

Default order on associations is probably OK, though I'd prefer no default order on them at all.

@timdiggins
Copy link
Collaborator Author

@glebm I get your reluctance for lack of default order, but it seemed a bit repititive to add the order to every single place we are using .posts...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants