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

Added login and logout routes #178

Merged
merged 5 commits into from Nov 15, 2019
Merged

Conversation

neilong31
Copy link
Contributor

Helps with #86 by adding routes that with OneLogin

What is added is:

  1. login route
  2. oauth callback route (We can change the redirection to index.htm
  3. logout route

Copy link
Contributor

@miggs125 miggs125 left a comment

Choose a reason for hiding this comment

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

app.js is not where we will be implementing the endpoints. Routes (endpoints) should be modulated in their respective file depending on the functionality. For example, these routes should be placed in a file with a name such as auth.js or similar. You are not making use of the router that was created in #141. Please checkout the contents of /src/backend/web and try to understand what is going on there. Checkout this article to find out how express router works. Here is another link regarding express router. This file should only be used to define the app and the middleware it uses, not the endpoints .

@miggs125
Copy link
Contributor

Additionally, any endpoint to be implemented needs a test. See #141 to see how these can be implemented.

@neilong31
Copy link
Contributor Author

Oh okay thanks! I think I see my mistake. I'll try and post my fix tonight.

@neilong31 neilong31 merged commit a256392 into Seneca-CDOT:master Nov 15, 2019
@humphd
Copy link
Contributor

humphd commented Nov 27, 2019

@neilong31 it looks like you merged this in without getting review. I see @miggs125 has comments above that don't seem to be addressed.

I'm not clear what happened here, but I'd like to have it fixed in a follow-up PR. Please make one to deal with the outstanding comments, and let's not merge our own code without review approvals.

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.

None yet

3 participants