Part 6. Acceptance tests. #4

metacorn · 2019-06-18T23:07:50Z

No description provided.

…Refactor code.

… context in AnswersController spec.

…e feature 'user can create answer while being on question page'. Update AnswersController specs.

…date feature 'user can delete his questions but not another's'.

aishek · 2019-06-19T08:41:50Z

app/controllers/answers_controller.rb

+  end
+
+  def destroy
+    @answer = Answer.find(params[:id])


В этом методе нет вьюх, поэтому можно обойтись исключительно локальными переменными.

aishek · 2019-06-19T08:42:00Z

app/controllers/answers_controller.rb

+
+  def destroy
+    @answer = Answer.find(params[:id])
+    @question = @answer.question


А это зачем?

aishek · 2019-06-19T08:43:42Z

app/controllers/answers_controller.rb

+  def destroy
+    @answer = Answer.find(params[:id])
+    @question = @answer.question
+    if @answer.user == current_user


Во-первых, оформи это особым случаем.

Во-вторых, по коду часто встречается похожая проверка на авторство, оформи её методом.

aishek · 2019-06-19T08:44:24Z

app/controllers/questions_controller.rb

@@ -34,8 +37,10 @@ def update
  end

  def destroy
-    @question.destroy
-    redirect_to questions_path
+    if @question.user == current_user


Тут "особый случай" прямо просится.

Для изменения не нужна аналогичная проверка?

по логике -- да, по ТЗ -- пока нет) я думал над этим, но говорилось, что "нужно реализовывать только то, что было озвучено".

Да, моя ошибка — для изменения пока рано делать.

aishek · 2019-06-19T08:44:56Z

app/models/user.rb

@@ -0,0 +1,10 @@
+class User < ApplicationRecord
+  has_many :questions


Что должно произойти со связанными вопросами и ответами при удалении пользователя?

на мой, взгляж, ничего, просто вместо юзернейма должно стоять "ДЕЛЕТЕД ЮЗЕР". я не знаю, как именно это реализовано на стэковерфлоу, но по идее, вопросы и ответы -- это то, за чем приходят на ресурс. удаление юзеров -- это окей, удаление ключевого контента -- не очень. имхо.

Окей, главное что это мотивированное решение.

aishek · 2019-06-19T09:12:34Z

spec/features/question/show_spec.rb

+  scenario 'unauthenticated user sees a question and answers' do
+    visit question_path(question)
+
+    expect(page).to have_content "#{"a" * 50}"


Тут не очень понятно что за текст. Может expect(page).to have_content question.title?

aishek · 2019-06-19T09:13:15Z

spec/features/question/show_spec.rb

+    visit question_path(question)
+
+    expect(page).to have_content "#{"a" * 50}"
+    expect(page).to have_content "#{"b" * 50}", count: n


Вот опять не понятно, если использовать question.answers.first.title и так далее, то будет лучше.

aishek · 2019-06-19T09:14:45Z

spec/features/answer/create_spec.rb

+} do
+  given(:question) { create(:question) }
+
+  scenario 'unauthenticated user answers to a question' do


Не понял что за тест.

Не понял что за тест.

Исправил имя сценария (забыл поменять после того, как отвечать на вопросы стало возможно только авторизованным пользователям).

spec/features/answer/create_spec.rb

+      fill_in 'Body', with: "#{"body" * 25}"
+      click_on 'Leave'
+
+      expect(page).to have_content 'Your answer was saved.'


aishek · 2019-06-19T09:17:44Z

spec/features/question/destroy_spec.rb

+    visit question_path(question1)
+    click_on 'Delete the question'
+
+    expect(page).to have_content "Your question was successfully deleted!"


Ещё бы проверить что тест вопроса пропал.

Ещё бы проверить что тест вопроса пропал.

Лучше добавлю проверку на отсутствие вопроса в БД -- текст вопроса при большом их кол-ве и многостраничной структуре отображения будет отсутствовать на главной не только после удаления вопроса.

