Page MenuHomePhabricator

Refactor UW statements code
Closed, ResolvedPublic

Description

We have this:

Statement widgets are currently being constructed in 2 separate files:

  • resources/controller/uw.controller.Metadata.js for the default statements (depicts)
  • resources/metadata/uw.MetadataContent.js for the others ("add statement" manually)

They're being constructed with different config.
This thing is currently pretty brittle.
See: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/UploadWizard/+/513631/2/resources/metadata/uw.MetadataContent.js#59
See: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/UploadWizard/+/513631/2/resources/controller/uw.controller.Metadata.js#110

We want this:

This should be refactored so that creation of statements doesn't happen in different ways in different places.

Event Timeline

Restricted Application added a project: Multimedia. · View Herald TranscriptJun 5 2019, 2:33 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Ramsey-WMF moved this task from Untriaged to Next up on the Multimedia board.
Ramsey-WMF moved this task from To Do to Doing on the Structured-Data-Team-Current-Work board.
Ramsey-WMF moved this task from To Do to Doing on the Structured Data Engineering board.
matthiasmullie removed matthiasmullie as the assignee of this task.Jul 9 2019, 9:11 AM
matthiasmullie moved this task from Doing to To Do on the Structured-Data-Team-Current-Work board.

Change 529840 had a related patch set uploaded (by Eric Gardner; owner: Eric Gardner):
[mediawiki/extensions/UploadWizard@master] WIP: Refactor UW Statements Code

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

I'm going to take a stab at this. I spent a large chunk of today walking through the UW code in a JS debugger to get a better sense of the flow.

Would it make sense to move all the statement-widget-creation logic into the controller class? That seems like the best place for it, but OOUI makes it hard to separate logic and presentation; moving logic there from the MetadataContent file means that setting up event listeners on user-generated StatementWidgets becomes more of a hassle, for example. I'm going to attempt to do this anyway to see how far I can get.

I'm also fighting with some kind of OOUI default, where new StatementWidget input elements get an inline display: none style for some reason.

WIP patch is here, I'll continue working on it tomorrow: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/UploadWizard/+/529840

Change 529840 merged by jenkins-bot:
[mediawiki/extensions/UploadWizard@master] Refactor UW Statements Code

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

This patch has been merged. Regarding QA needs, we've done extensive testing locally but it would be good to go through the basic UW tasks (both within and outside of campaigns) on Beta just to ensure everything is ok. Then this ticket can probably be closed.

Something I've seen on UW today (autosuggest box getting stuck in "loading" mode), CORS JavaScript errors in the console:

That's on beta, I suspect?
What steps did you do to trigger this? Just added the new property, and that's how it then appeared? Or did you start typing an item? Or did it only appear once you added the other statements or edited anything else?

Yes, on Beta.

Steps were simple: I just tried to add the new property (AErBWxVT) and type in letters to trigger the autosuggest. That property got stuck in the loading/wait state, so I tried adding another. I was able to add it but got nothing in the autosuggest for those subsequent properties (not even No Results Found).

The JavaScript console listed CORS errors in trying to hit beta wikidata, and also a "abortable.abort is not a function" error.

Oh, forgot to add that this seems to be intermittent. I did a lot of UW testing and it mostly worked fine. Every now and then something weird like this would happen though.

Ok cool, thanks for the info!
I suspect the CORS (CSP) errors in console have nothing to do with it (see T135963), but the "abort is not a function" might.

Change 532549 had a related patch set uploaded (by Matthias Mullie; owner: Matthias Mullie):
[mediawiki/extensions/WikibaseMediaInfo@master] No need to deal with formatvalue caches if they've already been cleaned up

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

Change 532549 merged by jenkins-bot:
[mediawiki/extensions/WikibaseMediaInfo@master] No need to deal with formatvalue caches if they've already been cleaned up

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

Change 532956 had a related patch set uploaded (by Matthias Mullie; owner: Matthias Mullie):
[mediawiki/extensions/WikibaseMediaInfo@wmf/1.34.0-wmf.20] No need to deal with formatvalue caches if they've already been cleaned up

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

Change 532956 merged by jenkins-bot:
[mediawiki/extensions/WikibaseMediaInfo@wmf/1.34.0-wmf.20] No need to deal with formatvalue caches if they've already been cleaned up

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

I backported the patch that I believe should fix this - are you still able to get into this stuck state now?

I have tried to reproduce the bug on Beta for a while now, and I haven't seen it! Good sign that it's no longer an issue 👍🏼

Ramsey-WMF closed this task as Resolved.Sep 10 2019, 12:22 AM

As discussed in today's backlog grooming, if UW works, then the refactor worked :) Closing.