Bug(バグ) #2058
完了コミュニティのメンバー管理画面に戻るためのYes/Noフォームで「いいえ」を選ぶとアクセスエラーになる
100%
説明
現象¶
- 自分が管理者であるコミュニティページ(/community/:id)にアクセスする
- メンバー管理ページ(/community/member/manage?id=:id)にアクセスする
- メンバー管理画面に戻るためのYes/Noフォームがあるページへ遷移する
- 副管理者に対する「副管理者から降格」リンクから遷移できるページ (/community/removeSubAdmin/id/1/member_id/2) あるいは、
- コミュニティメンバーに対する「退会させる」リンクから遷移できるページ (/community/removeSubAdmin/id/1/member_id/2) のどちらでもよい。
- メンバー管理画面に戻るための「いいえ」ボタンを押下する
- /default/error エラーページへ遷移する
原因¶
#1004 での 修正 によって、以下のルーティングが指定されており、
community_memberManage: url: /community/member/manage param: { module: community, action: memberManage }
このルーティングにマッチした上で、クエリストリングとしてパラメータを含めようとする。 /community/member/manage?id=1 のようなURLとなる。
GET method のフォームに対して action 属性値に ?id=xxx の形のクエリストリングが付与されたURLを渡すと、action 属性値のパラメータを無視する実装のUAが多いらしい。 関連記事(「form action クエリ」や「form action querystring」で検索すると記事がいくつか見つかる) [1] [2]
上記のルーティングが定義されていない場合は lib/routing/opSymfonyDefaultRouteCollection.class.php で
$this->routes['default'] = new opDeprecatedRoute( '/:module/:action/*' );
のように定義されているルーティングにマッチし、 /community/memberManage/id/123 のようなURLとなり、パラメータが渡っていたため想定通りに動作していた。
修正内容¶
PC版、携帯版ともに、ルーティングの定義を
community_memberManage: url: /community/member/manage param: { module: community, action: memberManage }
から、id パラメータを含めた形
community_memberManage: url: /community/member/manage/:id param: { module: community, action: memberManage } requirements: { id: \d+ }
に変更する。
当該のYes/Noフォームにおいて、action 属性値に含まれるクエリストリングを input 要素などを介して渡すことでこの問題を回避することも可能だが、本質的な解決策ではない。
補足¶
この問題は #1004 での修正における書き換え時のミスだと思われる。
- id を付加する必要性について
- community/memberManage アクションは、特定の id のコミュニティのメンバー管理画面を表示するためのアクションである。つまり id を受け取ることを前提としている。
- lib/action/opCommunityAction.class.php の preExecute() メソッドおよび executeMemberManage($request) を見れば明らかである。
- community/memberManage アクションは、特定の id のコミュニティのメンバー管理画面を表示するためのアクションである。つまり id を受け取ることを前提としている。
- ルーティング定義を変更してしまうことによる副作用はないか
- id を受け取らない community/memberManage の呼び出しは前述のとおり想定されていないため、もしそのようなリンクがあったとしてもエラーページに飛ばされるだけである。
- community/memberManage アクションを指すURL /community/member/manage (idを含まないURL)が無くなっても問題はないと判断している。
Masato Nagasawa さんが13年以上前に更新
テスト
区分 | 期待結果 | 結果 |
携帯側のコミュニティ副管理者を降格画面で「いいえ」を選択 | メンバー管理画面へ戻る | ○ |
PC側のコミュニティ副管理者を降格画面で「いいえ」を選択 | メンバー管理画面へ戻る | ○ |
Masato Nagasawa さんが13年以上前に更新
- ステータス を Accepted(着手) から Pending Review(レビュー待ち) に変更
- 進捗率 を 0 から 50 に変更
更新履歴 032f641badd65e02400a432dd28a5baef1819bc0 で適用されました。
Naoya Tozuka さんが13年以上前に更新
修正箇所確認しました。OKです。
【メモ】「副管理者から降格」に対応する英文の誤りについて¶
今回指摘されている問題とは異なりますが、「副管理者から降格」に対応する英文
"Do you demotion %0% from this %community%'s sub-administrator?" // %0% には副管理者の名前が、%community% にはコミュニティ名が入ります。において demotion は名詞なので、これは demote に置き換えるべきと思われます。
修正すべき箇所は、
- apps/mobile_frontend/i18n/messages.ja.xml
- apps/mobile_frontend/modules/community/templates/memberManageSuccess.php
- apps/mobile_frontend/modules/community/templates/removeSubAdminInput.php
- apps/pc_frontend/i18n/messages.ja.xml
- apps/pc_frontend/modules/community/templates/memberManageSuccess.php
- apps/pc_frontend/modules/community/templates/removeSubAdminInput.php
になります。
Naoya Tozuka さんが13年以上前に更新
takai: @Naoya Tozuka その間違った文言のコミットに対して、修正すべきは戸塚さんが指摘した文言のみでしょうか。もし間違った文言が他にもあるのであれば全面的に修正した方が好ましい気がしました。間違った文言のコミットは 43ae2a74 らしいですね(masterブランチ)。 http://redmine.openpne.jp/issues/228 の4個目のコミット。
tozuka: @Minoru Takai なるほど( 43ae2a74 みました)。「副管理者から降格」に射影される英文が2つあるので両方辿ってみます。
Naoya Tozuka さんが13年以上前に更新
更新履歴 commit:"054285bf3cd48d0f963e8becc51914e24afcc13a" で適用されました。 → 043cb4ac に統合のため削除
Naoya Tozuka さんが13年以上前に更新
更新履歴 commit:"dd065c59f1674bbf5bf0d26e7f87609c10838587" で適用されました。 → 043cb4ac に統合のため削除
Minoru Takai さんが13年以上前に更新
- ステータス を Pending Review(レビュー待ち) から Rejected(差し戻し) に変更
このチケットに対して¶
このチケット自体をレビューします。
3. 適当なメンバーで「退会させる」を押下:/community/removeSubAdmin/id/19/member_id/2
これは『3. 副管理者に対して「副管理者から降格」を押下』の誤りでしょうか。
原因¶
get の form のaction では action="URL?id=○○" のパラメータが引き継げないので URL? ページに遷移してしまう
『get の form のaction では action="URL?id=○○" のパラメータが引き継げない』この理由・原因について可能であれば、コメントかチケットに記述してください。修正方針が妥当かどうか、他の箇所について修正する必要がないかを判断するのに有用です。
note-3 での修正内容について¶
修正内容¶
inputタグでパラメータを引き継ぐように修正。
この修正方針が妥当であれば、修正内容は問題ないと思います。
- 'no_url' => url_for('@community_memberManage?id='.$community->getId()), + 'no_url' => url_for('@community_memberManage'), 'no_method' => 'get', + 'no_form' => '<input type="hidden" name="id" value="'.$community->getId().'"/>', )) ?>
PC版の修正について、追加している input 要素タグの末尾のスラッシュの直前に空白がありませんが、本質的な問題ではないためこの修正内容で問題ないと思います。
note-10 での修正内容について¶
修正内容自体はOKです。
チケット自体に文言について修正する旨が記述されていないため、note-3 の修正に併せて、文言についての修正も扱ったことが分かるようチケットを書き換える必要がありそうです。
また、バックポートチケットでも同じ修正を取り込むか、あるいは取り込まないことが分かるようになっているべきです。3.4.x 向けのBPチケット #2063 は、この親チケットが完了する前にクローズされているため、 note-10 での修正は #2063 の派生チケット #2093 で扱うようにした方がよいかと思います。
一旦、担当者に差し戻します。
Minoru Takai さんが13年以上前に更新
- ステータス を Rejected(差し戻し) から Accepted(着手) に変更
- 担当者 を Naoya Tozuka から Minoru Takai に変更
一先ず、チケットの概要(現象・原因・修正内容)を書き換えました。
Minoru Takai さんが13年以上前に更新
note-10 の文言に関する修正についてですが、チケットの関連をより分かりやすくするために別チケット対応としました。文言の修正については #2250 で扱います。
Minoru Takai さんが13年以上前に更新
- 題名 を コミュニティの副管理者降格画面(/community/removeSubAdmin/id/:id/member_id/:id)で「いいえ」を選ぶとアクセスエラーになる から コミュニティのメンバー管理画面に戻るためのYes/Noフォームで「いいえ」を選ぶとアクセスエラーになる に変更
Minoru Takai さんが13年以上前に更新
- ステータス を Accepted(着手) から Pending Review(レビュー待ち) に変更
更新履歴 81b179d454f13ffed2dc02ccf4aa72905cb777ca で適用されました。
Naoya Tozuka さんが13年以上前に更新
- ステータス を Pending Review(レビュー待ち) から Pending Testing(テスト待ち) に変更
- 進捗率 を 50 から 70 に変更
新しい修正方針に基づいた修正を確認しました。レビューOKです。
チケットの整理どうもありがとうございます。
Minoru Takai さんが13年以上前に更新
- ステータス を Pending Testing(テスト待ち) から Fixed(完了) に変更
- 進捗率 を 70 から 100 に変更
動作テスト¶
実装時に動作テストを行っており、原理的にもこの修正による副作用はないものと判断しています。
このチケットで挙げられた当初の問題点については改善できていることが確認できており、修正内容も妥当であると判断しているため、動作上も問題がないと判断します。
完了とします。