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

Contacts #5

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

Contacts #5

wants to merge 2 commits into from

Conversation

max-borisov
Copy link
Contributor

No description provided.

has_and_belongs_to_many :users, join_table: :users_contacts

validates :name, :email, presence: true, length: { maximum: 255 }
end
Copy link
Contributor Author

@max-borisov max-borisov Sep 19, 2016

Choose a reason for hiding this comment

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

Модель для новых контактов. Мапится на существующюю таблицу users.
Пробросил связь с User через has_and_belongs_to_many.

Copy link
Contributor

Choose a reason for hiding this comment

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

я бы все же завел промежуточную модель Membership как принадлежность контакта какому-то юзеру и обратно тогда можно было бы has_many :through использовать http://guides.rubyonrails.org/association_basics.html#the-has-many-through-association

А в дальнейшем в эту модель Membership можно было добавить поля с информацией о связи, например принял ли contact вашу подписку или нет.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Модель Membership завел

@@ -4,6 +4,7 @@ class User < ApplicationRecord
:trackable, :validatable

has_many :newsletters
has_and_belongs_to_many :contacts, join_table: :users_contacts
Copy link
Contributor Author

@max-borisov max-borisov Sep 19, 2016

Choose a reason for hiding this comment

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

Теперь можно делать так:
User.first.contacts.create(name: 'name', email: '[email protected]')

Но поле email в users уникальное и кинет ошибку если другой юзер захочет добавить такой же контакт. Как быть ? Сделать email не уникальным ?

Я все равно склоняюсь к тому, чтобы отдельно хранить users и contacts :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Разделять схожие данные пока смысла нет, они очень похожи.
Имея has_many :contacts, through: :membership мы точно так же можем делать
User.first.memberships << User.find_or_create_by(email: params[:email]) и тд. тогда e-mail будет уникальный всегда, только связи между User - Membership - User будут меняться

Copy link
Contributor

@mpakus mpakus left a comment

Choose a reason for hiding this comment

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

Отлично, глянь только идею про Membership для has_many :through, именование связующих таблиц и надо небольшой тестик сделать на связи, уникальность добавления и e-mail

@@ -0,0 +1,8 @@
class CreateUsersContacts < ActiveRecord::Migration[5.0]
def change
create_table :users_contacts, id: false do |t|
Copy link
Contributor

Choose a reason for hiding this comment

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

Тут глобальная проблема эти join_table нужно называть в порядке отсортированных моделей соединяемых, тогда не надо будет явно указывать join_table contacts_users

3.3.2 Creating Join Tables for has_and_belongs_to_many Associations

If you create a has_and_belongs_to_many association, you need to explicitly create the joining table. Unless the name of the join table is explicitly specified by using the :join_table option, Active Record creates the name by using the lexical book of the class names. So a join between author and book models will give the default join table name of "authors_books" because "a" outranks "b" in lexical ordering.

http://guides.rubyonrails.org/association_basics.html#updating-the-schema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

точно, не обратил внимания

has_and_belongs_to_many :users, join_table: :users_contacts

validates :name, :email, presence: true, length: { maximum: 255 }
end
Copy link
Contributor

Choose a reason for hiding this comment

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

я бы все же завел промежуточную модель Membership как принадлежность контакта какому-то юзеру и обратно тогда можно было бы has_many :through использовать http://guides.rubyonrails.org/association_basics.html#the-has-many-through-association

А в дальнейшем в эту модель Membership можно было добавить поля с информацией о связи, например принял ли contact вашу подписку или нет.

@@ -4,6 +4,7 @@ class User < ApplicationRecord
:trackable, :validatable

has_many :newsletters
has_and_belongs_to_many :contacts, join_table: :users_contacts
Copy link
Contributor

Choose a reason for hiding this comment

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

Разделять схожие данные пока смысла нет, они очень похожи.
Имея has_many :contacts, through: :membership мы точно так же можем делать
User.first.memberships << User.find_or_create_by(email: params[:email]) и тд. тогда e-mail будет уникальный всегда, только связи между User - Membership - User будут меняться

t.integer "contact_id"
t.index ["contact_id"], name: "index_users_contacts_on_contact_id", using: :btree
t.index ["user_id"], name: "index_users_contacts_on_user_id", using: :btree
end
Copy link
Contributor

Choose a reason for hiding this comment

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

можно еще уникальность поставить на [:user_id, :contact_id] индекса

@@ -31,6 +31,7 @@ gem 'sass-rails', '~> 5.0'
gem 'sextant', group: [:development]
gem 'slim-rails'
gem 'spring', group: [:development]
gem 'spring-commands-rspec', group: [:test]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

чтобы из Vim можно было запускать тесты


def edit; end

def destroy; end
Copy link
Contributor Author

@max-borisov max-borisov Sep 26, 2016

Choose a reason for hiding this comment

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

Будем удалять только саму связь из memberships

end
end

def edit; end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Редактирование нужно, пока не совсем понятны такие моменты:

  • если редактировать email. Я добавил контакт [email protected]. После этого, кто-то другой тоже хотел добавить этот же email, но так как он уже в БД, будет сохранена только связь. Если отредактирую email, то это коснется и других, кто тоже добавил этот контакт
  • тоже самое и с именем. Если я поменяю имя контакта, то другие пользователь это заметят :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ничего страшного, agile и подразумевается, что запустил некую фичу за какой-то определенный период (спринт), пошел отзыв, что-то не понравилось переделал, это всегда можно сделать например хранить имена в Membership

else
@contacts = contacts
@contact = contact
render :index
Copy link
Contributor Author

