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

Allow devlocal user to choose the type of user to create #1905

Merged
merged 31 commits into from Apr 8, 2019

Conversation

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj commented Mar 22, 2019

Description

tl;dr:

  • Can create a new user of any type from the devlocal sign in page and redirects to the appropriate site
  • Servernames are now collected passed through as a struct to auth handlers instead of individually
  • Cypress tests now POST to the login form instead of searching for available button
  • Devlocal Sign In page was styled a bit
  • Adds many more tests for devlocal

It would be helpful if local sign in allowed you to create a new user of any type and sign you into the appropriate application. This PR adds a drop down for the user type and then will create the appropriate associated models when it creates the user. Finally it will redirect to the appropriate site for that user type.

The logic that creates a user will look in the request for data including First Name, Last Name, Phone Number, and Email. If any of those are present it will use those instead of defaults. (These extra ones were not added to the html form). This allows a regular POST request to /devlocal-auth/create to create a user of any type instead of having to do DB operations. In turn that provides a hook that means a Load Testing application can create these users at will.

Finally, I've limited the page to returning only 25 users. Load testing has created 1000s of users and this page seems to take forever to load unless I have a limit. I'm not convinced we're ready for pagination so either 25 is good or a slightly higher number. But we can't do unlimited anymore.

Reviewer Notes

The change here got a little big because of the need to redirect to the correct application when a user was created of a type that is different than the application you start from. If there's a way to make this PR more readable I'm happy to do it.

Setup

Run the server and the client, go to create a new devlocal user and select the type from the dropdown. Then submit the form to create a new user.

Code Review Verification Steps

  • Request review from a member of a different team.

Screenshots

Screen Shot 2019-03-26 at 1 53 40 PM

@chrisgilmerproj chrisgilmerproj self-assigned this Mar 22, 2019

@chrisgilmerproj chrisgilmerproj referenced this pull request Mar 22, 2019

Open

WIP - Try out load testing framework locust.io #1597

0 of 2 tasks complete
@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 25, 2019

Codecov Report

Merging #1905 into master will decrease coverage by 0.27%.
The diff coverage is 42.56%.

@@            Coverage Diff             @@
##           master    #1905      +/-   ##
==========================================
- Coverage   60.81%   60.54%   -0.27%     
==========================================
  Files         193      193              
  Lines       12310    12487     +177     
==========================================
+ Hits         7486     7560      +74     
- Misses       3951     4044      +93     
- Partials      873      883      +10

@chrisgilmerproj chrisgilmerproj marked this pull request as ready for review Mar 26, 2019

@jim jim requested review from reggieriser and Ronolibert Mar 29, 2019

@chrisgilmerproj chrisgilmerproj requested review from lynzt and rdhariwal Apr 5, 2019

@stangah
Copy link
Contributor

stangah left a comment

This looks pretty good to me, but how does this get us away from using the devlocal page to login for Cypress?

<p>
<input type="hidden" name="gorilla.csrf.Token" value="` + csrfToken + `">
<select name="userType">
<option value="milmove">MilMove</option>

This comment has been minimized.

Copy link
@stangah

stangah Apr 5, 2019

Contributor

As a lazy user I think I'd rather have a list of buttons instead of a dropdown so I can do less clicking

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Apr 5, 2019

Author Contributor

I'll probably come back and fix this at some point, good call.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

chrisgilmerproj commented Apr 5, 2019

This looks pretty good to me, but how does this get us away from using the devlocal page to login for Cypress?

It doesn't get us away from devlocal signin. It gets us away from using the devlocal signin page. So effectively this would get rid of cy.signInAsUser() and replace it with cy.signInAsUserPostRequest(), which is a direct POST. No longer would we need to use the button for the user we want to interact with. That should mean we don't need to worry about how cypress renders the devlocal signin page, and thus no need to modify the behavior of the cookies being generated for the forms there.

FWIW, this PR also limits the number of returned users, which makes it mostly unusable for automated testing.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

chrisgilmerproj commented Apr 5, 2019

Folks - I had a little trouble merging master in this morning and that added a few more bugs I had to fix. But this is Ready! Would really appreciate a review on this so I can get back to load testing.

@Ronolibert
Copy link
Contributor

Ronolibert left a comment

Looks good!

I did have some weird behavior when:

  1. Log in to MilMove
  2. Log Out
  3. Log in to office
    Expected Result: Be logged in to the office app
    Actual: Get redirected to the login page again

But I think for the purposes of the PR, this shouldn't be a major issue

@stangah

This comment has been minimized.

Copy link
Contributor

stangah commented Apr 5, 2019

I'm having trouble with being redirected to a logged in state after making a new user, seems to be the cookie issue you're dealing with? We talked about maybe POSTing to the respective hostname rather than using a relative path, hopefully that helps

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

chrisgilmerproj commented Apr 5, 2019

I'm going to try to fix the redirect with the suggestion from @stangah . If not then I'll probably split that into a separate PR and pull the changes from this PR. I don't need it but it sure was a nice convenience.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

chrisgilmerproj commented Apr 5, 2019

So posting to the different subdomains doesn't work here because of CSRF protection. What I needed to do was set the Cookie Domain explicitly. I've done that but I'm afraid its a little odd how I've done it. I'll take another pass at this and try to clean it up.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

chrisgilmerproj commented Apr 5, 2019

New Screenshot:

Screen Shot 2019-04-05 at 4 29 07 PM

cy.clearCookies();
});
const baseUrl = userTypeToBaseURL[userType]; // eslint-disable-line security/detect-object-injection
Cypress.config('baseUrl', baseUrl);

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Apr 8, 2019

Author Contributor

@lynzt - in my branch I ended up rewriting this to take advantage of constants.

This comment has been minimized.

Copy link
@lynzt

lynzt Apr 8, 2019

Contributor

@chrisgilmerproj I like your foreach much better... I was going to go back and refactor, but you beat me to it :)

@chrisgilmerproj chrisgilmerproj merged commit 934b58f into master Apr 8, 2019

17 of 19 checks passed

codecov/patch 42.56% of diff hit (target 60.81%)
Details
codecov/project/go 60.37% (-0.27%) compared to a688375
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details

@chrisgilmerproj chrisgilmerproj deleted the cg_devlocal_any_user_type branch Apr 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.