Project

General

Profile

Bug(バグ) #2425

プロフィール編集画面にて、プロフィール項目で重複した値が登録できる

Added by Yuma Sakata almost 8 years ago. Updated almost 4 years ago.

Status:
Fixed(完了)
Priority:
Normal(通常)
Assignee:
Target version:
Start date:
2011-09-25
Due date:
% Done:

100%

3.6 で発生するか:
Unknown (未調査)
3.8 で発生するか:
Unknown (未調査)

Description

Overview (現象)

プロフィール編集画面にて、プロフィール項目で重複した値が登録できる

Environment (再現環境)

OpenPNE 3.6RC1

Way to repro (再現手順)

1. 管理画面プロフィール項目登録ページ(/pc_backend.php/profile/edit)にアクセスする
2. 重複の可否を「表示する」、入力値タイプを「数値」に設定してプロフィール項目を作成する
3. メンバーAでSNSログイン後、プロフィール編集ページ(/member/edit/profile)にアクセスする
4. 手順2で作成したプロフィール項目に数値を入力する(入力した数値を控えておく)
5. メンバーBでSNSログイン後、プロフィール編集ページ(/member/edit/profile)にアクセスする
6. 手順2で作成したプロフィール項目に、手順4で入力した数値を入力する
7. 重複する値が登録できる。

Way to fix (修正内容)

プロフィール項目で重複した値が登録できないように修正お願いします。


Related issues

Related to OpenPNE 3 - Backport(バックポート) #2436: プロフィール編集画面にて、プロフィール項目で重複した値が登録できる Fixed(完了) 2011-09-26

Associated revisions

Revision bd5b7b04 (diff)
Added by Yuya Watanabe almost 8 years ago

