Feat/search : implement the search backend for events and associations #117

Redd87 · 2024-10-30T09:01:57Z

This PR implements the search backend of the project using Jetpack's AppSearch.

There is a local search database in the SearchRepository which is created on startup based on the data from the Association- and EventRepository, and that database can add, remove documents, and most importantly be queried. The ranking strategy - the order in which matching results will show - is currently set to none; as long as there is a matching keyword the result will show. This can later be modified to promote or hide certain documents.
In the Event and Association classes, I created an annex object EventDocument and AssociationDocument, which consists of the attributes on which to search.
The SearchViewModel exposes the query function of the SearchRepository, and allows access to the result data in its StateFlow.
The SearchRepository is accompanied with a SearchRepositoryTest file containing Unit tests, but the SearchViewModel is not. It will need to have integrations test as unit tests do not make sense in view of the file. This will be done at a later time as this PR is already quite large.
The main has been modified to properly set up the search backend, and is currently injected into the explore page.

…ction for AppSearch integration

…pSearch integration

…epository

…ationRepositoryFirestore

…t/search # Conflicts: # app/src/main/java/com/android/unio/MainActivity.kt # gradle/libs.versions.toml

…t/search # Conflicts: # app/src/main/java/com/android/unio/model/firestore/FirestoreReferenceList.kt

…chViewModel

Seriously why the fuck can't mockito mock a final class if all classes are final by default

Zafouche

Thank you for your amazing work on this difficult task! 🚀

Overall, great code! Good start on the tests as well, since I know you plan on adding more in the next Sprint, no need to comment more than that on the code coverage.

The only thing that bothers me is the choice of architecture for the Search view model and repository. In brief, I think it violates the MVVM pattern, because in my opinion the Search repository shouldn't be able to utilize the AssociationRepository and the EventRepository. I think what would be a correct pattern would be to have the Search view model utilize the AssociationViewModel and the EventViewModel. This would require passing the onSuccess and onFailure callbacks through the function parameters to the Search repository, so we would have the classic ViewModel-Repository relationship we have in all other such pairs.
Have a look at my comments on the respective files. Please explain to me your choice in those, and I will gladly approve this PR!

Other than that, you'll find my comments to be on small details which aren't very important. However don't forget to remove the debugging Log.d calls, and optimize the imports!

Zafouche · 2024-10-31T13:03:09Z

app/src/androidTest/java/com/android/unio/ui/ScreenDisplayingTest.kt

  fun testExploreDisplayed() {
-    composeTestRule.setContent { ExploreScreen(navigationAction) }
+    composeTestRule.setContent {
+      ExploreScreen(navigationAction, searchViewModel = searchViewModel)


I know it's just in the test file, but either parameter = value (e.g. here it's searchViewModel = searchViewModel) should be used for all parameters or none at all, not both

Zafouche · 2024-10-31T13:05:54Z

app/src/main/java/com/android/unio/MainActivity.kt

-  val associationRepository = AssociationRepositoryFirestore(Firebase.firestore)
-  val associationViewModel = AssociationViewModel(associationRepository)
+  val associationRepository = remember { AssociationRepositoryFirestore(db) }
+  val associationViewModel = remember { AssociationViewModel(associationRepository) }


Very nice! Thanks for this, as this was forgotten, it caused screen flickering when the UnioApp composable, which is not pleasant both to the eye but also in terms of useless re-computation!

Zafouche · 2024-10-31T13:12:36Z

app/src/main/java/com/android/unio/model/association/Association.kt

+    val fullName: String = "",
+    @StringProperty(indexingType = StringPropertyConfig.INDEXING_TYPE_PREFIXES)
+    val description: String = ""
+    // TODO add members


Well defined. I assume these are the properties that can be searchable? The most important are name and fullName, which are there, but can you enlighten me on if these are all searchable properties? If so, I don't think searching a description is really useful.

Yes these are all searchable, and I disagree, as I feel like you might not sometimes remember the association name but want can then find it with keywords that are in the description

Zafouche · 2024-10-31T13:13:22Z

app/src/main/java/com/android/unio/model/association/AssociationRepository.kt

@@ -1,5 +1,7 @@
 package com.android.unio.model.association

+import com.google.firebase.firestore.QuerySnapshot


Is this an unnecessary import? I don't see any other additions in the file that would justify this new import.

Zafouche · 2024-10-31T13:25:50Z

app/src/main/java/com/android/unio/model/association/AssociationRepositoryFirestore.kt

 import com.google.firebase.Firebase
 import com.google.firebase.auth.auth
 import com.google.firebase.firestore.FirebaseFirestore
+import com.google.firebase.firestore.QuerySnapshot


Again an other unnecessary import. Maybe you forgot to run optimize imports

Zafouche · 2024-10-31T14:28:36Z

app/src/main/java/com/android/unio/model/search/SearchRepository.kt

+   */
+  // Should be private, but I need it public for tests
+  fun addAssociations(associations: List<Association>) {
+    Log.d("SearchRepository", "Adding associations to search database")


Don't forget to remove this debug Log!

Zafouche · 2024-10-31T14:28:55Z

app/src/main/java/com/android/unio/model/search/SearchRepository.kt

+   */
+  // Should be private, but I need it public for tests
+  fun addEvents(events: List<Event>) {
+    Log.d("SearchRepository", "Adding events to search database")


Another debug Log!

Zafouche · 2024-10-31T14:33:41Z

app/src/main/java/com/android/unio/model/search/SearchViewModel.kt

+    Firebase.auth.addAuthStateListener {
+      if (it.currentUser != null) {
+        viewModelScope.launch { repository.init() }
+      }


Have a look at how the init block is defined in AssociationViewModel:

init { associationRepository.init { getAssociations() } }

It simply calls the repository's init function. You'll see that the addition of the Auth State Listener is done in that repository init function, e.g. in AssociationRepositoryFirestore:

override fun init(onSuccess: () -> Unit) { Firebase.auth.addAuthStateListener { if (it.currentUser != null) { onSuccess() } } }

I think it makes more sense to add the Auth State Listener there as it is the database's duty, but let me know if you disagree

Zafouche · 2024-10-31T14:41:56Z

app/src/test/java/com/android/unio/model/search/SearchRepositoryTest.kt

Strong tests! 💪

Zafouche · 2024-10-31T14:42:52Z

gradle/libs.versions.toml

 mockitoAndroid = "5.13.0"
 mockkMockk = "1.9.3"
-mockkVersion = "1.13.0"
+mockkVersion = "1.13.7"


Good call on upgrading the version, I see nothing breaks!

Redd87 · 2024-10-31T16:11:25Z

Thank you for your amazing work on this difficult task! 🚀

Overall, great code! Good start on the tests as well, since I know you plan on adding more in the next Sprint, no need to comment on the code coverage.

The only thing that bothers me is the choice of architecture for the Search view model and repository. In brief, I think it violates the MVVM pattern, because in my opinion the Search repository shouldn't be able to utilize the AssociationRepository and the EventRepository. I think what would be a correct pattern would be to have the Search view model utilize the AssociationViewModel and the EventViewModel. This would require passing the onSuccess and onFailure callbacks through the function parameters to the Search repository, so we would have the classic ViewModel-Repository relationship we have in all other such pairs. Have a look at my comments on the respective files. Please explain to me your choice in those, and I will gladly approve this PR!

Other than that, you'll find my comments to be on small details which aren't very important. However don't forget to remove the debugging Log.d calls, and optimize the imports!

Thanks for the feedback, I'll do these changes ASAP. The justification behind the SearchRepository is that it acts as a composite repository for the Association and EventRepository; however I honestly simply didn't consider doing it as you suggested. I don't feel that the SearchRepository violates the MVVM model, as it receives the other repositories by dependency injection and simply aggregates their data, there is no interaction between the repositories, but I do agree that the SearchViewModel is quite different to the other ViewModels, but I don't know if that itself is a violation of the MVVM. I could refactor it to adhere to the others but that would take quite a lot of time and I'm not sure if it's that advantageous

Let me know if you have other questions

# Conflicts: # app/src/androidTest/java/com/android/unio/ui/ScreenDisplayingTest.kt # app/src/androidTest/java/com/android/unio/ui/explore/ExploreScreenTest.kt # app/src/main/java/com/android/unio/MainActivity.kt

Romainhir · 2024-10-31T11:02:13Z

app/src/main/java/com/android/unio/MainActivity.kt

-  val associationRepository = AssociationRepositoryFirestore(Firebase.firestore)
-  val associationViewModel = AssociationViewModel(associationRepository)
+  val associationRepository = remember { AssociationRepositoryFirestore(db) }
+  val associationViewModel = remember { AssociationViewModel(associationRepository) }


Good change. This will be fixed with Hilt later though. Same for the ones below

Romainhir · 2024-10-31T11:08:25Z

app/src/main/java/com/android/unio/model/event/EventRepositoryFirestore.kt

  }

+  private fun <T> performFirestoreOperation(
+      task: Task<T>,


This is a duplicate of AssociationRepositoryFirestore.performFirestoreOperation(...). We should consider refactor this to put them in a separate file. This should be done in another task/PR though

Romainhir · 2024-10-31T11:13:19Z

app/src/main/java/com/android/unio/model/search/SearchRepository.kt

+   */
+  // Should be private, but I need it public for tests
+  fun addAssociations(associations: List<Association>) {
+    Log.d("SearchRepository", "Adding associations to search database")


I guess this is a resual testing mechanism. This should be removed in my opinion.

Romainhir · 2024-10-31T11:13:50Z

app/src/main/java/com/android/unio/model/search/SearchRepository.kt

+  // Should be private, but I need it public for tests
+  fun addEvents(events: List<Event>) {
+    Log.d("SearchRepository", "Adding events to search database")
+    val eventDocuments = events.map { it.toEventDocument() }


Same, testing mechanism to be removed

Romainhir · 2024-10-31T14:34:18Z

app/src/main/java/com/android/unio/model/search/SearchViewModel.kt

+   * Factory for creating a [SearchViewModel] with a constructor that takes a [SearchRepository].
+   */
+  companion object {
+    fun provideFactory(


Is the factory really useful here ? We will remove Factories anyway later so, maybe we can remove it now

…t/search # Conflicts: # app/src/main/java/com/android/unio/MainActivity.kt # app/src/main/java/com/android/unio/model/event/EventRepositoryFirestore.kt

sonarqubecloud · 2024-10-31T17:41:34Z

Quality Gate failed

Failed conditions
55.9% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

Redd87 added 30 commits October 17, 2024 10:10

build(gradle): Add AppSearch dependencies

0afe922

feat(association): Add partial AssociationDocument and conversion fun…

f0c7ddc

…ction for AppSearch integration

feat(event): Add partial EventDocument and conversion function for Ap…

af9923a

…pSearch integration

feat(association): add addAssociationsListener method to AssociationR…

10fb891

…epository

feat(association): implement addAssociationsListener method to Associ…

e7db30f

…ationRepositoryFirestore

feat(search): add SearchRepository for managing search using AppSearch

b708e87

feat(search): add SearchViewModel to interact with SearchRepository

67e1370

feat(search): use SearchViewModel in Explore

84b6dcf

chore(search): add empty test files for future tests

1b5d12c

Merge branch 'main' into feat/search

d3045b8

chore(dependencies): add guava dependencies, set min SDK 31

4bece8b

fix(association): extract associations path to constant

b1e11aa

feat(search): add auth logic to ViewModel init

9673dd2

refactor(search): change searchRepository functionality

13d3dae

test(search): begin adding tests for searchRepository

ee98fa4

refactor(main): change the init of searchViewModel in main

c992103

build(main): Add kapt and appsearch-compiler dependencies

3197622

refactor(association): change

5318a7d

test(search): add tests for the search repository

6c3187c

Merge branch 'main' of https://github.com/SwEnt-Group13/Unio into fea…

0045895

…t/search # Conflicts: # app/src/main/java/com/android/unio/MainActivity.kt # gradle/libs.versions.toml

fix(search): finish merge with main

2b1ab98

build(search): add dependencies for .await()

3be65a0

refactor(main): instantiate the search and event repo in UnioApp

69dedab

feat(event): add getEventWithId in EventRepositoryFirestore

9fe0064

feat(event): add getEventWithId in EventRepository

2d5f3de

feat(searchRepository): functional event and association search

7dab3ed

feat(searchViewModel): add searchEvents and documentation

60770d0

style: ktfmt

78a233f

style: ktfmt

7c8528a

fix(test): add mockEventRepository

33d7f57

Redd87 added 6 commits October 28, 2024 16:45

Merge branch 'main' of https://github.com/SwEnt-Group13/Unio into fea…

59d3101

…t/search # Conflicts: # app/src/main/java/com/android/unio/model/firestore/FirestoreReferenceList.kt

fix(test): add mock searchViewModel

f703cb2

test(search): finish searchRepository tests

d3df33a

fix(test): update ExploreScreenTest android test to include mock Sear…

413060b

…chViewModel

style: ktfmt

336c30d

fix(test): change a mock from mockito to mockk

0693d06

Seriously why the fuck can't mockito mock a final class if all classes are final by default

Redd87 linked an issue Oct 30, 2024 that may be closed by this pull request

Create backend of search feature #67

Closed

chore: remove comments

73ea1b7

Zafouche reviewed Oct 31, 2024

View reviewed changes

Redd87 added 3 commits October 31, 2024 17:14

fix(search): add failure check to functions

9a78d7a

style: ktfmt

8bb39bb

Merge branch 'main' into feat/search

ee20078

# Conflicts: # app/src/androidTest/java/com/android/unio/ui/ScreenDisplayingTest.kt # app/src/androidTest/java/com/android/unio/ui/explore/ExploreScreenTest.kt # app/src/main/java/com/android/unio/MainActivity.kt

Romainhir approved these changes Oct 31, 2024

View reviewed changes

Redd87 added 3 commits October 31, 2024 17:33

refactor(main): merge with main

2b6a9a2

refactor(search): move auth check from viewmodel to repo

f5f7f40

Merge branch 'main' of https://github.com/SwEnt-Group13/Unio into fea…

ec81319

…t/search # Conflicts: # app/src/main/java/com/android/unio/MainActivity.kt # app/src/main/java/com/android/unio/model/event/EventRepositoryFirestore.kt

Redd87 merged commit ee39706 into main Oct 31, 2024
1 of 2 checks passed

Redd87 deleted the feat/search branch October 31, 2024 17:42

		@@ -1,5 +1,7 @@
		package com.android.unio.model.association

		import com.google.firebase.firestore.QuerySnapshot

Feat/search : implement the search backend for events and associations #117

Feat/search : implement the search backend for events and associations #117

Uh oh!

Conversation

Redd87 commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zafouche left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Redd87 commented Oct 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Oct 31, 2024

Quality Gate failed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Redd87 commented Oct 30, 2024 •

edited

Loading

Zafouche left a comment •

edited

Loading