Page MenuHomePhabricator

Step 1: make edit based on fix/update choice (impact: high)
Closed, ResolvedPublic13 Estimated Story Points

Description

Problem:
In T237333 we built the UI part of deciding the right edit flow (fixing or updating a value). We now need to add the correct edit flow in the backend.

BDD

GIVEN a Bridge edit
AND the editor chooses fix
WHEN saving the edit
THEN the new value overwrites the existing value
AND the rank of the value is not changed
AND the qualifiers and references are preserved

GIVEN a Bridge edit
AND the editor chooses update
WHEN saving the edit
THEN the rank of the existing value is set to normal
AND the new value is stored with preferred rank without qualifiers and references
AND the old value keeps the existing qualifiers and references

Acceptance criteria:

  • the new statement is stored with preferred rank (update flow)
  • the old value is either overwritten (fix flow) or kept with normal rank (update flow)

Notes

Event Timeline

Restricted Application added a project: Wikidata. · View Herald TranscriptNov 19 2019, 3:41 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Lydia_Pintscher triaged this task as High priority.Nov 19 2019, 3:42 PM
Lydia_Pintscher moved this task from Backlog to Needs work on the Wikidata-Bridge board.
Charlie_WMDE updated the task description. (Show Details)Nov 28 2019, 3:24 PM
Lydia_Pintscher moved this task from Needs work to Ready to estimate on the Wikidata-Bridge board.

We likely want to do the update flow in two edits to have useful edit summaries.

Note: Then we cannot ensure that both edits succeed, which could leave the item in an incorrect state.

Charlie_WMDE updated the task description. (Show Details)Dec 3 2019, 11:09 AM

Estimation of a the story as it's written in this ticket: 13 points
Notes:

  • cost of ownership are higher
  • more error prone - would need communication to the users + follow up action items
  • better out of the box edit summaries

Estimation of the story where we do the edit in a single revision, with an out-of-the-box edit summary (i.e. whatever it currently is without making any changes to it): 13 points
Notes:

  • this would need some sort of communication because of the new edit pattern in the history
  • this will have not great edit summaries and will probably need an edit summary upgrade later

@Lydia_Pintscher we need a product decision here on which story to proceed with.

Charlie_WMDE set the point value for this task to 13.Dec 3 2019, 12:02 PM
darthmon_wmde renamed this task from make edit based on fix/update choice to Step 1: make edit based on fix/update choice.Dec 5 2019, 1:07 PM
Lydia_Pintscher renamed this task from Step 1: make edit based on fix/update choice to Step 1: make edit based on fix/update choice (impact: high).Dec 8 2019, 1:19 PM

Based on discussion with @Jakob_WMDE and @Rosalie_WMDE it should be possible to get a nicer edit summary for this specific usecase based on what they've worked on. Let's make use of that in T233395 and go with 1 edit here.

Notes from task breakdown, describing desired implementation:

  • statements module has StatementMap, which is only updated after save and always matches the revision ID in entity module
  • root module has edit decision and string value
  • on save, root module action clones statements module’s StatementMap, applies the edit (either replace or update), and sends this to the API
    • on success, update statements module’s StatementMap and entity module’s revision ID
    • on failure, not much to do – retry (compare T244356) can do the same thing again
    • the cloned StatementMap is ephemeral, not stored, probably just a local-scope variable of the save action
      • which does mean that the code to modify it (like the current setStringDataValue) is not an action/mutation, it just updates a plain object
  • the root module’s originalStatement can be removed, since the statements module’s StatementMap isn’t changed until save anyways

(This is not the current implementation! Currently, the statements module’s StatementMap is updated with each keystroke.)

We were unable to find any split more fine-grained than this in the task breakdown:

  1. add string value to root store (edit decision is already there)
  2. stop updating statements module with every keystroke, move save action to root module, have it clone the StatementMap and edit stuff
  3. respect edit decision, implement update (incl. browser test)

#2 is most of the work, but can’t really be split up further without producing a broken intermediate state where edits can’t be saved. And since all of these have to happen in series, it doesn’t seem useful to split them into subtasks either.

Change 571321 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] bridge: add targetValue to root state

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

Unassigned myself – anyone is free to pick up #2 and #3.

Change 571553 had a related patch set uploaded (by Pablo Grass (WMDE); owner: Pablo Grass (WMDE)):
[mediawiki/extensions/Wikibase@master] bridge: save targetValue into statement only on save

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

Pablo-WMDE moved this task from Peer Review to Doing on the Wikidata-Bridge-Sprint-13 board.

Change 571321 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] bridge: add targetValue to root state

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

