Feature/padlock auth #15

andrewr224 · 2018-04-06T08:25:07Z

Create delete job on token creation
Improve phone/mail validation
Implement email verification
Implement reset password

andrewr224 · 2018-04-10T14:57:00Z

app/services/padlock/token_generator.rb

-        key_type:   type,
-        expired_at: expired_at
-      )
+      ExpireTokenJob.set(wait_until: token.expired_at).perform_now(token_id: token.id) if token


This seems to work, but, and I know a good but when I see one, that's probably not how it's supposed to be used. Please advise

ailecksandr · 2018-04-10T19:28:29Z

app/controllers/v1/padlock/registration_controller.rb

+      def create
+        @user = V1::Padlock::Registrations::Create.call(user_params)
+
+        return render json: @user.errors, status: :unprocessable_entity unless @user.persisted?


if @user.new_record? => same

unless persisted seems to better convey my intention here, namely - I wanna render errors unless the user was saved

Ok, just noticed that => persisted? == !(new_record? || destroyed?)

Also, you have defined alias for this - user_valid?

ailecksandr · 2018-04-10T19:29:14Z

app/controllers/v1/padlock/session_controller.rb

+      def create
+        @user = V1::Padlock::Sessions::Create.call(params)
+
+        return render json: nil, status: :unauthorized unless @user


head :unauthorized

ailecksandr · 2018-04-10T19:29:44Z

app/controllers/v1/padlock/session_controller.rb

+      def destroy
+        return render json: nil, status: :unprocessable_entity unless token
+
+        return render json: nil, head: :ok if ::Padlock::TokenDestroyer.call(token: token)


does it work? :D

Why wouldn't it?

Because render checks status key. Change :ok, to :unprocessable_entity. Theoretically you'll see the same result.

..so you can ignore status here at all, because it is 200, when response has body, by default

ailecksandr · 2018-04-10T19:29:53Z

app/controllers/v1/padlock/session_controller.rb

+      end
+
+      def destroy
+        return render json: nil, status: :unprocessable_entity unless token


same as above

ailecksandr · 2018-04-10T19:30:12Z

app/jobs/expire_token_job.rb

+class ExpireTokenJob < ApplicationJob
+  queue_as :default
+
+  def perform(params = {})


ArgumentError: wrong number of arguments (given 1, expected 0)

ailecksandr · 2018-04-10T19:34:22Z

app/operations/v1/padlock/sessions/create.rb

+        def identity
+          @identity ||= case
+                        when email
+                          Identity.find_by(uid: email)


def uid email || phone end def identity @identity ||= Identity.find_by(uid: uid) end

ailecksandr · 2018-04-10T19:38:30Z

app/services/padlock/token_generator.rb

+    def initialize(params = {})
+      @user       = params[:user]
+      @type       = params[:type] || :auth
+      @expired_at = params[:expired_at] || Time.zone.now + 1.month


use fetch when you set default values in initializer.

But if you need to wrap cases like this:

Object.new(type: nil)

and have :auth instead of nil
=> use your approach

ailecksandr

Almost good to go. Plenty of small changes have left now
Well done!

ailecksandr · 2018-04-11T14:16:52Z

app/controllers/v1/padlock/padlock_controller.rb

+      attr_reader :user
+
+      def user_valid?
+        user&.persisted?


This is not a predicate, you can receive nil here

user && user.persisted?

And IMO, user_persisted? looks better, because user_valid? is a little bit smooth description here

Rubocop wouldn't stand it

ailecksandr · 2018-04-11T14:55:06Z

app/controllers/v1/padlock/padlock_controller.rb

+      end
+
+      def insert_token
+        response.headers[ACCESS_TOKEN_KEY] = user.tokens.last.key if user_valid?


thread-unsafe token
(someone else can add token to user right after creation of user)

Remember about refactoring here.

Now you can define has_one association last_access_token in User model

ailecksandr · 2018-04-13T08:01:03Z

app/controllers/v1/padlock/padlock_controller.rb

@@ -0,0 +1,27 @@
+module V1


We need to think about moving whole this module to lib folder.

It’s for libraries that are not packaged as gems but could be. (c) source

ailecksandr · 2018-04-13T08:15:45Z

app/controllers/v1/padlock/registration_controller.rb

+      def create
+        @user = V1::Padlock::Registrations::Create.call(user_params)
+
+        return render json: @user.errors, status: :unprocessable_entity unless @user.persisted?


Also, you have defined alias for this - user_valid?

ailecksandr · 2018-04-13T08:20:53Z

app/controllers/v1/padlock/session_controller.rb

+      def create
+        @user = V1::Padlock::Sessions::Create.call(params)
+
+        return head :unauthorized unless @user


user_valid?

ailecksandr · 2018-04-13T09:01:42Z

app/jobs/expire_token_job.rb

@@ -0,0 +1,9 @@
+class ExpireTokenJob
+  include Sidekiq::Worker


You can move include to ApplicationJob and then inherit from it. Would be cleaner

ArgumentError: You cannot include Sidekiq::Worker in an ActiveJob: ApplicationJob

