ensure that whether weekdays collide is only checked for opening hours that are otherwise supported (fixes #6715)

Author: westnordost

app/src/androidMain/kotlin/de/westnordost/streetcomplete/quests/opening_hours/AddOpeningHours.kt

@@ -13,7 +13,6 @@ import de.westnordost.streetcomplete.data.quest.AndroidQuest
 import de.westnordost.streetcomplete.data.user.achievements.EditTypeAchievement.CITIZEN
 import de.westnordost.streetcomplete.osm.Tags
 import de.westnordost.streetcomplete.osm.isPlaceOrDisusedPlace
-import de.westnordost.streetcomplete.osm.opening_hours.isLikelyIncorrect
 import de.westnordost.streetcomplete.osm.opening_hours.isSupported
 import de.westnordost.streetcomplete.osm.opening_hours.toOpeningHours
 import de.westnordost.streetcomplete.osm.updateCheckDateForKey
@@ -184,8 +183,7 @@ mapOf(
         // invalid opening_hours rules -> applicable because we want to ask for opening hours again
         // be strict
         val oh = ohStr.toOpeningHoursOrNull(lenient = false) ?: return true
-        // only display supported rules, or ambiguous rules that should be corrected
-        return oh.isSupported(allowTimePoints = false) || oh.isLikelyIncorrect()
+        return oh.isSupported(allowTimePoints = false, allowAmbiguity = true)
     }
 
     override fun getHighlightedElements(element: Element, getMapData: () -> MapDataWithGeometry) =

app/src/androidMain/kotlin/de/westnordost/streetcomplete/quests/postbox_collection_times/AddPostboxCollectionTimes.kt

@@ -12,7 +12,6 @@ import de.westnordost.streetcomplete.data.quest.AndroidQuest
 import de.westnordost.streetcomplete.data.quest.NoCountriesExcept
 import de.westnordost.streetcomplete.data.user.achievements.EditTypeAchievement.POSTMAN
 import de.westnordost.streetcomplete.osm.Tags
-import de.westnordost.streetcomplete.osm.opening_hours.isLikelyIncorrect
 import de.westnordost.streetcomplete.osm.opening_hours.isSupported
 import de.westnordost.streetcomplete.osm.opening_hours.toOpeningHours
 import de.westnordost.streetcomplete.osm.updateWithCheckDate
@@ -81,7 +80,7 @@ class AddPostboxCollectionTimes : OsmElementQuestType<CollectionTimesAnswer>, An
         // be strict
         val oh = ct.toOpeningHoursOrNull(lenient = false) ?: return true
         // only display supported rules, or ambiguous rules that should be corrected
-        return oh.isSupported(allowTimePoints = true) || oh.isLikelyIncorrect()
+        return oh.isSupported(allowTimePoints = true, allowAmbiguity = true)
     }
 
     override fun getHighlightedElements(element: Element, getMapData: () -> MapDataWithGeometry) =

app/src/androidUnitTest/kotlin/de/westnordost/streetcomplete/quests/opening_hours/AddOpeningHoursTest.kt

@@ -350,4 +350,16 @@ class AddOpeningHoursTest {
             timestamp = "2000-11-11".toCheckDate()?.toEpochMilli()
         )))
     }
+
+    @Test fun `isApplicableTo returns false for valid but not supported opening hours with ranges that are also colliding`() {
+        val millisecondsFor400Days: Long = 1000L * 60 * 60 * 24 * 400
+        assertFalse(questType.isApplicableTo(node(
+            tags = mapOf(
+                "shop" to "sports",
+                "name" to "Atze's Angelladen",
+                "opening_hours" to "\"by appointment, see timetable\"; PH \"by appointment\""
+            ),
+            timestamp = nowAsEpochMilliseconds() - millisecondsFor400Days
+        )))
+    }
 }

app/src/commonMain/kotlin/de/westnordost/streetcomplete/osm/opening_hours/OpeningHoursValidator.kt

@@ -36,28 +36,43 @@ import kotlin.jvm.JvmName
  *  - "off" rules with exclusively weekdays, weekday ranges, PH
  *  - clock time spans with optional open end, or clock time points
  *
