Page MenuHomePhabricator

Add baserevid to WikibaseLexeme API modules
Open, NormalPublic5 Story Points

Description

As user of the WikibaseLexeme API I want to be able to provide a baserevid for the edit i am making while using the WikibaseLexeme API modules.

A user of the WikibaseLexeme API = our UI

API modules that needs adjusting:

  • wbladdform
  • wbladdsense
  • wbleditformelements
  • wbleditsenseelements
  • wblremoveform
  • wblremovesense

Acceptance criteria:

All listed API modules now have the baserevid parameter

When baserevid is not valid (for a different entity .. etc)
Then API request should fail

When there's a conflict around baserevid (same behavior as wbeditentity endpoint)
Then API request should fail

Event Timeline

Restricted Application added a project: Wikidata. · View Herald TranscriptFeb 27 2019, 2:04 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
alaa_wmde updated the task description. (Show Details)Mar 5 2019, 4:05 PM
alaa_wmde set the point value for this task to 5.

Change 495908 had a related patch set uploaded (by Alaa Sarhan; owner: Alaa Sarhan):
[mediawiki/extensions/WikibaseLexeme@master] lexeme.api: require and check conflict on baserevid param

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

Change 495908 had a related patch set uploaded (by Alaa Sarhan; owner: Alaa Sarhan):
[mediawiki/extensions/WikibaseLexeme@master] lexeme.api: require and check conflict on baserevid param

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

Tarrow added a subscriber: Tarrow.Wed, Mar 27, 10:37 AM

I took a little look at this and I'm not feeling super clear on what the acceptance criteria are. My immediate thoughts would be that baseRevId not equalling the latest id shouldn't immediately result in a conflict.

@Addshore could you maybe give some examples in the acceptance criteria to make it explicit what the expected behaviour is?

Trying to mimic similar functionality from existing modules is hard because the checks are apparently spread about a bit. e.g. this red herring in https://github.com/wikimedia/mediawiki-extensions-Wikibase/blob/a7db4c9fd88bc1ddfdf3b5266b399cff7e808954/repo/includes/Api/EditEntity.php#L198 which makes the check look much simpler than I would expect it is

From Mattermost: according to @Addshore WikiPageEntityStore is the place to look for hints as to the existing logic

alaa_wmde added a comment.EditedThu, Mar 28, 1:41 PM

findings from reading through code again:

current logic and status quo:
in wikibase, explained here T126231
in lexeme, there was no such logic in lexeme -> we added very basic baserevid !== latestrevid

If I understand correctly, I think wikibase is not doing what we think it is. Wikibase only checks that the patch is valid on both base and current revisions, but does not do a two- or three-way diff (as far as I could see in code).

If that is true, then the current logic we did for lexeme is a better edit conflict prevention than what's in wikibase, and probably we can go with it for this iteration and leave implementing more sophisticated diffing and conflict detection for a later one.

But that might not be true. If someone knows from the top of their head I'd appreciate a shout. Otherwise, I'll have to go deeper later and figure out for sure.

alaa_wmde updated the task description. (Show Details)Wed, Apr 10, 8:38 AM

As @Tarrow pointed out the current patch has lot simpler solution compared to the existing logic on wbeditentity endpoint, so we have to change the endpoints covered by this task so that they behave consistently with wbeditentity behavior.