Code review of the skillgrid.ru test exercise

1_different_types_of_users_problem.md

Этот пункт для меня оказался самым сложным и времязатратным, так как проблемы были как и с придумыванием достаточно оптимального решения, так и с его дальнейшей реализацией. Причем с реализацией проблем было намного больше. Ну, по порядку.

Если бы поля у всех пользователей были одинаковые, то хватило бы только добавить еще одно поле Тип. Но, так как поля у разных типов пользователей разные, то сложности начинаются с того, что нужно решить, как избавиться от лишних полей для пользователей, у которых этих полей быть не должно. Я обдумывал три разных решений:

1. Тупо делать отдельную таблицу под каждый тип с полным набором полей.

Такое решение, например, предлагается в ридми Devise. В каждой таблице типа пользователя в этом варианте будут повторяющиеся атрибуты типа e-mail, пароля, итд, что уже является очевидным излишком. Повторяющиеся атрибуты надо отделить в свой отдельный логический кусок. Плюс, при таком варианте, при логине нужно было бы каждый раз делать юнион email/паролей всех этих таблиц. Хорошего, в итоге, в этом решение нет ничего, кроме, пожалуй, его простоты.

2. Оставить все в одной таблице, и делить на типы с помощью Single Table Inheritance.

Тогда в изначальную таблицу Users нужно было бы добавить все возможные варианты атрибутов и поле Тип, по которому бы ActiveRecord определял, какие атрибуты подгружать в одноименную модель. Иметь множество null атрибутов для каждой записи в принципе кажется мне признаком плохой схемы, и, если еще для всего трех типов пользователей может быть адекватным решением, то при еще дальнейшем расширении таблица все больше будет превращаться в кашу из nullов. Поэтому, это решение кажется мне не оптимальным.

3. Сделать одну таблицу с общими атрибутами, а для каждого уникального типа пользователя сделать отдельную таблицу, в которой хранить его уникальные атрибуты.

Общие атрибуты это логин/пароль, плюс у большинства типов пользователей должна быть возможность загрузки аватарки, поэтому я решил сделать пользовательский аватар тоже общим атрибутом. Плюс, у общей таблицы должно опять же быть поле Тип, по которому определяется в какой дополнительной таблице следует искать остаток пользовательских атрибутов. Все дополнительные таблицы должны иметь внешним ключом ID общей таблицы, и, в идеале, он же должен являться еще и первичным ключом в дополнительной таблице, чтобы не множить лишних идентификаторов.

Такое решение кажется мне оптимальным, так как не создает лишних атрибутов, является легко расширяемым и остается простым и логичным.

В итоге получается 4 таблицы: user_logins - здесь хранятся общие атрибуты admins - здесь хранятся уникальные атрибуты для админов shop_onwers - уникальные атрибуты для владельцев магазинов guests - уникальные атрибуты гостей

Теперь о возникших проблемах с реализацией.

Как это реализовать, используя ActiveRecord? Тут обычная связь один к одному, но по умолчанию AR связывает одну таблицу только с одной таблицей, поэтому либо писать кастомные методы для унификации работы с несколькими связями один к одному, либо использовать полиморфическую связь AR.

При полиморфической связи в таблице, user_logins необходимо, вкупе с полем user_type, иметь еще и поле user_id, которое ссылается на уникальный ID записи в одной из вспомогательных таблиц. AR по умолчанию создает первичные ключи для каждой таблицы, тем самым такая реализация уже отличается от моей изначальной задумки. Плюс, с полиморфической связью не используется внешний ключ, т.е. для этих таблиц не осуществляется целостность данных на уровне БД. В итоге, чтобы воспользоваться полиморфической связью, с этими двумя моментами я смирился, но вот не знаю насколько такое решение оптимально. Поэтому это получается первым кандидатом для рефакторинга. Итоговая схема приложена.

Далее, я создал 4 модели: UserLogins - логика для логинов/паролей User - абстрактный класс, инкапсулирует общее поведение разных типов пользователей Admins - модель админов, наследуется от User ShopOwners - модель владельцев магазинов Guests - модель гостей Модели все приложены.

Далее, начались проблемы с реализацией взаимодействия. С вложенными формами до этого не работал, поэтому разбирался с ними. Разбирался как работают нэймспейсы с маршрутами. Далеко не сразу понял, что при полиморфической связи AR по-умолчанию добавляет связанные записи одной транзакцией, подставляя куда нужно правильные ID. Также, чтобы сдвинуться с мертвой точки мне пришлось отказаться от Devise, потому что не полностью понимал, как он работает, и в совокупностью со всеми остальными моментами, которые я не до конца понимал, я надолго застрял. Переписав же авторизацию пользователей с нуля, удалось разобраться и со всем остальным.

Маршруты получились такие: я создал нэймспейс users, и в этом нэймспейсе под каждый тип пользователя свой ресурс. (см. файл routes.rb)

