Project

General

Profile

Bug(バグ) #2478

プロフィール項目で「単一選択」の状態で保存したあとに「複数選択」の状態で保存を行おうとしても保存されていないように見える

Added by Yuya Watanabe almost 8 years ago. Updated about 2 years ago.

Status:
Rejected(差し戻し)
Priority:
Normal(通常)
Assignee:
Target version:
Start date:
2011-10-07
Due date:
% Done:

50%

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

Description

概要

プロフィール項目で「単一選択」の状態で保存したあとに「複数選択」の状態で保存を行おうとしても保存されていないように見える.
DBを見ると保存されているが,プロフィール閲覧画面(/member/profile)で閲覧しても「単一選択」の時に保存された内容のみが表示される.

再現手順

  1. 管理画面でプロフィール一覧画面から「プロフィール項目登録」をクリックする
    • あるいはプロフィール項目登録画面(pc_backend.php/profile/edit)のページを開く
  2. 「フォームタイプ」を「単一選択(プルダウン)」を選択して「追加」をクリックする
  3. プロフィール項目一覧画面(pc_backend.php/profile/list)の「プロフィール選択肢一覧」で2つ以上の選択肢を保存する
  4. SNSのプロフィール編集画面(member/edit/profile)で先程追加した項目で任意の項目を選択して「送信」をクリックする
    • 選んだ項目が保存されていることを確認する
  5. 管理画面で先程作ったプロフィール項目を「複数選択(チェックボックス)」を選択して「変更」をクリックする
  6. SNSのプロフィール編集画面で先程と同じ項目で2つ以上の項目にチェックを入れて「送信」をクリックする
    • プロフィール閲覧画面(member/profile)において「単一選択(プルダウン)」で選択した項目のみが表示される
    • DB上で確認すると「単一選択(プルダウン)」で保存されたデータと「複数選択(チェックボックス)」で保存されたデータ両方が存在している

確認環境

OpenPNE 3.7.0-dev (master)

原因

「複数選択(チェックボックス)」のデータ保存方法はツリー構造となっており,ルートとなるレコードを元にしてリーフのレコードが「複数選択(チェックボックス)」で選択した項目が保持される.
このとき,ルートとなるレコードが無い場合は新規に作成されてデータがないレコードがルートとして作成され,そのツリーのリーフにデータが保持されたレコードが作成される.そしてプロフィール項目表示時にはそのリーフとなるデータをすべて取得し,表示が行われる.
しかし,「単一選択」ですでにルートとなるレコードが存在する場合(つまりprofile_option_idがNULLでないデータが存在する場合)にはそのレコードにリーフが存在したとしても,ルートのレコードのデータのみが取得されて表示されるようになる.
「テキスト」->「複数選択」の場合もルートとなるレコードは「テキスト」のデータ保存時に作成されたレコードであるが,profile_option_idがNULLであるために本問題が発生しない状態であると考えられる.

修正方針

修正方針は2通り考えられる.
  1. 「複数選択」で保存される場合にルートとなるレコードの情報を削除する
  2. プロフィール閲覧画面で表示する際にフォームタイプが「複数選択(チェックボックス)」であるプロフィール項目はツリー構造のリーフの部分を表示する

Related issues

Related to OpenPNE 3 - Bug(バグ) #2074: MemberConfigのフィードタイプcheckboxが機能していない Invalid(無効) 2011-05-10

Associated revisions

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

