Project

General

Profile

Backport(バックポート) #2294

When a value of MemberConfig's "age_public_flag" is not exists, the label of "Settings" is inconsistent with behavior (MemberConfigの "age_public_flag" の値が存在しないとき、「設定変更」の表示と実際の挙動が異なる)

Added by Yuma Sakata almost 8 years ago. Updated over 7 years ago.

Status:
Fixed(完了)
Priority:
High(高め)
Target version:
Start date:
2011-01-17
Due date:
% Done:

100%


Description

Overview

When a value of MemberConfig's "age_public_flag" is not exists, the caption of "Settings" is inconsistent with behavior.

MemberConfigの "age_public_flag" の値が空のとき、
「設定変更」での表示は「全員に公開」と表示されているのにも関わらず、
実際は「非公開」として取り扱われる。

非常に紛らわしい状況になっている。

Causes

lib/config/config/member_config.yml の age_public_flag
の Default が "1" になっている。

Way to fix

Default を 3 (private) としておくべき。

(表示の方を現状の挙動に合わせる。
これは、今までリリースしてきた各安定版バージョンが
存在するからである。)

Environment

OpenPNE3.4.x 〜


Related issues

Related to OpenPNE 3 - Bug(バグ) #1872: When a value of MemberConfig's "age_public_flag" is not exists, the label of "Settings" is inconsistent with behavior (MemberConfigの "age_public_flag" の値が存在しないとき、「設定変更」の表示と実際の挙動が異なる) Fixed(完了) 2011-01-17
Related to OpenPNE 3 - Backport(バックポート) #1881: When a value of MemberConfig's "age_public_flag" is not exists, the label of "Settings" is inconsistent with behavior (MemberConfigの "age_public_flag" の値が存在しないとき、「設定変更」の表示と実際の挙動が異なる) Fixed(完了) 2011-01-17 2011-06-24

Associated revisions

Revision c5e9121e (diff)
Added by Shinichi Urabe over 7 years ago

fixed the configuration of member_config, the label of settings is inconsistent with behavior when a value of MemberConfig's "age_public_flag" isn't exists (refs #2294, BP from #1872)

Conflicts:

lib/config/config/member_config.yml

Revision 1be0f847 (diff)
Added by Shinichi Urabe over 7 years ago

Member::getAge() now never handles null as age_public_flag value (refs #2294, BP from #1872)

Conflicts:

lib/model/doctrine/Member.class.php

Revision c977c64d
Added by Shinichi Urabe over 7 years ago

Merge branch 't2294' into stable-3.4.x (fixes #2294, BP from #1872)

Revision 895d027d (diff)
Added by Kousuke Ebihara over 7 years ago

fixed comparing ProfileTable::PUBLIC_FLAG_* to a value to follow conding standard in the Member::getAge() (fixes #2294, BP from #1872)

Conflicts:

lib/model/doctrine/Member.class.php

Revision ce11fcb0 (diff)
Added by Shinichi Urabe over 7 years ago

I changed the Clause 3 operator to the description not to use. (fixes #2294)

History

#1 Updated by Shinichi Urabe over 7 years ago

  • Assignee set to Shinichi Urabe

#2 Updated by Shinichi Urabe over 7 years ago

コンフリクトの修正について

c5e9121ebf2c95bb1b2e でのコンフリクトについて

  • Default:1は外部公開機能の c615f5d4f578043b1cdc3bcaa52e438793857f5a で混入されたもの (外部公開機能はないが、デフォルト値を設定する必要があるため、Default: 3 を設定)

c5e9121ebf2c95bb1b2e でのコンフリクトについて

  • Member::getConfig() の第二引数にデフォルト値がセット出来るのは3.6の機能で 3.4には存在しないので、nullと結果を比較する対処した

#3 Updated by Shinichi Urabe over 7 years ago

  • Status changed from New(新規) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

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

#4 Updated by Yuma Sakata over 7 years ago

テストOKです。

#5 Updated by Minoru Takai over 7 years ago

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

レビューしました。

修正内容としては問題が解消しているためこれで OK としても良いのですが、以下について修正可能であれば対応して頂きたいです。

差し戻し内容

2 個目の修正内容について

http://redmine.openpne.jp/issues/1872#note-7 では「(3.4 の場合は Member::getAge() 内に条件分岐を追加する)」と示されていますが、

    $publicFlag = $this->getConfig('age_public_flag');
    $publicFlag = null === $publicFlag ? ProfileTable::PUBLIC_FLAG_PRIVATE : $publicFlag;
    if (!$viewableCheck || $publicFlag == ProfileTable::PUBLIC_FLAG_SNS)

この箇所で、このように条件演算子を用いるとこの部分の意味が分かりにくいので、 if 文で記述した方がよいと思います。

    $publicFlag = $this->getConfig('age_public_flag');
    if (null === $publicFlag)
    {
      $publicFlag = ProfileTable::PUBLIC_FLAG_PRIVATE;
    }
    if (!$viewableCheck || $publicFlag == ProfileTable::PUBLIC_FLAG_SNS)

3 個目の修正が取り込まれていません

親チケットでは 3 個のコミットが行なわれていますが、このチケットでは 3 個目のコミット(コーディング規約違反を修正する内容)が取り込まれていません。取り込む必要がないと判断したのであればそれでもよいですが、コミットを見逃している可能性があるので確認しておきます。参考 http://redmine.openpne.jp/issues/1872#note-19


上記の修正を行うか、修正する必要がないと判断した旨をコメントするかの対応をお願いします。

#6 Updated by Shinichi Urabe over 7 years ago

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

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

#7 Updated by Kousuke Ebihara over 7 years ago

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

#8 Updated by Yuma Sakata over 7 years ago

コーディング規約違反の修正を取り込みにあたり、正常系動作のみ再テストしました。
問題ありません。

#9 Updated by Minoru Takai over 7 years ago

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

note-6, note-7 での取り込みをレビューしました。

親チケット 3 個目のコミットの取り込みについて、親チケットでは 3 箇所が修正されていますが、本チケットでは 2 箇所しか修正されておらずコンフリクトが生じています。しかしながら、これは 3 箇所目の修正部分が #667 でのコミット c615f5d4 (3.5.0 時点) で追加されたものであるためであり、取り込みは問題ないと判断できます。

親チケット 2 個目のコミットで、条件演算子(三項演算子)ではなく if 文を用いたほうがよいという指摘に対する修正について、修正が適切に行なわれていると判断できます。

ちなみに note-6 のコミットメッセージを拝見しましたが、一般的に expr ? val : val の演算子は「条件演算子」または「三項演算子(条件演算子以外に三項の演算子がないため)」と呼ばれており、英語では Conditional Operator (条件演算子)または Ternary Operator (三項演算子 : see PHP Manual )と表記されます。

レビュー OK とします。

#10 Updated by Yuma Sakata over 7 years ago

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

Also available in: Atom PDF