Admin Authentication Refactor #5

Reltre · 2017-12-04T02:33:46Z

This PR adds the following:

Authentication logic for admins
Views related to log in for admins
Database schema for users, with required attributes, including admin role.
Testing for related models and controllers, as well as a spec for the log in feature.
A small amount of jQuery has been added to enable error messaging.

This PR so far only cover authentication of existing users. To run a sanity test on this, be sure to have an existing user, preferably one with admin role, in the database.

Some filters should probably be added in the future to enforce the presence of a logged in user when accessing different parts of the site. This was left out to avoid potential merge conflicts with other branches.

…ion of user login feature test.

…d of flash.

seport

Generally looking awesome! A few requests:

Can you add an admin user with username admin and password password to the setup script for testing purposes?
It would also be nice if you could rebase onto the rails branch and remove unnecessary files like some of the helpers and email things and other random things that don't really contribute to the ticket. (Just to really cross our t's and dot our i's)

Thanks so much! Other than these small changes it looks really good to me so far. :)

seport · 2017-12-21T02:44:40Z

app/channels/application_cable/channel.rb

@@ -0,0 +1,4 @@
+module ApplicationCable


Can you please explain channels and application cables?

Or remove this if it was just something rails generated at some point.

I looked into this. ApplicationCable seems good for implementing features for currently logged users, in particular, for tracking things in real time. Not sure this is something we need at the moment. Might be a nice to have for later on though.

then again, YAGNI; I'll remove it for the time being.

seport · 2017-12-21T02:45:46Z

app/helpers/application_helper.rb

@@ -0,0 +1,2 @@
+module ApplicationHelper


Are these helpers helping anything? Will they in the future, or should we delete these?

seport · 2017-12-21T02:46:10Z

app/jobs/application_job.rb

@@ -0,0 +1,2 @@
+class ApplicationJob < ActiveJob::Base


Is this job doing anything?

seport · 2017-12-21T02:46:51Z

app/mailers/application_mailer.rb

@@ -0,0 +1,4 @@
+class ApplicationMailer < ActionMailer::Base
+  default from: 'from@example.com'


What do we need the mailer for?

seport · 2017-12-21T02:47:39Z

app/models/user.rb

@@ -0,0 +1,7 @@
+class User < ApplicationRecord
+  validates :email, presence: true
+  validates :password, presence: true


where are we encrypting the password?

Once has_secure_password was added below, passwords are encrypted with the bcrypt gem when a User model is saved.

seport · 2017-12-21T02:50:48Z

bin/setup

@@ -0,0 +1,38 @@
+#!/usr/bin/env ruby


Could we add a setup step that creates an admin user? This can also be a separate ticket though.

seport · 2017-12-21T03:10:31Z

config/cable.yml

@@ -0,0 +1,10 @@
+development:


is this a websockets thing? We might not need Cable either.

seport · 2017-12-21T03:12:11Z

config/locales/en.yml

@@ -0,0 +1,33 @@
+# Files in the config/locales directory are used for internationalization


Eventually we should do something like this, but I don't think we need it in this iteration of the app, and there are a couple of new ways I've been learning about doing this other than a yml file which seem pretty interesting.... we can put it in the icebox and discuss it later.

seport · 2017-12-21T03:13:21Z

config/routes.rb

+Rails.application.routes.draw do
+  root 'welcome#index'
+
+  namespace :admin do


can we also add a route for /admin which is either the admin index (can just be a hello world page for now) if there is an admin session logged in, or redirects to admin/login otherwise?

I've added in a before filter for this and an index action for an admin session, well the index page works as a placeholder I guess? I think until we have a dedicated admin view, that we should just have admins navigate to the welcome page on log in.

seport · 2017-12-21T03:15:29Z

spec/controllers/admin/sessions_controller_spec.rb

@@ -0,0 +1,19 @@
+require "rails_helper"


yay testing!!

seport · 2017-12-21T03:25:21Z

I have a few more comments actually,

please set the password field in the view to be a password field rather than a text field
After login the admin should be redirected to an admin dashboard at /admin. For now it can just be a simple hello world page for proof of concept.
I'm not totally sure the session stuff is working. When I go back to /admin/login after logging in (and being redirected to the homepage) I should be redirected immediately, because I am already logged in.

Reltre · 2018-01-02T21:32:16Z

@seport I've made the changes you've request above. One thing we'll need eventually is a way to log out. At the moment, we have to hop into the inspector and delete the session cookie directly to log out.

There's an admin user setup in our seeds file now. I've also added some filters related to login. Also, I realized that this branch was set to merge into master. Once I set it to merge into the "rails" branch, most of those unrelated files went away 😃

seport

LGTM!

Reltre added 13 commits November 15, 2017 20:01

Set up log in route, controller, and initial view for admins.

1cd6968

Setup user model and sessions controller spec for login.

ee5b7dc

Add timestamps to users table.

d4e601a

Change user role attribute to admin for easier usage.

b1f0161

Add user model validations and associated spec.

70d583c

Add pry gem to Gemfile.

77aeab7

Add controller testing to Gemfile.

3ca960b

Fix merge conflict.

b82e55f

f58450a

Add email contexts to users controller spec.

d147fea

Change controller spec for user to test error paths. Add initial vers…

8eb79b8

…ion of user login feature test.

Add authentication for log in path.

1133675

Add jquery gem. Update session controller spec to check status instea…

1c30e2d

…d of flash.

Reltre requested a review from seport December 4, 2017 02:33

seport requested changes Dec 21, 2017

View reviewed changes

Reltre changed the base branch from master to rails January 2, 2018 20:01

Reltre added 3 commits January 2, 2018 12:48

Add in filter for unlogged in admin.

db8296f

Change text to passworld field for login.

92a196a

Setup seed data for admin. Redirect to admin index upon admin login.

2a5e4a4

seport approved these changes Jan 11, 2018

View reviewed changes

seport merged commit aacf0a7 into rails Jan 11, 2018

seport deleted the chores/admin-authentication-refactor branch January 18, 2018 03:54

seport mentioned this pull request Jan 29, 2018

Admin Panel Basic Implementation #11

Open

seport added this to the v1.0.0 milestone Mar 20, 2018

seport added the v1.0.0 label Mar 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Admin Authentication Refactor #5

Admin Authentication Refactor #5

Reltre commented Dec 4, 2017

seport left a comment

seport Dec 21, 2017

seport Dec 21, 2017

Reltre Dec 21, 2017

Reltre Dec 21, 2017

seport Dec 21, 2017

seport Dec 21, 2017

seport Dec 21, 2017

seport Dec 21, 2017

Reltre Dec 21, 2017

seport Dec 21, 2017

seport Dec 21, 2017

seport Dec 21, 2017

seport Dec 21, 2017

Reltre Jan 2, 2018 •

edited

Loading

seport Dec 21, 2017

Reltre Jan 2, 2018

seport commented Dec 21, 2017 •

edited

Loading

Reltre commented Jan 2, 2018

seport left a comment

		@@ -0,0 +1,4 @@
		class ApplicationMailer < ActionMailer::Base
		default from: 'from@example.com'

		@@ -0,0 +1,33 @@
		# Files in the config/locales directory are used for internationalization

Admin Authentication Refactor #5

Admin Authentication Refactor #5

Conversation

Reltre commented Dec 4, 2017

seport left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reltre Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seport commented Dec 21, 2017 • edited Loading

Reltre commented Jan 2, 2018

seport left a comment

Choose a reason for hiding this comment

Reltre Jan 2, 2018 •

edited

Loading

seport commented Dec 21, 2017 •

edited

Loading