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

Remove deprecated settings and code #2165

Merged
merged 3 commits into from
Jun 22, 2013
Merged

Remove deprecated settings and code #2165

merged 3 commits into from
Jun 22, 2013

Conversation

seanlinsley
Copy link
Contributor

Things removed:

  • Dashboard sections
  • ActiveAdmin.default_namespace => ActiveAdmin.application.default_namespace
  • config.csv_column_separator = ';' => config.csv_options[:col_sep] = ';'
  • default_sort_order => ??

end
name = namespace.root? ? nil : namespace.name
scope name, :as => name, :module => name do
root :to => namespace.root_to
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 used scope instead of namespace because of rails/rails#10456

@seanlinsley
Copy link
Contributor Author

Okay, this is ready for 👀

@seanlinsley
Copy link
Contributor Author

Nevermind. All the deprecation warnings you get when running the test suite really bug me, so I removed them!

This isn't working quite yet.

end
name = namespace.root? ? nil : namespace.name
namespace name, :module => name do
root :to => namespace.root_to
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, perhaps I should use the old format after all...

if namespace.root?
  root :to => namespace.root_to
else
  namespace namespace.name do
    root :to => namespace.root_to
  end
end

@seanlinsley
Copy link
Contributor Author

Note to self: remove deprecated methods in the form builder.

end
it "should route the admin dashboard" do
get('/admin').should route_to 'admin/dashboard#index'
get('/').should route_to 'admin/dashboard#index'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does / route to the admin dashboard in this spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. I just threw that in there because that's what the test environment is giving me.

@macfanatic
Copy link
Contributor

@daxter - Other than the tests failing on Rails 3.0 & my one comment, this looks great.

@seanlinsley
Copy link
Contributor Author

I'm gonna take another stab at those tests... it feels weird just removing them and not putting something in their place.

BTW the Rails 3.0 test errored out before it had a chance to fail. Travis recently introduced a "rerun" feature, so I'm rerunning that instance right now.

@seanlinsley seanlinsley mentioned this pull request Jun 21, 2013
@seanlinsley
Copy link
Contributor Author

Okay... I spent way too long trying to get a set of tests pass that weren't that important anyway. It didn't pan out, though, because of the way we inject our routes into the host app. Specifically, since the earliest defined routes take precedence, and we just keep on adding extra routes when we call reload_routes!, there's no solution when RSpec runs the test suite in an unpredicatble order. The only real option would be to set up AA to use the Rails mountable engine approach, but that's a discussion for another time.

Same behaviour, save that all actions now support multiple HTTP verbs.

Removes outdated test from #780
seanlinsley added a commit that referenced this pull request Jun 22, 2013
…logic

Remove deprecated settings and code
@seanlinsley seanlinsley merged commit 58043b6 into activeadmin:master Jun 22, 2013
@seanlinsley seanlinsley deleted the cleanup/remove_custom_dashboard_logic branch June 22, 2013 07:34
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.

2 participants