Page MenuHomePhabricator

Mentor tools: In production/beta, submitting the away dialog causes a JavaScript error
Closed, ResolvedPublic

Description

Elena found this in T280307#7473617. When an user submits the away dialog from mentor tools:

Screen Shot 2021-11-01 at 6.33.15 PM.png (784×1 px, 135 KB)

a JavaScript error happens, as the API response does not contain the timestamp when the mentor will be back:

Uncaught TypeError: Cannot read properties of undefined (reading 'human')
    at MentorTools.onMentorBackTimestampChanged 
    at AwaySettingsDialog.OO.EventEmitter.emit

This works fine in my local dev setup. I tried adding READ_LATEST to ApiSetMentorStatus::execute (the line where it calls getMentorBackTimestamp), which appears to fix the issue in prod.

However, MentorStatusManager::markMentorAsAway is called right before that, and UserOptionsManager is supposed to update its in-process cache, so READ_LATEST shouldn't be necessary?

I'm not sure why should READ_LATEST be necessary. In theory, it shouldn't be an issue, because ApiSetMentorStatus is a POST API endpoint (so READ_LATEST is appropriate there generally). @Tgr, can you advice please?

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

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

[mediawiki/extensions/GrowthExperiments@master] ApiSetMentorStatus: Use READ_LATEST to request back timestamp

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

Urbanecm_WMF moved this task from Backlog to In progress on the User-Urbanecm_WMF (Engineering) board.
Urbanecm_WMF edited projects, added Growth-Team (Current Sprint); removed Growth-Team.

Moving to CR, even though "code discussion" would be more appropriate.

This is the relevant logic. The in-process cache is discarded when the changes are saved, so the next read will go to the replica, which is lagged. (You won't see that in your local setup since you don't have a replica.) So

$userOptionsManager->setOption( 'foo', 'bar' );
$userOptionsManager->saveOptions();
$userOptionsManager->getOption( 'foo' );

will give you the old value of foo.

I wouldn't call that a bug, having to use READ_LATEST if you want the latest data is the general expectation for storage (so I think your patch is the correct fix). It doesn't seem to hard to prevent in core though (just need a third cache alongside originalOptionsCache and modifiedOptions, for options modified in a previous save) so I wonder what @Pchelolo thinks about it.

Change 737445 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] ApiSetMentorStatus: Use READ_LATEST to request back timestamp

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

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

[mediawiki/extensions/GrowthExperiments@wmf/1.38.0-wmf.9] ApiSetMentorStatus: Use READ_LATEST to request back timestamp

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

Change 740355 merged by Urbanecm:

[mediawiki/extensions/GrowthExperiments@wmf/1.38.0-wmf.9] ApiSetMentorStatus: Use READ_LATEST to request back timestamp

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

Mentioned in SAL (#wikimedia-operations) [2021-11-22T08:44:21Z] <urbanecm@deploy1002> Synchronized php-1.38.0-wmf.9/extensions/GrowthExperiments/: 4418c4367b7420139cd8b30cb003d697b58c618f: ApiSetMentorStatus: Use READ_LATEST to request back timestamp (T295305) (duration: 01m 08s)