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

担当者が決まってない提出物(未アサイン)の数をテキストで表示する #4136

Merged
merged 6 commits into from
Feb 15, 2022

Conversation

kaisumi
Copy link
Contributor

@kaisumi kaisumi commented Feb 4, 2022

Issue: #3897

概要

すでに未確認の提出物のうち提出日から5日、6日、7日以上が経過した提出物の数をDiscordのメンターチャンネルに投稿するためのAPIがある。今回はその対象を未アサインかつ未確認の提出物としたAPIを実装した。
image

注意点

  • .txtで表示させるために:
    • ルーティングをresourcesではなくresourceにしています。
    • コントローラーをunassignedとは分けています。
  • テストでpassedと異なる結果であることを確認するために、テストフィクスチャの提出物に担当メンターをつけ「アサイン済み・未確認」がカウントされることを確認しています。

@kaisumi kaisumi changed the title テキストで未アサイン件数を表示する機能を実装、テストを追加 担当者が決まってない提出物(未アサイン)の数をテキストで表示する Feb 4, 2022
@kaisumi kaisumi force-pushed the feature/notify-products-to-be-assigned branch from 7932e26 to 425a72c Compare February 4, 2022 06:06
@kaisumi
Copy link
Contributor Author

kaisumi commented Feb 4, 2022

@AudioStakes お疲れ様です!こちらお手すきの際にレビューをお願いいたします 🙇

@kaisumi kaisumi marked this pull request as ready for review February 4, 2022 06:30
@kaisumi kaisumi requested a review from AudioStakes February 4, 2022 06:30
@AudioStakes
Copy link
Contributor

@kaisumi
レビュー依頼ありがとうございますー😄
こちらのレビュー、来週の月曜以降でも問題ありませんでしょうか?

@kaisumi
Copy link
Contributor Author

kaisumi commented Feb 4, 2022

@AudioStakes 大丈夫です!よろしくお願いしますー 😄

Copy link
Contributor

@AudioStakes AudioStakes left a comment

Choose a reason for hiding this comment

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

@kaisumi
ひとつだけコメントしました〜(途中なので、対応してもらわなくて大丈夫です。)
残りは来週月曜以降にレビューさせてください🙏

Comment on lines 5 to 13
@products = Product
.unassigned
.unchecked
.not_wip
.list
.order_for_not_wip_list
@passed5 = @products.count { |product| product.elapsed_days == 5 }
@passed6 = @products.count { |product| product.elapsed_days == 6 }
@over7 = @products.count { |product| product.elapsed_days >= 7 }
Copy link
Contributor

@AudioStakes AudioStakes Feb 4, 2022

Choose a reason for hiding this comment

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

ここでインスタンス変数にしている @products は、app/views/api/products/unassigned_text/show.text.erb で使われていないため、インスタンス変数にしなくても大丈夫そうですー

(追記)以下の修正案よりも、 Product モデルにメソッドを作るという修正案の方がより良いと思ったため、こちらの案は取り下げます。

※ 以降については、好みが分かれると思いますので修正は任意です

app/views/api/products/unassigned_text/show.text.erb へ渡したいデータは、 elapsed_days が 5, 6, もしくは 7以上の個数(カウント数)のみですね。
それらを次のように、一つの変数にまとめることができるのかなと思いました。どうでしょう?(かえって読みにくいかもしれません😅)

$ rails c
>> elapsed_days_hash = Product.unassigned.unchecked.not_wip.list.map(&:elapsed_days).map { |days| days >= 7 ? 'over7' : days }.tally
=> {1=>1, 5=>1, 6=>1, "over7"=>8, 0=>46}
>> elapsed_days_hash[5]
=> 1
>> elapsed_days_hash[6]
=> 1
>> elapsed_days_hash['over7']
=> 8

※ 変数名は例示用です。より適切な名前がありそうです。

Copy link
Contributor

@AudioStakes AudioStakes Feb 8, 2022

Choose a reason for hiding this comment

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

こちらについて以前コメントした内容について、 Product モデルにメソッドを作る方が良いと考え直しましたので、取り下げさせてください🙏

@AudioStakes
Copy link
Contributor

AudioStakes commented Feb 8, 2022

@kaisumi
気になることがありましたので質問させてくださいー🙏

