> []# おひさまログのポートフォリオレビュー会

## githubのリポジトリ
https://github.com/taik0527/ohisama_app

## サイトのURL
URL：https://onagawaohisama.herokuapp.com
ゲストユーザー
email: [e-mail]
password: password

## だいそん
- rspec-railsはtestだけで良さそう
- https://github.com/taik0527/ohisama_app/blob/0c05be5340fdcdc9489404af435145fc73549b61/Gemfile#L49
- 他にもpry-byebugやpry-railsはdevだけで良さそう
- ゲストメールアドレスが「[e-mail]」は良くない。実在しうるドメインなので。`example.com`などを使うべし。← 調べてみてください
- recordsというテーブル名がちょっと抽象的でわかりづらい
- https://github.com/taik0527/ohisama_app/blob/0c05be5340fdcdc9489404af435145fc73549b61/db/schema.rb#L87
- emailにユニーク制約がなくて大丈夫？
- https://github.com/taik0527/ohisama_app/blob/0c05be5340fdcdc9489404af435145fc73549b61/db/schema.rb#L106
- 蔵書の新規登録使いやすいですね！
- 活動記録投稿失敗時に「Bodyを入力してください」のように翻訳ができてなかった。rails-18n入れてja.ymlもちゃんと作っといた方が良い
- 蔵書について毎回検索して入れるのが面倒なのでGoogleBookAPIのbook_idのcsvか何かをアップロードしたらバルクインサートできるような機能があったらよさそう

## その他の方の名前

## みけた

### 雑感

- APIもうまく駆使してここまで作ったのは凄いです！
- 見た目が整っていて、placeholderも丁寧なので、ユーザー目線で作られたアプリだなと思いました
- 活動投稿機能にて、本を検索して画像付きで投稿できるっていうのは、特にいいですよね！
- あと、あくまで信頼関係が成立しているコミュニティで仕様する想定だと思うので不要なのでしょうが（むしろ今回の場合、そこをしっかりとしないことで利便性は向上させている）、権限管理についてもきちんとしたい場合、対策をしていく必要があるなと感じました！

### records_controllerのupdateアクション

updateアクションについては、修正しがいがあるので、みけたなりの案を出してみます笑

まず該当のコードを確認します。

def update
  @record = Record.find(params[:record_id])
  user_ids = params[:user_ids]
  userrecords = UserRecord.where(record_id: params[:record_id])
  if @record.update(record_params)
    userrecords.each(&:destroy)
    user_ids.each do |user_id|
      user = User.find(user_id)
      @record.users << user
    end
    redirect_to record_path, notice: '記録を更新しました'
  else
    @search_form = SearchBooksForm.new
    flash.now[:danger] = '更新できません'
    render :edit
  end
end

こんな感じにすればよいのではないかと思いました。
（動作確認をしていないので、動かなかったらごめんなさい）

def update
  @record = Record.find(params[:record_id])
  if @record.update(record_params)
    @users = User.find(params[:user_ids])
    @record.users = @users
    redirect_to record_path, notice: '記録を更新しました'
  else
    @search_form = SearchBooksForm.new
    flash.now[:danger] = '更新できません'
    render :edit
  end
end

これは、単純にメソッド自体の理解の問題ですが、以下を把握していると、より綺麗に書けたはずです。

- findメソッド
- 引数に配列の形式で複数のidを指定してもよい
- association
- `<<`というメソッドだけではなく、`=`という置き換えるメソッドがある
- https://railsguides.jp/association_basics.html#has-many%E3%81%A7%E8%BF%BD%E5%8A%A0%E3%81%95%E3%82%8C%E3%82%8B%E3%83%A1%E3%82%BD%E3%83%83%E3%83%89-collection-objects

また脆弱性については、特に仕事をする際に注意しなければならない点ですが、
**クライアント側から送信するパラメータは、好き勝手に改ざんすることができます。**

recordのidはどこから改ざんすることができるでしょうか？


ここを勝手に修正されてしまえば、ユーザーに紐付かない活動記録であっても、改ざんされてしまいます。じゃあ、どうすればいいのか？

Railsだと、デフォルトの状態の場合、user_idをハッシュ化したものをcookieとして保存する仕組みが備わっています。この機能をうまく活用してabeさんが実装したのが、ログイン状態の維持をする以下のコードです。

def current_user
  @current_user　 ||= User.find_by(id: session[:user_id]) if session[:user_id]
end

クライアントは、通信するたびにクッキーの情報も送ってきています。

もしログインが既に済んでいる場合、クッキーの中に`user_id=1`に対応するような`Ag87#$%$341#@`的な文字が保存されているはずなので、それをRailsが受け取り、「`user_id=1`の人が通信しているな」と判別します。

さて、前置きがなくなりましたが、ログインしているユーザーに紐付かないレコードだけ修正を許可したい場合どうすればいいのか。結論は、こうなります。(ん、けど、これはそもそも仕様的に、他の人も削除してOK?)

# Before
def update
  @record = Record.find(params[:record_id])
  if @record.update(record_params)
    @users = User.find(params[:user_ids])
    @record.users = @users
    redirect_to record_path, notice: '記録を更新しました'
  else
    @search_form = SearchBooksForm.new
    flash.now[:danger] = '更新できません'
    render :edit
  end
end

# After
def update
  @record = current_user.records.find(params[:record_id])
  if @record.update(record_params)
    @users = User.find(params[:user_ids])
    @record.users = @users
    redirect_to record_path, notice: '記録を更新しました'
  else
    @search_form = SearchBooksForm.new
    flash.now[:danger] = '更新できません'
    render :edit
  end
