Fixes #4260 - Item with P582 (end time) shown as existing (#4292)

* fix issue with item with endtime shown as existing

* Removed destroyed and endtime, Added comments and also fixed the broken tests

* minor fix

* Added comments

* fix no such column location_exists error

* minor improvement

Co-authored-by: Pratham2305 <Pratham2305@users.noreply.github.com>

app/src/main/java/fr/free/nrw/commons/bookmarks/locations/BookmarkLocationsDao.java

@@ -165,7 +165,7 @@ public class BookmarkLocationsDao {
                 cursor.getString(cursor.getColumnIndex(Table.COLUMN_CATEGORY)),
                 builder.build(),
                 cursor.getString(cursor.getColumnIndex(Table.COLUMN_PIC)),
-                cursor.getString(cursor.getColumnIndex(Table.COLUMN_DESTROYED))
+                Boolean.parseBoolean(cursor.getString(cursor.getColumnIndex(Table.COLUMN_EXISTS)))
         );
     }
 
@@ -183,7 +183,7 @@ public class BookmarkLocationsDao {
         cv.put(BookmarkLocationsDao.Table.COLUMN_LAT, bookmarkLocation.location.getLatitude());
         cv.put(BookmarkLocationsDao.Table.COLUMN_LONG, bookmarkLocation.location.getLongitude());
         cv.put(BookmarkLocationsDao.Table.COLUMN_PIC, bookmarkLocation.pic);
-        cv.put(BookmarkLocationsDao.Table.COLUMN_DESTROYED, bookmarkLocation.destroyed);
+        cv.put(BookmarkLocationsDao.Table.COLUMN_EXISTS, bookmarkLocation.exists.toString());
         return cv;
     }
 
@@ -203,7 +203,7 @@ public class BookmarkLocationsDao {
         static final String COLUMN_WIKIDATA_LINK = "location_wikidata_link";
         static final String COLUMN_COMMONS_LINK = "location_commons_link";
         static final String COLUMN_PIC = "location_pic";
-        static final String COLUMN_DESTROYED = "location_destroyed";
+        static final String COLUMN_EXISTS = "location_exists";
 
         // NOTE! KEEP IN SAME ORDER AS THEY ARE DEFINED UP THERE. HELPS HARD CODE COLUMN INDICES.
         public static final String[] ALL_FIELDS = {
@@ -220,7 +220,7 @@ public class BookmarkLocationsDao {
                 COLUMN_WIKIDATA_LINK,
                 COLUMN_COMMONS_LINK,
                 COLUMN_PIC,
-                COLUMN_DESTROYED
+                COLUMN_EXISTS
         };
 
         static final String DROP_TABLE_STATEMENT = "DROP TABLE IF EXISTS " + TABLE_NAME;
@@ -239,7 +239,7 @@ public class BookmarkLocationsDao {
                 + COLUMN_WIKIDATA_LINK + " STRING,"
                 + COLUMN_COMMONS_LINK + " STRING,"
                 + COLUMN_PIC + " STRING,"
-                + COLUMN_DESTROYED + " STRING"
+                + COLUMN_EXISTS + " STRING"
                 + ");";
 
         public static void onCreate(SQLiteDatabase db) {
@@ -299,6 +299,13 @@ public class BookmarkLocationsDao {
                     Timber.e(exception);
                 }
             }
+            if (from == 14){
+                try {
+                    db.execSQL("ALTER TABLE bookmarksLocations ADD COLUMN location_exists STRING;");
+                } catch (SQLiteException exception){
+                    Timber.e(exception);
+                }
+            }
         }
     }
 }

app/src/main/java/fr/free/nrw/commons/data/DBOpenHelper.java