この PR のスコープ外だと思うものの、対処した方が良いのかもしれない(対処しなくても良いのかどうか僕がわかっていない)ことが下記2点あります。
これらについて、「対処した方が良いかどうか」「対処した方が良い場合は、対処後にこの PR の実装内容が変わるかどうか」教えてもらいたいですー
※ いずれにしてもこの PR の実装内容は変わらないようでしたら、引き続きレビューを進めさせてもらいたいと思います👍

提出後の経過日数として扱われる Product#elapsed_days の返り値は、product が WIP の場合は作成後の経過日数を示している

Product#elapsed_days は次のような処理になっています

def elapsed_days
t = published_at || created_at
((Time.current - t) / 1.day).to_i
end

この計算式は、WIP (すなわち published_atnil )の product は created_at を基準に経過日数を返しています。たとえば、WIP のまま7日経過すると返り値は「7」です。これは「提出後7日経過」を示しているように見える(直感に反しているように思える)ため、WIP の場合は経過日数を返さない( nil-1 を返す?)などの修正をした方が良いかもしれない、と考えています。

一度提出した後に WIP へ戻し、その後に再び提出した場合「提出後の経過日数」は最初に提出した日時を起点に計算されている

次のとおり、提出した後に6日超えた提出物を WIP に戻し、その後に再提出しても6日超えたままです。

Screen.Recording.2022-02-08.at.10.06.04.mov

この場合、次のような予期しない事象が生じるかもしれません。

  1. (受講生が)操作を誤って提出してしまった
  2. (受講生が)WIP に戻した
  3. (受講生が)その7日後、完成した提出物を提出した(WIP ではない状態にした)
  4. (メンターが)メンター向けの提出物一覧画面を見ると、期限の7日を超えた提出物がいきなり現れた

@kaisumi
Copy link
Contributor Author

kaisumi commented Feb 8, 2022

@AudioStakes コメントありがとうございます 😄
おっしゃる通りですね!結論からお答えします。
「対処した方が良いかどうか」→対処すべきと思います。
「対処した方が良い場合は、対処後にこの PR の実装内容が変わるかどうか」→変わらないと思います。
他の機能に影響する部分なので別にIssueを立てた方がいいかと思いました。
私の方でIssueを立ててもいいでしょうか?

  1. 以下の部分でWIPが考慮されていない件について→ご指摘のメソッド内で修正できると思います。
    def elapsed_days
    t = published_at || created_at
    ((Time.current - t) / 1.day).to_i
    end
  2. 提出→WIP→提出、でpublished_atが更新されない件について→
    提出物については以下でpublished_atを入力しているようですが、提出後にWIPに戻して再提出した場合は考慮されていないようですね。&& product.published_at.nil?を削除したら解消しそうです。
    def after_save(product)
    if !product.wip? && product.published_at.nil?
    notify_to_watching_mentor(product)
    if product.user.trainee?
    send_notification(
    product: product,
    receivers: product.user.company.advisers,
    message: "#{product.user.login_name}さんが#{product.title}を提出しました。"
    )
    create_watch(
    watchers: product.user.company.advisers,
    watchable: product
    )
    end
    product.published_at = Time.current
    product.save
    product.change_learning_status(:submitted)
    end
    Cache.delete_unchecked_product_count
    end

@AudioStakes
Copy link
Contributor

@kaisumi
ご確認ありがとうございます!

「対処した方が良い場合は、対処後にこの PR の実装内容が変わるかどうか」→変わらないと思います。

なるほど、わかりました👍
それでは、引き続きレビューを進めさせてもらいたいと思いますー

私の方でIssueを立ててもいいでしょうか?

はい、もちろんです!
お手数おかけしますが、立ててもらえますととてもありがたいです🙏

&& product.published_at.nil?を削除したら解消しそうです。

たしかに、そのように対応すると published_at の値をリセットできますね!
ただ、懸念事項としまして「軽微な修正をするつもりが、誤って WIP に戻してしまった(経過日数が0日にリセットされてしまった)」というケースもありそうです。
僕自身もどのような挙動が理想的なのかわかっていないため、Issue では「再提出時に published_at の値をリセットして良いのかどうか?」も含めて期待される挙動を整理することから始められるようにすると良いかもしれないですねー。

@kaisumi
Copy link
Contributor Author

kaisumi commented Feb 8, 2022

@AudioStakes
コメントありがとうございます!

