Feature/coach login with GitHub & admin login with username/password

[junyu-w]

feature: user login with github (no whitelist check in this PR) pivotal tracker story: https://www.pivotaltracker.com/story/show/132353109

feature:

1. migration to create root user
2. coach login with github account

[tansaku]

thanks @DrakeW - some initial feedback is that this PR is quite big, and seems to possibly cover more than one feature - that makes it more challenging to review.

If you can please do try and get smaller micro slices of working code in as PRs earlier. I would much rather review 5 really small PRs than one big one.

I see that the CI is failing for lots of reasons - I'm just pulling that out locally - have you looked at the details of the failures here: https://travis-ci.org/AgileVentures/projectscope/builds/169454278 ?

are you getting the same set of failures locally? I get this set locally:

Failing Scenarios:
cucumber features/add_project_with_config.feature:8 # Scenario: add project with config info for CodeClimate gem
cucumber features/dashboard_drag.feature:15 # Scenario: drag upwards
cucumber features/dashboard_drag.feature:20 # Scenario: drag downwards
cucumber features/dashboard_sort.feature:15 # Scenario: sort by code_climate
cucumber features/dashboard_sort.feature:20 # Scenario: sort by github
cucumber features/dashboard_sort.feature:25 # Scenario: sort by slack
cucumber features/dashboard_sort.feature:30 # Scenario: sort by pivotal tracker
cucumber features/edit_project_with_config.feature:8 # Scenario: Cannot see or edit credentials without login
cucumber features/user_login.feature:23 # Scenario: unauthrozied user login
cucumber features/view_metrics.feature:27 # Scenario: view metric for projects
cucumber features/view_metrics.feature:35 # Scenario: update metrics for projects
cucumber features/view_metrics_with_secrets.feature:33 # Scenario: view metric for projects
cucumber features/view_metrics_with_secrets.feature:39 # Scenario: update metrics for projects

17 scenarios (13 failed, 2 undefined, 2 passed)
97 steps (13 failed, 13 skipped, 22 undefined, 49 passed)

[junyu-w]

@tansaku thanks for the review!

this is actually just the user login feature but a lot of files are automatically generated by the devise gem... but i'll keep that in mind and try to make future PRs smaller.

As for the failed tests, some of them were added during iteration1-1 and as I mentioned on slack, they will pass only after code from my other teammates are merged together. And for the old tests that failed, except for Scenario: Cannot see or edit credentials without login others are caused by "Given I am logged in" since the old auth was replaced. And for Scenario: Cannot see or edit credentials without login it should be fixed in my latest commit.

Thanks!

[tansaku]

@DrakeW so ideally you'd be merging your team-mates code into your feature branch, or better yet, removing those tests.

In any given feature branch you shouldn't be anticipating code that will come in from others ...

If your pr is reliant on your team-mates code, shouldn't they put their PRs in first?

Or have you gotten into a situation where your code relies on theirs and vice versa?

BTW, you can all have "Work in Progess" PRs open from the moment you create branches so you can get ongoing input on all your code all the time ...

[junyu-w]

@tansaku the "work in progress" PR is a good point.

And yea the test part is exactly where we kind of messed up, what we did was we merged all the tests together into develop branch before creating our own feature branch...and therefore when we branched out from develop all the tests came with it...

We'll try to avoid this nex time :)

[junyu-w]

@tansaku Actually let's put this PR on hold and let me fix those old tests by defining a new login step tmr. do you want me to remove those unrelated tests as well?

[tansaku]

@DrakeW understood - if you have a green develop branch then please open a PR from there so I can easily review.

Depending on what's there I can perhaps help you break it into several PRs - let's see.

And yeah, so the trick is to make sure that each feature can be worked on in isolation and that colleagues don't start on features that build on things before they are not there. If people are twiddling their thumbs get them to pair or mob on the same feature and then everyone understands everything better :-)

[junyu-w]

@tansaku sounds good

Feel free to close this PR and I'll open a new one from our develop branch after everything is green 👍

[tansaku]

@DrakeW that wasn't necessarily what I was saying. Don't wait for green to put the PR in :-)

If there's more work to be done to make everything green, the best approach is not necessarily to continue adding code until things go green. You might prefer trying to work out how you put in a single PR with a smaller amount of code (focused single feature) starting from a clean base - and it's great to start again if something messed up - you can implement faster the second time around.

The danger is thinking "we did all this work, we need to keep this valuable code" - anyway, it's complicated :-) I'll stand by to see what you send next ...

[junyu-w]

@tansaku Hmm I kind of get what you said but not 100% sure..

So re-doing or breaking them into smaller PR is a little bit too much work I think since I need to go back to some specific commit (which I probably don't remember what the exact content is) and branch out to make a PR from there... for this feature I'll just remove those unrelated tests and get the related tests green and another pain and the whitelist feature are sort of mutually dependent so one of the tests won't pass until that branch is merged in...

[tansaku]

okay, let me know when you want me to do a more detailed review :-)

[junyu-w]

@tansaku those are the only two failed tests right now and the first one depends on the whitelist feature being developed by one of my teammates However, I don't quite understand how the 2nd failed test works... seems like it ran the resample rake task but how is the updated value decided? Can you help me take a look at this one? Thanks!

[tansaku]

@DrakeW we are having some inconsistency with that second test: cucumber features/view_metrics_with_secrets.feature:39 - it is supposed to be completely sandboxed, but it does seem to fail intermittently. We can get around that by deleting the cached network connections for it - see https://github.com/AgileVentures/projectscope/blob/develop/features/support/fixtures/cassettes/View_A_Projects_Metrics_with_Secrets/update_metrics_for_projects.yml re running and checking that in, but a value of 0 there suggests a more serious issue.

I note the same test is failing in the other open PR:

https://github.com/AgileVentures/projectscope/pull/51

which is relying on a change to one of the gems - perhaps that needs to be pulled in first ...?

[junyu-w]

@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....

[junyu-w]

@tansaku failed view_metrics_with_secrets has been fixed with newest upstream changes, and the other failed test will be fixed by the whitelist feature. It's good for a detailed review

[armandofox]

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 :-)

[tansaku]

@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

[tansaku]

@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 unauthrozied should be unauthorised- perhaps we could fix that typo :-)

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 git bisect and the ability to safely role production code back to any instance.

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 :-)

[junyu-w]

@tansaku thanks for the review and I'll make changes accordingly. Also as a side not we had a short meeting with @armandofox today and decided to dump the code for username/password login (so only github login is allowed).

[junyu-w]

@tansaku so after I added @omniauth tag to all tests that require login, some tests began failing randomly one error is the other one is

The first one happened randomly at any test that tries to add data into the db like And the following projects exist... The second one only happened in the add_projecst_with_config test and I was thinking if that was caused by some conflict between the @omniatuh and @javascript?

Any thoughts?

[junyu-w]

@tansaku found out the issue was caused by the Given I am logged in step, that step is a declarative one and contains several substeps as shown in https://github.com/DrakeW/projectscope/blob/feature/login-with-github/features/step_definitions/project_steps.rb#L58

And the problem is that cucumber proceeded to the next step before all the substeps actually finishes, which caused

1. db lock problem by possibly trying to create user & projects at the same time
2. the Unable to find link "New Project" problem because the login was not actually finished.

It's all fixed now