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.

Sign up for GitHub

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

Jump to bottom

Revert "fixes #6573 - foretello should error when sorting by non-sortabl... #4540

Merged
merged 1 commit into from Aug 7, 2014
Merged

Revert "fixes #6573 - foretello should error when sorting by non-sortabl... #4540

merged 1 commit into from Aug 7, 2014

Conversation

komidore64
Copy link
Contributor

...e field, BZ 1110431"

This reverts commit 3ec51c0.

will re-introduce this fix at a later date when we have some testing
around sorting

@komidore64
Revert "fixes #6573 - foretello should error when sorting by non-sort…
c92d331 
…able field, BZ 1110431"

This reverts commit 3ec51c0.

will re-introduce this fix at a later date when we have some testing
around sorting
@ehelms
Copy link
Member

ehelms commented Aug 6, 2014

Whats the issue?
On Aug 6, 2014 4:55 PM, "Adam Price" notifications@github.com wrote:

...e field, BZ 1110431"

This reverts commit 3ec51c0
3ec51c0
.

will re-introduce this fix at a later date when we have some testing

around sorting

You can merge this Pull Request by running

git pull https://github.com/komidore64/katello revert-rmi6573

Or view, comment on, or merge it at:

#4540
Commit Summary

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#4540.

@komidore64
Copy link
Contributor Author

@ehelms Walden was running into multiple instances of Cannot sort by 'blah' when it should have been allowed.

This is related to certain ElasticSearch mixins that utilize ActiveSupport::Concerns. You were correct in that Concerns extends ClassMethods for me, but it does so in an undesired (or undetermined) order preventing the sortable_fields method from being overridden like what I was after. This can be fixed by renaming all the modules I defined to something other than ClassMethods and then explicitly extending those. This would give me the proper load order.

while investigating his issue, i found that it's also trying to append _sort to the sort field before handing it to elasticsearch when appending was not the desired outcome.

@waldenraines waldenraines mentioned this pull request Aug 6, 2014
Merged
@ehelms
Copy link
Member

ehelms commented Aug 6, 2014

@komidore64 what's an example of a page or API query that failed?

@komidore64
Copy link
Contributor Author

@ehelms anything with ActiveSupport::Concerns that should have sortable fields other than name will fail on those fields with Cannot sort by '<field-name>'.

Puppet-Modules index was the first one that we ran into.

@waldenraines
Copy link
Contributor

@ehelms I encountered this with puppet_modules.rb. You would have to add a field to https://github.com/Katello/katello/blob/master/app/models/katello/glue/elastic_search/puppet_module.rb#L19 and then try to sort on it using ?order=<field>+DESC.

@bbuckingham
Copy link
Member

@komidore64, i ran in to the same issue with errata_id; however, submitted a pr to fix it: #4538

@waldenraines waldenraines mentioned this pull request Aug 7, 2014
Merged
@mccun934
Copy link
Member

mccun934 commented Aug 7, 2014

ACK - sounds like we want to re-do how we solve this issue and this code above is causing more issues than it is worth. talked to @komidore64 on G+ to get more of an overview

komidore64 added a commit that referenced this pull request Aug 7, 2014
@komidore64
Merge pull request #4540 from komidore64/revert-rmi6573
ad46bd2 
Revert "fixes #6573 - foretello should error when sorting by non-sortabl...
@komidore64 komidore64 merged commit ad46bd2 into Katello:master Aug 7, 2014
@komidore64 komidore64 deleted the revert-rmi6573 branch August 7, 2014 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs testing Not yet reviewed
Projects
None yet
Milestone
No milestone
6 participants
@komidore64 @ehelms @waldenraines @bbuckingham @mccun934 @theforeman-bot