Page MenuHomePhabricator

Mentor selection is error prone
Closed, ResolvedPublic

Description

in Mentor::selectMentor(), we have:

		$selectedMentorName = $possibleMentors[ rand( 0, count( $possibleMentors ) - 1 ) ];
		$selectedMentor = User::newFromName( $selectedMentorName );
		if ( !$selectedMentor || !$selectedMentor->getId() ) {
			throw new WikiConfigException( 'Invalid mentor: ' . $selectedMentorName );
		}
		return $selectedMentor;

This means that if the Mentor page contains an invalid username (e.g. someone has a typo) and $selectedMentor is unlucky enough to be that invalid username, the mentor selection fails and the user has no mentor. We should probably keep trying the other mentor names until we find a valid one.

Event Timeline

kostajh created this task.Dec 11 2019, 1:45 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 11 2019, 1:45 PM

Proposal: Validate usernames in Mentor::getMentors(). What do you think, Kosta?

Yep, sounds good to me!

Urbanecm claimed this task.Feb 10 2020, 3:44 PM

I'll implement that then! I'll also remove the exception, given it wouldn't be possible to raise it.

Restricted Application added a project: User-Urbanecm. · View Herald TranscriptFeb 10 2020, 3:44 PM

Change 571480 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/extensions/GrowthExperiments@master] Exclude invalid users from Mentor::getMentors()

https://gerrit.wikimedia.org/r/571480

Tgr added a subscriber: Tgr.Feb 12 2020, 6:19 AM

While it's not foolproof, it would be nice to have a page save warning so people who put a typo in their name get a fighting chance instead of just never seeing questions.

Moving this back to in progress since it doesn't pass the unit tests.

While it's not foolproof, it would be nice to have a page save warning so people who put a typo in their name get a fighting chance instead of just never seeing questions.

I believe we can afford to ignore this one for now - at our target wikis, we have ambassadors that should have the mentor list in their watchlists, and notice if something bad happens there.

Moving this back to in progress since it doesn't pass the unit tests.

Should be fixed, moving back.

Change 571480 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Exclude invalid users from Mentor::getMentors()

https://gerrit.wikimedia.org/r/571480

Etonkovidova closed this task as Resolved.Feb 19 2020, 12:09 AM
Etonkovidova added a subscriber: Etonkovidova.

Tested in betalabs with an extreme test case when the only mentor's name existing on Wikipedia:Mentors page was an invalid user name. A user won't get a mentor assigned in such a case.

Additionally action=growthsetmentor (for T244744) was checked for invalid and non-existing usernames:

  • invalid usernames
/w/api.php?action=growthsetmentor&format=json&mentee=%23%20%3C%20%3E%20%5B%20%5D%20%7B%20%7D%20%7C&mentor=%23%20%3C%20%3E%20%5B%20%5D%20%7B%20%7D%20%7C%7C

{
    "error": {
        "code": "baduser",
        "info": "Invalid value \"# < > [ ] { } |\" for user parameter \"mentee\".",
        "*": "See https://en.wikipedia.beta.wmflabs.org/w/api.php for API usage. Subscribe to the mediawiki-api-announce mailing list at &lt;https://lists.wikimedia.org/mailman/listinfo/mediawiki-api-announce&gt; for notice of API deprecations and breaking changes."
    },
    "servedby": "deployment-mediawiki-07"
}
  • non-existing users
/w/api.php?action=growthsetmentor&format=json&mentee=ET59999&mentor=ET760
{
    "error": {
        "code": "nosuchuser",
        "info": "There is no user by the name \"ET59999\". Check your spelling.",
        "*": "See https://en.wikipedia.beta.wmflabs.org/w/api.php for API usage. Subscribe to the mediawiki-api-announce mailing list at &lt;https://lists.wikimedia.org/mailman/listinfo/mediawiki-api-announce&gt; for notice of API deprecations and breaking changes."
    },
    "servedby": "deployment-mediawiki-07"
}