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

ACStatusDetails
1โœ…T228912#5475221
2โœ…T228912#5475221
3โœ…T228912#5475221
4โœ…T228912#5475221
5โœ…T228912#5475221
6โœ…T228912#5475221
7โœ…T228912#5475221
8โœ…T228912#5475221
9โœ…T228912#5475221
10โœ…T228912#5475221

Event Timeline

Restricted Application added a project: Multimedia. ยท View Herald TranscriptJul 24 2019, 5:37 PM
Restricted Application added a subscriber: Aklapper. ยท View Herald Transcript
egardner claimed this task.Jul 24 2019, 7:00 PM

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

egardner added a comment.EditedAug 5 2019, 11:55 PM

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

Aklapper edited projects, added Structured-Data-Backlog; removed Multimedia.Aug 10 2019, 11:54 PM

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 reassigned this task from Edtadros to Ramsey-WMF.Sep 9 2019, 2:46 PM
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

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

Edtadros updated the task description. (Show Details)Sep 9 2019, 2:47 PM
Ramsey-WMF closed this task as Resolved.Sep 10 2019, 1:46 AM

Confirmed working on production, both mobile and desktop.