懸念事項としまして「軽微な修正をするつもりが、誤って WIP に戻してしまった(経過日数が0日にリセットされてしまった)」というケースもありそうです。
僕自身もどのような挙動が理想的なのかわかっていないため、Issue では「再提出時に published_at の値をリセットして良いのかどうか?」も含めて期待される挙動を整理することから始められるようにすると良いかもしれないですねー。

おっしゃる通りですね。それではIssueを立てて議論できる場を作りたいと思います。
レビューを引き続きよろしくお願いします 🙇

@kaisumi
Copy link
Contributor Author

kaisumi commented Feb 8, 2022

@AudioStakes すみません先程の1.の件ですが、以下の部分で既に非WIPに絞っているので、ご指摘の箇所ではWIPの考慮は不要そうでした 👀

@products = Product
.unassigned
.unchecked
.not_wip
.list
.order_for_not_wip_list

@AudioStakes
Copy link
Contributor

AudioStakes commented Feb 8, 2022

@AudioStakes すみません先程の1.の件ですが、以下の部分で既に非WIPに絞っているので、ご指摘の箇所ではWIPの考慮は不要そうでした 👀

たしかに、現状は非 WIP に絞っているため Product#elapsed_daysを修正しなくても大丈夫そうです。
ただ、 Product#elapsed_days で知りたいこと(返してほしい値)は提出後の経過日数だと思いますので、クエリ条件の絞り込みとは別に Product#elapsed_days が WIP を考慮できるように修正した方が良さそうに思います。
その修正をすることで、Product#elapsed_days を使う側は product が WIP かどうかを気にしなくて済むようになる(クエリ条件で絞り込み忘れていても大丈夫)という利点があると考えています。どうでしょうか?

Copy link
Contributor

@AudioStakes AudioStakes left a comment

Choose a reason for hiding this comment

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

@kaisumi
変更内容を確認しました〜!
すでに動いている他の実装を参考にされたようで、基本的に要件が満たされているのかなと思いました👍
ただいくつか教えてほしいことやより良くできそうな箇所がありましたので、それらをコメントしました。ご確認お願いいたしますー🙏

@@ -0,0 +1,15 @@
# frozen_string_literal: true

class API::Products::UnassignedTextController < API::BaseController
Copy link
Contributor

@AudioStakes AudioStakes Feb 5, 2022

Choose a reason for hiding this comment

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

新たなコントローラーを作成していますね。
他の方法として、既存の API::Products::UnassignedController に新たなアクションを追加するという方法でも対応できそうに思いました。そのような対応にすることで、 Unassigned に関係するアクションを一つのコントローラーにまとめることができて良さそうです。

PR の Description にある「.txtで表示させるために...コントローラーをunassignedとは分けています。」が新しいコントローラーを作成した(新たなアクションでは対応できない)理由でしょうか?
僕が何かわかっていないことがありそうな気がするため、教えてもらいたいですー🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

私も当初unassigned_controllerに統一しようとしていました。しかしそうするとindexの方もindex.txtを探してしまうようでエラーとなったので、コントローラーを分けてあります。

Copy link
Contributor

Choose a reason for hiding this comment

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

routes の方でコメントしました collection を使う方法でアクションを追加をしますと、そのアクション名にしたがって探しにいく(たとえば count というアクション名であれば、count.txt を探しにいく)ようになりそうですー

Copy link
Contributor Author

Choose a reason for hiding this comment

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

コントローラー内でrender 'text.txt'(そしてindexの方にrender 'index.json')を呼び出すことでコントローラーを統合できました!

.unassigned
.unchecked
.not_wip
.list
Copy link
Contributor

@AudioStakes AudioStakes Feb 5, 2022

Choose a reason for hiding this comment

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

list は目的の product (提出後の経過日数が5日以上の product)を取得する条件には影響しないクエリ条件のように見えますー

scope :list, lambda {
with_avatar
.preload(:practice,
:comments,
{ user: :company },
{ checks: { user: { avatar_attachment: :blob } } })
}

この条件なしでは目的の product を取得できないようでしたら、教えてほしいです🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

数のカウントのみなので、listは不要でした!ご指摘ありがとうございます 😄

.unchecked
.not_wip
.list
.order_for_not_wip_list
Copy link
Contributor

@AudioStakes AudioStakes Feb 5, 2022

Choose a reason for hiding this comment

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

