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

Convert namespace to sym to prevent duplicate namespaces #5931

Merged
merged 1 commit into from Nov 20, 2019

Conversation

@westonganger
Copy link
Contributor

westonganger commented Nov 8, 2019

If namespace :foo is passed in one place and "foo" is passed in another it will try to register the namespace twice. This causes an error for duplicate root routes.

With this PR we convert the passed namespace to sym to prevent duplicate namespaces

@westonganger westonganger changed the title Convert namespace to string to prevent duplicate namespaces Convert namespace to sym to prevent duplicate namespaces Nov 8, 2019
@westonganger westonganger force-pushed the westonganger:patch-1 branch from 9c98944 to 153589b Nov 8, 2019
@deivid-rodriguez

This comment has been minimized.

Copy link
Member

deivid-rodriguez commented Nov 20, 2019

Thanks! This makes sense to me. I wrote a small regression unit test for you:

diff --git a/spec/unit/application_spec.rb b/spec/unit/application_spec.rb
index 7a26814de..8433f4f55 100644
--- a/spec/unit/application_spec.rb
+++ b/spec/unit/application_spec.rb
@@ -162,6 +162,15 @@ RSpec.describe ActiveAdmin::Application do
       }.to raise_error("found")
     end
 
+    it "should admit both strings and symbols" do
+      expect {
+        application.namespace "admin" do |ns|
+          expect(ns).to eq application.namespaces[:admin]
+          raise "found"
+        end
+      }.to raise_error("found")
+    end
+
     it "should not pollute the global app" do
       expect(application.namespaces).to be_empty
       application.namespace(:brand_new_ns)

Can you add it?

Also, can you add a change log entry following the existing format explaining the fix?

@westonganger westonganger force-pushed the westonganger:patch-1 branch from f7aff83 to 9880105 Nov 20, 2019
@westonganger

This comment has been minimized.

Copy link
Contributor Author

westonganger commented Nov 20, 2019

I have made the requested changes.

Thanks for writing the spec for me, appreciate that.

@westonganger westonganger force-pushed the westonganger:patch-1 branch from 1a4eba0 to f0476ea Nov 20, 2019
@westonganger westonganger force-pushed the westonganger:patch-1 branch from e4913aa to 663990d Nov 20, 2019
Copy link
Member

deivid-rodriguez left a comment

No problem, this looks good to me! 👍

Copy link
Member

javierjulio left a comment

Thank you!

@javierjulio javierjulio merged commit e0ac14d into activeadmin:master Nov 20, 2019
17 checks passed
17 checks passed
ci/circleci: jruby92rails52 Your tests passed on CircleCI!
Details
ci/circleci: jruby92rails60 Your tests passed on CircleCI!
Details
ci/circleci: lint_and_docs Your tests passed on CircleCI!
Details
ci/circleci: ruby24rails52 Your tests passed on CircleCI!
Details
ci/circleci: ruby25rails52 Your tests passed on CircleCI!
Details
ci/circleci: ruby25rails60 Your tests passed on CircleCI!
Details
ci/circleci: ruby26rails52 Your tests passed on CircleCI!
Details
ci/circleci: ruby26rails60 Your tests passed on CircleCI!
Details
ci/circleci: ruby26rails60turbolinks Your tests passed on CircleCI!
Details
ci/circleci: setup_coverage Your tests passed on CircleCI!
Details
ci/circleci: testapp52 Your tests passed on CircleCI!
Details
ci/circleci: testapp60 Your tests passed on CircleCI!
Details
ci/circleci: testapp60turbolinks Your tests passed on CircleCI!
Details
ci/circleci: upload_coverage Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/diff-coverage 100% (70% threshold)
Details
codeclimate/total-coverage 99% (0.0% change)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.