Идея приёмочных тестов в том, что они работают максимально близко "к пользователю". То есть как будто это тестировщик в ручном режиме запустил браузер и кликает. В базу тестировщик обычно не добирается (ибо лень). То есть суть именно в проверке внешних эффектов, в этом смысле база данных — просто инструмент для получения нужного результата (текста на странице).

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

Идея приёмочных тестов в том, что они работают максимально близко "к пользователю". То есть как будто это тестировщик в ручном режиме запустил браузер и кликает. В базу тестировщик обычно не добирается (ибо лень). То есть суть именно в проверке внешних эффектов, в этом смысле база данных — просто инструмент для получения нужного результата (текста на странице).

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

Окей, логику понял. Убрал запрос в БД, сделал проверку контента.

spec/features/answer/create_spec.rb

+    scenario 'answers to a question with errors' do
+      click_on 'Leave'
+
+      expect(page).to have_content 'Your answer was not saved.'


metacorn · 2019-06-19T16:37:50Z

spec/features/question/create_spec.rb

+    scenario 'asks a question with errors' do
+      click_on 'Ask'
+
+      expect(page).to have_content "Title can't be blank"


Сюда бы ещё вывод ошибки валидации.

Не очень понял. "Title can't be blank" - это ведь и есть одна из ошибок валидации?

Да, верно.

aishek · 2019-06-19T09:33:41Z

spec/features/question/destroy_spec.rb

+  scenario "user tries to delete another's question" do
+    visit question_path(question2)
+
+    expect(page).to_not have_content 'Delete the question'


Наличие ссылок можно также проверять с помощью have_link.

aishek · 2019-06-19T09:58:32Z

spec/features/question/index_spec.rb

+  scenario 'unauthenticated user sees a list of questions' do
+    visit questions_path
+
+    expect(page).to have_content "Title of the question #1"


Просится цикл.

…ibutes' 'creates answer by the name of logged user' test.

…y' 'tries to delete another user's answer' AnswerController tests.

… name of logged user' test.

…ionsController test.

…re tests of Answers.

…rs'.

aishek · 2019-06-19T19:15:46Z

spec/controllers/answers_controller_spec.rb

+      it 'creates answer by the name of logged user' do
+        post :create, params: { question_id: question.id, answer: attributes_for(:answer) }
+
+        expect(assigns(:current_user)).to eq Answer.last.user


Это не очень надёжная проверка: Answer.last может вернуть непонятно что. Ещё тут нарушается принцип "чёрного ящика" для контроллера.

Одним из возможных решений может быть поискать ответ в ответах вопроса по атрибутам (https://api.rubyonrails.org/classes/ActiveRecord/FinderMethods.html#method-i-find_by-21):

new_answer_params = attributes_for(:answer) post :create, params: { question_id: question.id, answer: new_answer_params } created_answer = question.answers.find_by! new_answer_params expect(created_answer.user).to eq user

aishek · 2019-06-19T19:19:05Z

spec/controllers/questions_controller_spec.rb

+      it 'creates a question by the name of logged user' do
+        post :create, params: { question: attributes_for(:question) }
+
+        expect(assigns(:current_user)).to eq question.reload.user


Тут две проблемы: нарушается принцип "чёрного ящика" для контроллера и усложняется модификация теста.

На усложнении модификации остановлюсь подробнее:

question.reload expect(assigns(:current_user)).to eq question.user

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

aishek · 2019-06-19T19:25:38Z

spec/features/answer/create_spec.rb

@@ -0,0 +1,61 @@
+require 'rails_helper'
+
+feature 'user can create answer while being on question page', %q{


Этот тест дублирует следующий, насколько я понял.

Этот тест дублирует следующий, насколько я понял.

Как я понимаю, не совсем -- это же две разные фичи как бы: 1) юзер может отвечать прямо на странице вопроса 2) юзер может отвечать только если он залогинен.

Я понимаю, что там много одинакового кода, но фич как бы две и они поступили не одновременно, потому и в тестах двойное их отображение.

Как обычно поступают в таких случаях? Просто добавляют тест на вторую фичу (как в моём случае)? Объединяют две фичи в один фиче-тест? Выносят общий код в общую "предфичевую" зону? в таком случае, есть ли метод оператор, который предоставляет такое общее для двух фич пространство для кода?

