Page MenuHomePhabricator

Make it impossible to set the same content in the same language for label and description
Closed, ResolvedPublic5 Story Points

Description

Problem: Although with T100933 we already have this check for Special:NewItem, it's still possible to create an Item with the same label and description in a certain language through other interfaces, and it's always possible to set them once the Item has been created. The same should also not be possible for Properties.

BDD
GIVEN an Item or Property
WHEN adding a new label or description in one language that leads to them being identical
OR changing an existing label or description in one language that leads to them being identical
THEN an error is shown
AND the edit not saved

Acceptance criteria:

  • Given any Item or Property, it's not possible to set the same non-empty content for the label and description in that language via UI or API

Event Timeline

abian created this task.Jan 3 2019, 4:14 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 3 2019, 4:14 PM
abian triaged this task as Normal priority.Jan 3 2019, 4:18 PM
Lydia_Pintscher updated the task description. (Show Details)
Lydia_Pintscher moved this task from Incoming to Ready to go on the Wikidata-Campsite board.
WMDE-leszek updated the task description. (Show Details)Apr 10 2019, 1:38 PM
WMDE-leszek updated the task description. (Show Details)
WMDE-leszek set the point value for this task to 5.

A few questions: should this affect Special:NewProperty?
Should it also prevent undo/restore/rollback, if the resulting content has labels and descriptions that are the same?

alaa_wmde added a subscriber: alaa_wmde.EditedApr 15 2019, 7:00 PM

For Special:NewProperty yes it is part of the acceptance criteria so it should also fix that one.

For rollback, I think that it should not disallow it even if it will set the item/property into a kinda invalid. Otherwise, a vandal might introduce a change that we want to revert, and if the item/property had same label&description before that point, the editor's only option is to make a new edit instead of rolling back. That sounds less ideal for editors, I think. Sounds sensible to you?

Change 504283 had a related patch set uploaded (by Greta WMDE; owner: Greta Doçi):
[mediawiki/extensions/Wikibase@master] Prevent label = description to Special:NewProperty

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

Greta and I tried yesterday to implement this in the SetLabel and SetDescription APIs. Their parent class ModifyEntity has a convenient validateEntitySpecificParameters method, where you can validate the array $preparedParameters against the EntityDocument $entity and raise an error if the new label/description in the $preparedParameters matches the description/label in the $entity. However, we found that it didn’t work correctly in the case where you set both label and description to the same new value in the UI. That’s because in that case, the second API request (to set the description) specifies baserevid as the revision ID of the original loaded page, not the new revision ID returned by the first API request (to set the label); and ModifyEntity loads the entity at the base revision and runs validateEntitySpecificParameters against that, so it would check if the new description doesn’t match the old label, rather than the new one in the latest revision.

Also, this approach requires separate implementations in Special:SetLabel, Special:SetDescription, Special:SetLabelDescriptionAliases, wbeditentity, and possibly more places that we forgot.

An alternative place to implement this check would be deep in the bowels of the entity saving code (at the point where we actually have the parent revision), e. g. in MediawikiEditEntity::attemptSave(), which ends up calling the EditFilterMergedContent hook. But this might also affect undo/restore/rollback…

But this might also affect undo/restore/rollback…