order_for_not_wip_list は目的の product (提出後の経過日数が5日以上の product)を取得する条件には影響しないクエリ条件のように見えますー

scope :order_for_not_wip_list, -> { order(published_at: :desc, id: :desc) }

この条件なしでは目的の product を取得できないようでしたら、教えてほしいです🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

数のカウントのみなので、こちらも不要でした!ご指摘ありがとうございます 😄

<% end %>

<% else %>
提出されたから5日以上経過している提出物はありません。
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
提出されたから5日以上経過している提出物はありません
提出されてから5日以上経過している提出物はありません

タイポのようですー

Copy link
Contributor Author

Choose a reason for hiding this comment

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

失礼しました 👀 修正しますー

Comment on lines 11 to 13
@passed5 = @products.count { |product| product.elapsed_days == 5 }
@passed6 = @products.count { |product| product.elapsed_days == 6 }
@over7 = @products.count { |product| product.elapsed_days >= 7 }
Copy link
Contributor

@AudioStakes AudioStakes Feb 8, 2022

Choose a reason for hiding this comment

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

※こちらの修正は任意でお願いします

この処理と似た処理が、他のコントローラーでも使われていますね。

@passed5 = products.count { |product| product.elapsed_days == 5 }
@passed6 = products.count { |product| product.elapsed_days == 6 }
@over7 = products.count { |product| product.elapsed_days >= 7 }

Product モデルに次のようなメソッドを作り、コントローラーではそのメソッドを呼び出すようにすると良いのかなと思いました。

  • 引数に products を受け取る
  • 返り値として、経過日数が5日, 6日, 7日以上それぞれの個数をカウントした次のハッシュを返す
{'passed5': 1, 'passed6': 2, 'over7': 3}

passed5over7 は、単体で読むと何を示しているか想像できないということもありそうなので、「経過日数が5日」「経過日数が7日以上」ということをより具体的に表す名前にしても良いかもしれません。
※ もしこの修正をする場合、その作業量が大きいようでしたら新たな Issue として切り出し、そちらで対応した方が良いかもしれません。

Copy link
Contributor

@AudioStakes AudioStakes Feb 8, 2022

Choose a reason for hiding this comment

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

この処理と似たような処理が、いくつかのコントローラーで繰り返し使われていますね。

例として挙げたうちの以下2つは「経過日数x日のうちの最新の1件を取得」しているようで、個数はカウントしていませんでした(処理が異なっていました)。失礼しました🙇‍♂️

@latest_product_submitted_just_5days = @products.find { |product| product.elapsed_days == 5 }
@latest_product_submitted_just_6days = @products.find { |product| product.elapsed_days == 6 }
@latest_product_submitted_over_7days = @products.find { |product| product.elapsed_days >= 7 }

@latest_product_submitted_just_5days = @products.find { |product| product.elapsed_days == 5 }
@latest_product_submitted_just_6days = @products.find { |product| product.elapsed_days == 6 }
@latest_product_submitted_over_7days = @products.find { |product| product.elapsed_days >= 7 }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

コメントありがとうございます 😄
passedの方は、unassigned_textが動くようになったら削除することが検討されているので、現状では統合しないでおこうと思います 👍

Comment on lines 5 to 13
@products = Product
.unassigned
.unchecked
.not_wip
.list
.order_for_not_wip_list
@passed5 = @products.count { |product| product.elapsed_days == 5 }
@passed6 = @products.count { |product| product.elapsed_days == 6 }
@over7 = @products.count { |product| product.elapsed_days >= 7 }
Copy link
Contributor

@AudioStakes AudioStakes Feb 8, 2022

Choose a reason for hiding this comment

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

こちらについて以前コメントした内容について、 Product モデルにメソッドを作る方が良いと考え直しましたので、取り下げさせてください🙏

Comment on lines 5 to 8
@products = Product
.unassigned
.unchecked
.not_wip
Copy link
Contributor

Choose a reason for hiding this comment

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

※修正は任意でお願いします

目的の product (提出後の経過日数が5日以上の product)以外の product を除くため、クエリ条件に「提出してから5日以上経過した」という条件も追加できたら良さそうだなーと思いました。
Product モデルに「提出してからx日以上経過した」というクエリ条件のスコープを作ってみても良いかもしれません。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

いいですね!修正してみようと思います 😄