В задании вот что:

Пользователь, находясь на странице вопроса, может написать ответ на вопрос (т.е. форма нового ответа должна быть прямо на странице вопроса, без перехода на другую страницу)
...

Реализовать следующие истоиии:

Только аутентифицированный пользователь может создавать вопросы и ответы

При этом у нас есть противоречие — отвечают на вопрос только на странице вопроса и только в залогиненном состоянии. То есть если написать тест, то он одновременно проверит оба сценария из задания. И это верно.

Перечитаем второй сценарий:

Только аутентифицированный пользователь может создавать вопросы и ответы

Хорошо, а как быть неаутентифицированным? Это и будет второй тест.

Итого: первым тестом проверяем что только залогиненный может ответить на вопрос на странице вопроса, вторым тестом проверяем что незалогиненный не имеет возможности ответить на странице вопроса.

aishek · 2019-06-19T19:32:01Z

app/controllers/answers_controller.rb

    end
  end

+  def destroy
+    answer = Answer.find(params[:id])
+    return if non_owned?(answer)


Неплохо, но можно лучше.

Во-первых, не надо обратной логики, некоторым людям это взрывает мозг. return if non_owned?(answer) --> return unless owned?(answer) — и да, это единственный случай где уместен unless.

Во-вторых, хочется аналогичную проверку использовать и во вьюшках. Ну и минимум кода написать. А значит можно завести метод в пользователе, который будет проверять, является ли пользователь автором переданного объекта.

В-третьих, в самом методе хочется избежать дополнительного запроса в базу. Хоть это и микрооптимизация, и, возможно, преждевременная, но тут полезная.

Неплохо, но можно лучше.

Во-первых, не надо обратной логики, некоторым людям это взрывает мозг. return if non_owned?(answer) --> return unless owned?(answer) — и да, это единственный случай где уместен unless.

Во-вторых, хочется аналогичную проверку использовать и во вьюшках. Ну и минимум кода написать. А значит можно завести метод в пользователе, который будет проверять, является ли пользователь автором переданного объекта.

В-третьих, в самом методе хочется избежать дополнительного запроса в базу. Хоть это и микрооптимизация, и, возможно, преждевременная, но тут полезная.

Да, я про unless подумал в какой-то момент, но в тот момент правил что-то другое, а потом вернуться забыл)

Метод в модель перенёс, но насчёт избежать допзапроса в БД -- не понимаю, как реализовать. Проверка происходит в методе модели, объект проверки (вопрос, ответ и т.д.) передаётся в метод в качестве аргумента, до того в модели в инстанс-переменной этот объект закэширован не был -- как в модели избежать запроса на создателя объекта?
Вот этот запрос в логах:

User Load (0.4ms) SELECT "users".* FROM "users" WHERE "users"."id" = $1 LIMIT $2 [["id", 1], ["LIMIT", 1]] ↳ app/models/user.rb:12 (0.1ms) BEGIN ↳ app/controllers/answers_controller.rb:24 Answer Destroy (0.9ms) DELETE FROM "answers" WHERE "answers"."id" = $1 [["id", 3]] ↳ app/controllers/answers_controller.rb:24

Например вот так:

def author_of?(resource) resource.user_id == id end

Например вот так:

def author_of?(resource) resource.user_id == id end

У меня пока вот так:

def owned?(content) self == content.user end

Не очень понимаю. По user_id -- не будет запроса в базу, но по user -- будет? В чём логика? Модель предоставляет ассоциативные методы (в данном случае user), сам объект вопроса/ответа в памяти есть -- но при запросе айди запроса в базу нет, при запросе ассоциации -- запрос в базу есть? По ассоциации всегда запрос в базу, так? По объекту из памяти -- соответственно, нет? Просто обращение к памяти?

Например вот так:

def author_of?(resource) resource.user_id == id end

Да, стало на один запрос меньше) Прикольно, спасибо.

По resource.user_id запроса в базу не будет, потому что тип у этого поля — число. И когда загружен из базы например вопрос, то соответственно загружены все поля строки вопроса, то есть и наше число для resource.user_id.