I don't think (as an editor) that's a problem. The wrong edit, and the one that should be reverted, is the one with identical label and description. Unless low-level data looses its integrity or Wikidata crashes (I don't think so) in the very unlikely event of reverting to an edit with identical label and description, I think this option would be equally great.

hoo added a subscriber: hoo.Apr 17 2019, 7:10 AM

Why not do this as part of ChangeOpDescription and ChangeOpLabel? I know some places don't use this, but this seems to be the right place to me.

Change 504283 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Prevent label = description to Special:NewProperty

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

Unfortunately the ChangeOp approach seems to have the same problem: it uses the base revision, not the parent revision, to check if the label and the description are the same :(

Change 506172 had a related patch set uploaded (by Greta WMDE; owner: Greta Doçi):
[mediawiki/extensions/Wikibase@master] Add LabelDescriptionNotEqualValidator

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

Change 506181 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Add CompositeFingerprintValidator

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

Change 506182 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Use LabelDescriptionNotEqualValidator in TermValidatorFactory

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

hoo added a comment.Apr 24 2019, 8:07 PM

Unfortunately the ChangeOp approach seems to have the same problem: it uses the base revision, not the parent revision, to check if the label and the description are the same :(

But you still went with it (indirectly, as the FingerprintValidator you're adding is used there)? Does that cause problems elsewhere? Is there a task for that?

alaa_wmde added a comment.EditedApr 24 2019, 8:38 PM

Sorry to come late to this one. I can still see one problem here even if we find a way to tell the second request to look at the right revision id.

The problem comes from the fact that UI is making two separate requests for what appears to be an atomic action (save all changed terms) to the editor. While that haven't caused any issues before now, and probably was decided to be done that way so that changing labels, descriptions and aliases get their own edit revisions separate from each other (for easier rollbacks and more atomic history on level of term edits), bubbling up this kind of separation up to the UI is problematic in this case.

Why is that a problem now?
Let's assume that second request (description in this case) is "fixed" and will look at the parent revision id instead of baserevid and it will detect the duplication correctly and fail.
In that case, the label will be changed, but the description will not, which is not the right thing to do here from UX perspective. The editor is editing both, and press save expecting both to be saved or nothing to be saved if they did a mistake or didn't know they can't use same label and description. Their next step will be to go and rollback their edit to the label, that if they even noticed that it was saved after they get an error message (due to the failing second request)

How to solve UX issue?
If that part of editing does not work without javascript anyway, we can add some client-side validation. If not, then we have to do one request for saving (can we use wbeditentity endpoint instead for instance?).

Unfortunately the ChangeOp approach seems to have the same problem: it uses the base revision, not the parent revision, to check if the label and the description are the same :(

But you still went with it (indirectly, as the FingerprintValidator you're adding is used there)? Does that cause problems elsewhere? Is there a task for that?

We might have tested something wrong – with the changes linked above, I can’t reproduce this problem anymore.

The problem comes from the fact that UI is making two separate requests for what appears to be an atomic action (save all changed terms) to the editor. While that haven't caused any issues before now, and probably was decided to be done that way so that changing labels, descriptions and aliases get their own edit revisions separate from each other (for easier rollbacks and more atomic history on level of term edits), bubbling up this kind of separation up to the UI is problematic in this case.

I’m pretty sure it has caused issues before now, we just haven’t addressed them yet. Part of the reason why it’s not great to use wbeditentity for these edits is that it results in a very generic edit summary (compare T67846 or T191885), but that will be tackled soon as part of the termbox hike, which already uses wbeditentity: T220696: Create better edit summaries for wbeditentity API endpoint

However, I don’t think that needs to block this task. For now, it’s okay if the label edit goes through and the description is blocked (my understanding is that these cases are often mistakes by confused users, where the label is correct but the description isn’t), and we can switch to wbeditentity later, once it has better edit summaries.

@Lucas_Werkmeister_WMDE gold 👍

Just wanted to raise awareness of that aspect, and seems I was lacking behind the current awareness around the topic.. that's awesome news!

Change 506172 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add LabelDescriptionNotEqualValidator

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

Change 506181 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add CompositeFingerprintValidator

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

Change 506182 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Use LabelDescriptionNotEqualValidator in TermValidatorFactory

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

I tested it on beta and prod and both seems to work, but now I'm a bit confused. Is it already live, or did I test the wrong thing?

I tested it on beta and prod and both seems to work, but now I'm a bit confused. Is it already live, or did I test the wrong thing?

It is already live and it should work on both. I guess if you followed the steps written in the description, then you tested it right.

Lea_Lacroix_WMDE closed this task as Resolved.

OK that's what I wanted to know, thanks :)