Page MenuHomePhabricator
Create Task
Maniphest T295305

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

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?

Details

SubjectRepoBranchLines +/-
ApiSetMentorStatus: Use READ_LATEST to request back timestampmediawiki/extensions/GrowthExperimentswmf/1.38.0-wmf.9+39 -14
ApiSetMentorStatus: Use READ_LATEST to request back timestampmediawiki/extensions/GrowthExperimentsmaster+39 -14
Customize query in gerrit

Related Objects
Search...

Event Timeline

Urbanecm_WMF created this task.Nov 8 2021, 6:10 PM
Restricted Application added a project: Growth-Team. · View Herald TranscriptNov 8 2021, 6:10 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Urbanecm_WMF added projects: Wikimedia-production-error, JavaScript.Nov 8 2021, 6:10 PM
gerritbot added a comment.Nov 8 2021, 6:12 PM

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

gerritbot added a project: Patch-For-Review.Nov 8 2021, 6:12 PM
Urbanecm_WMF mentioned this in T280307: Mentor dashboard: M2 mentor tools/settings.Nov 8 2021, 6:18 PM
Urbanecm_WMF triaged this task as Low priority.Nov 9 2021, 9:28 AM
Urbanecm_WMF moved this task from Backlog to In progress on the User-Urbanecm_WMF (Engineering) board.
Urbanecm_WMF edited projects, added Growth-Team (Sprint 0 (Growth Team)); removed Growth-Team.

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

Urbanecm_WMF claimed this task.Nov 9 2021, 9:29 AM
Urbanecm_WMF moved this task from Incoming to Code Review on the Growth-Team (Sprint 0 (Growth Team)) board.
thcipriani moved this task from Untriaged to Nov 2021 on the Wikimedia-production-error board.Nov 18 2021, 4:52 PM
Tgr added a subscriber: • Pchelolo.Nov 22 2021, 1:18 AM

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.

gerritbot added a comment.Nov 22 2021, 5:17 AM

Change 737445 merged by jenkins-bot:

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

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

ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.12; 2021-12-06).Nov 22 2021, 6:00 AM
Maintenance_bot removed a project: Patch-For-Review.Nov 22 2021, 6:10 AM
Urbanecm_WMF moved this task from Code Review to QA on the Growth-Team (Sprint 0 (Growth Team)) board.Nov 22 2021, 8:27 AM
Urbanecm_WMF moved this task from In progress to My Part Done on the User-Urbanecm_WMF (Engineering) board.
gerritbot added a comment.Nov 22 2021, 8:29 AM

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

gerritbot added a project: Patch-For-Review.Nov 22 2021, 8:29 AM
gerritbot added a comment.Nov 22 2021, 8:40 AM

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

Stashbot added a comment.Nov 22 2021, 8:44 AM

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)

Urbanecm_WMF removed a project: Patch-For-Review.Nov 22 2021, 8:45 AM
ReleaseTaggerBot edited projects, added MW-1.38-notes (1.38.0-wmf.9; 2021-11-16); removed MW-1.38-notes (1.38.0-wmf.12; 2021-12-06).Nov 22 2021, 9:00 AM
Etonkovidova closed this task as Resolved.Dec 3 2021, 10:03 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL