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

Resurvey quests and Undo #2861

Closed
peternewman opened this issue May 8, 2021 · 11 comments
Closed

Resurvey quests and Undo #2861

peternewman opened this issue May 8, 2021 · 11 comments
Labels
bug wontfix idea rejected because it is out of scope or because required work is not matching expected benefits

Comments

@peternewman
Copy link
Collaborator

If you undo a resurvey quest, the resurvey quest doesn't reappear. This is understandable because the node has been modified more recently.

How to Reproduce

  1. Answer a quest e.g. is this shop still vacant, incorrectly tag it as now being something (as you've looked at the adjacent shop).
  2. Undo the answer
  3. The quest doesn't re-appear.

In my case it was vacant anyway, so aside from it still being tagged as shop=vacant rather than disused:shop=vacant there wasn't any real issue, but if I'd just incorrectly tagged the type of shop, I'd have to add a note or wait quite some time for it to re-appear.

I'm wondering if you could fix this by cheating it with the check_date? i.e. when you undo an edit could it set the check_date to the value of when the last edit to the item was? If a node has an old check_date and a more recent last modified date, which value does SC use? Which is deemed to be valid more generally?

Versions affected
v32.0-alpha1

@peternewman peternewman added the bug label May 8, 2021
@westnordost
Copy link
Member

auto-sync on I presume?

@westnordost
Copy link
Member

If a node has an old check_date and a more recent last modified date, which value does SC use? Which is deemed to be valid more generally?

The filter is

...
older today -X years
or check_date < today -X years
or lastcheck < today -X years
or last_checked < today -X years
or survey:date < today -X years
or survey_date < today -X years

So, in that sense, it picks the oldest date. Your cheat would work, but this is very specific to the check existance quest.

That also means, that if the element before you answered the quest already had any kind of check date, that check date will get re-added on the revert. Thus, there is no problem in that case.

The revert functionality is generic and very much decoupled from the specific quests. (I.e.: Not quest answers are reverted, but edits). Thus, it would be quite a hack to do this and thus, it is a will not fix.

To solve this issue for you, you could turn off auto-sync.

@westnordost westnordost added the wontfix idea rejected because it is out of scope or because required work is not matching expected benefits label May 8, 2021
@peternewman
Copy link
Collaborator Author

auto-sync on I presume?

Only on Wi-Fi, although there's a chance I might have had a connection at the time.

Indeed retesting in airplane mode I can't reproduce so I presumably did have a connection.

So, in that sense, it picks the oldest date. Your cheat would work, but this is very specific to the check existance quest.

Maybe I'm misreading the code, but won't it do the same thing for any of the resurvey quests, e.g. the surface one:

or surface ~ ${ANYTHING_UNPAVED.joinToString("|")} and surface older today -4 years
or surface older today -12 years

That also means, that if the element before you answered the quest already had any kind of check date, that check date will get re-added on the revert. Thus, there is no problem in that case.

Yes, I appreciate it will be fine if it had a check date, i.e. it has already been resurveyed, but it seems like this will trigger if it's just gone stale.

The revert functionality is generic and very much decoupled from the specific quests. (I.e.: Not quest answers are reverted, but edits). Thus, it would be quite a hack to do this and thus, it is a will not fix.

As above, given it affects all re-survey quests, I don't think it really needs a special case.

To solve this issue for you, you could turn off auto-sync.

So given my normal Wi-Fi usage, I won't ordinarily hit this then I guess. Although I'm assuming most people are on a data connection so will.

@westnordost
Copy link
Member

Maybe I'm misreading the code, but won't it do the same thing for any of the resurvey quests, e.g. the surface one:

What do you mean, what same thing would it do? You cited the quest filter. The quest filter and what is being tagged is two different pairs of shoes.

Yes, I appreciate it will be fine if it had a check date, i.e. it has already been resurveyed, but it seems like this will trigger if it's just gone stale.

Currently, StreetComplete deletes the check date if a StreetComplete user answered that a thing changed. F.e. if there was

amenity=post_box
collection_times=Mo-Fr 16:00
check_date:collection_times=2020-09-05

and the user changed it to collection times Mo-Sa 16:00, StreetComplete would then retag it to

amenity=post_box
collection_times=Mo-Sa 16:00

I know at least one person who would be very unhappy about this behavior of StreetComplete. So maybe at least if already a check date exists, it should not delete it but update it.

@peternewman
Copy link
Collaborator Author

Maybe I'm misreading the code, but won't it do the same thing for any of the resurvey quests, e.g. the surface one:

What do you mean, what same thing would it do? You cited the quest filter. The quest filter and what is being tagged is two different pairs of shoes.

I'm assuming from your previous comment older today -X years means the node/way was last modified more than X years ago? i.e. it's equivalent to finding the file age of a file on a filing system.

Yes, I appreciate it will be fine if it had a check date, i.e. it has already been resurveyed, but it seems like this will trigger if it's just gone stale.

Currently, StreetComplete deletes the check date if a StreetComplete user answered that a thing changed.

But that's not relevant in the case of an undo, as it presumably just puts back the old data.

I think there are two paths, and this issue will trigger on one of them.


Node created 2019-01-01:

amenity=post_box
collection_times=Mo-Fr 16:00

Node resurveyed a year later:

amenity=post_box
collection_times=Mo-Fr 16:00
check_date:collection_times=2020-01-01

Node reresurveyed another year later:

amenity=post_box
collection_times=Mo-Fr 16:00
check_date:collection_times=2021-01-01

That's fine, it all ticks along nicely.


Whereas in the problematic path:
Node created 2019-01-01:

amenity=post_box
collection_times=Mo-Fr 16:00

Node resurveyed a year later:

amenity=post_box
collection_times=Mo-Fr 16:00
check_date:collection_times=2020-01-01

At this point I realise I misread it, and it is now Mo-Sa 16:00, so I undo the answer. The node now looks like this:

amenity=post_box
collection_times=Mo-Fr 16:00

But it was modified today (because of the undo). In filesystem terms, although the md5sum of the before and after files match, your undo has essentially done a touch of the file.

Therefore it will be another whole year until the file has aged enough again and I get the resurvey question coming up again.

Whereas if it did the undo, but also added an additional (fake old) check_date field, so it looks like so:

amenity=post_box
collection_times=Mo-Fr 16:00
check_date:collection_times=2019-01-01

It would immediately trigger the resurvey and I could correct my error.

I know at least one person who would be very unhappy about this behavior of StreetComplete. So maybe at least if already a check date exists, it should not delete it but update it.

Yeah I can see the logic in just refreshing the check date, rather than modifying or deleting depending on what was already present.

The delete also exacerbates the above, as instead of just being possible on new nodes, it also becomes possible on changed nodes too.

@matkoniecz
Copy link
Member

Whereas if it did the undo, but also added an additional (fake old) check_date field, so it looks like so:

Fake data also could end in OSM database. At least in history, and in case of not resurveying it - in the latest object version.

@mnalis
Copy link
Member

mnalis commented May 10, 2021

But it is not really fake data, is it? It is actual correct data, only that data was implicit before, but (due to technicality how OSM API works) should become explicit now. And I see no reason why correct data should not end in node history and even in latest object version.

Eg. in case above, the node data was really last time surveyed/checked when it was created 2019-01-01. But modification date was implicit in node timestamp, not as any tag. But due to the fact that we (incorrectly) updated that node, it's timestamp is now 2020-01-01 (as well as having additional check_date:collection_times or similar tag). We undo it, and thus get rid of check_date:collection_times tag, but node modification time remains at 2020-01-01, which (without any additional tags) would indicate that it was last resurveyed on 2020-01-01, which is wrong.

Ie. after the undo, the node currently does not convey the same information as it did before the SC start changing it (and IMHO, after undo, it should convey the same information as it did before it was changed the first time by SC, even in means adding some tag that was not there before).

So adding a check_date* tag that explicitly adds back the correct last resurvey date (via check_date:collection_times=2019-01-01 in example above) does not look problematic to me when looking on truthfulness issue (but I have no idea about easiness of technical implementation side of things, though)

@peternewman
Copy link
Collaborator Author

Yeah that's what I meant @mnalis . You've put it rather more clearly than I'd managed.

I guess technically we'd need to find the last time a particular tag changed, rather than just the most recent modification of the whole object.

@westnordost
Copy link
Member

Yes, I appreciate it will be fine if it had a check date, i.e. it has already been resurveyed, but it seems like this will trigger if it's just gone stale.

Currently, StreetComplete deletes the check date if a StreetComplete user answered that a thing changed.

But that's not relevant in the case of an undo, as it presumably just puts back the old data.

An undo if the edit has been uploaded already is applying the exact change made but in reverse.
So, if the change was to change the collection times from X to Y and also delete the check date Z, the reverse of this is to change collection times times from Y to X and add the check date Z.

I have been trying to explain this. The edit itself has nothing to do with the quest and does know nothing about what the tags it is going to change mean. Thus, it cannot decide on a case by case basis to instead of reverting a tag to a certain value, tag something else instead.

@westnordost
Copy link
Member

In a nutshell: It is not possible. My posts here after closing this issue are here to explain to you why this is the case.

@mnalis
Copy link
Member

mnalis commented May 10, 2021

Yes, I was afraid the technical way it is implemented would make it problematic to change. I could imagine some kludges that probably might work around it, but I understand why they are unwanted and add complexity.

Anyway, it seems like v32 (when released) would work quite nice with auto-upload disabled (as opposed to previous versions which were severely crippled with auto-upload disabled), which would avoid this specific issue from happening (as well as saving battery power due to less network activity - which is always good).

Or we as users could try to be little more careful when solving quests 😄 (and not relying so much on undo capability being perfect, which it cannot be in auto-upload mode due to way OSM API works)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug wontfix idea rejected because it is out of scope or because required work is not matching expected benefits
Projects
None yet
Development

No branches or pull requests

4 participants