Closed (fixed)
Project:
Password Policy
Version:
4.0.x-dev
Component:
Code
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
2 Aug 2024 at 01:31 UTC
Updated:
21 Aug 2024 at 18:04 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
ericgsmith commentedComment #4
ericgsmith commentedComment #5
ericgsmith commentedComment #6
rollins commentedI can confirm that changes from the MR make sense here
It will prevent us to have the fatal error
Comment #7
ankitv18 commentedLeft a comment otherwise looks good
Comment #8
ericgsmith commentedFixed the comment, thanks for spotting that
Comment #9
yeniatencio commentedCreating patch based on @ericgsmith as need an urgent solution in the meantime.
Comment #10
ankitv18 commentedRTBC++
Comment #11
kosa ilma commentedI get the same error, the patch from #9 fixed the error.
Core 10.3.1, Password policy 4.0.2.
Thank you!
Comment #12
vishalkhode commentedChanges looks good to me. However, If we add test coverage for the same, that would be great.
Comment #13
rajab natshahFacing the same issue.
Thanks, the MR and patch in #9 are working.
Comment #15
pooja_sharma commentedAdded test coverage in the newly created testPasswordResetBehaviorsUi() as it covers specific scenario, however reused the existing test file as most of code can be leverage from it rather than creating separate test file & applied a super nitpicky :void return on the existing test function as well. Apart form it nothing seems to be left.
Please review, moving NR
Comment #16
ankitv18 commentedThanks for covering the test but it would be great if it is in password_policy_history sub-module.
Comment #17
pooja_sharma commentedThanks for reviewing, have the similar thought, however kept the test coverage in the main module by considering some couple of things like:
Any specific concerns/suggestions over it?
Comment #18
pooja_sharma commentedComment #19
vishalkhode commented@pooja_sharma: Thanks for the adding test coverage, let's not add tests in PasswordResetBehaviorsTest as that test is specifically to validating password reset functionality. And here we want verify password_history functionality, so that it doesn't accept repeated password. Lets see if we can add test coverage for following:
For this, we can either update the existing tests i.e PasswordPolicyInterfaceTest.php or we can add a new tests in password_history submodule similar to PasswordCharacterOperationsTest.php
Comment #20
ankitv18 commentedLooks good ~~RTBC
Comment #21
pooja_sharma commentedThanks @vishalkhode for providing detailed suggestions, chosen the first opt (to update the existing tests i.e PasswordPolicyInterfaceTest.php), as I need to add only minimal single line code change for the test coverage along with leverage complete developed code, I believe if similar issue encounter by any other sub-module that can also be cover similarly (test coverage perspective).
Comment #23
vishalkhode commentedThanks @pooja_sharma, changes looks good to me. Verified and it's seems to be working fine. However, I also noticed that we don't have test coverage which checks for Password Policy history constraint, but that's fine, we can create a separate ticket for it and fix it there. Hence, merged.
Thanks everyone for working on this.