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.

Sign up for GitHub

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

Jump to bottom

招待URL作成ページを追加 #7542

Merged
merged 11 commits into from
May 28, 2024
Merged

招待URL作成ページを追加 #7542

merged 11 commits into from
May 28, 2024

Conversation

naokinaokiboo
Copy link
Contributor

@naokinaokiboo naokinaokiboo commented Mar 14, 2024 •
edited
Loading

Issue

概要

管理画面に以下の項目を選択して、招待用URLを作成するページを作成しました。

  • 企業
  • ロール
  • コース

変更確認方法

  1. feature/add-invitation-url-pageをローカルに取り込む。
  2. 管理者アカウントでログインし、/admin/invitation_urlにアクセスする。
  3. 企業、ロール、コースのドロップダウンから任意の項目を選択する。
  4. 以下を確認する。
    • 表示されているURLが変わること
    • 表示されているURLにアクセスすると、参加登録画面が表示されること
    • 表示された参加登録画面に選択した企業、ロール、コースが反映されていること（※ただし、メンターを選択した場合は登録画面側が対応していないため現状反映されません。登録画面側の対応については別のIssueで対応するとのことでした。）

※以下、参加登録画面に反映される項目の表示例です。
image

image

image

Screenshot

変更前

新規ページのため無し

変更後

image

@naokinaokiboo naokinaokiboo self-assigned this Mar 14, 2024
@machida machida self-assigned this Mar 16, 2024
@naokinaokiboo naokinaokiboo force-pushed the feature/add-invitation-url-page branch 2 times, most recently from 8c13ad5 to f8ebbf7 Compare March 22, 2024 09:48
@naokinaokiboo
Copy link
Contributor Author

@machida
お疲れさまです
こちら機能の実装が一旦完了したので、デザインをお願い致します。🙏

@machida
Copy link
Member

machida commented Mar 22, 2024

@naokinaokiboo 承知しましたー🙆‍♂️

@machida machida removed their assignment Mar 25, 2024
@machida
Copy link
Member

machida commented Mar 25, 2024

@naokinaokiboo

お待たせしました！！デザイン入れましたー

@naokinaokiboo naokinaokiboo force-pushed the feature/add-invitation-url-page branch from b0ed1a4 to f08f9e8 Compare March 28, 2024 03:51
@naokinaokiboo naokinaokiboo marked this pull request as ready for review March 28, 2024 04:12
@naokinaokiboo
Copy link
Contributor Author

@umizaru
お疲れ様です。
こちらレビューをお願いできますでしょうか🙏（急ぎではありません）
もしご都合悪いなどありましたら、その旨、仰っていただけたらと思います。
よろしくお願い致します。🙇‍♂️

@naokinaokiboo naokinaokiboo requested a review from umizaru March 28, 2024 04:16
@umizaru
Copy link
Contributor

umizaru commented Mar 30, 2024

@naokinaokiboo
返信遅くなりました！
レビュー承知しました👍少し時間かかってしまうかもですが対応させていただきます！

umizaru
umizaru reviewed Apr 14, 2024
View reviewed changes
Copy link
Contributor

@umizaru umizaru left a comment

Choose a reason for hiding this comment

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

大変お待たせしました！
コードレビューしましたのでご確認お願いします🙏

test/system/admin/invitation_url_test.rb Outdated
course = Course.order(:created_at).first
expected = "#{@new_user_url}?company_id=#{company.id}&course_id=#{course.id}&role=#{role}&token=token"

Timeout.timeout(Capybara.default_max_wait_time, StandardError) do
Copy link
Contributor

Choose a reason for hiding this comment

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

Timeoutの処理が重複しているので1つのメソッドにまとめてはどうでしょう？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Timeoutの処理、全く同一でしたね。😅
これはメソッドにまとめたいと思います！ありがとうございます👍

app/controllers/api/admin/invitation_url_controller.rb Outdated

class API::Admin::InvitationUrlController < API::Admin::BaseController
def index
url = new_user_url(
Copy link
Contributor

Choose a reason for hiding this comment

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

_urlだとURIを絶対パスで指定できるんですね。
URLヘルパーメソッドは_pathしか知らなかったので勉強になりました！

_pathメソッドと_urlメソッドの使い分け
https://qiita.com/higeaaa/items/df8feaa5b6f12e13fb6f

「Rails」urlとpathの違いを初心者向けに解説
https://qiita.com/nichidai3_0514/items/582b466b4a8cf9505dac

app/javascript/invitation-url-updater.js
@@ -0,0 +1,45 @@
document.addEventListener('DOMContentLoaded', async () => {
const invitationElements = Array.from(
Copy link
Contributor

Choose a reason for hiding this comment

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

CSSセレクタのselectを変数名に反映してあげてselectedInvitationElementsでも良いかな、と思いました。
ただ、他の変数名がinvitationXXで統一されていますし少し悩むところで採否はお任せします！

Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちら、私も悩ましいところではあるのですが、今回はinvitationXXで統一するのを優先する方向でいきたいと思います🙏
ありがとうございます👍

@naokinaokiboo
Copy link
Contributor Author

@umizaru
レビューありがとうございます！😄
コメント頂いた通り、テストコードでの重複処理をメソッド化してみました。
またお時間あるときに、ご確認よろしくお願い致します。🙏

@umizaru
Copy link
Contributor

umizaru commented Apr 15, 2024

@naokinaokiboo
確認しました！問題なさそうなので私からはLGTMです！👍

@naokinaokiboo
Copy link
Contributor Author

@umizaru
お疲れ様です！
お忙しい中レビューして頂きありがとうございました！🙏

@komagata
チームメンバーによるレビューが完了しましたので、レビューよろしくお願い致します🙏

@naokinaokiboo naokinaokiboo requested a review from komagata April 16, 2024 05:14
komagata
komagata reviewed Apr 24, 2024
View reviewed changes
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.

conflictの修正をお願い致します～。

@naokinaokiboo
Copy link
Contributor Author

@komagata
conflict修正しておきました〜。

komagata
komagata reviewed May 15, 2024
View reviewed changes
app/controllers/api/admin/invitation_url_controller.rb Outdated
@@ -0,0 +1,13 @@
# frozen_string_literal: true

class API::Admin::InvitationUrlController < API::Admin::BaseController
Copy link
Member

Choose a reason for hiding this comment

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

例えばですが、JavaScriptの変数としてこれらが定義してあれば、時間のかかるHTTPリクエストを飛ばさずにプルダウンが表示できるかなと思いました。

@naokinaokiboo naokinaokiboo force-pushed the feature/add-invitation-url-page branch from afc0019 to 67537ec Compare May 28, 2024 12:19
@naokinaokiboo naokinaokiboo force-pushed the feature/add-invitation-url-page branch from 67537ec to ea7a2ca Compare May 28, 2024 12:54
@naokinaokiboo
Copy link
Contributor Author

@komagata
コメント頂いた箇所について、URLの雛形をJavaScirpt側に渡して、その雛形をもとにURLを変更するようにしました。
お時間のあるときに、再度ご確認お願いします🙏

komagata
komagata approved these changes May 28, 2024
View reviewed changes
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です～👌

@komagata komagata merged commit ec518cd into main May 28, 2024
2 checks passed
@komagata komagata deleted the feature/add-invitation-url-page branch May 28, 2024 20:35
@github-actions github-actions bot mentioned this pull request May 28, 2024
Merged
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@umizaru umizaru umizaru approved these changes

@komagata komagata komagata approved these changes

Assignees

@naokinaokiboo naokinaokiboo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@naokinaokiboo @machida @umizaru @komagata