farend / redmine_message_customize

This is a plugin for Redmine.
GNU General Public License v2.0
29 stars 11 forks source link

Change the key with non-string value to be not customizable #21

Closed ishikawa999 closed 5 years ago

ishikawa999 commented 5 years ago

Fix: #11

Make it impossible to customize a key whose value is something other than a string. example: date.day_names, number.precision

ishikawa999 commented 5 years ago

I will close this pull request as there are other things to fix.

ishikawa999 commented 5 years ago

@yui-har

Processing is created on the assumption that CustomMessageSetting#custom_messages returns Hash. However, there is some coding that expects something other than Hash to be returned. I think that it should be unified in Hash(or ActiveSupport::HashWithIndifferentAccess).

レビュー対象行ではありませんけど。。。 CustomMessageSetting#custom_messages は Hash(または ActiveSupport::HashWithIndifferentAccess) を返していますが、 このメソッドを呼び出している箇所にHash以外が返ってくることを想定した処理を記述しているところが存在します。 たぶん通過することのない処理と思われます。

update_with_custom_messages_yamlで例外が発生した場合にvalueにStringを入れるケースがあったため、 custom_messagesがStringを返すことがありました。 その部分を修正し、custom_messagesはHashのみを返すようにしました。( d113005) 再度確認をお願いいたします。

ishikawa999 commented 5 years ago

@yui-har Thank you for the review. Could you please review again?

変更内容についての説明 Fix test to succeed without setting RAILS_ENV=test 853208e テストがRAILS_ENV=testなしでも動くように修正。本来config/enviroinments/test.rbで読み込まれる必要な設定を追加しています。 Clean up CustomMessageSetting from #21#pullrequestreview-263452731 1fb797e https://github.com/ishikawa999/redmine_message_customize/pull/21#pullrequestreview-263452731 で貼っていただいたdiffをそのまま取り込みました。 Change to catch Psych::SyntaxError 2665b65 1fb797e の変更では ``` en: label_home: home1 a ``` などのようにYAMLのルールに沿わないものを保存しようとしたとき例外が発生します。 それをキャッチしてerrors.add出来るように修正しました。 Change to return the result of YAML.dump when a validation error occu… … 5eae987 self.valid?の場合hashをyamlに変換、そうでない場合はそのままを返していましたが、custom_message_languages_are_availableなどのバリデーションで引っかかった場合はYAMLに変換して値を返して欲しいため、custom_messages.is_a?(Hash)という条件に変更しました。 Fix the problem of merging keys in custom_messages ba52685 deep_mergeを利用すると、既存の設定が ``` en: label_home: aaa label_my_page: bbb ``` だったものを ``` en: label_home: ccc ``` に書き換えた場合、 ``` en: label_home: ccc label_my_page: bbb ``` のようになってしまいます。 そうならないように修正しました。 Fix the process self.to_hash that always results in an error fbc07a0 flatten_hashはクラスメソッドなので、CustomMessageSetting.to_hashのような処理となっていたのを修正しました。 Change the value returned by update_with_custom_messages_yaml to true… … 4ac430d Psych::SyntaxErrorをキャッチしたときメソッドがfalseを返すよう修正 Fix test b0b8ed9 @invalid_yamlを使わなくなったため、テストを修正 Support to setting.value ["custom_messages"] 27eaa12 raw_custom_messagesなどで'custom_messages'がキーになっている場合も対応していましたが、ここはシンボルのみでとるようになっていたため修正しました。
ishikawa999 commented 5 years ago

@yui-har ご提案いただいたコードに合わせて修正を加えました。 他に気になる点はございますか?