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

Allow item set sorting #1210

Closed
wants to merge 2 commits into from
Closed

Conversation

pols12
Copy link
Contributor

@pols12 pols12 commented Feb 7, 2018

In ItemSetAdapter, moves ORDER BY site_item_set.position from buildQuery() to end of sortQuery(), in order to prioritize user sorting choice.

Note that $site seems to be unused.

Previously in buildQuery.
Now allows to choose how sorting item sets.
@zerocrates
Copy link
Member

The issue with doing that is that the innerJoin affects the actual returned records from the query. We probably want to keep the sort methods just handling sorting. But, moving the default position sort there is probably the way to go in terms of fixing this problem.

As for site not being used... that is interesting for sure... It actually seems like there's a(nother) fairly significant bug there, if I'm reading correctly.

@zerocrates
Copy link
Member

Well, I guess it's not a bug per se... there's just an oddity where the passed-in ID is directly used in the query to limit to the correct site, where it could be read from the actually-retrieved site record... this section should either be actually checking against $site and aborting if its not found, or else dropping the check and just using the ID directly with no read at all. We probably want to go the former route...

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

Successfully merging this pull request may close these issues.

None yet

2 participants