(fixes #2478) fixed to change the flow for saving and displaying MemberProfile

Revision acb9b1cc (diff)
Added by Yuya Watanabe over 6 years ago

Revert "(refs #2478) fixed to change the flow for saving and displaying MemberProfile"

This reverts commit f84facbf5ea0f7433881b5ccfd6735763ecf61bc.

History

#1 Updated by Yuya Watanabe almost 8 years ago

問題かどうかはわからないが,「テキスト」と「単一選択」の場合にも同じレコードにデータが保存されてしまう.

#2 Updated by Yuya Watanabe almost 8 years ago

メモ

記録に関して関係ありそうな部分のコードを記載.

lib/action/opMemberAction.class.php

209   public function executeEditProfile($request)
210   {
211     $this->memberForm = new MemberForm($this->getUser()->getMember());
212 
213     $profiles = $this->getUser()->getMember()->getProfiles();
214     $this->profileForm = new MemberProfileForm($profiles);
215     $this->profileForm->setConfigWidgets();
216 
217     if ($request->isMethod('post'))
218     {
219       $this->memberForm->bind($request->getParameter('member'));
220       $this->profileForm->bind($request->getParameter('profile'));
221       if ($this->memberForm->isValid() && $this->profileForm->isValid())
222       {
223         $this->memberForm->save();
224         $this->profileForm->save($this->getUser()->getMemberId());
225         $this->redirect('@member_profile_mine');
226       }
227     }
228 
229     return sfView::SUCCESS;
230   }

lib/form/doctrine/MemberProfileForm.class.php

 43   public function save($memberId)
 44   {
 45     $values = $this->getValues();
 46 
 47     foreach ($values as $key => $value)
 48     {
 49       $profile = Doctrine::getTable('Profile')->retrieveByName($key);
 50       if (!$profile)
 51       {
 52         continue;
 53       }
 54 
 55       $memberProfile = Doctrine::getTable('MemberProfile')->retrieveByMemberIdAndProfileId($memberId, $profile->getId());
 56 
 57       if (is_null($value['value']))
 58       {
 59         if ($memberProfile)
 60         {
 61           if ($profile->isMultipleSelect())
 62           {
 63             $memberProfile->clearChildren();
 64           }
 65           $memberProfile->delete();
 66         }
 67         continue;
 68       }
 69       if (!$memberProfile)
 70       {
 71         $memberProfile = new MemberProfile();
 72         $memberProfile->setMemberId($memberId);
 73         $memberProfile->setProfileId($profile->getId());
 74       }
 75 
 76       $memberProfile->setPublicFlag($memberProfile->getProfile()->getDefaultPublicFlag());
 77       if (isset($value['public_flag']))
 78       {
 79         $memberProfile->setPublicFlag($value['public_flag']);
 80       }
 81       $memberProfile->save();
 82 
 83       if ($profile->isMultipleSelect())
 84       {
 85         $ids = array();
 86         $_values = array();
 87         if ('date' === $profile->getFormType())
 88         {
 89           $_values = array_map('intval', explode('-', $value['value']));
 90           $options = $profile->getProfileOption();
 91           foreach ($options as $option)
 92           {
 93             $ids[] = $option->getId();
 94           }
 95           $memberProfile->setValue($value['value']);
 96         }
 97         else
 98         {
 99           $ids = $value['value'];
100         }
101         Doctrine::getTable('MemberProfile')->createChild($memberProfile, $memberId, $profile->getId(), $ids, $_values);
102       }
103       else
104       {
105         $memberProfile->setValue($value['value']);
106       }
107 
108       $memberProfile->save();
109     }
110 
111     return true;
112   }

lib/model/doctrine/MemberProfileTable.class.php

 88   public function retrieveByMemberIdAndProfileId($memberId, $profileId)
 89   {
 90     return $this->createQuery()
 91       ->where('member_id = ?', $memberId)
 92       ->andWhere('profile_id = ?', $profileId)
 93       ->fetchOne();
 94   }

#3 Updated by Yuya Watanabe almost 8 years ago

  • Status changed from New(新規) to Accepted(着手)
  • Assignee set to Yuya Watanabe
  • Target version set to OpenPNE 3.7.0

#4 Updated by Yuya Watanabe almost 8 years ago

メモ

データ取得に関して関係ありそうなコードを記載.

lib/action/opMemberAction.class.php 213行目

209   public function executeEditProfile($request)
210   {
211     $this->memberForm = new MemberForm($this->getUser()->getMember());
212 
213     $profiles = $this->getUser()->getMember()->getProfiles();
214     $this->profileForm = new MemberProfileForm($profiles);

lib/model/doctrine/Member.class.php

 13   public function getProfiles($viewableCheck = false, $myMemberId = null)
 14   {
 15     if ($viewableCheck)
 16     {
 17       return Doctrine::getTable('MemberProfile')->getViewableProfileListByMemberId($this->getId(), $myMemberId);
 18     }
 19 
 20     return Doctrine::getTable('MemberProfile')->getProfileListByMemberId($this->getId());
 21   }

lib/model/doctrine/MemberProfileTable.class.php

 12 {
 13   public function getProfileListByMemberId($memberId)
 14   {
 15     $profiles = Doctrine::getTable('Profile')->createQuery()
 16       ->select('id')
 17       ->orderBy('sort_order')
 18       ->execute(array(), Doctrine::HYDRATE_NONE);
 19 
 20     $queryCacheHash = '';
 21 
 22     $q = $this->createQuery()
 23       ->where('member_id = ?')
 24       ->andWhere('profile_id = ?')
 25       ->andWhere('level = 0')
 26       ->orderBy('id');
 27 
 28     $memberProfiles = array();
 29     foreach ($profiles as $profile)
 30     {
 31       if ($queryCacheHash)
 32       {
 33         $q->setCachedQueryCacheHash($queryCacheHash);
 34       }
 35 
 36       $memberProfile = $q->fetchOne(array($memberId, $profile[0]));
 37       if ($memberProfile)
 38       {
 39         $memberProfiles[] = $memberProfile;
 40       }
 41 
 42       if (!$queryCacheHash)
 43       {
 44         $queryCacheHash = $q->calculateQueryCacheHash();
 45       }
 46     }
 47 
 48     // NOTICE: this returns Array not Doctrine::Collection
 49     return $memberProfiles;
 50   }

apps/pc_frontend/modules/member/templates/_profileListBox.php

 28   $profileValue = (string)$profile;

lib/model/doctrine/MemberProfile.class.php

 13   public function __toString()
 14   {
 15     if ('date' !== $this->getFormType())
 16     {
 17       if ($this->getProfileOptionId())
 18       {
 19         $option = Doctrine::getTable('ProfileOption')->find($this->getProfileOptionId());
 20         return (string)$option->getValue();
 21       }
 22 
 23       $children = $this->getChildrenValues(true);
 24       if ($children)
 25       {
 26         return implode(', ', $children);
 27       }
 28     }
 29 
 30     return (string)$this->getValue();
 31   }

 44   public function getValue()
 45   {
 46     if ($this->_get('value_datetime'))
 47     {
 48       if ($this->_get('value'))
 49       {
 50         if ('0000-00-00 00:00:00' === $this->_get('value_datetime'))
 51         {
 52           return null;
 53         }
 54 
 55         $obj = new DateTime($this->_get('value_datetime'));
 56         return $obj->format('Y-m-d');
 57       }
 58 
 59       return null;
 60     }
 61 
 62     if ($this->getProfile()->isPreset())
 63     {
 64       if ('op_preset_birthday' === $this->getProfile()->getName())
 65       {
 66         return null;
 67       }
 68 
 69       return $this->_get('value');
 70     }
 71     elseif ('date' !== $this->getFormType() && $this->getProfileOptionId())
 72     {
 73       return $this->getProfileOptionId();
 74     }
 75 
 76     $children = $this->getChildrenValues();
 77     if ($children)
 78     {
 79       if ('date' === $this->getFormType())
 80       {
 81         if (count($children) == 3 && $children[0] && $children[1] && $children[2])
 82         {
 83           $obj = new DateTime();
 84           $obj->setDate($children[0], $children[1], $children[2]);
 85           return $obj->format('Y-m-d');
 86         }
 87         return null;
 88       }
 89       return $children;
 90     }

#5 Updated by Yuya Watanabe almost 8 years ago

メモ

データベース内で木構造を表現する手法として「入れ子集合モデル」というものがあり,本チケットで扱われている複数項目のレコード記録方法としてこのモデルが使われているようである.

config/doctrine/schema.yml 145行目でNestedSetと定義されている

143 MemberProfile:
144   actAs:
145     NestedSet:
146       hasManyRoots: true
147       rootColumnName: tree_key
148     Timestampable:
149   columns:
150     id: { type: integer(4), primary: true, autoincrement: true, comment: "Serial number" }
151     member_id: { type: integer(4), notnull: true, comment: "Member id" }
152     profile_id: { type: integer(4), notnull: true, comment: "Profile id" }
153     profile_option_id: { type: integer(4), comment: "Profile option id" }
154     value: { type: string, default: "", notnull: true, comment: "Text content for this profile item" }
155     value_datetime: { type: timestamp, comment: "Profile datetime value" }
156     public_flag: { type: integer(1), comment: "Public flag" }
157   relations:
158     Member: { local: member_id, foreign: id, onDelete: cascade }
159     Profile: { local: profile_id, foreign: id, onDelete: cascade }
160     ProfileOption: { local: profile_option_id, foreign: id, onDelete: cascade }
161   indexes:
162     lft_INDEX:
163       fields: [lft]
164   options:
165     type: INNODB
166     collate: utf8_unicode_ci
167     charset: utf8
168     comment: "Saves informations of every member''s profile" 

#6 Updated by Yuya Watanabe almost 8 years ago

保存する際に既存の値を保持しておく必要はないと思うので,あるメンバがそのプロフィール項目ですでに保存した値がある場合はそのレコードを上書きするのではなく削除してあたらしく作りなおす方針を検討する.

このとき,$profile->isMultipleSelect()は管理画面で設定した値を用いているため,これから記録しようとするデータに対してこれを用いるのは問題ないが,DB内にあるデータを表していないので記録の前処理で行う削除時にisMultipleSelect()の値を用いるとデータに則さない処理を行なってしまう可能性があることに注意する必要がある.おそらく$memberProfile->getNode()->hasChildren()などで判定できると思われる.

#7 Updated by Yuya Watanabe almost 8 years ago

note-6の修正案を適用した実装案について

実装案

diff --git a/lib/form/doctrine/MemberProfileForm.class.php b/lib/form/doctrine/MemberProfileForm.class.php
index 83bb56f..0ab1d03 100644
--- a/lib/form/doctrine/MemberProfileForm.class.php
+++ b/lib/form/doctrine/MemberProfileForm.class.php
@@ -54,25 +54,24 @@ class MemberProfileForm extends BaseForm

       $memberProfile = Doctrine::getTable('MemberProfile')->retrieveByMemberIdAndProfileId($memberId, $profile->getId());

-      if (is_null($value['value']))
+      if ($memberProfile)
       {
-        if ($memberProfile)
+        if ($memberProfile->getNode()->hasChildren())
         {
-          if ($profile->isMultipleSelect())
-          {
-            $memberProfile->clearChildren();
-          }
-          $memberProfile->delete();
+          $memberProfile->clearChildren();
         }
-        continue;
+        $memberProfile->delete();
       }
-      if (!$memberProfile)
+
+      if (is_null($value['value']))
       {
-        $memberProfile = new MemberProfile();
-        $memberProfile->setMemberId($memberId);
-        $memberProfile->setProfileId($profile->getId());
+        continue;
       }

+      $memberProfile = new MemberProfile();
+      $memberProfile->setMemberId($memberId);
+      $memberProfile->setProfileId($profile->getId());
+
       $memberProfile->setPublicFlag($memberProfile->getProfile()->getDefaultPublicFlag());
       if (isset($value['public_flag']))
       {

問題点

表面の動作的には問題ないが,プロフィールを更新するたびにDB側のmember_profileテーブルでプロフィール数+複数選択分のIDが消費されてゆく.

#8 Updated by Yuya Watanabe almost 8 years ago

修正方針

テキスト,単一選択,複数選択それぞれのフォームタイプの場合の値が同時に保存されているようなので,フォームタイプに即した値を出力時に返すようにする.

実装案

diff --git a/lib/model/doctrine/MemberProfile.class.php b/lib/model/doctrine/MemberProfile.class.php
index a085613..a6368a4 100644
--- a/lib/model/doctrine/MemberProfile.class.php
+++ b/lib/model/doctrine/MemberProfile.class.php
@@ -14,16 +14,23 @@ class MemberProfile extends BaseMemberProfile implements opAccessControlRecordIn
   {
     if ('date' !== $this->getFormType())
     {
-      if ($this->getProfileOptionId())
+      if ($this->Profile->isMultipleSelect())
       {
-        $option = Doctrine::getTable('ProfileOption')->find($this->getProfileOptionId());
-        return (string)$option->getValue();
+        $children = $this->getChildrenValues(true);
+        if ($children)
+        {
+          return implode(', ', $children);
+        }
+        else
+        {
+          return '';
+        }
       }
-
-      $children = $this->getChildrenValues(true);
-      if ($children)
+      if ($this->Profile->isSingleSelect() && $this->getProfileOptionId())
       {
-        return implode(', ', $children);
+        $option = Doctrine::getTable('ProfileOption')->find($this->getProfileOptionId());
+
+        return (string)$option->getValue();
       }
     }

@@ -68,17 +75,27 @@ class MemberProfile extends BaseMemberProfile implements opAccessControlRecordIn

       return $this->_get('value');
     }
-    elseif ('date' !== $this->getFormType() && $this->getProfileOptionId())
+    elseif ('date' !== $this->getFormType())
     {
-      return $this->getProfileOptionId();
+      if ($this->Profile->isMultipleSelect())
+      {
+        $children = $this->getChildrenValues();
+        if ($children)
+        {
+          return $children;
+        }
+      }
+      elseif ($this->Profile->isSingleSelect() && $this->getProfileOptionId())
+      {
+        return $this->getProfileOptionId();
+      }
     }
-
-    $children = $this->getChildrenValues();
-    if ($children)
+    else
     {
-      if ('date' === $this->getFormType())
+      $children = $this->getChildrenValues();
+      if ($children)
       {
-        if (count($children) == 3 && $children[0] && $children[1] && $children[2])
+        if (3 =< count($children) && $children[0] && $children[1] && $children[2])
         {
           $obj = new DateTime();
           $obj->setDate($children[0], $children[1], $children[2]);
@@ -86,7 +103,6 @@ class MemberProfile extends BaseMemberProfile implements opAccessControlRecordIn
         }
         return null;
       }
-      return $children;
     }

     return parent::rawGet('value');

問題点

この修正案の場合には日付がどのように記録されているかを考慮していなかった.
実際にDBを確認してみると,日付の値は年月日それぞれを子ノードの列valueに保存されていることを確認した.
しかし,年月日が保存されるかどうかは子ノードが存在しないまたは子ノードの数が3つ以上であるときのみ正しく記録されることを確認した.
つまり,複数選択によってすでに値がDBに記録されている場合,フォームタイプが日付の場合に正しく記録できない状態であると言える.
そのため,表示部分だけでなく同時に記録部分も修正する必要がある.

#9 Updated by Yuya Watanabe almost 8 years ago

修正方針

DB内にはテキスト,単一選択,複数選択,日付の4つのフォームタイプの値を共存させつつ,取得時にはそのフォームタイプに従った値を返すように修正します.

現在の状態だと取得時にはDB内の値が存在するかどうかで返す値を決定しているので,複数のフォームタイプの値がDB内に記録されていると正しい値を返さないという問題を解決するためにnote-8のような修正を行います.

また,note-8の修正だけでは4つのフォームタイプの値を共存させるという条件を満たしていないため,正しく値を記録できるような実装を行います.

#10 Updated by wa ta almost 8 years ago

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

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

#11 Updated by Minoru Takai almost 8 years ago

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

詳細まで追っていないので不具合が生じているというだけの指摘になりますが、次の問題が生じています。

  1. 管理画面からプロフィール項目に「日付」のものを追加
  2. メンバー側でプロフィール編集を開き、(特に値を変更せずに)保存する
  3. 次の Fatal Error が生じる
    Fatal error: Call to a member function setValue() on a non-object
     in /path/to/OpenPNE/lib/form/doctrine/MemberProfileForm.class.php on line 99
    
    lib/form/doctrine/MemberProfileForm.class.php
    87-        $children = $memberProfile->getNode()->getChildren();
    88-        if ('date' === $profile->getFormType())
    89-        {
    90-          foreach ($children as $child)
    91-          {
    92-            $child->setValue(null);
    93-            $child->save();
    94-          }
    95-          $_values = array_map('intval', explode('-', $value['value']));
    96-          $options = $profile->getProfileOption();
    97-          foreach ($_values as $i => $value)
    98-          {
    99-            $children[$i]->setValue($value);
    100-            $children[$i]->save();
    101-          }
    102-        }
    103-        else
    

#12 Updated by Shouta Kashiwagi over 7 years ago

  • Target version changed from OpenPNE 3.7.0 to 252

#13 Updated by Shouta Kashiwagi over 7 years ago

  • Target version changed from 252 to OpenPNE 3.8.x

#14 Updated by Yuya Watanabe almost 7 years ago

  • Target version changed from OpenPNE 3.8.x to OpenPNE 3.9.0-old
  • 3.6 で発生するか set to Unknown (未調査)
  • 3.8 で発生するか set to Unknown (未調査)

#15 Updated by isao sano over 2 years ago

対象バージョン変更のため、修正内容の確認を行います。

#16 Updated by isao sano over 2 years ago

  • Target version changed from OpenPNE 3.9.0-old to OpenPNE 3.9.0

#17 Updated by isao sano over 2 years ago

再現を確認しました(2017/04/06)

#18 Updated by Shinichi Urabe about 2 years ago

最初に登録したフォームタイプを編集で変更できない仕様としてもいいように思いますが、どうでしょうか。

→ フォームタイプが違うプロフィールに変更したい場合は、削除して追加する仕様とする

Also available in: Atom PDF