# 7/22 ポートフォリオレビュー会
### githubのリポジトリ

https://github.com/shizuka-yamamoto/maccho-app#readme

オンラインエディタで確認できるやつ
https://github1s.com/shizuka-yamamoto/maccho-app

### サイトのURL
URL：http://52.196.227.202/
ID：macho　PASS：5555

# だいそん
## サービス的側面
- ストイックな方なんだなぁというのが伝わってきた。筋トレ好きに受けそうなサービス。
- httpsになってない
- 結構入力項目が多いのでユーザーが継続して使ってくれるか怪しい
- 振り返りなど毎日書くのは自分は結構しんどいなぁと思っちゃう
- メモ書きに関しては保存する機能はない？この機能を用意した意図は？
- 誰がどういうモチベーションで他人の宣言にコメントしようと思うのか
- ヘッダーの「メニュー」や「ユーザー名部分」がドロップダウンということに気付けなかった
- 日付のフォーマットが気になった`19:46 2021/07/22`
- コメントするボタンが薄くて押せないのかと思った
- 他人のプロフィールが編集できる ( http://52.196.227.202/users/6/edit )
## 技術的側面
- 1日を振り返る、のURLがおかしい ( http://52.196.227.202/reviews/new.7 )
- https://github.com/shizuka-yamamoto/maccho-app/blob/078eb2a4894e66c9e6cf52dd52228bd8e22420b3/app/controllers/comments_controller.rb#L3

  def create
    comment = Comment.create(comment_prarms)
    redirect_to "/targets/#{comment.target.id}"
  end

`current_user.comments.create`のほうがRailsっぽい。
`redirect_to ~~~_url`のほうがRailsっぽい。
rubocop入れてない？
→ CI入れた方がいいかも

CicleCIのハンズオン
https://circleci.com/docs/ja/2.0/getting-started/?section=getting-started

- https://github.com/shizuka-yamamoto/maccho-app/blob/078eb2a4894e66c9e6cf52dd52228bd8e22420b3/app/controllers/reviews_controller.rb#L2

  def move_to_index
    redirect_to new_user_registration_path unless user_signed_in?
  end

親のコントローラに定義しておけばよさそう

- https://github.com/shizuka-yamamoto/maccho-app/blob/078eb2a4894e66c9e6cf52dd52228bd8e22420b3/app/models/user.rb#L11

`dependent: :destroy` とかは？

- target.rbというモデル名がわかりづらい
- https://github.com/shizuka-yamamoto/maccho-app/blob/078eb2a4894e66c9e6cf52dd52228bd8e22420b3/app/models/target.rb#L5

`max length`のバリデーションがあった方が安全

- https://github.com/shizuka-yamamoto/maccho-app/blob/078eb2a4894e66c9e6cf52dd52228bd8e22420b3/app/models/achievement.rb#L1

class Achievement < ActiveHash::Base
  self.data = [
    { id: 1, name: '--' },
    { id: 2, name: '達成できた' },
    { id: 3, name: '達成できなかった' },
    { id: 4, name: 'そもそも目標を立てていない' }
  ]
  include ActiveHash::Associations
  has_many :reviews
end

enum achivement: { ok: 1, ng: 2, none: 3 }

`enum_help`
`enumerize`

# Taiki Abe

### 使ってみて
- カレンダーに丸がつく形で見える化してあるところが良いと思った
- GitHubの草みたいな感じで継続する気持ちになれそう！

- 振り返りがどの目標に対する振り返りなのか指定できるとより良さそう。
- 特に目標を2つ以上作った日は、どの目標に対する振り返りなのかわかりづらいなと感じた
- Reviewsテーブルとtargetsテーブルを関連づけて、セットで表示できたらとても分かりやすいと思う

- 自分のコメントを削除か編集できるとより嬉しいと感じた
- 誤字った時のためにあった方がいいかなと思った

- メモ書き機能が直感的ではないかな？と感じた
- 説明書きを見たら使い方はわかったけど、自分はめんどくさがりなので紙用意するのめんどいなと思ってしまった笑
- ニッチな機能だから逆にそれがいいのかも！

# taichi

# 高島

# carolina
### 使ってみて
- 画像やデザインが凝っていて素敵だと思いました。
- コードや機能やシンプルだなと感じました。ユーザーが直感で使用しやすいように意識されているのかなと感じました。
[](https://gyazo.com/7372fb5fbe48bde66d8691c79d14bb14)
- アプリ自体シンプルなので、画像の導入部分ももう少し簡潔でも伝わるかと思います。ユーザーは「早く使ってみたい！」と思っているので、さらっと読んで、すぐアプリを使えるとより親切かな？と感じました。
- headerの「メニュー」プルダウンは、ログインしていない状態だと表示させなくても良いかな？と思いました。
- 投稿時や削除時にメッセージの表示がないので、フラッシュメッセージや確認のポップアップがあるとユーザーに親切だと思いました。

### コード
- app/testディレクトリと、app/specが共存している点。RSpecでテストを記述しているようなので、使用していないならtestディレクトリは必要ないのかな？と思いました。（間違っていたらすみません）
- 他と関連しているインスタンスの生成の場合は、慣習的に○○.newではなく○○.buildと記述した方が好ましいかと思いました。

app/controllers/comments_controller.rb

class CommentsController < ApplicationController
  def create
    comment = Comment.create(comment_prarms)
    redirect_to "/targets/#{comment.target.id}"
  end

  private

  def comment_prarms
    params.require(:comment).permit(:text).merge(user_id: current_user.id, target_id: params[:target_id])
  end
end

- commentコントローラーのcomment_paramsがcomment_prarmsになってる？（たいぽ？）

# まっつん

# K tomoya
### 使ってみて
- UIのリンク(ユーザー名やメニュー)が背景色に溶け込んでいてわかりにくいように感じました。（実際私はマイページの存在に最初気づけませんでしたw）皆がREADMEを読んでから使うのであれば良いのですがいきなり使い出すような人も一定数いると思いましたので！
- 最近投稿された一覧がトップメニューで見られるのは嬉しいのですがせっかくコメント機能が付いているので投稿に対してコメントが付いていることを一目で分かるようにしたほうが良いかと思いました。（コメント数を表示する等)
- 目標とそれに対応する振り返りが同時に見える方が良いと感じました。目標一覧タブと振り返りタブを分けないor分けても良いけど左右に並べて対応がわかる方がいいのかなぁと思います。
- 瞑想中のタイマーなので(目を瞑っていると予想できるので)５分経過を分かるような音が出るとかは難しいですかね？
- メモ書き機能が機能ではないように感じました。この内容ならばメモ書き紹介程度で良さそうに思います。
- 目標立てやすくさせるために瞑想タイマーを設置して誘導しているのは良いなと思いました！目標を書くページがシンプルに設計されており個人的には好きです。
- できれば目標、振り返りの例がある方がターゲットとする人にとっては目標なども立てやすくなりよりサービスの本質に根ざしたものになるように感じました。

# みけた

## Comments Controller


class CommentsController < ApplicationController
  def create
    comment = Comment.create(comment_prarms)
    redirect_to "/targets/#{comment.target.id}"
  end

  private

  def comment_prarms
    params.require(:comment).permit(:text).merge(user_id: current_user.id, target_id: params[:target_id])
  end
end

ここは好みにもよる部分もありますが、Rails慣れている人はこう書くことが多いかと思います。
書き方間違えていたらごめんなさい。

class CommentsController < ApplicationController
  def create
    comment = current_user.comments.create(comment_params)
    redirect_to target_comments_path(comment.target)
  end

  private

  def comment_params
    params.require(:comment).permit(:text).merge(target_id: params[:target_id])
  end
end

## users_controller.rb

これは書き換えられちゃうので、他人のやつを変更できちゃうので要注意。

class UsersController < ApplicationController
  before_action :set_user, only: [:edit, :update, :show]
  before_action :authenticate_user!, except: :index

  def edit
  end

  def　update
    if @user.update(user_params)
      redirect_to action: :show
    else
      render :edit
    end
  end

  def show
    @targets = @user.targets.order('created_at DESC')
    @reviews = @user.reviews.order('created_at DESC')
  end

  private

  def user_params
    params.require(:user).permit(:nickname, :email, :image)
  end

  def set_user
    @user = User.find(params[:id])
  end
end

こんな感じがよさそう。あまり考えずに書いているので、間違え等があったらすみません。

class UsersController < ApplicationController
  before_action :set_user, only: [:edit, :update, :show]
  before_action :authenticate_user!, except: :index

  def edit
    # erb側でcurrent_userと書いているのであれば不要
    @user = current_user
  end

  def update
    # @user = User.find(current_user.id)
    # if @use.update(user_params)
    if current_user.update(user_params)
      redirect_to action: :show
    else
      render :edit
    end
  end

  def show
    @targets = current_user.targets.order('created_at DESC')
    @reviews = current_user.reviews.order('created_at DESC')
  end

  private

  def user_params
    params.require(:user).permit(:nickname, :email, :image)
  end

  # def set_user
  #  @user = User.find(params[:id])
  # end
end

## review_spec.rb

class Review < ApplicationRecord
  extend ActiveHash::Associations::ActiveRecordExtensions
  belongs_to :achievement
  belongs_to :user

  validates :text, presence: true
  validates :achievement_id, numericality: { other_than: 1 }
end

モデルテストで確認すべきは、
- textが空であればエラーとなるか
- 逆にからでなければ保存されるか
- achievement_idが1であればエラーとなるか
- achievement_idが1以外であれば保存できるか

テストすべき部分はカバーできていて、とてもいいと思います！

FactoryBot.define do
  factory :review do
    text { Faker::Lorem.sentence }
    achievement_id { 2 }
    association :user
  end
end

require 'rails_helper'

RSpec.describe Review, type: :model do
  before do
    @review = FactoryBot.build(:review)
  end

  describe '投稿の保存' do
    context '投稿が保存できる場合' do
      it 'テキストがありachievement_id1以外であれば投稿できる' do
        expect(@review).to be_valid
      end
    end
    it 'achievement_idがid:1以外であれば登録できる' do
      @review.achievement_id = 2
      expect(@review).to be_valid
    end

    context '投稿が保存できない場合' do
      it 'テキストが空では投稿できない' do
        @review.text = ''
        @review.valid?
        expect(@review.errors.full_messages).to include('Textを入力してください')
      end
      it 'achievement_idがid:1では登録できない' do
        @review.achievement_id = 1
        @review.valid?
        expect(@review.errors.full_messages).to include('Achievementは1以外の値にしてください')
      end
      it 'ユーザーが紐付いていなければ投稿できない' do
        @review.user = nil
        @review.valid?
        expect(@review.errors.full_messages).to include('Userを入力してください')
      end
    end
  end
end

こちらについては、context：条件を示す、it：期待する結果を書くということを意識して書いてあげると、可読性が高まるように思います。`context: '投稿が保存できる場合'`というのはあまり見ない分け方です。

require 'rails_helper'

RSpec.describe Review, type: :model do
  before do
    # letも覚えると、初学者感が薄れます！
    @review = FactoryBot.build(:review)
  end

  describe 'validates :text' do
    context '投稿が空の場合' do
      it 'エラーとなる' do
        @review.text = ''
        @review.valid?
        # full_messagesだと全てのエラーメッセージに含まれないか確認することになるので一応修正してみました
        expect(@review.errors[:text]).to include('Textを入力してください')
      end
      it '保存できる' do
        # FactoryでFakerを使っているので不要ですが、明示的に書いてあげるとテストの可読性が上がる
        @review.text = 'hogehoge'
        expect(@review).to be_valid
      end
    end
  end
  describe 'validates :achievement_id' do
    context 'achievement_idが１の場合' do
      it 'エラーとなる' do
        @review.achievement_id = 1
        @review.valid?
        # full_messagesだと全てのエラーメッセージに含まれないか確認することになるので一応修正してみました
        expect(@review.errors[:achievement_id]).to include('Achievementは1以外の値にしてください')
      end
      it '保存できる' do
        # achievement_idへの代入は不要ですが、明示的に書いてあげるとテストの可読性が上がる
        @review.achievement_id = 2
        expect(@review).to be_valid
      end
    end
  end
end

あと話が逸れますが、そもそも論として、achievement_idが1というのに意味を持たせるのであれば、enumというのを使えばいいように思いました。(ちょっとどういう意味があるのか分かっていませんが)

[RSpec スタイルガイド](https://github.com/willnet/rspec-style-guide)

[FactoryBot the Right Way / toshimaru](https://www.youtube.com/watch?v=n0epZM-lZvw&feature=youtu.be)

[Clean Test Code Revised \- Speaker Deck](https://speakerdeck.com/willnet/clean-test-code-revised)

[rspecを読みやすくメンテしやすく書くために](https://zenn.dev/yuji_developer/articles/52cc0e356b3748)

https://github.com/satour/rails-style-guide-jp