Page MenuHomePhabricator

Special:MentorDashboard broken
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • If you're a mentor go to Special:MentorDashboard

What happens?:
Geen recentelijk actieve leerlingen gevonden (No recent active mentees found)

What should have happened instead?:
List of mentees shown

Software version (on Special:Version page; skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):
Multiple mentors on nlwiki report this issue

Event Timeline

Urbanecm_WMF triaged this task as High priority.

I can confirm this on both testwiki and cswiki. Seems to be a significant problem. Thank you for reporting!

The problem is that requests like https://cs.wikipedia.org/w/rest.php/growthexperiments/v0/mentees?order=&sortby=&limit=25&offset=0&prefix=&activedaysago=&maxedits=500&minedits=1&onlystarred=false&uselang=cs now fail with a validation error:

{
  "error": "parameter-validation-failed",
  "name": "activedaysago",
  "value": "",
  "failureCode": "badinteger",
  "failureData": null,
  "errorKey": "badinteger",
  "messageTranslations": {
    "cs": "Invalid value \"\" for integer parameter \"activedaysago\".",
    "en": "Invalid value \"\" for integer parameter \"activedaysago\"."
  },
  "httpCode": 400,
  "httpReason": "Bad Request"
}

Removing the no-value parameters from the URL returns data, so hopefully, the fix here should be easy.

Change #1061125 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/GrowthExperiments@master] MenteeOverviewApi: Do not apply undefined/null params

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

Change #1061125 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] MenteeOverviewApi: Do not apply undefined/null params

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

Change #1061148 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/GrowthExperiments@wmf/1.43.0-wmf.17] MenteeOverviewApi: Do not apply undefined/null params

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

Change #1061148 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@wmf/1.43.0-wmf.17] MenteeOverviewApi: Do not apply undefined/null params

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

Mentioned in SAL (#wikimedia-operations) [2024-08-12T08:57:30Z] <urbanecm@deploy1003> Started scap sync-world: Backport for [[gerrit:1061148|MenteeOverviewApi: Do not apply undefined/null params (T372164)]]

Mentioned in SAL (#wikimedia-operations) [2024-08-12T09:10:41Z] <urbanecm@deploy1003> urbanecm: Backport for [[gerrit:1061148|MenteeOverviewApi: Do not apply undefined/null params (T372164)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2024-08-12T09:17:24Z] <urbanecm@deploy1003> Finished scap: Backport for [[gerrit:1061148|MenteeOverviewApi: Do not apply undefined/null params (T372164)]] (duration: 19m 54s)

Hi @daniel, this error was likely caused by the REST refactorings your team is working on IIRC (previously, /w/rest.php/growthexperiments/v0/mentees?order=&sortby=&limit=10&offset=0&prefix=&activedaysago=&maxedits=500&minedits=1&onlystarred=&uselang=en worked, while now it fails with badinteger. The failure is technically correct, as activedaysago is indeed not an integer (but an empty string), but...it used to work before :). Letting you know, in case this is significant to your work. On the Growth's side, we stopped passing params that we don't have a value for, and that remedied the issue.

Hi @daniel, this error was likely caused by the REST refactorings your team is working on IIRC (previously, /w/rest.php/growthexperiments/v0/mentees?order=&sortby=&limit=10&offset=0&prefix=&activedaysago=&maxedits=500&minedits=1&onlystarred=&uselang=en worked, while now it fails with badinteger. The failure is technically correct, as activedaysago is indeed not an integer (but an empty string), but...it used to work before :). Letting you know, in case this is significant to your work. On the Growth's side, we stopped passing params that we don't have a value for, and that remedied the issue.

I don't see how that ever worked... It doesn't work in the action API either, and we are sharing the validation code.

We didn't (at least intentionally) change anything about the validation of query parameters. Body params, yes. But query and path params should behave the same as before.

I'm getting the same error on 1.41 and 1.39 by adding a corresponding test case to HandlerTest.

Thanks for the quick response @daniel. Based on the details you shared, I looked more into what actually happened, and I was wrong in my original guess of what happened. With the help of git bisect, I was able to identify the actual breaking point.

The error happened in 1057374: build: Update eslint-config-wikimedia to 0.28.2 by @taavi. That patch changes MenteeOverviewApi.js in a bunch of places, including the problematic part. Previously, new params were set in the client by this.apiParams = $.extend( this.apiParams, params ). In the linked patch, this changed to Object.assign( this.apiParams, params ).

It looks like those two constructs have functional differences. Most importantly:

> $.extend({a: 1, b: 2}, {a: undefined, b: 1})
{a: 1, b: 1}
> Object.assign({a: 1, b: 2}, {a: undefined, b: 1})
{a: undefined, b: 1}

The UI form submits values to params it wishes to set, and undefined for params that should keep the default behaviour (undefined is used, as this corresponds to cases when the user selected no value; as opposed to not changing a value selected by default). Thanks to the differences between $.extend and Object.assign, the undefined got precedence, was casted to an empty string and failed validation.

Since we didn't make any intentional changes to the client or the module, I (wrongly) assumed the request was the same, and I was looking for reasons why it might stop working, but it looks like we were actually emitting a valid request in the past. I'll update T372188 with more information.

That's partly also my bad. I didn't realize that this change in its over 1000 lines of changes also included $.extend replacements. Those have been causing problems before when they broke Special:Notifications (T368029).

Etonkovidova subscribed.

Checked in testwiki wmf.18` - Special:MentorDashboard displays recently (less than 6 months) active mentees.