Defect #28000
closedDeletion of an LDAP authentication mode may fail silently
Added by Go MAEDA almost 7 years ago. Updated over 6 years ago.
0%
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.
Files
show-ldap-deletion-error.diff (1.35 KB) show-ldap-deletion-error.diff | patch | Go MAEDA, 2018-01-16 16:34 | |
show-ldap-deletion-error-v2.diff (1.79 KB) show-ldap-deletion-error-v2.diff | Go MAEDA, 2018-01-17 14:26 | ||
show-ldap-deletion-error-v3.diff (1.8 KB) show-ldap-deletion-error-v3.diff | Go MAEDA, 2018-01-20 03:11 |
Updated by Toshi MARUYAMA almost 7 years ago
- Target version deleted (
4.0.0)
Could you add test?
source:trunk/test/functional/auth_sources_controller_test.rb#L154
Updated by Go MAEDA almost 7 years ago
Added an assertion in the test.
Updated by Toshi MARUYAMA almost 7 years 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
Updated by Go MAEDA almost 7 years 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".
Updated by Go MAEDA almost 7 years 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:
- 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.
Updated by Toshi MARUYAMA almost 7 years 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#L142Do you think using raw text is better?
Yes. Rails changed behaviour many times. It cannot guarantee test result in the future.
Updated by Jens Krämer almost 7 years 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.
Updated by Go MAEDA almost 7 years ago
Toshi MARUYAMA and Jens Krämer, thank you for your advice!
I have updated the test to use raw text.
Updated by Go MAEDA almost 7 years 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.
Updated by Go MAEDA over 6 years ago
- Status changed from New to Closed
- Assignee set to Go MAEDA
- Resolution set to Fixed
Committed.