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.

Sign up for GitHub

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

Jump to bottom

Cascade delete #64

Merged
merged 21 commits into from
Sep 3, 2020
Merged

Cascade delete #64

merged 21 commits into from
Sep 3, 2020

Conversation

br648
Copy link
Contributor

@br648 br648 commented Aug 27, 2020 •
edited by landonreed
Loading

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Description

Tests to simulate API user flow. The following config parameters must be set in configurations/default/env.yml for these end-to-end tests to run:

  • AUTH0_DOMAIN set to a valid Auth0 domain
  • AUTH0_API_CLIENT set to E2E Test client id (see Auth0)
  • AUTH0_API_SECRET set to E2E Test client secret (see Auth0)
  • DEFAULT_USAGE_PLAN_ID set to a valid usage plan id (AWS requires this to create an api key).
  • OTP_API_ROOT set to http://localhost:8080/otp so the mock OTP server is used
  • OTP_PLAN_ENDPOINT set to /routers/default/plan
  • An AWS_PROFILE is required, or AWS access has been configured for your operating environment e.g. C:\Users<username>.aws\credentials in Windows or Mac OS equivalent.

The following environment variable must be set for these tests to run:

  • RUN_E2E: true

Auth0 must be correctly configured as described here: https://auth0.com/docs/flows/call-your-api-using-resource-owner-password-flow.

@br648 br648 requested a review from evansiroky August 27, 2020 10:41
evansiroky
evansiroky suggested changes Aug 27, 2020
View reviewed changes
Copy link
Contributor

@evansiroky evansiroky left a comment

Choose a reason for hiding this comment

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

See comments. Also, I'm having trouble running the E2E tests locally. Let's discuss my configuration details off Github.

@evansiroky evansiroky assigned br648 and unassigned evansiroky Aug 27, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2020 •
edited
Loading

Codecov Report

Merging #64 into dev will decrease coverage by 2.53%.
The diff coverage is 8.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev      #64      +/-   ##
============================================
- Coverage     34.87%   32.33%   -2.54%     
  Complexity      139      139              
============================================
  Files            71       71              
  Lines          1580     1676      +96     
  Branches        143      159      +16     
============================================
- Hits            551      542       -9     
- Misses         1016     1121     +105     
  Partials         13       13
Impacted Files Coverage Δ Complexity Δ
.../opentripplanner/middleware/OtpMiddlewareMain.java 46.34% <ø> (ø) 4.00 <0.00> (ø)
...entripplanner/middleware/auth/Auth0Connection.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...ripplanner/middleware/bugsnag/BugsnagReporter.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...leware/controllers/api/AbstractUserController.java 53.84% <ø> (+8.68%) 3.00 <0.00> (ø)
...eware/controllers/api/MonitoredTripController.java 30.76% <0.00%> (+4.10%) 2.00 <0.00> (ø)
...dleware/controllers/api/TripHistoryController.java 48.48% <0.00%> (+5.81%) 3.00 <0.00> (ø)
...g/opentripplanner/middleware/models/AdminUser.java 0.00% <0.00%> (-85.72%) 0.00 <0.00> (-1.00)
...org/opentripplanner/middleware/models/ApiUser.java 0.00% <0.00%> (-66.67%) 0.00 <0.00> (-1.00)
...a/org/opentripplanner/middleware/models/Model.java 62.50% <0.00%> (-8.93%) 1.00 <0.00> (ø)
...entripplanner/middleware/models/MonitoredTrip.java 13.04% <0.00%> (-6.96%) 1.00 <0.00> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d2a77b...4f43685. Read the comment docs.

@br648 br648 requested a review from evansiroky August 28, 2020 11:00
binh-dam-ibigroup
binh-dam-ibigroup requested changes Aug 28, 2020
View reviewed changes
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

The mock createApiKeyRequest call and something in canSimulateApiUserFlow broke with the changes introduced. (On the dev branch the tests are working fine for me.)

src/test/java/org/opentripplanner/middleware/TestUtils.java Show resolved Hide resolved
src/test/java/org/opentripplanner/middleware/ApiKeyManagementTest.java Show resolved Hide resolved
binh-dam-ibigroup
binh-dam-ibigroup reviewed Aug 28, 2020
View reviewed changes
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

I added more comments.

src/main/java/org/opentripplanner/middleware/controllers/api/ApiController.java Show resolved Hide resolved
src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/middleware/models/AbstractUser.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/middleware/models/OtpUser.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/middleware/auth/Auth0Users.java Show resolved Hide resolved
src/test/java/org/opentripplanner/middleware/ApiUserFlowTest.java Show resolved Hide resolved
binh-dam-ibigroup
binh-dam-ibigroup reviewed Sep 2, 2020
View reviewed changes
README.md
Comment on lines +112 to +113
| AUTH0_CLIENT_ID | N/A | Special E2E application client ID. |
| AUTH0_CLIENT_SECRET | N/A | Special E2E application client secret. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

These new variables should appear in env.yml.tmp.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that's the case if they're only intended for e2e.

binh-dam-ibigroup
binh-dam-ibigroup requested changes Sep 2, 2020
View reviewed changes
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

User signup is still broken, for a different reason this time. Merge conflicts need to be resolved.

evansiroky
evansiroky approved these changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@evansiroky evansiroky left a comment

Choose a reason for hiding this comment

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

See comment here: #64 (comment). I guess it's good to go after that.

@evansiroky evansiroky removed their assignment Sep 2, 2020
@landonreed landonreed removed their assignment Sep 3, 2020
binh-dam-ibigroup
binh-dam-ibigroup reviewed Sep 3, 2020
View reviewed changes
src/main/java/org/opentripplanner/middleware/models/AbstractUser.java

@Override
public boolean delete() {
// FIXME: Check that the Auth0 user ID being deleted does not exist as a different child class?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep. Admin users will have to be careful when deleting ApiUsers or OtpUsers.

binh-dam-ibigroup
binh-dam-ibigroup reviewed Sep 3, 2020
View reviewed changes
src/test/java/org/opentripplanner/middleware/TestUtils.java
* Helper method to determine if end to end is enabled and auth is disabled. (Used for checking if tests should run.)
*/
public static boolean isEndToEndAndAuthIsDisabled() {
return getBooleanEnvVar("RUN_E2E") && authDisabled();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Philosophical question: do we still need to have the DISABLE_AUTH config parameter in that case?

Copy link
Contributor

@landonreed landonreed Sep 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Totally agree with what you're getting at. I think I commented this somewhere else. I'll create an issue: #68

binh-dam-ibigroup
binh-dam-ibigroup approved these changes Sep 3, 2020
View reviewed changes
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Great job @br648 and @landonreed

@landonreed landonreed merged commit fd1db2e into dev Sep 3, 2020
@landonreed landonreed deleted the cascade-delete branch September 3, 2020 15:25
@br648 br648 mentioned this pull request Sep 17, 2020
Closed
@landonreed landonreed mentioned this pull request Sep 22, 2020
Closed
@binh-dam-ibigroup binh-dam-ibigroup mentioned this pull request Dec 29, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@landonreed landonreed landonreed left review comments

@evansiroky evansiroky evansiroky approved these changes

@binh-dam-ibigroup binh-dam-ibigroup binh-dam-ibigroup approved these changes

Assignees

@landonreed landonreed

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@br648 @codecov-commenter @binh-dam-ibigroup @landonreed @evansiroky