Pablo-WMDE set the cover image to Unknown Object (File).Feb 13 2020, 2:55 PM
Pablo-WMDE updated the cover image to Unknown Object (File).Feb 13 2020, 3:52 PM
Lucas_Werkmeister_WMDE updated the cover image to Restricted File.Feb 14 2020, 12:49 PM
Lucas_Werkmeister_WMDE updated the cover image to Restricted File.Feb 14 2020, 12:54 PM
Lucas_Werkmeister_WMDE updated the cover image to Restricted File.Feb 14 2020, 1:04 PM

Change 573972 had a related patch set uploaded (by Pablo Grass (WMDE); owner: Pablo Grass (WMDE)):
[mediawiki/extensions/Wikibase@master] bride: prepare for different statement mutations

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

Change 571553 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] bridge: put targetValue into statement only on save

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

Change 574476 had a related patch set uploaded (by Pablo Grass (WMDE); owner: Pablo Grass (WMDE)):
[mediawiki/extensions/Wikibase@master] bridge: add update strategy and wire it into save

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

Change 574476 abandoned by Pablo Grass (WMDE):
bridge: add update strategy and wire it into save

Reason:
Will be continued in I6e53f0a378f8835007046da846daeead7e244e6f

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

Pablo-WMDE removed Pablo-WMDE as the assignee of this task.Feb 24 2020, 5:39 PM
Pablo-WMDE added a subscriber: Pablo-WMDE.

Unassigning myself. https://gerrit.wikimedia.org/r/573972 is not merged yet, but step 3 is in the making.

Change 574997 had a related patch set uploaded (by Pablo Grass (WMDE); owner: Pablo Grass (WMDE)):
[mediawiki/extensions/Wikibase@master] bridge: add update strategy and wire it into save

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

Change 575218 had a related patch set uploaded (by Pablo Grass (WMDE); owner: Pablo Grass (WMDE)):
[mediawiki/extensions/Wikibase@master] bridge: move and rename SnakActionErrors

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

Thought during demo: "value was never correct and has never been" - people with conflicting opinion (think: politics) will most certainly select this option, even though from a technical standpoint we would want them to file a "competing" statement instead. Maybe this could be taken into account in the labels/explanatory copy somehow...

/cc @Lydia_Pintscher @Charlie_WMDE

Change 573972 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] bridge: prepare for different statement mutations

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

Change 575519 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] bridge: declare Statement ID as optional

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

Change 575519 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] bridge: declare Statement ID as optional

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

Change 575218 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] bridge: move and rename SnakActionErrors

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

Pablo-WMDE reassigned this task from Pablo-WMDE to Michael.Mar 2 2020, 10:29 AM
Restricted Application added a project: User-Michael. · View Herald TranscriptMar 2 2020, 10:29 AM

Change 575596 had a related patch set uploaded (by Michael Große; owner: Michael Große):
[mediawiki/extensions/Wikibase@master] bridge: Add UpdateMutationStrategy

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

Change 576063 had a related patch set uploaded (by Michael Große; owner: Michael Große):
[mediawiki/extensions/Wikibase@master] bridge: choose strategy based on edit decision

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

Change 575596 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] bridge: Add UpdateMutationStrategy

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

Change 574997 abandoned by Pablo Grass (WMDE):
bridge: add update strategy and wire it into save

Reason:
Will be looted in Idbd7b90e973710a7b5976010568a4b852fcd891a ff.

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

Change 576063 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] bridge: choose strategy based on edit decision

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

Yay :) I just tested it. The edits look coorect \o/

One bummer still: When making an edit and selecting the "I updated an outdated value" choice and then clicking publish then I get a flash of one of the error screens before the Bridge closes and the page reloads. It is too quick for me to see which one.

@Lydia_Pintscher There was some discussion about this very topic during the implementation (of this and recently in T235208). The hope - in my understanding - was that T240333 would be a better place to discuss how this should be modeled in detail - when we don't only know what to transition away from, but also what to transition to. Not sure how acceptable this intermediate state is.

I think the discussion we had was only about flashing the normal editing interface again – I at least didn’t expect any error to flash up, and I don’t remember seeing that in my own testing :/

Oh no, wait – if you use the update choice, the error will be the “ambiguous value” bail-out, I suppose.

Lydia_Pintscher closed this task as Resolved.Mar 10 2020, 8:21 AM
Lydia_Pintscher moved this task from To do to Done on the Wikidata-Bridge-Sprint-15 board.

Discussed it with Pablo. I'll close this here now and then we can fix it with the reference screen as said earlier.