-
Notifications
You must be signed in to change notification settings - Fork 71
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
未アサインの提出物のアラートを変更した #7732
未アサインの提出物のアラートを変更した #7732
Conversation
f07252b
to
6c89a9e
Compare
1652791
to
d84842c
Compare
お疲れ様です! |
@nakamu-kazu222 お待たせしてすいません!!これから作業に入ります!! |
お手数をおかけしますが、よろしくお願いいたします! |
@nakamu-kazu222 お待たせしました🙇♂️デザインを入れました。文言の調整も加えました。ご確認お願いします🙏 |
ありがとうございます! デザインが綺麗で感動しています! |
4c32227
to
2c86179
Compare
お疲れ様です! |
b08e11f
to
4f7bfff
Compare
お疲れ様です🙏 何かご不明点やご指摘がございましたら、お知らせください |
44925f8
to
af7334b
Compare
@nakamu-kazu222 |
大丈夫です! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nakamu-kazu222
お疲れ様です!時間がかかってしまい申し訳ございません 💦
変更確認方法に従って動作に問題ないことは確認できました👍
変更確認方法について
環境構築
の2. メンターユーザーでログインする
の順番はサーバー実行の後かと思います~アラート変更の動作確認
の今日提出のアラートラベル
が56ではなく55になっています
コードレビューについて
いくつかコメントさせていただきました。
的外れなことを書いてしまっていることがあるかもしれませんので、その際は逆にご指摘いただけますとありがたいです 🙏
よろしくお願いいたします~
app/javascript/products.vue
Outdated
@@ -116,12 +154,12 @@ export default { | |||
isDashboard() { | |||
return location.pathname === '/' | |||
}, | |||
isNotProduct5daysElapsed() { | |||
isNotProductselectedDaysElapsed() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
キャメルケースの命名規則のため、isNotProductSelectedDaysElapsed
とした方が良さそうです。
| #{ProductDeadline.first_or_create(alert_day: 4).alert_day + 2}日以内にメンターがレビューしますので、次のプラクティスにお進みください。 | ||
br | ||
| もし、7日以上経ってもレビューされない場合は、メンターにお問い合わせください。 | ||
| もし、#{ProductDeadline.first_or_create(alert_day: 4).alert_day + 2}日以上経ってもレビューされない場合は、メンターにお問い合わせください。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProductDealine.first_or_create(alert_day: 4).alert_day + 2)
が複数回呼ばれているため、値を変数に格納するか、renderする際に引数として渡してもらうというのはいかがでしょうか?
app/javascript/products.vue
Outdated
span.card-header__count | ||
| ({{ countProductsGroupedBy(product_n_days_passed) }}) | ||
//- prettier-ignore: need space between v-else-if and id | ||
header.card-header.a-elapsed-days.is-reply-deadline( | ||
v-else-if='product_n_days_passed.elapsed_days === 7', id='7days-elapsed' | ||
v-else-if='product_n_days_passed.elapsed_days === selectedDays + 2', id='6days-elapsed' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id
が6days-elapsed
で固定になっているのに対し、実際はselectedDays
の値によって何日以上経過を表すかが変わるため、id
と実際に表示している日数が一致しないことがあると思います。
可能であればid
が実際に表示している日数と一致するようにした方が良いと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
以下の対応はいかがでしょうか?
- 修正前:6days-elapsed → 修正後:deadline-alert
- 修正前:5days-elapsed → 修正後:second-alert
- 修正前:4days-elapsed → 修正後:first-alert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
個人的にはid="#{selected_day + 2}-days-elapsed"
とすると直感的だと思います。
ただ、他の箇所でdeadline-alert
などを変数名としても使っており、ご提案いただいた命名の方が一貫性が出ますのでどちらにするかはお任せいたします!
@@ -2,5 +2,7 @@ | |||
|
|||
class Products::UnassignedController < ApplicationController | |||
before_action :require_staff_login | |||
def index; end | |||
def index | |||
@product_deadline_day = ProductDeadline.first_or_create(alert_day: 4).alert_day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProductDeadline.first_or_create(alert_day: 4).alert_day
の処理を複数の箇所で見かけるので、モデル側にメソッドを作ってDRYにするというのはいかがでしょうか?
# モデルの実装
def self.review_deadline
first_or_create(alert_day: 4).alert_day + 2
end
# 呼び出し
ProductDeadline.review_deadline
this.products.push(product) | ||
|
||
const json = await response.json() | ||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
好みかもしれませんが、以下のように書くと簡潔だと思います。
if(['/products/unassigned', '/products/unchecked', '/'].includes(location.pathname))
class ProductDeadlinesController < ApplicationController | ||
def update | ||
product_deadline = ProductDeadline.first_or_initialize | ||
product_deadline.update(alert_day: params[:alert_day]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strong Parameterで受け取れるパラメータを明示したほうが良いかと思います。
app/javascript/products.vue
Outdated
countAlmostPassedSelectedDays() { | ||
const previousDaysElement = | ||
this.productsGroupedByElapsedDays === null | ||
? undefined | ||
: this.getElementNdaysPassed(4) | ||
return elementPassed4days === undefined | ||
: this.getElementNdaysPassed(this.selectedDays - 1) | ||
return previousDaysElement === undefined | ||
? 0 | ||
: this.PassedAlmost5daysProducts(elementPassed4days.products).length | ||
: this.PassedAlmostSelectedDaysProducts(previousDaysElement.products) | ||
.length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
好みかもしれませんが、以下のようなガード節を入れて早めにreturnを返すことでif文がシンプルになって可読性が上がりそうです。
if (!this.productsGroupedByElapsedDays) {
return 0;
}
@@ -57,13 +58,36 @@ export default function Products({ | |||
return element === undefined ? 0 : element.products.length | |||
} | |||
|
|||
const isNotProduct5daysElapsed = () => { | |||
const isNotProduct4daysElapsed = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
経過日は4日に限られないと思いますので、isNotProductDeadlineDaysElapsed
などはいかがでしょうか?
data.products_grouped_by_elapsed_days = updateElapsedDays( | ||
data.products_grouped_by_elapsed_days | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data
の中身を書き換えてしまってもよいものなのでしょうか?
新たな変数を定義してもいいかもしれません。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ご指摘されて気づきましたが、dataの中身を書き換えない方がいいですね
新しく変数を定義して対応いたしました
<li | ||
className={`page-nav__item is-reply-deadline border-b-0 ${activeClass( | ||
countProductsGroupedBy(7) | ||
countProductsGroupedBy(productDeadlineDay + 2) | ||
)}`}> | ||
<a className="page-nav__item-link" href="#7days-elapsed"> | ||
<a className="page-nav__item-link" href="#6days-elapsed"> | ||
<span className="page-nav__item-link-inner"> | ||
7日以上経過{` (${countProductsGroupedBy(7)})`} | ||
{productDeadlineDay + 2}日以上経過 | ||
{` (${countProductsGroupedBy(productDeadlineDay + 2)})`} | ||
</span> | ||
</a> | ||
</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<li>~</li>
の中身は一般化した関数にすることができそうです。
レビューしていただきありがとうございます!
こちらのアラートラベルが56ではなく55になっているのは、実機のブラウザ上でしょうか? コードレビューのコメントの修正は時間がかかりますので、もう少々お待ちいただけると幸いです |
言葉足らずで申し訳ございません 💦 |
作成した未アサインの提出物のメッセージ
上記の動作確認で新しく提出物を作成すると、 アラート変更の動作確認でアラートラベルが56で表示されるようになります 誤解が生まれやすいので、動作確認を変更した方がよろしいでしょうか? |
正しく
ちゃんと確認していれば問題ないと思います! 例1. ダッシュボード確認... 2. 提出物の未アサインページ(/products/unassigned)... 3. ...... |
デフォルトの未アサインの提出物のアラートを`4日経過 5日経過 6日以上経過`にした セレクトボックスから未アサイン提出物のアラートを変更して表示できるようにした
5日に到達するメッセージをデフォルトは4日にし、選択後は最低経過日数にした 5daysなどの命名を変更した
57e24d2
to
2639652
Compare
コミットが残っているファイルから ご協力いただきありがとうございました |
@nakamu-kazu222 おおー、無事解決できて良かったですー😄 |
仕様変更により、提出期限をGUIではなく定数で管理するようにした そのため、ProductDeadlineモデルを削除し、Productモデルに定数を追加した
お疲れ様です! 仕様変更のため、再度レビューをしていただければと思い連絡いたしました
前回のレビューから時間が経過してしまい申し訳ございませんが、ご都合よろしければ、レビューをお願いしてもよろしいでしょうか? |
@nakamu-kazu222 レビューですが、1週間ほどお時間いただいた良ければ対応可能です 👍 |
お疲れ様です!
ありがとうございます!よろしくお願いいたします。 |
@nakamu-kazu222
|
お疲れ様です! 今後ともよろしくお願いいたします! |
お疲れ様です! 仕様変更の実装を完了し、メンバーからApproveをいただきましたので、 お手隙の際にご確認のほどよろしくお願いいたします🙇♂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確認させて頂きました。OKです〜🙆♂️
ご確認いただきありがとうございます! |
Issue
概要
未アサインの提出物のアラートが
5日経過、6日経過、7日以上経過
の経過日数で表示されているのを、4日経過、5日経過、6日以上経過
に変更した/products/unassigned
)/api/products/passed.txt
)/api/products/unassigned/counts.text
)作成した未アサインの提出物のメッセージを変更した
7日以内にメンターがレビューしますので、次のプラクティスにお進みください。
もし、7日以上経ってもレビューされない場合は、メンターにお問い合わせください。
経過日数を定数で変更できるようにした
変更確認方法
1. 環境構築
feature/change-alert-for-unassigned-products
をローカルに取り込むrails db:reset
を実行するbin/setup
を実行するforeman start -f Procfile.dev
を実行する2. デフォルトのアラートの動作確認
ダッシュボード確認
しばらく4日経過に到達する提出物はありません。
と表示され、クリックすると/products/unassigned#4days-elapsed
に遷移できるか確認する提出物の未アサインページ(
/products/unassigned
)/products/unassigned
を開く作成した未アサインの提出物のメッセージ
/courses/829913840/practices#category-685020562
)を開く6日以内にメンターがレビューしますので、次のプラクティスにお進みください。 もし、6日以上経ってもレビューされない場合は、メンターにお問い合わせください。
とメッセージが表示されるか確認するメンター向けの通知
3. アラート変更の動作確認
ダッシュボード
app/models/product.rb
のPRODUCT_DEADLINE
の値を変更して提出期限を変更するPRODUCT_DEADLINE
の値を変更して、それぞれの経過日数の日報が表示されているか確認する3日以上経過(12)、2日経過(1)、1日経過(1)、今日提出(56)
1件の提出物が、8時間以内に1日経過に到達します。
/products/unassigned#1days-elapsed
4日以上経過(11)、3日経過(1)、2日経過(1)、1日経過(1)、今日提出(56)
しばらく2日経過に到達する提出物はありません。
/products/unassigned#2days-elapsed
5日以上経過(9)、4日経過(2)、3日経過(1)、2日経過(1)、1日経過(1)、今日提出(56)
しばらく3日経過に到達する提出物はありません。
/products/unassigned#3days-elapsed
6日以上経過(8)、5日経過(1)、4日経過(2)、3日経過(1)、2日経過(1)、1日経過(1)、今日提出(56)
しばらく4日経過に到達する提出物はありません。
/products/unassigned#4days-elapsed
7日以上経過(7)、6日経過(1)、5日経過(1)、4日経過(2)、3日経過(1)、2日経過(1)、1日経過(1)、今日提出(56)
1件の提出物が、8時間以内に5日経過に到達します。
/products/unassigned#5days-elapsed
PRODUCT_DEADLINE
の値を3に変更する提出物の未アサインページ(/products/unassigned)
作成した未アサインの提出物のメッセージ
メンター向けの通知
アラートの変更方法をwikiに記載した
[メンター向け]提出物確認のアラートのデッドラインを変更する方法
Screenshot
変更前
ダッシュボード
提出物の未アサインページ(/products/unassigned)
作成した未アサインの提出物のメッセージ
メンター向けの通知
http://localhost:3000/api/products/passed.txt
http://localhost:3000/api/products/unassigned/counts.txt
変更後
ダッシュボード
提出物の未アサインページ(/products/unassigned)
作成した未アサインの提出物のメッセージ
メンター向けの通知
http://localhost:3000/api/products/passed.txt
http://localhost:3000/api/products/unassigned/counts.txt