(fixes #2425) validate duplicated value

Revision 42eb6859 (diff)
Added by Yuya Watanabe almost 8 years ago

(refs #2425) fixed for coding standard

Revision 74305381 (diff)
Added by Yuya Watanabe almost 8 years ago

(fixes #2425) optimize DQL and rename method

Revision 1c9e4cfc (diff)
Added by Yuya Watanabe almost 8 years ago

(refs #2425) fixed i18n

Revision 096bf206 (diff)
Added by Yuya Watanabe almost 8 years ago

(fixes #2425) add help to unique setting

Revision 66dea256 (diff)
Added by Yuya Watanabe almost 8 years ago

(fixes #2425) fixed for coding standard

Revision 3cddf475 (diff)
Added by Yuya Watanabe almost 8 years ago

(fixes #2425) fixed to make check duplication in the case of 'text' or 'textarea'

Revision 85461319 (diff)
Added by Yuya Watanabe almost 8 years ago

(fixes #2425) fixed invalid input type

Revision 9bbd4c1c (diff)
Added by Yuya Watanabe over 7 years ago

(fixes #2425) validate duplicated value

Revision ac394cc2 (diff)
Added by Yuya Watanabe over 7 years ago

(refs #2425) fixed for coding standard

Revision e2aa49e6 (diff)
Added by Yuya Watanabe over 7 years ago

(fixes #2425) optimize DQL and rename method

Revision 57d09825 (diff)
Added by Yuya Watanabe over 7 years ago

(refs #2425) fixed i18n

Revision 9149cba3 (diff)
Added by Yuya Watanabe over 7 years ago

(fixes #2425) add help to unique setting

Revision 55f906f5 (diff)
Added by Yuya Watanabe over 7 years ago

(fixes #2425) fixed for coding standard

Revision e3bcf422 (diff)
Added by Yuya Watanabe over 7 years ago

(fixes #2425) fixed to make check duplication in the case of 'text' or 'textarea'

Revision 32f9d0fa (diff)
Added by Yuya Watanabe over 7 years ago

(fixes #2425) fixed invalid input type

History

#1 Updated by Mutsumi Imamura almost 8 years ago

  • Target version set to OpenPNE 3.7.0
  • 360対象 set to 3.6.0

#2 Updated by Shingo Yamada almost 8 years ago

  • Assignee set to Yuya Watanabe

#3 Updated by Yuya Watanabe almost 8 years ago

  • Status changed from New(新規) to Accepted(着手)

#4 Updated by wa ta almost 8 years ago

  • Status changed from Accepted(着手) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

更新履歴 bd5b7b04d6ff81746b63a6966a94985b93ad11c5 で適用されました。

#5 Updated by Kousuke Ebihara almost 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Rejected(差し戻し)
  • グローバルな i18n/messages.ja.xml において、 Duplicate という一語に対して「すでに登録されています」という訳語を当てるのはどうかなと思います(別のカタログが使用されているのであればまだ理解できますが)。たとえば、「Your inputted value is already registered (もしくは動詞 exist を使う)」や「You've inputted duplicated value」などとするわけにはいかないでしょうか。ちなみに、 sfValidatorDoctrineUnique のデフォルトのエラーメッセージは「An object with the same "%column%" already exist.」です。
  • MemberProfileTable::retrieveByProfileIdAndValueExceptMemberId() で発行されるクエリの NOT IN に指定される id は 1 件のみなので、普通に member_id <> ? と書くのがよいと思います。もっとも、この程度のクエリであれば MySQL 側で最適化するかもしれませんが……
  • 重複確認をおこなう目的であれば、入力値に合致するプロフィールの値を持つ id をすべて検索する必要はないはずです。また、これらの結果をハイドレートする必要はないはずです。ですので、 MemberProfileTable::retrieveByProfileIdAndValueExceptMemberId() を以下のように変更してみてはどうでしょうか。
    • fetchArray() ではなく fetchOne() を使用する (暗黙的に DQL に "LIMIT 1" が追加される)
    • fetchOne() の第二引数に Doctrine_Core::HYDRATE_NONE を指定し、ハイドレートをおこなわないようにする
    • このメソッドの返り値を bool にし、メソッド名を MemberProfileTable::checkDuplicatedProfileValue() などとする (bool を返すのであれば isExist... とか has... とかのほうが適当か?)

#6 Updated by wa ta almost 8 years ago

  • Status changed from Rejected(差し戻し) to Pending Review(レビュー待ち)

更新履歴 74305381b7d2487e7ec770e93273c4195c956ff4 で適用されました。

#7 Updated by Yuya Watanabe almost 8 years ago

以下の点を修正しました.

  • 「すでに登録されています」の訳を「Your inputted value is already exist」とする
  • メッセージ末尾に「。」および「.」を付与する
  • 重複したプロフィールが存在するかどうかを得るメソッド名を変更して返り値をboolになるようにする
  • 検索結果数を1つにしてハイドレートを行わないようにする

#8 Updated by Kousuke Ebihara almost 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
  • % Done changed from 50 to 70

OK です。

fetchOne() の第一引数に空配列ではなく null を指定していることについて、妥当かどうか熟考しましたが、

  • この第一引数は内部で array 型へのキャストをしたうえで用いられる。 null を array 型にキャストした場合は空配列が得られるので事実上等価(ただしこの型キャストの前に第一引数を利用した場合の結果は異なる場合がありうる)
  • fetchOne() など DQL 実行系のメソッドで、バインドする値の数が 1 つのみである場合、配列ではなくスカラー値を指定しているケースはかなり存在していると思われる。これはこの第一引数が array 型にキャストされることを想定した使われ方である

ことから、特に問題はないだろうと判断しました。

#9 Updated by Fumie Toyooka almost 8 years ago

  • Status changed from Pending Testing(テスト待ち) to Rejected(差し戻し)
  • % Done changed from 70 to 50

重複登録できてしまいます。(プルダウンメニューでの重複禁止というと、プルダウンメニュー項目がメンバーの数だけ存在せねばならず、現実的ではないと思いますが。)テスト手順は以下のとおりです。

1. 管理画面プロフィール項目登録ページ(/pc_backend.php/profile/edit)にアクセスする
2. 重複の可否を「禁止」にする
3. フォームタイプを「単一選択(プルダウン)」に設定してプロフィール項目を作成する
4. プロフィール選択肢を複数作成する。
5. メンバーAでSNSログイン後、プロフィール編集ページ(/member/edit/profile)にアクセスする
6. 手順2,3で作成したプロフィール項目で手順4で作成した項目を選択する
7. 招待メールを新規のユーザーに送る。(frontend,backend問わない)
7. 新規ユーザーでメールにあるURLからプロフィール登録ページにアクセスする
8. 手順2で作成したプロフィール項目に、手順6で選択した値を選択し、送信ボタンを押す

数値文字列内に改行が入るためか、frontend の方で「整数ではありません。」と表示されます。
間違えて記述してしまいましたので、この部分は無視してください(2011/10/07)

#10 Updated by Yuya Watanabe almost 8 years ago

  • Status changed from Rejected(差し戻し) to Accepted(着手)

#11 Updated by Yuya Watanabe almost 8 years ago

note-9について

Fumie Toyooka は書きました:

重複登録できてしまいます。(プルダウンメニューでの重複禁止というと、プルダウンメニュー項目がメンバーの数だけ存在せねばならず、現実的ではないと思いますが。)テスト手順は以下のとおりです。

[...]
数値文字列内に改行が入るためか、frontend の方で「整数ではありません。」と表示されます。

プロフィール項目の「フォームタイプ」を「単一選択(プルダウン)」とした場合に重複登録が可能であることを確認しました.
しかし,重複登録が成功するため「整数ではありません。」という表示はされず,確認できませんでした.
以降では『プロフィール項目の「フォームタイプ」を「単一選択(プルダウン)」とした場合に重複登録が可能である』という問題について議論を行います.

「フォームタイプ」を「単一選択(プルダウン)」とした場合に重複登録が可能な原因

MemberProfileTable::isDuplicatedProfileValue()ではmember_profileテーブルのvalueについてのみ重複しているかどうかを判定しています.
しかし,「フォームタイプ」を「単一選択(プルダウン)」とした場合にはmember_profileテーブルのprofile_option_idが更新され,valueの列にはデータは格納されないという状態になっていました.同様に,「フォームタイプ」が選択式および日付の場合に同様の現象が発生することを確認しました.

#12 Updated by Yuya Watanabe almost 8 years ago

修正方針(追記)

選択式及び日付については,重複不可の設定を反映するとnote-9で言及されているようにプルダウンメニュー項目数とメンバ数によってはプロフィールが編集不可という状態になりえます.また,複数選択可能な状態での重複不可はどうするのかという点については議論の余地が存在すると思います.そのため,本チケットでは重複可否の設定を反映しない方針とします.

対応方針としては重複可否の設定項目には「フォームタイプがテキストの場合にのみ適用されます」という旨の文章を追加します.

また,この時点では「数値文字列内に改行が入るためか、frontend の方で「整数ではありません。」と表示されます。」という問題について確認できないため対応をしないことします.

#13 Updated by wa ta almost 8 years ago

  • Status changed from Accepted(着手) to Pending Review(レビュー待ち)

更新履歴 096bf2065cc5a82e1123bc58cc6dbef7f47d3542 で適用されました。

#14 Updated by wa ta almost 8 years ago

更新履歴 66dea25693b91703e003538d6d0d38e4807b67f3 で適用されました。

#15 Updated by Kousuke Ebihara almost 8 years ago

以下のような問題があることを疑っています(まだ確認はしていません。追って調査する予定です)。

  • 重複の可否を「許可」にした、フォームタイプがテキストのプロフィール項目を作成し、 2 名以上のメンバーで重複する入力値をプロフィールとして設定したあと、重複の可否を「禁止」かつフォームタイプを「単一選択(プルダウン)」等に設定した場合に、プロフィールの更新がおこなえなくなるのではないか

#16 Updated by Yuya Watanabe almost 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Accepted(着手)

問題

note-15で懸念された問題は発生しませんでしたが,以下の問題が発生する可能性があることがわかりました.

問題1 重複不可,「単一選択(プルダウン)」の状態のとき,選択可能なprofile_option_idを「テキスト」等で入力されている場合にプルダウンメニューでそのIDに相当する項目を保存しようとすると「すでに登録されています」と表示され保存できない
問題2 重複不可,「複数選択(チェックボックス)」の状態のとき,Internal Server Errorが発生する

それぞれの問題について以下に記述します.

問題1の原因

下記箇所において$value['value']が返す値がプルダウン等の単一選択の場合に,選択されたprofile_options_idが入っているため,その値をmember_profileテーブルのvalueの値を検索してしまうため,問題1が発生してしまう状態になっています.

lib/validator/opValidatorProfile.class.php 55行目

 50     if ($this->profile->getIsUnique())
 51     {
 52       $profileId = $this->profile->getId();                                                                        
 53       $memberId = sfContext::getInstance()->getUser()->getMemberId();                                              
 54       
 55       $isDuplicate = Doctrine::getTable('MemberProfile')->isDuplicatedProfileValue($profileId, $value['value'], $memberId);   
 56       if ($isDuplicate)
 57       {
 58         throw new sfValidatorError($this, 'The inputted value is already exist.');                                 
 59       }                                                                                                            
 60     }   

問題2の原因

問題1と同じ場所で$value['value']の値が配列で渡されるためDQLのエラーが発生してしまう状態になっています.具体的には以下のエラーメッセージが得られます.

SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens

修正方針

lib/validator/opValidatorProfile.class.php 50行目で$this->profile->isUnique()がtrueの場合のみに重複判定をしてますが,これをnote-12の方針通り,「テキスト」および「テキスト(複数行)」のみ実行するように修正を行います.

#17 Updated by Yuya Watanabe almost 8 years ago

実装案

diff --git a/lib/validator/opValidatorProfile.class.php b/lib/validator/opValidatorProfile.class.php
index 5609f03..91bcaba 100644
--- a/lib/validator/opValidatorProfile.class.php
+++ b/lib/validator/opValidatorProfile.class.php
@@ -47,7 +47,7 @@ class opValidatorProfile extends sfValidatorBase
       $clean['public_flag'] = $validator->clean($value['public_flag']);
     }

-    if ($this->profile->getIsUnique())
+    if ($this->profile->getIsUnique() && in_array($this->profile->getFormType(), array('text', 'textarea')))
     {
       $profileId = $this->profile->getId();
       $memberId = sfContext::getInstance()->getUser()->getMemberId();

#18 Updated by wa ta almost 8 years ago

  • Status changed from Accepted(着手) to Pending Review(レビュー待ち)

更新履歴 3cddf475eaa9ca80a936c91b1b7364b3874651a4 で適用されました。

#19 Updated by Kousuke Ebihara almost 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
  • % Done changed from 50 to 70

#20 Updated by Mutsumi Imamura almost 8 years ago

  • Status changed from Pending Testing(テスト待ち) to Rejected(差し戻し)
  • % Done changed from 70 to 50
新規にプロフィール項目を作成し重複禁止動作を確認しましたが、重複チェックがおこなれていません。
  • フォームタイプはテキスト
  • 入力値タイプは文字列
    でプロフィール項目を新規作成し、プロフィール編集でメンバーAとBそれぞれ同じ文字列を入力し編集ボタンを押すと編集が完了できてしまいます。

#21 Updated by wa ta almost 8 years ago

  • Status changed from Rejected(差し戻し) to Pending Review(レビュー待ち)

更新履歴 854613196b5bf1e54b28d25b8c32b7a00e6eff39 で適用されました。

#22 Updated by Yuya Watanabe almost 8 years ago

原因

適切でないフォームタイプを設定していたため「テキスト」だった場合に重複登録が可能な状態になっていました.

#23 Updated by Kousuke Ebihara almost 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
  • % Done changed from 50 to 70

#24 Updated by Yuma Sakata almost 8 years ago

  • Status changed from Pending Testing(テスト待ち) to Fixed(完了)
  • % Done changed from 70 to 100

テストOKです。

#25 Updated by Yuya Watanabe over 7 years ago

  • 3.6 で発生するか set to Unknown (未調査)
  • 3.4 で発生するか set to Unknown (未調査)

本チケットのコミットは以下のものです.

https://redmine.openpne.jp/projects/op3/repository/revisions/bd5b7b04d6ff81746b63a6966a94985b93ad11c5
https://redmine.openpne.jp/projects/op3/repository/revisions/42eb6859bea4d4ef3928ff274eb02b6b582cf430
https://redmine.openpne.jp/projects/op3/repository/revisions/74305381b7d2487e7ec770e93273c4195c956ff4
https://redmine.openpne.jp/projects/op3/repository/revisions/1c9e4cfcd90a73d1c4121e3e90215af2bd0bfaab
https://redmine.openpne.jp/projects/op3/repository/revisions/096bf2065cc5a82e1123bc58cc6dbef7f47d3542
https://redmine.openpne.jp/projects/op3/repository/revisions/66dea25693b91703e003538d6d0d38e4807b67f3
https://redmine.openpne.jp/projects/op3/repository/revisions/3cddf475eaa9ca80a936c91b1b7364b3874651a4

下記チケット群は release-3.8beta1 でコミットされたものです.

https://redmine.openpne.jp/projects/op3/repository/revisions/9bbd4c1cb046e4aabfa3fa6804c2a0d3779df91c
https://redmine.openpne.jp/projects/op3/repository/revisions/ac394cc28a660f1e659a5ce2690087b84a7d09d1
https://redmine.openpne.jp/projects/op3/repository/revisions/e2aa49e6863a6c13b7cb383b0beebdae855693d2
https://redmine.openpne.jp/projects/op3/repository/revisions/57d09825d55e402cc72d17ef1d4f70f3fc5208fd
https://redmine.openpne.jp/projects/op3/repository/revisions/9149cba3836f86cf44a3841f61ed216e1a3579ae
https://redmine.openpne.jp/projects/op3/repository/revisions/55f906f5876f2ae38dcbba2e2fe8b9c578eeef57
https://redmine.openpne.jp/projects/op3/repository/revisions/e3bcf4224699318ca79e1de3fde0dfbf1601209b
https://redmine.openpne.jp/projects/op3/repository/revisions/32f9d0fa1271f37e14614ad6b25bd3667f23b94a

#26 Updated by kaoru n almost 4 years ago

  • 3.8 で発生するか set to Unknown (未調査)

Also available in: Atom PDF