Feature/coach login with GitHub & admin login with username/password#50
Conversation
|
okay, let me know when you want me to do a more detailed review :-) |
|
|
|
@DrakeW we are having some inconsistency with that second test: I note the same test is failing in the other open PR: which is relying on a change to one of the gems - perhaps that needs to be pulled in first ...? |
|
@tansaku #51 relies on changes to all metric gems we are using, and there's no dependency between our two branches so the order doesn't matter regarding dependency? but it makes sense to pull in those small PRs first since otherwise they might need to merge my changes (a lot of files) first before they can be merged.... |
|
@tansaku failed |
|
the moral of the story here is make each PR small enough to review independently, even if there's a string of related ones.... welcome to real life software development on an existing product with an existing team :-) |
|
@DrakeW going to try and get some things sorted tonight - I think credentials for all gems should return array of symbols ... gonna try and impose that as I think it's the right thing to do |
| @@ -0,0 +1,28 @@ | |||
| class Users::UnlocksController < Devise::UnlocksController | |||
There was a problem hiding this comment.
do we really need all these devise controllers filled with only comments - could we just delete the ones we're not actually customizing?
There was a problem hiding this comment.
I'm concerned that having lots of files filled with comments makes the whole projects more burdensome to navigate - if we really must have these files for devise to operate that's one thing, but these should all exist in the gem - can't we add them individually as needed?
| before_action :authenticate_user! | ||
|
|
||
| http_basic_authenticate_with name: "cs169", password: ENV['PROJECTSCOPE_PASSWORD'] | ||
| # http_basic_authenticate_with name: "cs169", password: ENV['PROJECTSCOPE_PASSWORD'] |
There was a problem hiding this comment.
let's remove this if not being used any more - we should avoid commenting old code - old code is kept in git history
|
@DrakeW thanks for this. So this test is failing (as you mentioned): Scenario: unauthrozied user login
Given I am on the login page
When I sign in as coach with github email "test-coach-not-exist@test.com"
Then I should be on the login page
And I should see "You are not authroized."I note that Also, if this test doesn't work, shouldn't we remove it and put it in a separate PR that builds on the whitelist work of your colleague? It's important to keep every commit on non-feature branches green to support operations like The set of cucumber tests with the PR indicate the new functionality that's being added right? The two authorised logins (coach and admin) are passing - let's merge those in, and keep the exclusion of unauthorised users to a later PR? Isn't the current functionality excluding people who don't have github logins? so you could just rewrite the step definition to fail the github oauth for the particular user you don't want to be able to log in here, e.g. Scenario: non github user cannot log in
Given I am on the login page
When I sign in with a non github email
Then I should be on the login page
And I should see "You are not unauthorised."See https://github.com/omniauth/omniauth/wiki/Integration-Testing#mocking-failure for how to mock the github oauth fail. Does that make sense? I think it's really important to write your code as self contained and not having failing tests that will pass once other code is in place. The feature you have here excludes non-github users right? So the sad path here is non-github users can't log in. The whitelist feature is separate and should be addressed in a separate PR no? Let's make this PR address https://www.pivotaltracker.com/story/show/132353109 and we can address https://www.pivotaltracker.com/story/show/132353501 in another and both can be green and clean :-) |
|
@tansaku thanks for the review and I'll make changes accordingly. |
|
@tansaku so after I added The first one happened randomly at any test that tries to add data into the db like Any thoughts? |
|
@tansaku found out the issue was caused by the And the problem is that cucumber proceeded to the next step before all the substeps actually finishes, which caused
It's all fixed now |
| ADMIN = "admin" | ||
| COACH = "coach" | ||
|
|
||
| def self.from_omniauth(auth) |
There was a problem hiding this comment.
this method is a bit long - I'll still pull in but it would be good to refactor to make it easier to see what the components are doing
| def self.from_omniauth(auth) | ||
| email = auth.info.email.nil? ? auth.extra.raw_info.email : auth.info.email | ||
| login = auth.extra.raw_info.login | ||
| if !login.nil? and !email.nil? |
There was a problem hiding this comment.
this block of code is ripe for pulling out as a separate method - we could also rewrite as unless login.nil or email.nil?
ah, I guess the method is not so huge because it's just block syntax on that first_or_create, but we could extract that as a separate method and self document like so:
unless login.nil or email.nil?
find_or_create_user_from(login, email, auth)
end
just some thoughts ...
| <%= form_for(resource, as: resource_name, url: registration_path(resource_name)) do |f| %> | ||
| <%= devise_error_messages! %> | ||
|
|
||
| <!-- <div class="field"> |
4 participants
feature: user login with github (no whitelist check in this PR)
pivotal tracker story:
https://www.pivotaltracker.com/story/show/132353109
feature: