From 1e64acdf1d98d55ea6b918e9d056ea961451687c Mon Sep 17 00:00:00 2001 From: Tanmay Gupta <119003089+savsch@users.noreply.github.com> Date: Thu, 16 Jan 2025 14:34:04 +0530 Subject: [PATCH] If depicted Wikidata item has no associated Commons category property, then suggest categories from its P18 (#6130) * Fix NPE with UploadMediaDetails.captionText * Store P18 instead of processed image url in DepictedItem * Add routes for fetching category info from titles * Consider depict's P18 when suggesting categories * Add tests * Corrected DepictedItem constructor arguments * Add test for DepictedItem::primaryImage --- .../nrw/commons/category/CategoriesModel.kt | 74 ++++++++++++++----- .../nrw/commons/category/CategoryClient.kt | 18 +++++ .../nrw/commons/category/CategoryInterface.kt | 15 ++++ .../structure/depictions/DepictedItem.kt | 3 + .../commons/category/CategoriesModelTest.kt | 52 +++++++++++-- .../commons/category/CategoryClientTest.kt | 62 ++++++++++++++++ .../structure/depictions/DepictedItemTest.kt | 16 ++++ 7 files changed, 212 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/category/CategoriesModel.kt b/app/src/main/java/fr/free/nrw/commons/category/CategoriesModel.kt index 7e6fee2fc..fd90be95f 100644 --- a/app/src/main/java/fr/free/nrw/commons/category/CategoriesModel.kt +++ b/app/src/main/java/fr/free/nrw/commons/category/CategoriesModel.kt @@ -127,30 +127,64 @@ class CategoriesModel /** * Fetches details of every category associated with selected depictions, converts them into * CategoryItem and returns them in a list. + * If a selected depiction has no categories, the categories in which its P18 belongs are + * returned in the list. * * @param selectedDepictions selected DepictItems * @return List of CategoryItem associated with selected depictions */ - private fun categoriesFromDepiction(selectedDepictions: List): Observable>? = - Observable - .fromIterable( - selectedDepictions.map { it.commonsCategories }.flatten(), - ).map { categoryItem -> - categoryClient - .getCategoriesByName( - categoryItem.name, - categoryItem.name, - SEARCH_CATS_LIMIT, - ).map { - CategoryItem( - it[0].name, - it[0].description, - it[0].thumbnail, - it[0].isSelected, - ) - }.blockingGet() - }.toList() - .toObservable() + private fun categoriesFromDepiction(selectedDepictions: List): Observable>? { + val observables = selectedDepictions.map { depictedItem -> + if (depictedItem.commonsCategories.isEmpty()) { + if (depictedItem.primaryImage == null) { + return@map Observable.just(emptyList()) + } + Observable.just( + depictedItem.primaryImage + ).map { image -> + categoryClient + .getCategoriesOfImage( + image, + SEARCH_CATS_LIMIT, + ).map { + it.map { category -> + CategoryItem( + category.name, + category.description, + category.thumbnail, + category.isSelected, + ) + } + }.blockingGet() + }.flatMapIterable { it }.toList() + .toObservable() + } else { + Observable + .fromIterable( + depictedItem.commonsCategories, + ).map { categoryItem -> + categoryClient + .getCategoriesByName( + categoryItem.name, + categoryItem.name, + SEARCH_CATS_LIMIT, + ).map { + CategoryItem( + it[0].name, + it[0].description, + it[0].thumbnail, + it[0].isSelected, + ) + }.blockingGet() + }.toList() + .toObservable() + } + } + return Observable.concat(observables) + .scan(mutableListOf()) { accumulator, currentList -> + accumulator.apply { addAll(currentList) } + } + } /** * Fetches details of every category by their name, converts them into diff --git a/app/src/main/java/fr/free/nrw/commons/category/CategoryClient.kt b/app/src/main/java/fr/free/nrw/commons/category/CategoryClient.kt index 5571e0ea7..b031f12f1 100644 --- a/app/src/main/java/fr/free/nrw/commons/category/CategoryClient.kt +++ b/app/src/main/java/fr/free/nrw/commons/category/CategoryClient.kt @@ -78,6 +78,24 @@ class CategoryClient ), ) + /** + * Fetches categories belonging to an image (P18 of some wikidata entity). + * + * @param image P18 of some wikidata entity + * @param itemLimit How many categories to return + * @return Single Observable emitting the list of categories + */ + fun getCategoriesOfImage( + image: String, + itemLimit: Int, + ): Single> = + responseMapper( + categoryInterface.getCategoriesByTitles( + "File:${image}", + itemLimit, + ), + ) + /** * The method takes categoryName as input and returns a List of Subcategories * It uses the generator query API to get the subcategories in a category, 500 at a time. diff --git a/app/src/main/java/fr/free/nrw/commons/category/CategoryInterface.kt b/app/src/main/java/fr/free/nrw/commons/category/CategoryInterface.kt index aa5ac8c3a..3888ef889 100644 --- a/app/src/main/java/fr/free/nrw/commons/category/CategoryInterface.kt +++ b/app/src/main/java/fr/free/nrw/commons/category/CategoryInterface.kt @@ -61,6 +61,21 @@ interface CategoryInterface { @Query("gacoffset") offset: Int, ): Single + /** + * Fetches non-hidden categories by titles. + * + * @param titles titles to fetch categories for (e.g. File:) + * @param itemLimit How many categories to return + * @return MwQueryResponse + */ + @GET( + "w/api.php?action=query&format=json&formatversion=2&generator=categories&prop=categoryinfo|description|pageimages&piprop=thumbnail&pithumbsize=70&gclshow=!hidden", + ) + fun getCategoriesByTitles( + @Query("titles") titles: String?, + @Query("gcllimit") itemLimit: Int, + ): Single + @GET("w/api.php?action=query&format=json&formatversion=2&generator=categorymembers&gcmtype=subcat&prop=info&gcmlimit=50") fun getSubCategoryList( @Query("gcmtitle") categoryName: String, diff --git a/app/src/main/java/fr/free/nrw/commons/upload/structure/depictions/DepictedItem.kt b/app/src/main/java/fr/free/nrw/commons/upload/structure/depictions/DepictedItem.kt index 0e4ed482d..4ae366535 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/structure/depictions/DepictedItem.kt +++ b/app/src/main/java/fr/free/nrw/commons/upload/structure/depictions/DepictedItem.kt @@ -68,6 +68,9 @@ data class DepictedItem constructor( entity.id(), ) + val primaryImage: String? + get() = imageUrl?.split('-')?.lastOrNull() + override fun equals(other: Any?) = when { this === other -> true diff --git a/app/src/test/kotlin/fr/free/nrw/commons/category/CategoriesModelTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/category/CategoriesModelTest.kt index cc5340fbb..8c336470a 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/category/CategoriesModelTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/category/CategoriesModelTest.kt @@ -1,7 +1,9 @@ package fr.free.nrw.commons.category import categoryItem +import com.nhaarman.mockitokotlin2.any import com.nhaarman.mockitokotlin2.mock +import com.nhaarman.mockitokotlin2.times import com.nhaarman.mockitokotlin2.verify import com.nhaarman.mockitokotlin2.whenever import depictedItem @@ -90,14 +92,18 @@ class CategoriesModelTest { val depictedItem = depictedItem( commonsCategories = - listOf( - CategoryItem( - "depictionCategory", - "", - "", - false, - ), + listOf( + CategoryItem( + "depictionCategory", + "", + "", + false, ), + ), + ) + val depictedItemWithoutCategories = + depictedItem( + imageUrl = "testUrl" ) whenever(gpsCategoryModel.categoriesFromLocation) @@ -159,6 +165,23 @@ class CategoriesModelTest { ), ), ) + whenever( + categoryClient.getCategoriesOfImage( + "testUrl", + 25, + ), + ).thenReturn( + Single.just( + listOf( + CategoryItem( + "categoriesOfP18", + "", + "", + false, + ), + ), + ), + ) val imageTitleList = listOf("Test") CategoriesModel(categoryClient, categoryDao, gpsCategoryModel) .searchAll("", imageTitleList, listOf(depictedItem)) @@ -171,8 +194,21 @@ class CategoriesModelTest { categoryItem("recentCategories"), ), ) + CategoriesModel(categoryClient, categoryDao, gpsCategoryModel) + .searchAll("", imageTitleList, listOf(depictedItemWithoutCategories)) + .test() + .assertValue( + listOf( + categoryItem("categoriesOfP18"), + categoryItem("gpsCategory"), + categoryItem("titleSearch"), + categoryItem("recentCategories"), + ), + ) imageTitleList.forEach { - verify(categoryClient).searchCategories(it, CategoriesModel.SEARCH_CATS_LIMIT) + verify(categoryClient, times(2)).searchCategories(it, CategoriesModel.SEARCH_CATS_LIMIT) + verify(categoryClient).getCategoriesByName(any(), any(), any(), any()) + verify(categoryClient).getCategoriesOfImage(any(), any()) } } diff --git a/app/src/test/kotlin/fr/free/nrw/commons/category/CategoryClientTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/category/CategoryClientTest.kt index 5c95215a8..5edf55f6e 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/category/CategoryClientTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/category/CategoryClientTest.kt @@ -132,6 +132,45 @@ class CategoryClientTest { ) } + @Test + fun getCategoriesByTitlesFound() { + val mockResponse = withMockResponse("Category:Test") + whenever( + categoryInterface.getCategoriesByTitles( + anyString(), + anyInt(), + ), + ).thenReturn(Single.just(mockResponse)) + categoryClient + .getCategoriesOfImage("tes", 10) + .test() + .assertValues( + listOf( + CategoryItem( + "Test", + "", + "", + false, + ), + ), + ) + categoryClient + .getCategoriesOfImage( + "tes", + 10, + ).test() + .assertValues( + listOf( + CategoryItem( + "Test", + "", + "", + false, + ), + ), + ) + } + @Test fun getCategoriesByNameNull() { val mockResponse = withNullPages() @@ -160,6 +199,29 @@ class CategoryClientTest { .assertValues(emptyList()) } + @Test + fun getCategoriesByTitlesNull() { + val mockResponse = withNullPages() + whenever( + categoryInterface.getCategoriesByTitles( + anyString(), + anyInt(), + ), + ).thenReturn(Single.just(mockResponse)) + categoryClient + .getCategoriesOfImage( + "tes", + 10, + ).test() + .assertValues(emptyList()) + categoryClient + .getCategoriesOfImage( + "tes", + 10, + ).test() + .assertValues(emptyList()) + } + @Test fun getParentCategoryListFound() { val mockResponse = withMockResponse("Category:Test") diff --git a/app/src/test/kotlin/fr/free/nrw/commons/upload/structure/depictions/DepictedItemTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/upload/structure/depictions/DepictedItemTest.kt index e0d339eee..faec52051 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/upload/structure/depictions/DepictedItemTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/upload/structure/depictions/DepictedItemTest.kt @@ -181,4 +181,20 @@ class DepictedItemTest { fun `hashCode returns different values for objects with different name`() { Assert.assertNotEquals(depictedItem(name = "a").hashCode(), depictedItem(name = "b").hashCode()) } + + @Test + fun `primaryImage is derived correctly from imageUrl`() { + Assert.assertEquals( + DepictedItem( + entity( + statements = mapOf( + WikidataProperties.IMAGE.propertyName to listOf( + statement(snak(dataValue = valueString("prefix: example_image name"))), + ), + ), + ), + ).primaryImage, + "_example_image_name", + ) + } }