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

Earth - Olga #38

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Earth - Olga #38

wants to merge 8 commits into from

Conversation

OlgaSe
Copy link

@OlgaSe OlgaSe commented Nov 30, 2020

Assignment Submission: Media Ranker Revisited

Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.

Reflection

Prompt Response
What did you need to configure and setup to make OAuth installed and work? To configure I had to install gems (OmniAuth, omniauth-github, dotenv), configure a new app on Githhub and create secrets, after that I had to add an initializer file to let know the app where to look for secrets.
What areas of Rails app code did you need to create/write/modify in order to change logging in to use OAuth? Routes had to be added, added a user controller action for login, as well as, updated user's model attributes via migration.
What was one controller test you updated? Why did you need to update it? I updated users' attribute and messages didn't match the user's controller messages.
Why did we need to mock OAuth for testing? To run the tests without actually going to the Github, but recreating it by some made-up data instead.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

MediaRanker Revisted

Major Learning Goals/Code Review

Functional Requirements

Functional Requirement yes/no
Completed OAuth: Logging in goes through GitHub ✔️

Overall Feedback

Overall Feedback Criteria yes/no
Green (Meets/Exceeds Standards) 1 in Functional Requirements and controller methods are cleanly written with appropriate model methods for Oauth 💚

Summary

Nice work, this functions, just remember to remove the .env file from git.

# redirect_to root_path
# end
#
def create

Choose a reason for hiding this comment

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

👍 Looks good

Comment on lines +52 to +55
<% if session[:user_id] %>
<li class="nav-item app-header__nav_item">
<%= link_to "Log out", logout_path, method: "post" %>
</li>

Choose a reason for hiding this comment

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

This is fine, but it would be nice to have a controller filter that sets @current_user or something so you could use that instance variable.

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

2 participants