Vote #79072: Deletion of an LDAP authentication mode may fail silently - redmineorg-copy202205 - Redmine

編集操作

リンクをコピー

Vote #79072

完了

Deletion of an LDAP authentication mode may fail silently

Admin Redmine さんが3年以上前に追加. 3年以上前に更新.

ステータス:

Closed

優先度:

通常

担当者:

カテゴリ:

LDAP_28

対象バージョン:

4.0.0_99

開始日:

2022/05/09

期日:

進捗率:

予定工数:

Redmineorg_URL:

https://www.redmine.org/issues/28000

category_id:

version_id:

issue_org_id:

28000

author_id:

332

assigned_to_id:

332

comments:

status_id:

tracker_id:

plus1:

affected_version:

closed_on:

affected_version_id:

ステータス-->[Closed]

引用

説明

Deletion of an LDAP authentication mode fails without any error message if one or more users use it. An error message should be displayed.

journals

Setting target version to 4.0.0.
--------------------------------------------------------------------------------
Could you add test?
source:trunk/test/functional/auth_sources_controller_test.rb#L154
--------------------------------------------------------------------------------
Added an assertion in the test.
--------------------------------------------------------------------------------
You can use raw English text and I think you can use "assert_select_error".
source:trunk/test/functional/auth_sources_controller_test.rb#L142
--------------------------------------------------------------------------------
Toshi MARUYAMA wrote:
> You can use raw English text and I think you can use "assert_select_error".
> source:trunk/test/functional/auth_sources_controller_test.rb#L142

Do you think using raw text is better? I think I18n.t is better because we don't have to update the test when the English translation for error_can_not_delete_auth_source is changed. And we cannot use @assert_select_error@ for the page because there isn't "div#errorExplanation".
--------------------------------------------------------------------------------
Toshi MARUYAMA wrote:
> You can use raw English text and I think you can use "assert_select_error".
> source:trunk/test/functional/auth_sources_controller_test.rb#L142

I think that checking the @flash@ hash is a common way to test flash notices. Please see "7.7 Testing flash notices":http://guides.rubyonrails.org/testing.html#testing-flash-notices on Rails Guides. Also in Redmine, the same way is used. Examples as follows:

* source:tags/3.4.4/test/functional/timelog_controller_test.rb#L678
* source:tags/3.4.4/test/functional/timelog_controller_test.rb#L689
* source:tags/3.4.4/test/functional/my_controller_test.rb#L362

I think that @assert_select_error@ can be used for validation errors brought by @ActiveRecord::Errors@ object but cannot be used for @flash@ hash.
--------------------------------------------------------------------------------
Go MAEDA wrote:
> Toshi MARUYAMA wrote:
> > You can use raw English text and I think you can use "assert_select_error".
> > source:trunk/test/functional/auth_sources_controller_test.rb#L142
>
> Do you think using raw text is better?

Yes. Rails changed behaviour many times. It cannot guarantee test result in the future.

--------------------------------------------------------------------------------
I agree with Toshi, using the raw text in the assertion is preferable. It simplifies the statement and makes it more obvious what is expected.
--------------------------------------------------------------------------------
Toshi MARUYAMA and Jens Krämer, thank you for your advice!
I have updated the test to use raw text.
--------------------------------------------------------------------------------
I fixed the patch as advised by Toshi and Jens. Setting target version to 4.0.0 again.
--------------------------------------------------------------------------------
Committed.
--------------------------------------------------------------------------------