Check Date for opening_hours:signed

[kmpoppe]

Resolves #4273, ref #4276.

Is this, what we were looking for?

If the answer is that opening_hours:signed=no (either from quest/opening_hours or quest/opening_hours_signed) we add check_date:opening_hours:signed.

If the answer is that opening_hours:signed=yes (either from quest/opening_hours or quest/opening_hours_signed), that means opening_hours:signed is removed and opening_hours=* are set, we remove check_date:opening_hours:signed and try to set check_date:opening_hours to the last edit timestamp so that the apply check for opening_hours works.

quest/opening_hours_signed checks for this info being older than a year OR the element being last edited over a year.

quest/opening_hours now either filters for opening_hours:signed != no OR (opening_hours:signed = no AND check_date:opening_hours:signed older than 1 year)

Testing with applicable objects, the behaviour seems to be programmatically correct, yet, I am unable to formulate the Tests correctly, I might need help in understading how these work. I have thrown away my changes in the tests and will work on them again later -> WIP PR.

Also, please provide feedback on the setting/removing/interpretation of the various check dates.

[westnordost]

Thank you! Will look over this PR soon. Added tests would help (me) a big deal, because good tests are also good documentation of the behavior.

The tests are nothing special, just normal (Java-like) unit tests. If you are confused about OsmElementQuestType::answerAppliedTo, that's just a convenience function for the tests, to get the changes that are made as a Set of StringMapEntryChanges for easy comparison with assertEquals in the tests.

[kmpoppe]

Thanks for the info!

After throwing away my prior changes and re-doing them (thinking about them, being actually more awake than before) the tests I added/changed now (0d3626f and ea10d1f) work as expected.

[westnordost]

It should also be documented by the tests how the quest(s) behave with current tagging, e.g.

shop=kiosk
opening_hours:signed=no
check_date:opening_hours=2022-02-02

In other words, taking care of the "migration path" from the old tagging to new tagging.

[kmpoppe]

Currently AFK from my programming machine for a week, I shall check your comments then.

[kmpoppe]

will add tests as requested in a few.

[kmpoppe]

Tests added.

CheckOpeningSigned now checks for both check_dates (or older 1 year), that why for second test mentioned (in my eyes) correctly asserts true not false because it's older than a year and has no check_dates at all.

[westnordost]

Tests for

• that check_date:opening_hours is removed when answering NoOpeningHoursSign
• that check_date:opening_hours:signed is removed when answering with anything else
  are missing in AddOpeningHoursTest

When you add new behavior to tested code, always also test this new behavior (not only modify the tests so that they run through green again). If the tests are not kept up-to-date with the implementation, the tests don't make sense.

Also, the new tests you added are puzzling to me.

AddOpeningHours quest should NEVER be applicable while opening_hours:signed=no, because when it is not signed, it should never be asked. Instead, we have CheckOpeningHoursSigned which is asked in its place. When the user answered that it is signed now, only then AddOpeningHours should be asked again.

Instead of this desired behavior, the tests seem to test that the quest is being asked also for places with unsigned opening hours as long as it was checked more than one year ago.

[westnordost]

This test doesn't test what you want it to test. You want it to test that it takes the older of the two of check_date:opening_hours and check_date:opening_hours:signed, but to test this, timestamp would need to be "now" (i.e. the default). Otherwise the quest would anyway be applicable because the last edit date is 24 years old.

Suggested change

	@Test fun `is applicable if the opening hours not signed and only check date for OH exists`() {
	assertTrue(questType.isApplicableTo(node(
	tags = mapOf(
	"name" to "rundumdieuhr kiosk",
	"opening_hours:signed" to "no",
	"check_date:opening_hours" to "2021-03-01"
	),
	timestamp = "2000-11-11".toCheckDate()?.toEpochMilli()
	)))
	}
	@Test fun `is applicable if the opening hours not signed and only check date for OH exists`() {
	assertTrue(questType.isApplicableTo(node(
	tags = mapOf(
	"name" to "rundumdieuhr kiosk",
	"opening_hours:signed" to "no",
	"check_date:opening_hours" to "2021-03-01"
	)
	)))
	}