Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Mbodin opened this issue Jul 8, 2024 · 8 comments
Closed
Assignees

Comments

@Mbodin
Copy link

Mbodin commented Jul 8, 2024

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
Copy link
Member

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

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

@matkoniecz matkoniecz added the feedback required more info is needed, issue will be likely closed if it is not provided label Jul 8, 2024
@mnalis

This comment was marked as resolved.

@Mbodin
Copy link
Author

Mbodin commented Jul 8, 2024

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
Copy link
Member

mnalis commented Jul 8, 2024

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

small_Screen_Recording_20240708_210537_StreetComplete.mp4

@kmpoppe
Copy link
Collaborator

kmpoppe commented Jul 10, 2024

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

I think this might be due to

/** adds or modifies the given tag. If the updated tag is the same as before, sets the check date
* tag to today instead. */
fun Tags.updateWithCheckDate(key: String, value: String) {
val previousValue = get(key)
set(key, value)
/* if the value is changed, set the check date only if it has been set before. Behavior
* before v32.0 was to delete the check date. However, this destroys data that was
* previously collected by another surveyor - we don't want to destroy other people's data
*/
if (previousValue == value || hasCheckDateForKey(key)) {
updateCheckDateForKey(key)
}
}

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
Copy link
Collaborator

kmpoppe commented Jul 13, 2024

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
Copy link
Member

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 westnordost removed the feedback required more info is needed, issue will be likely closed if it is not provided label Jul 13, 2024
@westnordost westnordost self-assigned this Aug 15, 2024
@westnordost
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants