Are there any plans to add an additional config option to the password expiration form to disable the expiration functionality?

This is a really great module and this flexibility would be really useful for me.

CommentFileSizeAuthor
#13 password_policy-7.x-2.x-add_expiration_enabled_checkbox-2136727-13.patch7.96 KBAohRveTPV
#11 password_policy-7.x-2.x-add_expiration_enabled_checkbox-2136727-11.patch7.97 KBAohRveTPV
#9 password_policy-7.x-2.x-add_expiration_enabled_checkbox-2136727-9.patch8.07 KBAohRveTPV
#8 password_policy-7.x-2.x-add_expiration_enabled_checkbox-2136727-8.patch5.57 KBAohRveTPV
#5 password_policy-expire-enable-with-tests-2136727-5.patch5.83 KB8ballsteve
#3 2136727-password-policy-disable-expiration-8271525.patch4.74 KB8ballsteve
#1 2136727-password-policy-disable-expiration.patch4.16 KBcoltrane
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

coltrane’s picture

coltrane
he/him
CreditAttribution: coltrane commented
Status: Active » Needs review
FileSize
2136727-password-policy-disable-expiration.patch4.16 KB

Patch adds 'expire_enabled' config option checkbox that when checked uses #states to show the other options. Test for this coming soon from #2145649

Here's what it looks like prior to enabling https://www.monosnap.com/image/7bchJJZoNOUrvhxG3bTvqIv49

Edit: this functionality was actually already available by setting the expire_limit option to "0" but that's not super clear or usable IMO.

8ballsteve’s picture

8ballsteve CreditAttribution: 8ballsteve commented

Hi coltrane - looks great, exactly what i was hoping for.

Think it would be best to also validate the expire enabled flag on cron though:

Suggest adding the below to the cron function in expire.inc:

$enabled = $this->config['expire_enabled'];
if (!$enabled) {
  return;
}
8ballsteve’s picture

8ballsteve CreditAttribution: 8ballsteve commented
FileSize
2136727-password-policy-disable-expiration-8271525.patch4.74 KB

Have rerolled the patch including my suggestion - additionally have added the enabled check to the init function before the user load, might save some load time?

erikwebb’s picture

erikwebb CreditAttribution: erikwebb commented
Status: Needs review » Needs work
Issue tags: +Needs tests

To ensure this functionality works as expected, can we add a test for with and without this enabled?

8ballsteve’s picture

8ballsteve CreditAttribution: 8ballsteve commented
FileSize
password_policy-expire-enable-with-tests-2136727-5.patch5.83 KB

I haven't written a great deal of test cases so please feel free to adjust this if not correct.

Patch with tests attached.

Thanks!

erikwebb’s picture

erikwebb CreditAttribution: erikwebb commented
Status: Needs work » Needs review
erikwebb’s picture

erikwebb CreditAttribution: erikwebb commented
Status: Needs review » Needs work

@8ballsteve -

It will likely be easier to maintain if you wrap all of those fields into a container, so that we only have to declare the #states property once.

Also, the test should test the effect of actually using an expiration value, not just the policy creation.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs work » Needs review
FileSize
password_policy-7.x-2.x-add_expiration_enabled_checkbox-2136727-8.patch5.57 KB

- Add update code.
- Extend testExpire() to test this setting.
- Add container for controls that should only be visible when enabled (per #7).
- Change expire fieldset to be collapsed based on enabled setting rather than limit.
- Change default expire limit to "6 months". (My guess is that 6 months is more typical, and that 3 months is relatively quick.)

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Issue tags: -Needs tests
FileSize
password_policy-7.x-2.x-add_expiration_enabled_checkbox-2136727-9.patch8.07 KB

Wrong patch file in previous comment.

The last submitted patch, 8: password_policy-7.x-2.x-add_expiration_enabled_checkbox-2136727-8.patch, failed testing.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
FileSize
password_policy-7.x-2.x-add_expiration_enabled_checkbox-2136727-11.patch7.97 KB

Remove unused variable.

Status: Needs review » Needs work

The last submitted patch, 11: password_policy-7.x-2.x-add_expiration_enabled_checkbox-2136727-11.patch, failed testing.

AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs work » Needs review
FileSize
password_policy-7.x-2.x-add_expiration_enabled_checkbox-2136727-13.patch7.96 KB

Previous patch was incorrectly updated to apply on latest code.

  • AohRveTPV committed f0bb9bc on 7.x-2.x 
    Issue #2136727 by AohRveTPV, 8ballsteve, coltrane: Add a facility to...
AohRveTPV’s picture

AohRveTPV CreditAttribution: AohRveTPV commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.