Defect #28000

Deletion of an LDAP authentication mode may fail silently

Added by Go MAEDA about 1 month ago. Updated about 1 month ago.

Status:NewStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:LDAP
Target version:4.0.0
Resolution: Affected version:

Description

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

show-ldap-deletion-error.diff Magnifier - patch (1.35 KB) Go MAEDA, 2018-01-16 16:34

show-ldap-deletion-error-v2.diff Magnifier (1.79 KB) Go MAEDA, 2018-01-17 14:26

show-ldap-deletion-error-v3.diff Magnifier (1.8 KB) Go MAEDA, 2018-01-20 03:11

History

#1 Updated by Go MAEDA about 1 month ago

  • Target version set to 4.0.0

Setting target version to 4.0.0.

#2 Updated by Toshi MARUYAMA about 1 month ago

  • Target version deleted (4.0.0)

#3 Updated by Go MAEDA about 1 month ago

Added an assertion in the test.

#4 Updated by Toshi MARUYAMA about 1 month ago

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

#5 Updated by Go MAEDA about 1 month ago

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".

#6 Updated by Go MAEDA about 1 month ago

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 on Rails Guides. Also in Redmine, the same way is used. Examples as follows:

I think that assert_select_error can be used for validation errors brought by ActiveRecord::Errors object but cannot be used for flash hash.

#7 Updated by Toshi MARUYAMA about 1 month ago

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.

#8 Updated by Jens Krämer about 1 month ago

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.

#9 Updated by Go MAEDA about 1 month ago

Toshi MARUYAMA and Jens Krämer, thank you for your advice!
I have updated the test to use raw text.

#10 Updated by Go MAEDA about 1 month ago

  • Target version set to 4.0.0

I fixed the patch as advised by Toshi and Jens. Setting target version to 4.0.0 again.

Also available in: Atom PDF