ailecksandr · 2018-04-13T09:12:09Z

app/jobs/expire_token_job.rb

+class ExpireTokenJob
+  include Sidekiq::Worker
+
+  def perform(params = {})


Use explicit arguments, or at least key arguments (:+1:) here.

We haven't defined constructor for job to provide attributes assigning, call them from the params hash all the time looks messy.

ailecksandr · 2018-04-13T09:17:55Z

spec/factories/identity.rb

+  factory :identity do
+    trait :email do
+      uid      { Faker::Internet.email }
+      provider 'email'.freeze


Why not symbol?

ailecksandr · 2018-04-13T09:18:00Z

spec/factories/identity.rb

+
+    trait :phone do
+      uid      { Faker::PhoneNumber.phone_number }
+      provider 'phone'.freeze


ailecksandr · 2018-04-13T09:20:36Z

spec/jobs/expire_token_job_spec.rb

+
+  subject { described_class.perform_at(token.expired_at, token_id: token.id) }
+
+  let!(:token) { create(:token) }


Let's apply one conventional call order:

Definition of variables -> let, let!

subject

before

after

Agreed, but I vote for subject/let/before/after

That's the order they use in style guide
https://github.com/reachlocal/rspec-style-guide#good-example-2

andrewr224 force-pushed the feature/padlock-auth branch 3 times, most recently from 117ed28 to 52caa23 Compare April 10, 2018 14:55

andrewr224 commented Apr 10, 2018

View reviewed changes

ailecksandr reviewed Apr 10, 2018

View reviewed changes

andrewr224 added 5 commits April 11, 2018 10:04

Create User model along with identity and token

b8baf8b

Create RegistrationsController 🔒

3f45bc3

Create SessionsController #create 💂‍♂️

43efdb5

Switch Padlock controllers to singular resource 🐧

f114bb4

Add Session#destroy 🐗

ee08bf7

andrewr224 force-pushed the feature/padlock-auth branch 2 times, most recently from 2b286f8 to 136fcbb Compare April 11, 2018 07:12

Add delayed job to delete user token 🌵

ad3964b

andrewr224 force-pushed the feature/padlock-auth branch from 136fcbb to ad3964b Compare April 11, 2018 09:47

Switch to Sidekiq 🚬

774b9a1

andrewr224 force-pushed the feature/padlock-auth branch 2 times, most recently from 8ab1e3c to bbeef09 Compare April 11, 2018 14:23

ailecksandr requested changes Apr 13, 2018

View reviewed changes

andrewr224 force-pushed the feature/padlock-auth branch 3 times, most recently from b7625cb to e5d190a Compare April 13, 2018 13:28

andrewr224 force-pushed the feature/padlock-auth branch from e5d190a to ad5edd3 Compare May 4, 2018 11:40

andrewr224 added 2 commits May 4, 2018 16:39

Add Identity validations 🔪

712eb8e

Implement password restoration 🐙

fa53410

andrewr224 force-pushed the feature/padlock-auth branch from ad5edd3 to fa53410 Compare May 4, 2018 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/padlock auth #15

Feature/padlock auth #15

andrewr224 commented Apr 6, 2018 •

edited

Loading

andrewr224 Apr 10, 2018

ailecksandr Apr 10, 2018

andrewr224 Apr 11, 2018

ailecksandr Apr 11, 2018

ailecksandr Apr 13, 2018

ailecksandr Apr 10, 2018

ailecksandr Apr 10, 2018

andrewr224 Apr 11, 2018

ailecksandr Apr 11, 2018

ailecksandr Apr 11, 2018

ailecksandr Apr 10, 2018

ailecksandr Apr 10, 2018 •

edited

Loading

andrewr224 Apr 11, 2018

ailecksandr Apr 10, 2018

andrewr224 Apr 11, 2018

ailecksandr Apr 10, 2018

ailecksandr left a comment

ailecksandr Apr 11, 2018 •

edited

Loading

andrewr224 Apr 13, 2018

ailecksandr Apr 11, 2018

ailecksandr Apr 13, 2018

ailecksandr Apr 13, 2018

ailecksandr Apr 13, 2018

ailecksandr Apr 13, 2018

andrewr224 Apr 13, 2018

ailecksandr Apr 13, 2018

ailecksandr Apr 13, 2018

ailecksandr Apr 13, 2018

ailecksandr Apr 13, 2018

andrewr224 Apr 13, 2018

andrewr224 Apr 13, 2018


		subject { described_class.perform_at(token.expired_at, token_id: token.id) }

		let!(:token) { create(:token) }

Feature/padlock auth #15

Are you sure you want to change the base?

Feature/padlock auth #15

Conversation

andrewr224 commented Apr 6, 2018 • edited Loading

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

ailecksandr Apr 10, 2018 • edited Loading

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

ailecksandr left a comment

Choose a reason for hiding this comment

ailecksandr Apr 11, 2018 • edited Loading

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

andrewr224 commented Apr 6, 2018 •

edited

Loading

ailecksandr Apr 10, 2018 •

edited

Loading

ailecksandr Apr 11, 2018 •

edited

Loading