notify_login_failures executed even when no recipients

Looking at the code, the same condition happens if when it last happened under an hour ago, it runs, but doesnt update/do anything. Im making a patch that looks at all of these things.

Thanks Eric, for spot-on patch
Pushing if for integration review.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Nice one, integrated (21, 22 and master).

Eric, I've added one extra commit to fix one notice in all branches... also... could you tidy a bit testing instructions... I've added one Note there... but perhaps it would be better to have them written in some more "deterministic" way, to be 100% sure testing goes ok.

My apologies in advance if this is not supposed to be failed, but what happens is that after step 6, if I refresh on and on the cron, the sentence "Notified login failures" keeps on appearing every time, there is not need to wait for an hour.

I think that is because the 'lastnotifyfailure' config value is not updated when there is nothing to notify about. I assume that if the function returns true, we should also update the 'lastnotifyfailure'.

Everything else but this worked fine.

Hrm, I get what you are saying about the notification with lastnotifyfailure. I've checked the code base, and it is only used in that function, so I think it is safe to slightly change it's meaning to basically be the last time we checked for bad logins, not the last time they occurred.

I would fix it by moving:
// Update lastnotifyfailure with current time
set_config('lastnotifyfailure', time());

from it's current location to just above "// Finally, delete all the temp records we have created in cache_flags"

I'm not sure if I should add this to the branches I have setup, because they are already integrated and Eloy had a commit over it, or if it would just be easier for one of you to directly do it. If you want me to, just let me know and I'll update the branches.

I've updated the instructions so it should be more deterministic - particularly if we integrate the additional fix just described.

I would move notifylastfailure where you suggested it as well.
Regarding the patch/branch update, I'll let the integrator/reviewer answer.

I've created new commits on top of integration that include this additional fix.

They are:
MDL-32985_integration
https://github.com/merrill-oakland/moodle/commit/54c2756ca21e74ad89a5c922e47287477f3add55
MDL-32985_integration_22
https://github.com/merrill-oakland/moodle/commit/4e6fbf663cef70f8d9e7985fc8f79eee1cda9e88
MDL-32985_integration_21
https://github.com/merrill-oakland/moodle/commit/bfe1d304442450165cae6baf579f686e48c703e9

I'm not sure if I should replace the branches listed in the ticket meta-data, since those were still the primary branches, so I'm just putting these right here for now.

Can someone mark this back for integration?

Thanks

New commit integrated, ready for a new testing round. Thanks!

Test passed on 2.1, 2.2 and master. But to properly test 2.1 you have to refresh the page more than once as notify_login_failures() only gets executed once out of 5 times (ish).

Also, I would add to the testing instructions that we should delete the config key 'lastnotifyfailure' in mdl_config table at the beginning of the test.

We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

Many thanks for your collaboration!

Ciao