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

Code review #137

Merged
merged 8 commits into from
Aug 31, 2022
Merged

Code review #137

merged 8 commits into from
Aug 31, 2022

Conversation

eatplaynap
Copy link
Owner

@eatplaynap eatplaynap commented Aug 16, 2022

ご指摘いただいた箇所+途中で気づいたバグ修正(及びテスト)を追加しました。

レビューに対して

  • Editフォームを編集できるようにした
  • 新規ユーザー登録失敗時の例外をraiseするようにした
  • 長すぎるテストコードを改行
  • Time.zone.todayDate.currentに変更

バグ修正

Time.zone.todayDate.currentの違いを調べている中で、アプリ全体にtimezoneが設定されていないため自動的にUTCに変換され、日本時間で午前0時〜午前8時59分の間に学習記録を作ると、前日の日付で学習記録が作られてしまうというバグを発見しました。
ユーザーに合わせて動的にtimezoneを設定したいと考えているため、デフォルトはUTCのままにしておき、ブラウザからtimezoneの情報を取得し、cookieに保存し、学習記録作成のタイミングでtimezoneを変更するという処理を追加しました。
(参考: kbaum/browser-timezone-rails#45 (comment) )
バグ修正のためテストを追加しています。

Copy link

@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です〜🙆‍♂️

@eatplaynap eatplaynap merged commit 9e97fe2 into main Aug 31, 2022
@eatplaynap eatplaynap deleted the code-review branch August 31, 2022 00:51
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.

2 participants