I need a module to lock accounts after X days if they have not been used.

This module seems like it does 80% of that, and so I'd like to add the remaining 20%. I can also see it being a separate module if you prefer to keep them separate.

I don't think Role Expire is a good match - it removes a role from the account, which is not really what I want I want them to be blocked.

So...any thoughts on if it should be a feature in this module or not? I'll gladly write it up :)

CommentFileSizeAuthor
#19 2585591_user_expire_by_role_19.patch14.08 KBgreggles
#16 2585591_user_expire_by_role_16.patch14.08 KBgreggles
#14 interdiff.txt8.63 KBgreggles
#14 2585591_user_expire_by_role_14.patch13.83 KBgreggles
#12 2585591_user_expire_by_role_5.patch7.81 KBgreggles
#10 2585591_user_expire_by_role_4.patch7.81 KBgreggles
#8 interdiff.txt1.42 KBgreggles
#8 2585591_user_expire_by_role_3.patch7.65 KBgreggles
#6 2585591_user_expire_by_role_2.patch7.65 KBgreggles
#3 2585591_user_expire_by_role.patch7.64 KBgreggles

Comments

greggles created an issue. See original summary.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Active » Needs review

OK, here's a patch to add this feature. Ideally I would like to add a test or two for this.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
StatusFileSize
new 2585591_user_expire_by_role.patch7.64 KB

Ooops.

Status: Needs review » Needs work

The last submitted patch, 3: 2585591_user_expire_by_role.patch, failed testing.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Title: Lock account after X days by role » Lock account after X days if inactivity by role

That's really what this is about, x days of inactivity.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Title: Lock account after X days if inactivity by role » Lock account after X days of inactivity by role
Status: Needs work » Needs review
StatusFileSize
new 2585591_user_expire_by_role_2.patch7.65 KB

Right...old style arrays.

Status: Needs review » Needs work

The last submitted patch, 6: 2585591_user_expire_by_role_2.patch, failed testing.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
StatusFileSize
new 2585591_user_expire_by_role_3.patch7.65 KB
new interdiff.txt1.42 KB

OK one more syntax error and an interdiff between first and current.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs work » Needs review

Forgot to set back to needs review.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
StatusFileSize
new 2585591_user_expire_by_role_4.patch7.81 KB

OK, here it is after rerolling for the recent 7.x-1.x commits.

Status: Needs review » Needs work

The last submitted patch, 10: 2585591_user_expire_by_role_4.patch, failed testing.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 2585591_user_expire_by_role_5.patch7.81 KB

Forgot to renumber the update hooks.

neerajskydiver queued 12: 2585591_user_expire_by_role_5.patch for re-testing.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
StatusFileSize
new 2585591_user_expire_by_role_14.patch13.83 KB
new interdiff.txt8.63 KB

OK, here's a new patch that adds a ton of tests for all the functionality in this issue and for many of the bugs in #2586719: Preparing for 7.x-1.3 release. I also added the expiry by role to cron and simplified cron by moving some functionality out of cron into a specific function. I also renamed the form keys to be a bit richer than just the role id - made the simpletest code easier to read.

Since there are a fair bit of changes I'm adding an interdiff as well :)

banviktor’s picture

banviktor commented
Status: Needs review » Needs work

The patch @greggles provided is pretty good with very nice tests.

Few minor issues though:
- I would remove the empty lines at user_expire.admin.inc line 110 and user_expire.test line 18
- It might be my fault, but the only way I can access the administration page is by typing in the URL (using the Seven admin theme). IMO it could be in the People box on the Configuration page (admin/config/people/user-expire), also user_expire.info should contain a
configure = admin/config/people/user-expire line.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 2585591_user_expire_by_role_16.patch14.08 KB

Thanks for the feedback! Those are all great points. Now fixed.

I'm also adding credit for your review :)

Status: Needs review » Needs work

The last submitted patch, 16: 2585591_user_expire_by_role_16.patch, failed testing.

banviktor’s picture

banviktor commented

Thanks! :) The modifications look good.
user_expire.test line 105 uses the old URL, that's why the test failed.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
StatusFileSize
new 2585591_user_expire_by_role_19.patch14.08 KB

Right, thanks!

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs work » Needs review

  • greggles committed 6480aed on 7.x-1.x 
    Issue #2585591 by greggles, banviktor: Lock account after X days of...
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs review » Fixed

OK, committed. Thanks, Viktor!

Status: Fixed » Closed (fixed)

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