Для каждого типа юзера я с создал свой контроллер. Приведу пример только одного, так как они по сути почти одинаковые, отличаются только приватными методами с strong parameters. Тут очевидно получается не DRY и нужно рефакторить. Следует ли это вынести в один контроллер, в котором тип юзера будет определяться по URL? Или есть какой-нибудь еще более верный способ? Не приведет ли совмещение всех пользовательских контроллеров в один к излишней запутанности при расширении?

Views. Я сделал общий партиал для вложенной формы с user_login атрибутами, и отдельный форму для каждого юзера. Форма для каждого юзера тоже партиал, чтобы использовать как для new, так и для edit.

Что в итоге мне кажется здесь нужно переделать:

1. Убрать лишние ID в схеме
2. Объеденить пользовательские контроллеры
3. Возможно следует вернуть Devise вместо кастомной регистрации.

_admin_form.html.haml

= form_for [:users, @user], html: { multipart: true } do |f|
  = render 'users/shared/errors'
  = render 'users/shared/user_login', f: f
  .form-group
    = f.label :first_name
    = f.text_field :first_name
  .form-group
    = f.label :last_name
    = f.text_field :last_name
  .form-group
    = f.label :photo
    = f.file_field :photo, accept: 'image/jpeg,image/gif,/image/png'
  .form-group
    = f.label :date_of_birth
    = f.date_field :date_of_birth
  .actions
    - if @user.id.nil?
      = f.submit "Sign up", class: %w(btn btn-primary)
    - else
      = f.submit "Update", class: %w(btn btn-primary)

_user_login_form.html.haml

= f.fields_for :user_login_attributes, @user.user_login do |login_form|
  .form-group
    = login_form.label :email, autofocus: true
    = login_form.email_field :email
  .form-group
  - unless @user.class == Guest  
    = login_form.label :avatar
    = login_form.file_field :avatar, accept: 'image/jpeg,image/gif,/image/png'
  .form-group
    = login_form.label :password
    = login_form.password_field :password, autocomplete: "off"
  .form-group
    = login_form.label :password_confirmation
    = login_form.password_field :password_confirmation, autocomplete: "off"

admin.rb

class Admin < User

  validates :first_name, presence: true
  validates :last_name, presence: true
  validates :date_of_birth, presence: true

  mount_uploader :photo, PhotoUploader 

end

admins_controller.rb

class Users::AdminsController < ApplicationController
  before_action :user_admin?, only: [:edit, :update]

  def new
    @user = Admin.new
  end

  def create
    @user = Admin.new(admin_params)
    if @user.valid? && @user.user_login.valid?
      @user.save
      log_in @user.user_login
      flash[:success] = "Welcome! You have signed up successfully."
      redirect_to products_path
    else
      render 'new'
    end
  end

  def edit
    @user = current_user.user
  end
  
  def update
    @user = current_user.user
    if @user.update_attributes(admin_params_on_edit)
      flash[:success] = "Profile updated."
      redirect_to root_path
    else
      render 'edit'
    end
  end


  private

    def user_admin?
      unless current_user.is_admin?
        flash[:danger] = "Access denied."
        redirect_to root_path
      end
    end

    def admin_params
      params.require(:admin).permit(:first_name, 
                                    :last_name,
                                    :date_of_birth,
                                    :photo,
                                    user_login_attributes: [:email, :avatar, :password, :password_confirmation])
    end

    def admin_params_on_edit
      params.require(:admin).permit(:first_name, 
                                    :last_name,
                                    :date_of_birth,
                                    :photo,
                                    user_login_attributes: [:avatar, :password, :password_confirmation])
    end
end

guest.rb

class Guest < User
end

new.html.haml

.container
  .row
    .col-md-6.col-md-offset-3
      .text-center
        %h1 Sign up
      = render 'admin_form'

routes.rb

get 'log_in' => 'session#new'
post 'log_in' => 'session#create'
delete 'log_out' => 'session#destroy'

namespace :users do
  resources :admins, except: [:index, :show, :destroy]
  resources :shop_owners, except: [:index, :show, :destroy]
  resources :guests, except: [:index, :show, :destroy]
end

schema.rb

ActiveRecord::Schema.define(version: 20151108104535) do

  # These are extensions that must be enabled in order to support this database
  enable_extension "plpgsql"

  create_table "admins", force: :cascade do |t|
    t.string "first_name"
    t.string "last_name"
    t.string "photo"
    t.date   "date_of_birth"
  end

  create_table "guests", force: :cascade do |t|
  end

  create_table "products", force: :cascade do |t|
    t.string   "name"
    t.text     "description"
    t.string   "image"
    t.datetime "created_at",  null: false
    t.datetime "updated_at",  null: false
    t.string   "status"
    t.string   "shop_name"
  end

  create_table "shop_owners", force: :cascade do |t|
    t.string "shop_name"
  end

  create_table "user_logins", force: :cascade do |t|
    t.string   "email",           default: "", null: false
    t.datetime "created_at",                   null: false
    t.datetime "updated_at",                   null: false
    t.integer  "user_id"
    t.string   "user_type"
    t.string   "avatar"
    t.string   "password_digest"
    t.string   "remember_digest"
  end

  add_index "user_logins", ["email"], name: "index_user_logins_on_email", unique: true, using: :btree
  add_index "user_logins", ["user_id"], name: "index_user_logins_on_user_id", using: :btree