@@ -54,6 +54,7 @@
namespace :products do
resources :unchecked, only: %i(index)
resources :unassigned, only: %i(index)
resource :unassigned_text, only: %i(show), controller: 'unassigned_text'
Copy link
Contributor

Choose a reason for hiding this comment

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

コントローラーの方へコメントした内容と同様に、こちらも新たな resource を追加せず unassigned に新たなアクションを追加した方が良いのかな、と考えていますー

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unassignedに新たなアクションを追加すると生成されるルートが/unassigned/:idとなり、パラメータ不足でエラーになってしまうため、resourceで記載を分ける必要がありました。

Copy link
Contributor

@AudioStakes AudioStakes Feb 10, 2022

Choose a reason for hiding this comment

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

なるほど、試行錯誤された上での判断だったのですね!説明ありがとうございます。

生成されるルートが/unassigned/:idとなり、パラメータ不足でエラーになってしまう

Rails ガイドの コレクションルーティングを追加する を参考に次のようにしますと、 :id のパラメーターなしで新たなアクションを追加できるようですー
期待されている振る舞いと何か違うところがありましたら、教えてください🙏

      resources :unassigned, only: %i(index) do
        get 'count', on: :collection
      end
$ rails routes | grep api_products_unassigned
     count_api_products_unassigned_index GET    /api/products/unassigned/count(.:format)                                                          api/products/unassigned#count
           api_products_unassigned_index GET    /api/products/unassigned(.:format)                                                                api/products/unassigned#index
            api_products_unassigned_text GET    /api/products/unassigned_text(.:format)                                                           api/products/unassigned_text#show

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ありがとうございます!:idなしでルーティングできました 😄

@kaisumi
Copy link
Contributor Author

kaisumi commented Feb 8, 2022

@AudioStakes コメントありがとうございます 😄

たしかに、現状は非 WIP に絞っているため Product#elapsed_daysをの修正しなくても大丈夫そうです。
ただ、 Product#elapsed_days で知りたいこと(返してほしい値)は提出後の経過日数だと思いますので、クエリ条件の絞り込みとは別に Product#elapsed_days が WIP を考慮できるように修正した方が良さそうに思います。
その修正をすることで、Product#elapsed_days を使う側は product が WIP かどうかを気にしなくて済むようになる(クエリ条件で絞り込み忘れていても大丈夫)という利点があると考えています。どうでしょうか?

おっしゃる通り、WIPかどうか迷った時点で改善の余地はありますね。機能の不具合にはなっていないのでバグではなく新機能でコードを整理する必要があるかもしれません。

レビューありがとうございました!
コメントいただいた内容への対応は明日以降になりそうです。修正が完了したらまたよろしくお願いします 🙏

@kaisumi kaisumi force-pushed the feature/notify-products-to-be-assigned branch 2 times, most recently from 0c42ce0 to 8349d3f Compare February 10, 2022 02:09
@kaisumi
Copy link
Contributor Author

kaisumi commented Feb 10, 2022

@AudioStakes 修正終わりましたので再レビューよろしくお願いします 😄

@kaisumi kaisumi requested a review from AudioStakes February 10, 2022 02:37
Copy link
Contributor

@AudioStakes AudioStakes left a comment

Choose a reason for hiding this comment

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

修正ありがとうございますー!OKです👍

@passed5 = products.count { |product| product.elapsed_days == 5 }
@passed6 = products.count { |product| product.elapsed_days == 6 }
@over7 = products.count { |product| product.elapsed_days >= 7 }
render 'counts.txt'
Copy link
Contributor

Choose a reason for hiding this comment

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

以下、調べてわかったことをお伝えします。現状でも動いてますので、修正不要です!

View ファイルを counts.txt.erb ではなく counts.text.erb とすることで、この render の指定はなくても動くようでしたー

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AudioStakes レビューありがとうございました 😄
counts.textで動くのですね!ありがとうございます 🎉
renderの記述は違和感があるので修正しようと思います。

@kaisumi kaisumi force-pushed the feature/notify-products-to-be-assigned branch from 30bed2f to bb8245b Compare February 10, 2022 07:27
@kaisumi kaisumi requested a review from komagata February 10, 2022 11:58
@kaisumi
Copy link
Contributor Author

kaisumi commented Feb 10, 2022

@komagata お疲れ様です!こちらお手すきの時にレビューをお願いいたします 🙇

Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

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

確認しました、OKですー🙆‍♂️

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.

3 participants