Don't set check_date:opening_hours unless surveyed

[spagy]

Fixes #4273

Previously, when resurveying opening hours, if a user said there was no
opening hours sign then we'd update check_date:opening_hours to the
survey date. This implies that the opening hours are correct but the
user hasn't actually checked that.

Instead of doing that, we now update check_date:opening_hours:signed.
This means that the quest will be hidden again for a while but we avoid
marking the opening hours as recently checked if they haven't actually
been confirmed.

[matkoniecz]

Have you tested it?

If you are not really planning to work on it let me know and I will test it is it actually working (you can help by finding some old shop with opening_hours check date).

But if you are interested - then https://github.com/streetcomplete/StreetComplete/blob/master/CONTRIBUTING.md#development has some docs (and yes, initial setup will take some time)

There are few things that could go wrong (not actually suppressing immediate asking "are opening hours signed", crashing or something)

[Helium314]

It might also be good to remove the opening_hours:signed check date when removing opening_hours:signed.

[spagy]

Have you tested it?

i'm not sure exactly what testing is needed. here's how i tested:

• ran the app, with the change, on my phone (without logging in to my osm account)
• found a shop with an opening hours resurvey quest
• selected 'No opening hours sign'
• clicked undo, so i could see the changes made
• verified that check_date:opening_hours wasnt updated, but check_date:opening_hours:signed was.

i don't know whether the shop in the quest that i tested with has an opening hours sign, so i didn't want to try uploading it the change. i'm assuming the upload process should work fine, but if it's worth testing that too i'm happy to go out and find an opening hours resurvey quest for shop that doesn't have an opening hours sign.

please let me know if there's anything else that'd be good to test.

It might also be good to remove the opening_hours:signed check date when removing opening_hours:signed.

yeah, that makes sense to me. i'll do that!

[matkoniecz]

note: I have not run this code.

---

Then it is fine! I was asking as some people submitted PRs without even compiling code.

I will not merge it immediately as maybe westnordost is noticing something that I have not noticed.

but check_date:opening_hours:signed was.

as in added? (as it is unlikely to be present before)

please let me know if there's anything else that'd be good to test.

The quest for presence of opening hours sign has not appeared immediately, right?

[spagy]

the check_date:opening_hours:signed tag is now also removed when opening_hours:signed is removed. the code builds and works fine, as far as i can tell.

as in added? (as it is unlikely to be present before)

yes check_date:opening_hours:signed is added.

and the quest for the presence of the opening hours sign isn't shown again immediately.

[westnordost]

Have you tested it?

i'm not sure exactly what testing is needed. here's how i tested:

There are tests in

• /app/src/test/java/de/westnordost/streetcomplete/quests/opening_hours/AddOpeningHoursTest.kt
• /app/src/test/java/de/westnordost/streetcomplete/quests/opening_hours/CheckOpeningHoursSignedTest.kt

that need to be updated / new tests need to be added.

If you are not prepared to do that (it would involve you download the Android Studio IDE and run the tests), you can close the PR and the reported issue will stay open until someone (maybe myself) will implement it fully.

I am also not sure if the implementation is complete or will work that way correctly. After all, does the quest that looks for whether the opening hours are signed look for the check date on opening hours? Or the check date on opening hours signed? Doesn't it have to look for both now?

I also remember making this decision (to use check date for opening hours also for signed-status of opening hours) deliberately back then. I can't recall now why I did that, maybe I wrote something in the ticket or commit message.

[peternewman]

I'm pretty certain this will need tweaking too, or we'll never re-survey non-signed opening hours which SC added:

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/quests/opening_hours_signed/CheckOpeningHoursSigned.kt

Lines 41 to 44 in 7c97885

	private val hasOldOpeningHoursCheckDateFilter: String get() =
	getLastCheckDateKeys("opening_hours").joinToString("\nor ") {
	"$it < today -1 years"
	}

Or we'll continually resurvey them.

This lot likely needs tweaking too:

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/quests/opening_hours_signed/CheckOpeningHoursSigned.kt

