Page MenuHomePhabricator

Update how "Remove all" works for statement panels
Closed, ResolvedPublic

Description

User story

As a Commons contributor, I want to be able to delete an entire statements panel at any point during the "add statements" flow, and have a statement be removed without having to select publish.

We have this

Currently, "Remove all" for statements doesn't show up at every point in the "Add statement" flow, and it's not immediately apparent how to delete a property with no values. Statements actually get removed when hitting publish, which works okay on UW, but can be seemingly impossible to do on FP if the publish button is inactive. For more info, see the replies in the previous ticket: T224249

We want this

For both UW and FP:

  • The "Remove all" button should always be visible for non-depicts panels. This includes when a property has been added but there are no property values
  • The "Remove all" button deletes the parent property as well as any property values
  • The warning modal text is updated to say "This action will delete this property and any items for that property. Are you sure?" cancel/OK
  • Selecting the OK button should bypass the publish step and immediately delete everything

Acceptance Criteria:

  • "Remove all" button is visible for every panel, except for depicts
  • "Remove all" button is available at any point during the statement adding flow
  • "Remove all" button deletes the parent property and any values for that property
  • Warning modal text is updated to: "This action will delete this property and any items for that property. Are you sure?"
  • Selecting the OK button immediately deletes the statement panel

QA Results

Event Timeline

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

Change 525671 had a related patch set uploaded (by Eric Gardner; owner: Eric Gardner):
[mediawiki/extensions/WikibaseMediaInfo@master] Update how "Remove all" works for statement panels

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

The current patch makes this change for FilePage, but not for UploadWizard (the latter has a different publishing flow). However as far as I can tell this change in MediaInfo doesn't break any UW functionality. (things continue to work the same as they did before), so patches can be merged separately without issues.

Change 525876 had a related patch set uploaded (by Eric Gardner; owner: Eric Gardner):
[mediawiki/extensions/UploadWizard@master] Change "Remove All" button behavior

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

Ok, the second patch here makes this same set of changes in UploadWizard. I don't know how to specify cross-extension git dependencies, but the MediaInfo patch should be merged first.

Change 528246 had a related patch set uploaded (by Eric Gardner; owner: Eric Gardner):
[mediawiki/extensions/WikibaseMediaInfo@master] Support new UW statement removal flow

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

Here is the current status of this change in UploadWizard:

  1. Some additional changes in MediaInfo are needed to support what we want. This patch makes the necessary changes. Because the Statements code is used in slightly different ways in both MediaInfo and UploadWizard, the MediaInfo side (the file page, basically) needs to be carefully checked for any regressions. I didn't notice any but another set of eyes would be good.
  2. The actual changes in UploadWizard are now ready for review as well. This patch makes the change on the UW side. The MediaInfo patch needs to be merged first, because the UW patch identifies it as a dependency.

Both patches are now ready for review. Maybe someone can update Test Commons with both relevant patches so that some non-developers can QA? Would be good to do that alongside code review to try and ensure there are no regressions.

Change 528246 abandoned by Eric Gardner:
Support new UW statement removal flow

Reason:
Redundant patch, use https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseMediaInfo/ /525671 instead

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

You can disregard the abandoned patch above (it was redundant); the two patches linked in my previous comment are the ones we need.

Change 525671 merged by jenkins-bot:
[mediawiki/extensions/WikibaseMediaInfo@master] Update how "Remove all" works for statement panels

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

Change 525876 merged by jenkins-bot:
[mediawiki/extensions/UploadWizard@master] Change "Remove All" button behavior

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

Tested on Beta. Works fine on File pages, but on UploadWizard, clicking Remove All will remove the value but the empty statement remains. There's no way to remove an empty property.

This should be fixed once the patch for T231225 is merged.

Edtadros added a subscriber: Edtadros.

Test Result

Status: ✅ PASS
OS: macOS Mojave
Browser: Chrome
Device: MBP

Test Artifact(s):

Acceptance Criteria:

Upload Wizard

✅ AC1: "Remove all" button is visible for every panel, except for depicts
✅ AC2: "Remove all" button is available at any point during the statement adding flow
✅ AC3: "Remove all" button deletes the parent property and any values for that property
✅ AC4: Warning modal text is updated to: "This action will delete this property and any items for that property. Are you sure?"
✅ AC5: Selecting the OK button immediately deletes the statement panel

T228912.gif (980×994 px, 1 MB)

File Page

✅ AC6 "Remove all" button is visible for every panel, except for depicts
✅ AC7: "Remove all" button is available at any point during the statement adding flow
✅ AC8: "Remove all" button deletes the parent property and any values for that property
✅ AC9: Warning modal text is updated to: "This action will delete this property and any items for that property. Are you sure?"
✅ AC10: Selecting the OK button immediately deletes the statement panel

T228912b.gif (890×902 px, 1 MB)

Confirmed working on production, both mobile and desktop.