end

shop_owner.rb

class ShopOwner < User
  validates :shop_name, presence: true
end

user.rb

class User < ActiveRecord::Base
  self.abstract_class = true

  has_one :user_login, as: :user
  accepts_nested_attributes_for :user_login, update_only: true
end

user_login.rb

class UserLogin < ActiveRecord::Base
  #UserLogin model contains registration logic
  #belongs to different types of users
  attr_accessor :remember_token

  belongs_to :user, polymorphic: true, dependent: :destroy

  mount_uploader :avatar, AvatarUploader

  EMAIL_VALIDATION = /\A[\w+\-.]+@[a-z\d\-.]+\.[a-z]+\z/i

  before_save { self.email = email.downcase }
  validates :email, presence: true, 
                    length: { maximum: 255 },
                    format: { with: EMAIL_VALIDATION },
                    uniqueness: { case_sensitive: false }

  has_secure_password

  with_options if: :is_admin? do |admin|
    admin.validates :password, presence: true, length: { minimum: 10 }, allow_nil: true
  end

  with_options if: :is_shop_owner? do |admin|
    admin.validates :password, presence: true, length: { minimum: 8 }, allow_nil: true
  end

  with_options if: :is_guest? do |admin|
    admin.validates :password, presence: true, length: { minimum: 6 }, allow_nil: true
  end

  def is_admin?
    user_type == Admin.to_s
  end

  def is_shop_owner?
    user_type == ShopOwner.to_s
  end

  def is_guest?
    user_type == Guest.to_s
  end

  def remember
    self.remember_token = UserLogin.new_token
    update_attribute(:remember_digest, UserLogin.digest(remember_token))
  end

  def authenticated?(remember_token)
    logger.debug "i am in authenticated? now. is there a remember_digest #{remember_digest}"
    logger.debug "remember_token is #{remember_token}"
    logger.debug "in digested form its #{UserLogin.digest(remember_token)}"
    return false if remember_digest.nil?
    logger.debug "check if it authenticates #{BCrypt::Password.new(remember_digest).is_password?(remember_token)}"
    BCrypt::Password.new(remember_digest).is_password?(remember_token)
  end

  def forget
    self.remember_token = nil
    update_attribute(:remember_digest, nil)
  end

  def UserLogin.digest(string)
    cost = ActiveModel::SecurePassword.min_cost ? BCrypt::Engine::MIN_COST :
                                                  BCrypt::Engine.cost
    BCrypt::Password.create(string, cost: cost)
  end

  def UserLogin.new_token
    SecureRandom.urlsafe_base64
  end
end

@slavone

1. user_logins - не совсем удачное имя.
   Если при отсутствии аватаров, это имя еще подходит, то при их наличии - нет.
   Logins и avatars имеют мало чего общего между собой.
   Возможно, имя могло быть profile_attribures или даже profile.
2. Наследование от User излишне: простоты не привносит, скорее наоборот.
   Также есть "знак", который заставляет задуматься: UserLogin знает о том, что в системе
   есть Admin, ShopOwner и Guest, что "приклеивает" UserLogin к ним.
   Структура классов могла быть, например, следущей:

class Admin < ActiveRecord::Base
  include Profilable

  def validate_profile_password
    # override me as you need
  end
end

class ShopOwner < ActiveRecord::Base
  include Profilable

  def validate_profile_password
    # override me again
  end
end

class Guest < ActiveRecord::Base
  include Profilable

  def validate_profile_password
    # you got the idea
  end
end

module Profilable
  extend ActiveSupport::Concern

  included do
    has_one :profile, as: profilable

    validate :validate_profile_password

    def validate_profile_password
      # here may be default validations or
      # nothing at all or
      fail('You must implement me!')
    end
  end
end

class Profile < ActiveRecord::Base
  belongs_to :profilable, polymorphic: true
end

В этом случае, Profile остается сам по себе, даже при добавлении новых сущностей,
которые могут играть роль Profilable, код Profile остается неизменным.

На самом деле есть еще несколько способов как реализовать валидации:
например, с validates_associated (см. пункт 3), с validates вместо validate и
делегированием.

# Inside Profilable
def profile_password
  profile.password
end

http://guides.rubyonrails.org/association_basics.html#polymorphic-associations
http://guides.rubyonrails.org/active_record_validations.html#custom-methods
http://guides.rubyonrails.org/active_record_validations.html#validates-associated
http://apidock.com/rails/Module/delegate
3. accepts_nested_attributes_for - лучше не использовать.
Так как она привносит больше проблем, чем решает.
Может на первых этапах, когда все очень просто, accepts_nested_attributes_for
и кажется подходящим решением, то в будущем еще аукнется.
Причина та же - связывание/"склеивание" (англ. coupling) сущностей.

profile = Profile.new(params[:profile])
admin = Admin.new(params[:admin])
admin.profile = profile

admin.valid? && profile.valid?
# or can do just admin.valid? when use validates_associated