Lines 69 to 86 in 7c97885

	/* it is now signed: we set the check date for the opening hours to the previous edit
	timestamp because this or an older date is the date the opening hours were last
	checked. This is set so that the app will ask about the (signed) opening hours in
	a follow up quest
	*/
	val hasCheckDate = getLastCheckDateKeys("opening_hours")
	.any { tags[it]?.toCheckDate() != null }
	if (!hasCheckDate) {
	tags.setCheckDateForKey("opening_hours", LocalDateTime.ofInstant(
	Instant.ofEpochMilli(timestampEdited), ZoneId.systemDefault()
	).toLocalDate())
	}
	} else {
	tags["opening_hours:signed"] = "no"
	/* still unsigned: just set the check date to now, user was on-site */
	tags.updateCheckDateForKey("opening_hours")
	}

Or in other words there are lots more corner cases around resurveying and transitions to/from signed which ought to be tested too. It will also need to now handle the both styles of existing opening hours check date tagging for a resurvey...

StreetComplete/app/src/test/java/de/westnordost/streetcomplete/quests/opening_hours_signed/CheckOpeningHoursSignedTest.kt

Lines 32 to 87 in d57c74c

	@Test fun `is applicable to place with old check_date`() {
	assertTrue(questType.isApplicableTo(node(tags = mapOf(
	"name" to "XYZ",
	"check_date:opening_hours" to "2020-12-12",
	"opening_hours:signed" to "no"
	))))
	}
	@Test fun `is not applicable to place with new check_date`() {
	assertFalse(questType.isApplicableTo(node(tags = mapOf(
	"name" to "XYZ",
	"check_date:opening_hours" to LocalDate.now().toCheckDateString(),
	"opening_hours:signed" to "no"
	))))
	}
	@Test fun `apply yes answer with no prior check date`() {
	questType.verifyAnswer(
	mapOf("opening_hours:signed" to "no"),
	true,
	StringMapEntryDelete("opening_hours:signed", "no"),
	StringMapEntryAdd("check_date:opening_hours", "1970-01-01"),
	)
	}
	@Test fun `apply yes answer with prior check date`() {
	questType.verifyAnswer(
	mapOf(
	"opening_hours:signed" to "no",
	"check_date:opening_hours" to "2020-03-04"
	),
	true,
	StringMapEntryDelete("opening_hours:signed", "no"),
	)
	}
	@Test fun `apply no answer`() {
	questType.verifyAnswer(
	mapOf("opening_hours:signed" to "no"),
	false,
	StringMapEntryModify("opening_hours:signed", "no", "no"),
	StringMapEntryAdd("check_date:opening_hours", LocalDate.now().toCheckDateString()),
	)
	}
	@Test fun `apply no answer with prior check date`() {
	questType.verifyAnswer(
	mapOf(
	"opening_hours:signed" to "no",
	"check_date:opening_hours" to "2020-03-04"
	),
	false,
	StringMapEntryModify("opening_hours:signed", "no", "no"),
	StringMapEntryModify("check_date:opening_hours", "2020-03-04", LocalDate.now().toCheckDateString()),
	)
	}

Have you run the tests, as they unfortunately aren't run automatically here for true CI, I'm pretty certain these will fail now:

StreetComplete/app/src/test/java/de/westnordost/streetcomplete/quests/opening_hours/AddOpeningHoursTest.kt

Lines 50 to 65 in 8eaf060

	@Test fun `apply no opening hours sign answer`() {
	questType.verifyAnswer(
	NoOpeningHoursSign,
	StringMapEntryAdd("opening_hours:signed", "no"),
	StringMapEntryAdd("check_date:opening_hours", LocalDate.now().toCheckDateString())
	)
	}
	@Test fun `apply no opening hours sign answer when there was an answer before`() {
	questType.verifyAnswer(
	mapOf("opening_hours" to "oh"),
	NoOpeningHoursSign,
	StringMapEntryAdd("opening_hours:signed", "no"),
	StringMapEntryAdd("check_date:opening_hours", LocalDate.now().toCheckDateString())
	)
	}

The tests could definitely do with some unsigned to signed tests too.

[peternewman]

Great minds @westnordost ...

If you are not prepared to do that (it would involve you download the Android Studio IDE and run the tests), you can close the PR and the reported issue will stay open until someone (maybe myself) will implement it fully.

This isn't entirely true, you should be able to run it as a GitHub action here:
https://github.com/spagy/StreetComplete/actions/workflows/test.yml

[westnordost]

Alright I'll close this then. A proper change would require redoing this anyway.