Page MenuHomePhabricator

Prune After Reject
Closed, ResolvedPublic

Description

"pruning of old requests will not trigger often, so old rejected requests may persist."
https://www.mediawiki.org/wiki/Extension:ConfirmAccount#Known_issues

This problem will prevent rejected emails from requesting an account again. After Admin rejected an account request, the same username/email could not submit another request. Error on 2nd attempt:

Username is already in use in a pending account request.

To fix, prune after every rejection, to clear the request cache.

Event Timeline

Johnywhy created this task.Mar 21 2018, 8:59 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 21 2018, 8:59 PM
Johnywhy updated the task description. (Show Details)Mar 21 2018, 9:04 PM
Johnywhy added a comment.EditedMar 21 2018, 9:08 PM

It appears that, currently, pruning occurs in file \ConfirmAccount\frontend\specialpages\actions\ConfirmAccount_body.php

# Every 30th view, prune old deleted items
if ( 0 == mt_rand( 0, 29 ) ) {
ConfirmAccount::runAutoMaintenance();
}

Therefor, the function runAutoMaintenance appears to be the pruning function. runAutoMaintenance lives in \ConfirmAccount\backend\ConfirmAccount.class.php

class ConfirmAccount {
/** * Move old stale requests to rejected list. Delete old rejected requests. */
public static function runAutoMaintenance() {...

In order to call runAutoMaintenance after every reject-action, I think the call to runAutoMaintenance should be placed in function rejectRequest, in file \extensions\ConfirmAccount\business\AccountConfirmSubmission.php

Specifically, i think it can go directly under:

# Clear cache for notice of how many account requests there are
ConfirmAccount::clearAccountRequestCountCache();

Maybe pruning should also happen after Accept, Hold, and Spam actions. Unsure. For now, pruning after Reject should handle the OP.

Johnywhy added a comment.EditedMar 21 2018, 9:22 PM

I attempted the above fix, and it did not seem to work. I'm at a loss.

Can someone determine why this fix did not work?

Original code:

protected function rejectRequest( IContextSource $context ) {
....
# Clear cache for notice of how many account requests there are
ConfirmAccount::clearAccountRequestCountCache();
....

New code:

protected function rejectRequest( IContextSource $context ) {
....
# Clear cache for notice of how many account requests there are
ConfirmAccount::clearAccountRequestCountCache();
# Prune
ConfirmAccount::runAutoMaintenance();
....

How i'm testing this:

  • install ConfirmAccounts.
  • Request an account.
  • Login as admin, and reject the request. Then logout.
  • Request again with same email.
  • Receive "Username is already in use in a pending account request."
  • Then insert new line of code into function rejectRequest (as above).
  • Then test again, with a different email.
  • On 2nd request, still getting "Username is already in use in a pending account request."

Johnywhy closed this task as Resolved.EditedMar 22 2018, 4:26 AM

Looks like i solved it. There are 2 steps:

  • Set rejected items to expire immediately (LocalSettings.php)
  • Prune rejected items at beginning of Request action (RequestAccount_body.php)

Details:

in LocalSettings.php, after required declaration, set Rejected-Age to 0. That ensures rejected requests will be removed on prune-action:

require_once "$IP/extensions/ConfirmAccount/ConfirmAccount.php";
$wgRejectedAccountMaxAge = 0;

Add Prune code to the function that shows the Request form, in /ConfirmAccount/frontend/specialpages/actions/RequestAccount_body.php, function showForm. Add very last command in the function:

old code:

$out->addWikiMsg( 'requestaccount-footer' );
}

new code:

$out->addWikiMsg( 'requestaccount-footer' );		
# PRUNE
ConfirmAccount::runAutoMaintenance();
}

A minor improvement might be to execute the prune when Submit button is clicked on new request, instead of when the request form loads. But above solution is 'good enuf' ™

Instructions added to
https://www.mediawiki.org/wiki/Extension:ConfirmAccount#Pruning_Frequency

Hey @Johnywhy

just saw, that you missed the IRC meeting yesterday and wanted to check if you're good. Since this ticket is resolved I guess you are. Don't hesitate to ask questions in the IRC channels even outside of the advice hours. Also you could always use https://discourse-mediawiki.wmflabs.org/ to discuss issues.

best,
Fisch

Johnywhy added a comment.EditedMar 22 2018, 11:28 AM

Thx Fisch. How did you know i missed the meeting? You guys are strict :D
I developed a php solution, explained above.
Indeed, i got a clue from an ancient Greek on the IRC :)

cheers
jw

@Johnywhy: You could remove duplicated empty lines (see the difference in the preview), and not use bold text for some file name/path? You could also either use <syntaxhighlight lang="php"> instead of <code>, or even better remove "old code" and "new code" sections and just describe which specific <code>line</code> to add after which existing line. I'm also wondering if there is some less complicated word than "prune" which I had to look up?

Johnywhy added a comment.EditedMar 28 2018, 11:36 AM

@Johnywhy: You could remove duplicated empty lines (see the difference in the preview), and not use bold text for some file name/path? You could also either use <syntaxhighlight lang="php"> instead of <code>, or even better remove "old code" and "new code" sections and just describe which specific <code>line</code> to add after which existing line. I'm also wondering if there is some less complicated word than "prune" which I had to look up?

Many thx for feedback!

  • IIRC, the <code> tag caused extra blank lines. Your syntaxhighlight tag fixes that. Thx!
  • a filepath is like code-- should have a different format, to improve clarity, so reader knows where text ends and filepath begins, improving understandability. The backtick doesn't work there, for inline code, but code tag does, thx for that! (is there a way to make backtick work in mediawiki?)
  • 'old code' and 'new code' sections helps readers see the key differences, improving understandability. i understand code better when see the code in context, not just a "description". i provided both for max clarity.
  • "prune"
    • a very common word. The sentence "Prune old items" has a Readability Score of A, and Dale–Chall readability rating of "easily understood by an average U.S. 5th or 6th-grade student." https://readable.io/text/
    • "Prune" is already used in the helpfile and the source-code.

Re functionality, will this mod break if the extension is updated?

@Johnywhy: Thanks for the readable.io reference, didn't know about that one! My comments on duplicated unneeded empty lines and the file path being bold (you could use <code> instead) remain valid.

@Aklapper : thx, see my revised comment above.

Pruning is broken for PostgreSQL:

Jan 10 22:55:14 db postgres[3239]: [8-1] ERROR:  insert or update on table "account_requests" violates for
eign key constraint "account_requests_acr_user_fkey"
Jan 10 22:55:14 db postgres[3239]: [8-2] DETAIL:  Key (acr_user)=(0) is not present in table "mwuser".
Jan 10 22:55:14 db postgres[3239]: [8-3] STATEMENT:  UPDATE /* ConfirmAccount::runAutoMaintenance 2A05:3580:D200:... */  "account_requests" SET acr_rejected = '2019-01-10 13:55:14 GMT',acr_user = '0',acr_comment = '(this request has automatically been discarded due to inactivity)',acr_deleted = '1' WHERE (acr_rejected IS NULL) AND (acr_registration < '2018-12-11 13:55:14 GMT') AND (acr_held < '2018-12-11 13:55:14 GMT' OR acr_held IS NULL)