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

Add options for default sorting #1644

Merged
merged 10 commits into from
May 27, 2020
Merged

Add options for default sorting #1644

merged 10 commits into from
May 27, 2020

Conversation

jbennett
Copy link
Contributor

Provide methods to override the default ordering attribute and direction.

Reference: #442

@sedubois sedubois mentioned this pull request May 12, 2020
Closed
nickcharlton
nickcharlton reviewed May 12, 2020
View reviewed changes
spec/controllers/admin/customers_controller_spec.rb Outdated
end

it "supports default sorting overrides" do
allow_any_instance_of(Admin::CustomersController).to(receive_messages(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way we can do this without using allow_any_instance_of? Perhaps by creating another controller subclass inside the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced it with an anonymous class. Does this help clarify the test or are you thinking something else?

pablobm
pablobm reviewed May 14, 2020
View reviewed changes
Copy link
Collaborator

@pablobm pablobm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Just one nitpick about private vs public.

app/controllers/administrate/application_controller.rb Outdated Show resolved Hide resolved
@pablobm
Copy link
Collaborator

pablobm commented May 14, 2020

Uh, I didn't realise that the tests were failing. Any idea of what's going on?

@jbennett
Copy link
Contributor Author

Figured out what was going on.

Replacing the @controller with an anonymous class to stub out the sorting methods with alternative values leaked into other tests since the controller is recycled, not re-instantiated between tests. Moving my test into a context and using the controller() rspec method isolated my test from the rest and stopped everything from blowing up.

@jbennett
Copy link
Contributor Author

CI failures are unrelated to this change. It's regarding CVE security issue with gems.

@pablobm
Copy link
Collaborator

pablobm commented May 22, 2020

@jbennett - Do you mind rebasing on top of master? It should fix the CVE warning.

@jbennett
Copy link
Contributor Author

Rebased and passing 👍🏻

@nickcharlton nickcharlton merged commit 65d3486 into thoughtbot:master May 27, 2020
@nickcharlton
Copy link
Member

Thanks for your work here!

@jbennett jbennett deleted the sorting branch May 29, 2020 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablobm pablobm pablobm approved these changes

@betelgeuse betelgeuse betelgeuse left review comments

@nickcharlton nickcharlton nickcharlton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jbennett @pablobm @nickcharlton @betelgeuse