Choose a reason for hiding this comment

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

не очень нравится, как сделал сохранение. Тут две операции: добавляем контакт, создаем связь. Это две разные модели, у каждой есть своя валидация.
Хотел бы показатьmembership.errors на форме, как например, contacts.errors, чтобы все ошибки показывались в одном месте и одном формате, но не знаю как это сделать. Поэтому, membership.errors показываю как alert.
Как правильно нужно сделать ?

Кстати, еще вопрос: в базе есть контактname: tom, email: [email protected]. Я добавляю контакт name: thomas, email: [email protected]. Т.е. я перезаписываю существующее имя ?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • для такого можно использовать сервис классы + транзакция, но я думаю попозже покажу как это можно рефакторить, в целом стоит посмотреть на методы save! которые вызывают exception и как обернуть такие атомарные операции в 1 транзакцию можно например тут http://vaidehijoshi.github.io/blog/2015/08/18/safer-sql-using-activerecord-transactions/
  • да, имя перезапишется на новое

Copy link
Contributor Author

@max-borisov max-borisov Sep 30, 2016

Choose a reason for hiding this comment

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

@mpakus понял, сервис попробую сделать.
Я думал про транзакции, а потом решил, что даже если юзер сохранился, а связь нет, это не страшно, так как при следующем добавлении этого юзера(а он уже будет в БД) сохраниться только связь.
Ну а вообще, конечно, согласен, что это должно быть одной транзакцией.

Copy link
Contributor

@mpakus mpakus left a comment

Choose a reason for hiding this comment

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

Все гуд, по мелочи только (если хочешь транзакции можно позднее добавить) и как тебе идея name перенести в связь Membership?

else
@contacts = contacts
@contact = contact
render :index
Copy link
Contributor

Choose a reason for hiding this comment

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

  • для такого можно использовать сервис классы + транзакция, но я думаю попозже покажу как это можно рефакторить, в целом стоит посмотреть на методы save! которые вызывают exception и как обернуть такие атомарные операции в 1 транзакцию можно например тут http://vaidehijoshi.github.io/blog/2015/08/18/safer-sql-using-activerecord-transactions/
  • да, имя перезапишется на новое

end
end

def edit; end
Copy link
Contributor

Choose a reason for hiding this comment

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

Ничего страшного, agile и подразумевается, что запустил некую фичу за какой-то определенный период (спринт), пошел отзыв, что-то не понравилось переделал, это всегда можно сделать например хранить имена в Membership

@@ -0,0 +1,9 @@
# frozen_string_literal: true
class Contact < ApplicationRecord
self.table_name = 'users'
Copy link
Contributor

Choose a reason for hiding this comment

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

такие штуки только путают людей мне кажется пусть тогда или таблица будет contacts или модель User

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mpakus А как тогда сделать user.contacts и contact.users ?
Ну и модель User требует password. Если так, тогда можно любую строку присваивать.

Ну это ладно, а как сделать такую связь, которую выше описал, без доп. модели Contact ?

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

create_table "memberships", force: :cascade, options: "ENGINE=InnoDB DEFAULT CHARSET=utf8" do |t|
t.integer "user_id"
Copy link
Contributor

Choose a reason for hiding this comment

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

кстати да, подумай может name действительно удобнее будет для каждой связки в memberships хранить

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mpakus согласен, это решит проблему перезатирания имен. Может, мы так дойдем что и email будем разделять. Но пока да, нет необходимости для этого. Редактирование email не будет. К нему могут привязаны много контактов. Если что, контакт можно удалить и добавить заново с правильным email.

- create Contact model
- create association between users and contacts
- create Contacts controller. Add contact
- update spec tests
t.integer "contact_id"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.string "name"
Copy link
Contributor Author

@max-borisov max-borisov Oct 1, 2016

Choose a reason for hiding this comment

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

@mpakus Добавил name


validates :name, :email, presence: true, length: { maximum: 255 }

delegate :name, to: :memberships
Copy link
Contributor Author

@max-borisov max-borisov Oct 1, 2016

Choose a reason for hiding this comment

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

@mpakus Вот тут застрял. Как получить имя контакта из memberships ?
Чтобы было так: current_user.contacts.first.name и Contact.find(:id).name. Сейчас вернет имя из таблицы users.

И опять сомнения по поводу контактов в таблице users.
user.contacts - это гуд. contact.users - вообще не гуд. В самом приложении мы так делать никогда не будем. В админке или консоли для дебага, возможно. Но не в приложении. Контакт не должен знать к каким еще пользователям он относится.
Т.е. как я думаю связь должна быть такая: contact.user, а не contact.users

Мы не можем написать contact.users, нужно делать что-то вроде этого:
Contact.first.users.where(users: { id: current_user.id}).first.name, чтобы получить контакт текущего юзера, а не любого другого, имеющего такой же контакт.
И не понятно, как получить имя из memberships, когда пишем .name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mpakus давай может сделаем users и contacts(name, email, user_id) отдельно ? :)
Да, будут повторения имейлов, но как-то это все прозрачней обслуживается.

Я бы уже давно контакты, а так застрял сильно.

@mpakus
Copy link
Contributor

mpakus commented Oct 4, 2016

Я внес корректировки, погляди на модели User - Membership, тестики тоже добавил.
Модель Contact удалил.

Суть что на самом деле User может являться как и subscriber - лицо подписавшим кого-то, так и subscribed - тем кого подписали (following-follower).

Вот у Hartla можно подробнее почитать https://www.railstutorial.org/book/following_users

@mpakus mpakus removed the wontfix label Oct 18, 2016
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.

2 participants