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

Derive param key from admin user class in sessions controller #210

Merged
merged 3 commits into from Feb 15, 2017

Conversation

Linuus
Copy link
Contributor

@Linuus Linuus commented Feb 2, 2017

Also fixes some minitest warnings and temporarily removes
codeclimate

Fixes #209

@jensljungblad Can you test this? My laptop has some issues so I can't create a new project at the moment.

Also fixes some minitest warnings and temporarily removes
codeclimate
@Linuus
Copy link
Contributor Author

Linuus commented Feb 6, 2017

@jensljungblad I just did a manual test for this and it worked fine. I also updated the changelog.

Can we write a good test for this somehow? It's a bit annoying because I guess we'd have to have another dummy app...

@jensljungblad
Copy link
Collaborator

@Linuus Depends on if we need an integration test or not. If we do, I think a separate sessions_controller would be enough. Something like

class SessionsControllerWithAnotherAdminUser < ApplicationController
  include Godmin::Authentication::SessionsController
  include Godmin::Authentication

  def admin_user_class
    AnotherAdminUser
  end
end

And then a separate route. But I think maybe its enough with something that tests the admin_user_params function? Then we could just create a sessions controller class in the test and instantiate it?

@Linuus
Copy link
Contributor Author

Linuus commented Feb 7, 2017

@jensljungblad That's true. I'll see what I can come up with.

@Linuus
Copy link
Contributor Author

Linuus commented Feb 7, 2017

@jensljungblad Pushed a test for this now.

Merge?

EDIT: They are failing in Rails < 5 because they changed syntax in the tests.

@Linuus Linuus force-pushed the fix/derive-param-key-from-admin-class branch from cc3a672 to 5725d04 Compare February 7, 2017 09:43
@Linuus
Copy link
Contributor Author

Linuus commented Feb 7, 2017

Ok, tests updated again. I'm using the old style now, without the params: keyword. This still works in Rails 5 but prints a deprecation warning.

Copy link
Collaborator

@jensljungblad jensljungblad left a comment

Choose a reason for hiding this comment

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

Other than the fact that our integration test setup is beginning to feel a bit ridiculous (which has nothing to do with this PR) it looks good to me!

@jensljungblad
Copy link
Collaborator

jensljungblad commented Feb 15, 2017

(We did some weird workaround for another function that changed between Rails 4 and 5, but perhaps we should get back to this once Rails 5.1 is released)

@jensljungblad jensljungblad merged commit 1340109 into master Feb 15, 2017
@jensljungblad jensljungblad deleted the fix/derive-param-key-from-admin-class branch February 15, 2017 10:19
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.

Admin user class not reflected in session controller params
2 participants