end

## RecordForm.rbのsaveメソッド

これも同様に修正できそう。

あと、おそらく色々な苦労があり、このモデルをupdateアクションで使うのはあきらめたのだと思われるが、うまく修正したらどうにか使うこともできそう（たぶん）。

## たいしろー

## 感想

ひとまず完成お疲れ様でした！完成させるのが最重要だと思うのですごいと思います！
ぱっとみたところ非同期の本検索すごいっすね！あと招待機能とかもついていて、自分はエンジニアになる前はここまでは作れなかったなと思いました！

てか本番運用してるんですね！すご！笑

### レビュー

- `test`ディレクトリはRspecを作るなら削除しても良いと思いました！
- `.vscode`は `.gitignore`に入れても良いかも？
- ruby3系を使ってるのはいいですね！自分は使った事ない。。
- `pry_byebug`と`pry-rails`はtest環境にも入れたら、便利です！
- バリデーションのテスト書いてるのいいですね！goodだと思います！レビュー会が終わった後は、モデルに書いてあるメソッドのユニットテストを書いたらいい勉強になると思います！実務で頭が追いつかない程、複雑なコードをいじる時はテストとレビューだけが心のよりどころになるので。。笑
- テストに関しては[この本](https://www.oreilly.co.jp/books/9784873118161/)がわかりやすいです。ユニットテスト、統合テスト、UIテストのカバーすべき役割や壊れやすさなど、書いてます。

## junnosuke

## 感想

当日に参加できそうにないので、初心者ですが感想だけでもと思い、記述させていただきたいと思います。

- ruby3系、rails6系とバーションが新しいところが設計、情報のキャッチ能力の高さを感じました。私のポートフォリオはruby2系、rails5系で作り始めてしまったのでこの時点で私の負けです。
- APIで検索機能が付いている点がすごいと思いました。
- 強いて言うなら、テストでsystemspecがあればもっと能力をアピールできるのかなと思いました。（コードを参考にして真似したいだけ笑）

ともに、TechEssentialsで頑張りましょう！

## みやいち
私も当日の参加が難しいので、ちょっとだけ思ったことを書かせていただきます。
### 感想
- まだ学習中の私としては、一つのアプリを最後まで作り上げられたTaikiさんすごい！と素直に尊敬です・・・
- アプリ作成の経緯が、お母様の困りごとを解決するためという、明確に誰かの問題を解決するものなのがとても良いなと思いました！お母様の感想などがあればお聞きしたいな〜と思いました。
### レビュー
コードの方はまだ全然みれないので、使用感についてです。なにか変なこといってたらすみません。
- アプリのUI、わかりやすくて使い勝手良さそうです。
- 活動記録の登録時、書籍を間違って選択したら削除できない
- 一度保存した後、編集画面から削除できるんですね。登録時に削除は処理的に難しい感じなんですかね・・・
- 活動記録の登録時、書籍を蔵書から選択できると便利かなと思いました。
- 複数の活動で同じ書籍を読んだ場合、毎回検索するのが面倒かなーと思いました。
- ただ、蔵書が増えてくるとその中から探して選ぶのも手間がかかりそうですね・・・どっちがいいんだろう

## todaka
- 当方初心者ですので、変なこと言ってたらビシバシご指摘ください笑
### 感想
- まず、サービスを完成させて本番環境で運用している事、「身近な人の課題を解決するサービス」を素直に具現化している事がすごいなと思いました！
- 他の方も書かれてますけど、お母様や他のユーザーの感想も聞いてみたいですね〜
### レビュー
- レイアウトはとても見やすい・操作しやすいと思いました。私はまだうまくできないので、すごいと思います。
- 活動の新規登録画面でバリデーションエラーで登録失敗した場合ですが、新規登録画面に遷移（戻る）時にGoogleBooksAPIの検索結果が消えてしまうんですね。他の日付や担当学級は入力内容が保持されているので、APIの検索結果も残れば良いなと思いました。（APIを使うため難しそうですが。。。）
- Booksコントローラの中で、以下のコードが時々出てくるので、切り出してモデルに書くと簡素化されて良いのかなと思いました。

@book.storage = true
@book.save

## naoki
### 感想
- 書籍を検索して表示することができるのすごいなと思いました。一回作ろうとしましたがその時は断念してしまっていたので自分も今度作って見たいなと思います。

### レビュー
- 自分の好みなのですが、　活動記録一覧の担当者が横並びでないのが気になりました。コロンをつけるならスクショの右側のようにしたほうがいいかなと思います。
[](https://gyazo.com/0d85bf6b05638326a5e8dc487c07cc5b)

- 画像投稿部分は投稿した画像が確認できるといいなと思いました。
- `book.rb`にて書き方は統一したほうがいいのかな？っと思いました自分が間違ってたらすみません

scope :search, lambda { |keyword|
    where('title like :q OR publisher like :q OR name like :q', q: "%#{keyword}%") if keyword.present?
  }
  scope :storage, -> { where(storage: true) }

scope :search, ->(keyword) { 
    where('title like :q OR publisher like :q OR name like :q', q: "%#{keyword}%") if keyword.present?
  }
  scope :storage, -> { where(storage: true) }

- rubocopの設定ファイル見当たらなかったのですがデフォルトで使ってるかんじなのでしょうか?

# その他
デザインはもうちょい勉強した方が良さげ。
[エンジニア向け デザイン基礎（社内勉強会の資料） \- Qiita](https://qiita.com/xrxoxcxox/items/01ae9d1515fdf794e1f5)