- *  Weekdays that collide (Mo-Fr 8:30-12:30; We 14:00-18:00) are not supported - they are likely
- *  tagging mistakes (Maybe user meant to use "," instead of ";"?).
+ *  Weekdays that collide (Mo-Fr 8:30-12:30; We 14:00-18:00) are considered ambiguous - they are
+ *  likely tagging mistakes (Maybe user meant to use "," instead of ";"?).
  *
- *  Also, when any rule has months defined, all rules must have months defined, for clarity.
+ *  Also, when any rule has months defined, all rules must have months defined, for clarity,
+ *  otherwise they are not supported at all (not even considered ambiguous, see code comment below)
  */
-fun OpeningHours.isSupported(allowTimePoints: Boolean = false): Boolean =
+fun OpeningHours.isSupported(
+    allowTimePoints: Boolean = false,
+    allowAmbiguity: Boolean = false,
+): Boolean =
     rules.all { rule -> rule.isSupported() } &&
         (allowTimePoints || !containsTimePoints()) &&
-        !rules.hasCollidingWeekdays() &&
+        (allowAmbiguity || !rules.hasCollidingWeekdays()) &&
+        /*
+          in #6175 we decided to not classify opening hours that have incomplete months as
+          ambiguous (=> ask user to clarify) and instead just don't support them at all. Reason:
+
+          Ambiguous opening hours are treated the same as invalid opening hours: they are not shown
+          to the user in the widget, as if they haven't been defined at all yet. Apart from
+          implementation effort, the reason for not displaying those at all is that the widget
+          doesn't show the difference between ";" (overwriting) and "," (additive) rules which are
+          usually the cause for the ambiguity. So, users would be unable to detect that something is
+          wrong with the currently mapped opening hours if they were displayed.
+
+          E.g.
+          `Mo-We 10:00-12:00; Tu 14:00-18:00` would be displayed the same as
+          `Mo-We 10:00-12:00, Tu 14:00-18:00` while the first is likely a mistake (and if it is not,
+          writing `Mo,We 10:00-12:00; Tu 14:00-18:00` would be not ambiguous)
+
+          For ambiguous opening hours with incomplete months like
+          `10:00-12:00; Apr-Sep 14:00-18:00` in particular, the concern has been that the user might
+          input only the hours for summer or only for winter in case the opening hours plates are
+          just swapped twice a year by the shop since he is unable to see in the app what was
+          already mapped.
+        */
         !rules.hasIncompleteMonths()
 
-/** Return whether the given valid opening hours are however ambiguous and thus should be corrected.
- *  */
-fun OpeningHours.isLikelyIncorrect(): Boolean =
-    // in #6175 we decided to not classify opening hours that have incomplete months as ambiguous
-    // (but just don't support them at all), mainly due to fears that if the previous opening hours
-    // like e.g. "12:00-18:00; Nov-Feb 12:00-14:00" are not shown in the opening hours widget, the
-    // user might input only the hours for summer or only for winter in case the opening hours
-    // plates are just swapped twice a year by the shop. (Showing those to the user would somewhat
-    // keep the user from overwriting, but they can't be shown because they are unsupported)
-    rules.hasCollidingWeekdays()
-
 fun Rule.isSupported(): Boolean =
     // comments not supported
     comment == null &&
@@ -145,7 +160,10 @@ private fun Range.hasMonths(): Boolean =
 
 /** For example, "Mo-Fr 10:00-12:00; We 14:00-16:00" self-collides: Wednesday is overwritten
  *  to only be open 14:00 to 16:00. A rule collides with another whenever the days overlap. When
- *  non-additive rules and additive rules mix, it becomes a bit difficult to find it out */
+ *  non-additive rules and additive rules mix, it becomes a bit difficult to find it out
+ *
+ *  @throws IllegalArgumentException if any of the rules is not supported
+ *  */
 fun List<Rule>.hasCollidingWeekdays(): Boolean {
     for (i in indices) {
         val rule1 = get(i)