По resource.user запрос будет, потому что такого поля нет, а ассоциация belongs_to в user есть.

aishek · 2019-06-19T19:36:14Z

spec/features/question/create_spec.rb

+    scenario 'asks a question with errors' do
+      click_on 'Ask'
+
+      expect(page).to have_content "Title can't be blank"


Да, верно.

aishek · 2019-06-19T19:49:05Z

spec/features/question/destroy_spec.rb

+    visit question_path(question1)
+    click_on 'Delete the question'
+
+    expect(page).to have_content "Your question was successfully deleted!"


Идея приёмочных тестов в том, что они работают максимально близко "к пользователю". То есть как будто это тестировщик в ручном режиме запустил браузер и кликает. В базу тестировщик обычно не добирается (ибо лень). То есть суть именно в проверке внешних эффектов, в этом смысле база данных — просто инструмент для получения нужного результата (текста на странице).

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

aishek · 2019-06-19T19:51:33Z

app/controllers/answers_controller.rb

    else
-      render :new
+      remember_answer_errors


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

А что если тут отрендерить страницу показа вопроса? Позволит ли это использовать стандартных механизм отображения ошибок?

…name of logged user' AnswerController test.

…the name of logged user' QuestionController test.

… and 'only authenticated user can create answers'.

vkurennov · 2019-06-20T17:56:51Z

app/controllers/answers_controller.rb

    end
  end

+  def destroy
+    answer = Answer.find(params[:id])
+    return unless current_user.owned?(answer)


тут лучше статуст какой-то вернуть (например head :forbidden), потому что экшн должен дать какой-то ответ на запрос, нельзя просто из метода выйти. иначе тут будет попытка отрендерить вид destroy, которого нет и будет ошибка.

vkurennov · 2019-06-20T17:57:01Z

app/controllers/questions_controller.rb

@@ -34,8 +37,9 @@ def update
  end

  def destroy
+    return unless current_user.owned?(@question)


тут тоже нужно вернуть статус

vkurennov · 2019-06-20T17:57:37Z

app/views/answers/_answer.html.slim

@@ -0,0 +1,2 @@
+p= answer.body
+p= render(partial: 'shared/manage_answer_links', locals: { answer: answer }) if answer.user == current_user


используй метод User#owned? везде, где нужна проверка на автора

vkurennov · 2019-06-20T17:57:47Z

app/views/questions/show.html.slim

@@ -0,0 +1,12 @@
+= render 'shared/errors', resource: @answer
+
+=render(partial: 'shared/manage_question_links', locals: { question: @question }) if @question.user == current_user


используй метод User#owned?

vkurennov · 2019-06-20T18:00:45Z

spec/models/user_spec.rb

@@ -0,0 +1,9 @@
+require 'rails_helper'
+
+RSpec.describe User, type: :model do


А где тесты модели на метод User#owned?? Любой код, который ты пишешь в каком-либо классе должен иметь соответсвующие тесты

aishek · 2019-06-21T07:03:08Z

spec/models/user_spec.rb

+    let(:question2) { create(:question, user: user2) }
+
+    it 'check if user is owner of resource created by himself' do
+      expect(user1.owned?(question1)).to be_truthy


А ещё лучше так: expect(user1).to be_owned(question1)

https://relishapp.com/rspec/rspec-expectations/docs/built-in-matchers/predicate-matchers

metacorn added 15 commits June 18, 2019 12:12

Update .ruby-version.

bba8df1

Add sign in functionality for Users.

1715cd3

Implement scenario 'authenticated user asks a question'.

0a31448

Implement scenario 'unauthenticated user tries to ask a question'.

3dfbbb5

Implement scenario 'authenticated user asks a question with errors'. …

f428438

…Refactor code.

Implement scenario 'visitor sees a list of questions'.

a371953

Fix 'renders new view' of 'POST #create' in 'with invalid attributes'…

c0b457b

… context in AnswersController spec.

Implement scenario 'unauthenticated user sees a question and answers'.

a3cb782

