streetcomplete / StreetComplete

Easy to use OpenStreetMap editor for Android
https://streetcomplete.app
GNU General Public License v3.0
3.81k stars 349 forks source link

'Is this still here' asked immediatly after 'vegetarian option' has been filled #5729

Closed Mbodin closed 1 week ago

Mbodin commented 1 month ago

This is a similar issue than #5674, but with the vegetarian option of a restaurant: it would be nice not to ask whether a restaurant is still there if we just answered that it had vegetarian option.

How to Reproduce Similar than in #5674, select a restaurant with a quest asking whether is has a vegetarian option. After completing the vegetarian quest, I was being asked whether the restaurant is still there.

Note that I possibly have changed the order of appearance of my quests: that might be the issue.

Expected Behavior If completing a quest about a restaurant, it must be there, so this quest came as a surprise.

Versions affected Android 13 StreetComplete v58.1

westnordost commented 1 month ago

What did you answer? Can you link the element for which this happened?

(AFAIK the implementation for #5674 should work for any quest)

mnalis commented 1 month ago

FYI, I can't reproduce in 58.1 on Android 14 (with auto-upload disabled) on few random old restaurants (like w800485910):

https://github.com/streetcomplete/StreetComplete/assets/156656/0f2b3fa9-611d-4982-a122-28af2b18a515

Even when trying different quest ordering (putting existence quest first, hiding it, answering the vegetarian quest, and unhiding existence quest - still the existence quest would not be shown again).

Mbodin commented 1 month ago

Thanks for the feedback. Here is the restaurant: https://www.openstreetmap.org/node/5138626545

I've been trying on other random restaurants, and indeed I can't reproduce it on other places. I'm happy to close the issue and reopening it once I found another ☺

mnalis commented 1 month ago

Thanks for the link! I now can reproduce in 58.1 on any restaurant having old survey:date, like e.g. n8892006690 "OLD smokey"

https://github.com/streetcomplete/StreetComplete/assets/156656/3ae5ce61-35ab-4abc-9475-6d35a856239f

kmpoppe commented 1 month ago

Looking at the changesets from @Mbodin, the diet:vegetarian quest (cs) did not remove survey:date.

I think this might be due to https://github.com/streetcomplete/StreetComplete/blob/fa538aaf983bc32559e8cc6f682fdbe10dd0133c/app/src/main/java/de/westnordost/streetcomplete/osm/ResurveyUtils.kt#L41-L53

previousValue and value are not identical (there isn't a previous value, we're just setting it) and there is also no check_date:diet:vegetarian on the restaurant, thus the code never reaches Tags.updateCheckDateForKey -> Tags.setCheckDateForKey -> Tags.setCheckDate -> Tags.removeCheckDates and subsequently will not remove survey:date.

(Another SC 58.1 example is this node where check_date was not set to the most recent date, even though the Opening Hours quest was answered ==>i.e. no opening_hours before and no check_date:opening_hours ==> no code execution of Tags.updateCheckDateForKey)

kmpoppe commented 1 month ago

So, when

we don't want to destroy other people's data

the question is: what good is a survey_date that's older than what we'd remove anyway when then checkExistence quest runs? I would argue (because I cannot wrap my head around how that would be done programatically in this project), that

if (previousValue == value || hasCheckDateForKey(key)) {

should be expanded to include isApplicableTo from .quest.existence (however that would be done without knowing the element in this method), and therefore call updateCheckDateForKey (and subsequently removeCheckDates) also when the Existence-Timer was up.

Thoughts?

westnordost commented 1 month ago

Uhm... my first thought would be removing/updating the general check date should maybe be done independently of whether the per-key check date is updated. But I haven't looked closely at the code yet, and I figure if I would, I would already implement a solution. Right now, as this is neither urgent nor important, I am busy with other stuff.

westnordost commented 1 week ago

I think the easy solution is to also set the check-date-for-key if the generic check date is set.