@@ -13,7 +13,7 @@ import fr.free.nrw.commons.explore.recentsearches.RecentSearchesDao;
 public class DBOpenHelper  extends SQLiteOpenHelper {
 
     private static final String DATABASE_NAME = "commons.db";
-    private static final int DATABASE_VERSION = 14;
+    private static final int DATABASE_VERSION = 15;
     public static final String CONTRIBUTIONS_TABLE = "contributions";
     private final String DROP_TABLE_STATEMENT="DROP TABLE IF EXISTS %s";
 

app/src/main/java/fr/free/nrw/commons/nearby/NearbyController.java

@@ -201,7 +201,7 @@ public class NearbyController {
                         nearbyBaseMarker.icon(IconFactory.getInstance(context)
                                 .fromBitmap(iconGreen));
                     }
-                } else if (!place.destroyed.trim().isEmpty()) { // Means place is destroyed
+                } else if (!place.exists) { // Means that the topic of the Wikidata item does not exist in the real world anymore, for instance it is a past event, or a place that was destroyed
                     if (iconGrey != null) {
                         nearbyBaseMarker.icon(IconFactory.getInstance(context)
                                 .fromBitmap(iconGrey));

app/src/main/java/fr/free/nrw/commons/nearby/Place.java

@@ -25,13 +25,15 @@ public class Place implements Parcelable {
     public final LatLng location;
     private final String category;
     public final String pic;
-    public final String destroyed;
+    // exists boolean will tell whether the place exists or not,
+    // For a place to be existing both destroyed and endTime property should be null but it is also not necessary for a non-existing place to have both properties either one property is enough (in such case that not given property will be considered as null).
+    public final Boolean exists;
 
     public String distance;
     public final Sitelinks siteLinks;
 
 
-    public Place(String language,String name, Label label, String longDescription, LatLng location, String category, Sitelinks siteLinks, String pic, String destroyed) {
+    public Place(String language,String name, Label label, String longDescription, LatLng location, String category, Sitelinks siteLinks, String pic, Boolean exists) {
         this.language = language;
         this.name = name;
         this.label = label;
@@ -40,7 +42,7 @@ public class Place implements Parcelable {
         this.category = category;
         this.siteLinks = siteLinks;
         this.pic = (pic == null) ? "":pic;
-        this.destroyed = (destroyed == null) ? "":destroyed;
+        this.exists = exists;
     }
     public Place(Parcel in) {
         this.language = in.readString();
@@ -52,8 +54,8 @@ public class Place implements Parcelable {
         this.siteLinks = in.readParcelable(Sitelinks.class.getClassLoader());
         String picString = in.readString();
         this.pic = (picString == null) ? "":picString;
-        String destroyedString = in.readString();
+        String existString = in.readString();
-        this.destroyed = (destroyedString == null) ? "":destroyedString;
+        this.exists = Boolean.parseBoolean(existString);
     }
     public static Place from(NearbyResultItem item) {
         String itemClass = item.getClassName().getValue();
@@ -74,7 +76,8 @@ public class Place implements Parcelable {
                         .setWikidataLink(item.getItem().getValue())
                         .build(),
                 item.getPic().getValue(),
-                item.getDestroyed().getValue());
+                // Checking if the place exists or not
+                (item.getDestroyed().getValue() == "") && (item.getEndTime().getValue() == ""));
     }
 
     /**
@@ -194,7 +197,7 @@ public class Place implements Parcelable {
                 ", distance='" + distance + '\'' +
                 ", siteLinks='" + siteLinks.toString() + '\'' +
                 ", pic='" + pic + '\'' +
-                ", destroyed='" + destroyed + '\'' +
+                ", exists='" + exists.toString() + '\'' +
                 '}';
     }
 
@@ -213,7 +216,7 @@ public class Place implements Parcelable {
         dest.writeString(category);
         dest.writeParcelable(siteLinks, 0);
         dest.writeString(pic);
-        dest.writeString(destroyed);
+        dest.writeString(exists.toString());
     }
 
     public static final Creator<Place> CREATOR = new Creator<Place>() {

app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java

@@ -1254,12 +1254,12 @@ public class NearbyParentFragment extends CommonsDaggerSupportFragment
 
             if (displayExists && displayNeedsPhoto) {
                 // Exists and needs photo
-                if (place.destroyed.trim().isEmpty() && place.pic.trim().isEmpty()) {
+                if (place.exists && place.pic.trim().isEmpty()) {
                     updateMarker(markerPlaceGroup.getIsBookmarked(), place, NearbyController.currentLocation);
                 }
             } else if (displayExists && !displayNeedsPhoto) {
                 // Exists and all included needs and doesn't needs photo
-                if (place.destroyed.trim().isEmpty()) {
+                if (place.exists) {
                     updateMarker(markerPlaceGroup.getIsBookmarked(), place, NearbyController.currentLocation);
                 }
             } else if (!displayExists && displayNeedsPhoto) {
@@ -1317,7 +1317,7 @@ public class NearbyParentFragment extends CommonsDaggerSupportFragment
             return (isBookmarked ?
                 R.drawable.ic_custom_map_marker_green_bookmarked :
                 R.drawable.ic_custom_map_marker_green);
-        } else if (!place.destroyed.trim().isEmpty()) { // Means place is destroyed
+        } else if (!place.exists) { // Means that the topic of the Wikidata item does not exist in the real world anymore, for instance it is a past event, or a place that was destroyed
             return (isBookmarked ?
                 R.drawable.ic_custom_map_marker_grey_bookmarked :
                 R.drawable.ic_custom_map_marker_grey);

app/src/main/java/fr/free/nrw/commons/nearby/model/NearbyResultItem.kt

@@ -11,7 +11,8 @@ class NearbyResultItem(private val item: ResultTuple?,
                        @field:SerializedName("classLabel") private val classLabel: ResultTuple?,
                        @field:SerializedName("commonsCategory") private val commonsCategory: ResultTuple?,
                        @field:SerializedName("pic") private val pic: ResultTuple?,
-                       @field:SerializedName("destroyed") private val destroyed: ResultTuple?) {
+                       @field:SerializedName("destroyed") private val destroyed: ResultTuple?,
+                       @field:SerializedName("endTime") private val endTime: ResultTuple?) {
 
     fun getItem(): ResultTuple {
         return item ?: ResultTuple()
@@ -57,4 +58,8 @@ class NearbyResultItem(private val item: ResultTuple?,
         return destroyed ?: ResultTuple()
     }
 
+    fun getEndTime(): ResultTuple {
+        return endTime ?: ResultTuple()
+    }
+
 }

app/src/main/resources/queries/nearby_query.rq

@@ -10,6 +10,7 @@ SELECT
      (SAMPLE(?commonsCategory) as ?commonsCategory)
      (SAMPLE(?pic) as ?pic)
      (SAMPLE(?destroyed) as ?destroyed)
+     (SAMPLE(?endTime) as ?endTime)
    WHERE {
      # Around given location...
      SERVICE wikibase:around {
@@ -31,6 +32,9 @@ SELECT
      # Get (P576)
      OPTIONAL { ?item wdt:P576 ?destroyed. }
 
+     # Get (P582)
+     OPTIONAL { ?item wdt:P582 ?endTime. }
+
      # Get the class label in the preferred language of the user, or any other language if no label is available in that language.
      OPTIONAL {
        ?item p:P31/ps:P31 ?classId.

app/src/test/kotlin/ModelFunctions.kt

@@ -83,9 +83,9 @@ fun place(
     category: String = "category",
     siteLinks: Sitelinks? = null,
     pic: String = "pic",
-    destroyed: String = "destroyed"
+    exists: Boolean = false
 ): Place {
-    return Place(lang, name, label, longDescription, latLng, category, siteLinks, pic, destroyed)
+    return Place(lang, name, label, longDescription, latLng, category, siteLinks, pic, exists)
 }
 
 fun entityId(wikiBaseEntityValue: WikiBaseEntityValue = wikiBaseEntityValue()) =

app/src/test/kotlin/fr/free/nrw/commons/bookmarks/locations/BookMarkLocationDaoTest.kt

@@ -38,7 +38,7 @@ class BookMarkLocationDaoTest {
             COLUMN_LAT,
             COLUMN_LONG,
             COLUMN_PIC,
-            COLUMN_DESTROYED)
+            COLUMN_EXISTS)
     private val client: ContentProviderClient = mock()
     private val database: SQLiteDatabase = mock()
     private val captor = argumentCaptor<ContentValues>()
@@ -63,7 +63,7 @@ class BookMarkLocationDaoTest {
 
 
         examplePlaceBookmark = Place("en", "placeName", exampleLabel, "placeDescription"
-                , exampleLocation, "placeCategory", builder.build(),"picName","placeDestroyed")
+                , exampleLocation, "placeCategory", builder.build(),"picName",false)
         testObject = BookmarkLocationsDao { client }
     }
 
@@ -98,7 +98,7 @@ class BookMarkLocationDaoTest {
                 assertEquals(builder.build().wikidataLink, it.siteLinks.wikidataLink)
                 assertEquals(builder.build().commonsLink, it.siteLinks.commonsLink)
                 assertEquals("picName",it.pic)
-                assertEquals("placeDestroyed", it.destroyed)
+                assertEquals(false, it.exists)
             }
         }
     }
@@ -163,7 +163,7 @@ class BookMarkLocationDaoTest {
             assertEquals(examplePlaceBookmark.siteLinks.wikidataLink.toString(), cv.getAsString(COLUMN_WIKIDATA_LINK))
             assertEquals(examplePlaceBookmark.siteLinks.commonsLink.toString(), cv.getAsString(COLUMN_COMMONS_LINK))
             assertEquals(examplePlaceBookmark.pic.toString(), cv.getAsString(COLUMN_PIC))
-            assertEquals(examplePlaceBookmark.destroyed.toString(), cv.getAsString(COLUMN_DESTROYED))
+            assertEquals(examplePlaceBookmark.exists.toString(), cv.getAsString(COLUMN_EXISTS))
         }
     }
 
@@ -271,13 +271,19 @@ class BookMarkLocationDaoTest {
         verify(database).execSQL("ALTER TABLE bookmarksLocations ADD COLUMN location_language STRING;")
     }
 
+    @Test
+    fun migrateTableVersionFrom_v14_to_v15() {
+        onUpdate(database, 14, 15)
+        verify(database).execSQL("ALTER TABLE bookmarksLocations ADD COLUMN location_exists STRING;")
+    }
+
 
     private fun createCursor(rowCount: Int) = MatrixCursor(columns, rowCount).apply {
 
         for (i in 0 until rowCount) {
             addRow(listOf("placeName", "en", "placeDescription", "placeCategory", exampleLabel.text, exampleLabel.icon,
                     exampleUri, builder.build().wikipediaLink, builder.build().wikidataLink, builder.build().commonsLink,
-                    exampleLocation.latitude, exampleLocation.longitude, "picName", "placeDestroyed"))
+                    exampleLocation.latitude, exampleLocation.longitude, "picName", "placeExists"))
         }
     }
 }

app/src/test/kotlin/fr/free/nrw/commons/bookmarks/locations/BookmarkLocationControllerTest.kt

@@ -33,8 +33,8 @@ class BookmarkLocationControllerTest {
     private val mockBookmarkList: List<Place>
         private get() {
             val list = ArrayList<Place>()
-            list.add(Place("en","a place",null,"a description",null,"a cat",null,null,null))
+            list.add(Place("en","a place",null,"a description",null,"a cat",null,null,true))
-            list.add(Place("en","another place",null,"another description",null,"another cat",null,null,null))
+            list.add(Place("en","another place",null,"another description",null,"another cat",null,null,true))
             return list
         }