Page MenuHomePhabricator
Create Task
Maniphest T220234

Unable to publish depicts changes after clicking "mark as prominent" when no other values are modified
Closed, ResolvedPublicBUG REPORT
Actions

Description

Noticed this for the first time this afternoon. I'm running master at b2e8e968cf0fc2f56a72594e69af8f6a84d02469 in MW-Docker-Dev locally. Curious if anyone else can see this problem.

Steps to Reproduce:

  1. Navigate to the File page of an image with multiple Depicts statements, one of which is not currently listed as prominent. Here's what my page looks like at this point:

Screen Shot 2019-04-05 at 12.54.25 PM.png (1×1 px, 1 MB)

  1. Click mark as prominent on the non-prominent statement. Do not change any other values.

Actual Results:
The Depicts Panel toggles into Edit mode but the "publish changes" button is disabled, as if the user has toggled the panel but not yet made any changes. It seems like the promotion of the statement to "prominent" is not getting picked up as a change.

Screenshot (note that both statements are now correctly labeled as prominent):

Screen Shot 2019-04-05 at 12.54.44 PM.png (1×1 px, 1 MB)

Expected Results:

The "publish changes" button should be active and click-able, and the user should be to submit their edit successfully even if no other values have changed.

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
Fix isEditable property/method collisionmediawiki/extensions/WikibaseMediaInfomaster+9 -9
Separate isEditable from hasChangesmediawiki/extensions/WikibaseMediaInfomaster+14 -13
Remove guard clause from DepictsPanel.hasChanges()mediawiki/extensions/WikibaseMediaInfomaster+0 -4
Customize query in gerrit

Related Objects

Event Timeline

egardner created this task.Apr 5 2019, 8:02 PM
Restricted Application added a project: Multimedia. · View Herald TranscriptApr 5 2019, 8:02 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
egardner added a comment.Apr 5 2019, 8:18 PM
This comment was removed by egardner.
egardner added a comment.Apr 5 2019, 9:09 PM

Ok, after a little more digging, here is what I think is going on:

Currently, the DepictsPanel has a method called hasChanges, which other methods of this object call in order to determine whether things have changed. This is used to determine whether to enable or disable that "publish" button for the user. A "change" here can mean both new data input in to a text field, but also the promotion/demotion of statements to a "prominent" state.

Here's the catch: this patch added a property called isEditable to the DepictsPanel object – a simple true or false value that says whether the panel is editable or not. The hasChanges method will bail out early if isEditable is set to false (in order to avoid some other potential errors).

So if the user is viewing the DepictsPanel in "read-only" state (meaning isEditable is false), and they click "make prominent" on a statement, the hasChanges method looks like it gets called before the panel gets switched over into editable state. This means that the change is not correctly detected, and the "publish" button does not get enabled properly (even though the panel does eventually get set to the edit state).

• Ramsey-WMF triaged this task as High priority.Apr 5 2019, 11:20 PM
• Ramsey-WMF moved this task from Untriaged to Triaged on the Multimedia board.
• Ramsey-WMF added a project: Structured Data Engineering.
• Ramsey-WMF added subscribers: Cparle, matthiasmullie.
gerritbot added a comment.Apr 5 2019, 11:36 PM

Change 501819 had a related patch set uploaded (by Eric Gardner; owner: Eric Gardner):
[mediawiki/extensions/WikibaseMediaInfo@master] Remove guard clause from DepictsPanel.hasChanges()

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

gerritbot added a project: Patch-For-Review.Apr 5 2019, 11:36 PM
egardner claimed this task.Apr 5 2019, 11:40 PM
egardner moved this task from To Do to Code Review on the Structured-Data-Team-Current-Work board.
egardner mentioned this in rEWBI687ef386c727: Remove guard clause from DepictsPanel.hasChanges().Apr 5 2019, 11:41 PM
• Ramsey-WMF moved this task from To Do to Doing on the Structured Data Engineering board.Apr 5 2019, 11:42 PM
• Ramsey-WMF moved this task from Triaged to Next up on the Multimedia board.
• Ramsey-WMF subscribed.Apr 5 2019, 11:49 PM

@Cparle and @matthiasmullie this might be a big issue we need to address quickly. On Monday, can you prioritize reviewing Eric's patch (which, as Eric mentions, probably needs a bit of work still)?

gerritbot added a comment.Apr 6 2019, 6:58 AM

Change 501856 had a related patch set uploaded (by Matthias Mullie; owner: Matthias Mullie):
[mediawiki/extensions/WikibaseMediaInfo@master] Separate isEditable from hasChanges

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

matthiasmullie mentioned this in rEWBI1e347f36f44f: Separate isEditable from hasChanges.Apr 6 2019, 6:58 AM
gerritbot added a comment.Apr 6 2019, 3:32 PM

Change 501819 abandoned by Eric Gardner:
Remove guard clause from DepictsPanel.hasChanges()

Reason:
Abandoning in favor of https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseMediaInfo/ /501856

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

gerritbot added a comment.Apr 6 2019, 3:51 PM

Change 501856 merged by jenkins-bot:
[mediawiki/extensions/WikibaseMediaInfo@master] Separate isEditable from hasChanges

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

ReleaseTaggerBot added a project: MW-1.33-notes (1.33.0-wmf.25; 2019-04-09).Apr 6 2019, 4:01 PM
gerritbot added a comment.Apr 8 2019, 12:56 PM

Change 502213 had a related patch set uploaded (by Matthias Mullie; owner: Matthias Mullie):
[mediawiki/extensions/WikibaseMediaInfo@master] Fix isEditable property/method collision

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

matthiasmullie mentioned this in rEWBI7dad35063ec2: Fix isEditable property/method collision.Apr 8 2019, 1:00 PM
gerritbot added a comment.Apr 8 2019, 2:25 PM

Change 502213 merged by jenkins-bot:
[mediawiki/extensions/WikibaseMediaInfo@master] Fix isEditable property/method collision

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

Cparle moved this task from Code Review to Needs QA on the Structured-Data-Team-Current-Work board.Apr 8 2019, 3:40 PM
egardner added a comment.Apr 16 2019, 9:48 PM

This feature seems to be working correctly on both Beta and Test commons; moving to "Verify on Production".

egardner moved this task from Needs QA to Verify on Production on the Structured-Data-Team-Current-Work board.Apr 16 2019, 9:48 PM
• Ramsey-WMF closed this task as Resolved.Apr 24 2019, 1:57 AM

Works on production.

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 · Credits