commons-app / apps-android-commons

The Wikimedia Commons Android app allows users to upload pictures from their Android phone/tablet to Wikimedia Commons
https://commons-app.github.io/
Apache License 2.0
985 stars 1.18k forks source link

[Bug]: Editing depicts clears every other structured data associated with the image #5728

Open sivaraam opened 2 months ago

sivaraam commented 2 months ago

Summary

Our editing of depicts seems to cause a bit more harm than good now. When trying to edit the depictions, the depictions are changed but all other structured data associated with the image such as the "caption", "creator" etc. are totally removed. We should make sure such a thing doesn't happen.

Sample edits that demonstrate this issue: [edit 1] [edit 2]

This one feels a bit serious and it is not even easy to revert such edits due to some abuse filter blocking such reverts [ref]. Can we just disable editing of depictions as a temporary measure until this one's fixed? If trivial, it would be great to just fix the core issue, of course 🙂

Steps to reproduce

  1. Login to the app
  2. Open an already uploaded image
  3. Try to edit the depiction for the image
  4. Check the edit history of that image on the website

Expected behaviour

Only the list of depictions of the structured data get modified as a consequence of the edit.

Actual behaviour

The edit results in all other structured data associated with the image being removed and only the new set of depicts being retained.

Device name

OnePlus Nord

Android version

12

Commons app version

5.0.1

Device logs

No response

Screen-shots

No response

Would you like to work on the issue?

None

rohit9625 commented 2 months ago

How long has this problem been here? Can I get any class reference to the related code?

sivaraam commented 2 months ago

This might be a good place to start: https://github.com/commons-app/apps-android-commons/blob/main/app/src/main/java/fr/free/nrw/commons/wikidata/WikidataEditService.java#L108

sivaraam commented 2 months ago

The clear=1 in the linked line looks suspicious. It's worth exploring if toggling that helps. Ref: https://www.wikidata.org/w/api.php?action=help&modules=wbeditentity

rohit9625 commented 2 months ago

Yes, you're right. This request is being sent to the server when updating depicts: image

As referenced, when the clear flag is set, i.e., 1 or true, it clears the entity which is shown here in the history: image

After deleting the clear=1 the entity is not considered to be cleared rather it changes the entity: image

However, the depictions are passed by the Commons App, are appended to the previous ones even one depiction was already there. image

sivaraam commented 2 months ago

After deleting the clear=1 the entity is not considered to be cleared rather it changes the entity:

Ah. That's an interesting observation.

However, the depictions are passed by the Commons App, are appended to the previous ones even one depiction was already there.

Got it. So, it seems like the clear=1 was there for a reason. But at least are the other structured data information retained when the clear is dropped? If so, we could explore other possible parameters / endpoints that'll help us ensure only a specific property is cleared.

rohit9625 commented 2 months ago

Yes, the other data information is retained. One more thing I noticed in the docs, i.e., the clear flag should be set like clear=true but in our codebase, it was clear=1. Well, it is the same thing in computers but does the API know that?

sivaraam commented 2 months ago

Yes, the other data information is retained. One more thing I noticed in the docs, i.e., the clear flag should be set like clear=true but in our codebase, it was clear=1. Well, it is the same thing in computers but does the API know that?

I don't think we have to worry about that discrepancy since the clearing was happening without any doubt. The examples in wbeditentity API page showcase how the data could be tweaked to remove something. It would be worth exploring it to see of we could tweak the structure of the data parameter we send such only the depictions are cleared/updated with the new ones.

rohit9625 commented 2 months ago

We have to pass the whole structured data so that the entity gets updated with previous data as well as edited data. Currently, we are passing only depicted item's IDs, that's why only that gets updated and other data is cleared out due to a clear flag.

rohit9625 commented 1 month ago

@sivaraam The bots keep adding the structured data even if it was completely deleted by our application. So, is this a feature or was it always like this?

rohit9625 commented 1 month ago

Look at here: updating categories

This updation doesn't delete the old structured data and the reason is, the previous data is appended rather than clearing old data and adding just the modified one.

Here, this function is responsible for updating categories and there is a parameter "wikiText" that is being passed and manipulated with the updated categories. image

Afterward, this function is responsible for replacing the old wikiText(The text that contains the file's data) with the new updated one. image

However, this is the wikiText that was first fetched from the server and there is nothing like depictions, why? image

What are your thoughts about this?

sivaraam commented 1 month ago

@sivaraam The bots keep adding the structured data even if it was completely deleted by our application. So, is this a feature or was it always like this?

Yeah. Those are friendly bots that run in Commons just doing their job. No issues about that :-)

sivaraam commented 1 month ago

What are your thoughts about this?

You are confusing category edit and depictions edit. Both are different and even use different APIs. Categories exist in commons. Depictions has to do with structured data. You could check this article for reference on Structured data on Commons.

Hope that clears your confusion.

rohit9625 commented 1 month ago

Hello Sivaraam,

I want to read about these terms(like mainsnak and snaktype) shown in the screenshot. I found another action that can help with the desired behaviour but I am unsure about the parameters. image

rohit9625 commented 1 month ago

Hey @sivaraam, I solved the issue but, the whole task is being done on two phases:-

  1. Deleting all depictions
  2. Updating depictions with the new data.

See history

Although, after so much research and debugging the second way I can see is, set clear flag, it'll delete the whole entity and then send updated depictions along with other related data that was deleted.

I hope I was helpful :)