Add spec for signing up for users.

141c5fb

Update forms for Questions and Answers with 'local: true'

07f403b

Add spec for logging out for users.

9f9b442

Implement feature 'only authenticated user can create answers'. Updat…

2d85358

…e feature 'user can create answer while being on question page'. Update AnswersController specs.

Implement feature "user can delete his questions but not another's".

fd5871e

Implement feature 'user can delete his answers but not another's'. Up…

03d1917

…date feature 'user can delete his questions but not another's'.

Fix.

9323065

aishek suggested changes Jun 19, 2019

View reviewed changes

metacorn added 11 commits June 19, 2019 15:31

Add Controller#non_owned? method. Add 'POST #create' 'with valid attr…

104dbb3

…ibutes' 'creates answer by the name of logged user' test.

Update 'DELETE #destroy' 'deletes the answer' and add 'DELETE #destro…

fe50693

…y' 'tries to delete another user's answer' AnswerController tests.

Add 'POST #create' 'with valid attributes' 'creates a question by the…

03d850f

… name of logged user' test.

Add 'DELETE #destroy' 'tries to delete another user's question' Quest…

d1d25c6

…ionsController test.

Update scenario 'authenticated user' 'answers to a question' in featu…

a5ecf53

…re tests of Answers.

Update scenario 'user deletes his question'

7cf3797

Update scenario 'user deletes his answer'

6c3300f

Add answers validation error messages output to Question Show view.

25392ff

Update answers validation error messages output to Question Show view.

082d0be

Update scenario 'authenticated user' 'answers to a question with erro…

d79c4dc

…rs'.

Refactor scenario 'unauthenticated user sees a list of questions'.

d0ae76e

aishek suggested changes Jun 19, 2019

View reviewed changes

metacorn added 9 commits June 20, 2019 10:56

Update 'POST #create' 'with valid attributes' 'creates answer by the …

2e8873c

…name of logged user' AnswerController test.

Update 'POST #create' 'with valid attributes' 'creates answer by the …

1432f1f

…name of logged user' AnswerController test.

Update 'POST #create' 'with valid attributes' 'creates a question by …

f60c47e

…the name of logged user' QuestionController test.

Move ownery checking of content to User model.

2ae802b

Update User#owned method.

f3f823f

Update features 'user can create answer while being on question page'…

5aa3e1d

… and 'only authenticated user can create answers'.

Update AnswersController, views and tests.

6ab8e3e

Refactor views and templates.

bb394f4

Refactor controllers.

35b4fdf

vkurennov suggested changes Jun 20, 2019

View reviewed changes

Refactor code. Update User model spec.

f0449a5

aishek approved these changes Jun 21, 2019

View reviewed changes

vkurennov approved these changes Jun 21, 2019

View reviewed changes

Refactor User model spec.

8973a4f

metacorn merged commit d4bca0f into master Jun 21, 2019

metacorn changed the title ~~Part 5. Acceptance tests.~~ Part 6. Acceptance tests. Jul 5, 2019

		@@ -0,0 +1,10 @@
		class User < ApplicationRecord
		has_many :questions

		@@ -0,0 +1,61 @@
		require 'rails_helper'

		feature 'user can create answer while being on question page', %q{

		@@ -0,0 +1,2 @@
		p= answer.body
		p= render(partial: 'shared/manage_answer_links', locals: { answer: answer }) if answer.user == current_user

		@@ -0,0 +1,12 @@
		= render 'shared/errors', resource: @answer

		=render(partial: 'shared/manage_question_links', locals: { question: @question }) if @question.user == current_user

		@@ -0,0 +1,9 @@
		require 'rails_helper'

		RSpec.describe User, type: :model do

Part 6. Acceptance tests. #4

Part 6. Acceptance tests. #4

Conversation

metacorn commented Jun 18, 2019

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

This comment was marked as resolved.

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

This comment was marked as resolved.

This comment was marked as resolved.

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

metacorn Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacorn Jun 20, 2019 • 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

metacorn Jun 20, 2019 •

edited

Loading

metacorn Jun 20, 2019 •

edited

Loading