ここの情報は古いです。ご理解頂いた上でお取り扱いください。

Opened 11 years ago

Closed 11 years ago

#2998 closed defect (fixed)

CSVダウンロードで存在しないメンバーIDを指定するとメンバー情報のないCSVファイルをDLできてしまう

Reported by: kiwa Owned by: shingo
Priority: minor Milestone: OpenPNE2.12.4
Component: 指定しない Version: 2.10.x & 2.12.x & 2.14.x
Keywords: OpenPNE2.10.10 OpenPNE2.13.3 Cc:

Description (last modified by shingo)

■現象

CSVダウンロードで存在しないメンバーIDを指定するとメンバー情報のないCSVファイルをDLできてしまう

■原因

該当するメンバー情報が無い時でもCSV出力がなされてしまう。

■修正内容

該当するメンバー情報が無い場合、エラーを出すように修正。

■関連情報

Change History (12)

comment:1 Changed 11 years ago by shingo

Description: modified (diff)
Keywords: OpenPNE2.10.10 OpenPNE2.13.3 確認中 added
Milestone: OpenPNE2.12.4
Owner: changed from nobody to shingo
Status: newassigned

作業します。

comment:2 Changed 11 years ago by shingo

Description: modified (diff)

■現象

CSVダウンロードで存在しないメンバーIDを指定するとメンバー情報のないCSVファイルをDLできてしまう

■原因

該当するメンバー情報が無い時でもCSV出力がなされてしまう。

■修正内容

該当するメンバー情報が無い場合、エラーを出すように修正。

■関連情報

comment:3 Changed 11 years ago by shingo

Description: modified (diff)

以下のリビジョンで修正しました。ご確認ください。

comment:4 in reply to:  2 Changed 11 years ago by shingo

Replying to shingo: 記入欄を間違えたので上記の情報は無視してください。

comment:5 Changed 11 years ago by ebihara

Keywords: 差し戻し added; 確認中 removed

次に挙げる理由から、この変更は少しわかりにくいと感じます。

  • 26行目の $errors と、今回40行目に追加された $errors が、同じ変数名なのに用途が微妙に食い違っています
    • 26行目の $errors は入力されたメンバーIDに関するエラーを表す一方、40行目の $errors はメンバーリストに関するエラーを表しています
    • 一度定義された変数の役割を途中から変更してしまうのはあまり好ましくないと感じます
    • 40行目の $errors の定義部分がわかりにくい(役割が変更されたことに気がつかない)のも混乱のもとかもしれません
  • 似たようなエラー処理が二ヶ所で個別に記述されています
  • エラーの種類が一個なのに、「空配列を定義」→「エラーが存在すれば配列の要素を追加」という処理をわざわざおこなうのはやりすぎです

もし修正するのであれば、以下のような修正方法を提案します。

  • $errors を別の名前にし、何についてのエラーを表しているのかを明確にする
        26	        $id_format_errors = array(); 
                :
        40	        $c_member_list_errors = array(); 
    
  • c_member_list についてのエラー処理には $errors を使わない
        38	        $c_member_list = $this->db_get_c_member_list($start_id,$end_id); 
        39	        if (!$c_member_list) { 
        40	            $this->handleError(array('該当するメンバーの情報がありません。')); 
        41	        } 
    
        38	        $c_member_list = $this->db_get_c_member_list($start_id,$end_id); 
        39	        if (!$c_member_list) { 
        40	            admin_client_redirect('csv_download', '該当するメンバーの情報がありません。'); 
        41	        } 
    
  • エラー処理をすべて冒頭部分でおこなえるようにする(具体案浮かばず)

comment:6 Changed 11 years ago by shingo

Keywords: 確認 added; 差し戻し removed

以下のリビジョンで修正しました。ご確認ください。

comment:7 Changed 11 years ago by kiwa

Keywords: 確認中 added; 確認 removed

comment:8 Changed 11 years ago by ebihara

Keywords: 差し戻し added; 確認中 removed
  • 38行目から47行目までのコードは明らかに不要です
  • 48行目でメソッドのコールがおこなわれていますが、引数と引数の間にスペースがありません。PEAR標準コーディング規約に準拠してください

comment:9 Changed 11 years ago by shingo

以下のリビジョンで修正しました。ご確認ください。

comment:10 Changed 11 years ago by shingo

Keywords: 確認中 added; 差し戻し removed

comment:11 Changed 11 years ago by ogawa

Keywords: テスト待ち added; 確認中 removed

comment:12 Changed 11 years ago by kiwa

Keywords: テスト待ち removed
Resolution: fixed
Status: assignedclosed

確認しました。問題ありません。

Note: See TracTickets for help on using tickets.