Hi,
The attached patch file will do the below things.

Added options for mail subjects to be configurable. So admin can configure the subjects for reminders, block and delete account emails. And fixed the issues of sending warning emails before block and delete the users.

Warn users before they are about to be deleted.
Suppose If I select the period of 10 months inactivity and auto block the users. And also select issue warning before 1 week of auto block. So I will select the warn period of 1 week before auto block. So the inactive user was given a warning reminder that his/her account is going to be blocked within one week due to no activity since 10 months.

Present module code checks the last access with current time minus warn time instead of block time. So it is sending the reminders to the users who haven't logged from last week. But it is wrong check here, It need to be check with current time minus block time only. I have changed that.

Same for Delete accounts.

CommentFileSizeAuthor
inactive_user_mail_subjects_&condition_fixes.patch11.45 KBvenkatadapa

Comments

jkarlsson’s picture

jkarlsson commented

This is something that hasn't ever worked properly and i'm afraid that neither this patch, or the change in #661882: Updated test case and several fixes (that has been committed to the official repository) is correct.

There is great confusion how to interpret the warn period. I'm pretty sure that the original author's intention was that it should work like this:
Given:
Block users that haven't logged in for more than: 6 months
Warn user's before they are blocked: 1 month
Result:
Warn users 1 MONTH BEFORE THEY ARE BLOCKED, in this case users that has been inactive for 5 months.

Git HEAD (changed by #661882: Updated test case and several fixes) warns all users with 1 month of inactivity, saying their accounts will be blocked in 1 months, but in fact doesn't block them until they've been inactive for 6 months.

This patch (if I read correctly, can't test it right now) will warn all users that have been inactive for 6 months, and then apply the warning period, effectively giving them a one month grace period. We're actually blocking people that have been inactive for 7 months, giving them a one month heads up.

Here's why I think the my interpretation of the warning period coincides with the original idea:

* It makes sense (to me at least ;-)
* It fits with the wording on the config page
* The default email body says "... your account will be disabled in %period unless ..." where the warning value is passed in as the %period parameter. This is really messed up by the current HEAD which really lies about when the account will be blocked.

* The original condition (since the first commit) looks like this:

... AND timestamp < (%d - %d - %d)', time(), $warn_time, $block_time)

Which unfortunately is wrong but very, very close to what would give the above stated behaviour

... AND timestamp < (%d + %d - %d)', time(), $warn_time, $block_time)

or perhaps slightly clearer:

... AND timestamp < (%d - (%d - %d))', time(), $block_time, $warn_time)

I'm sorry to make you read such a long post but I really feel it's time we get this right, once and for all!

One last thing, this is one of the bugs that is fixed in #1099978: Major refactoring, some squashed bugs and new features

venkatadapa’s picture

venkatadapa commented

Hi jkarlsson,

I think you didn't test this patch. It worked like a charm for me. Anyway you said one thing "effectively giving them a one month grace period", it is correct. Before blocking we are giving him one reminder. It would provide one month grace period if you configured like as you mentioned. Means block after 6 months inactive and 1 month before warn then users actually blocked after 7 months. If you didn't select the warn period then they will be blocked exactly after 6 months. If anyways we want to block exactly after the selected block time, then we need to take the block time as block time + warn time.

jkarlsson’s picture

jkarlsson commented

As I wrote in the post, this patch works a lot better than the current HEAD, and my only objection is that I think it's wrong that the warning time is added to the blocking time when using warnings.

It feels really wrong that the meaning of "Block users that logged in for more than: 6 months" changes when using warnings, but maybe I'm being over sensitive.