chore(association): refactor association data flow to integrate Firestone data and update impacted files.

Zafouche commented 1 week ago

Description:

This PR implements the necessary changes to retrieve association data from Firestore, adapt all the impacted files (repository, association view model, explore screen) and display it within the Explore screen. The primary goal is to structure the data using categories and properly handle the association's information through the AssociationViewModel.

Motivation and context:

Previously, we were using mock associations, first of all without using Firestore, nor having the associations' categories. This refactor makes the usage of actual association data that's fetched from Firestore possible. This new structure allows us to properly use the association view model, while having all relevant data (not their logos yet however). It enables us to use the view model in screens like the explore screen.

Association:

Added an enum class AssociationCategory that hold display names for categories.
Updated the Association data class to include a category field.
Hydrate and Serialization:
Updated Hydration.kt and Serialization.kt to use the new modified Association data class.
AssociationViewModel:
Added logic to get and group associations by category.
Modified the existing data flow to manage associations using the modified data class.
Explore Screen:
Removed usage of mock data.
Implemented the use of the AssociationViewModel.
Updated the UI to display associations grouped by categories and sorted alphabetically within each category.

How has this been tested?

As this is essentially refactoring data in relevant places, the already existing tests have also been refactored to match the modifications.

Screenshots:

Here is a screenshot of the explore screen, which contains all the associations that were scraped from the EPFL website and are stored on the Firestore database.

Note:

I will also sort the order in which the categories are displayed on the Explore screen. Other than that, this should be final. Please point out anything that I may have missed.

Zafouche commented 1 week ago

I suggest the code reviews are done asap as @armouldr needs these changes.

Zafouche commented 1 week ago

⚠️ The following test does not work as expected. The db variable is mocked properly, but never injected into the ExploreScreen, so it defaults to the ViewModelProvider.Factory. This then creates an actual instance of a AssociationRepositoryFirestore with the real Firestore database (and probably does real requests, so this needs to be fixed asap). The fix shouldn't be too complicated, just passing a mocked AssociationViewModel to the ExploreScreen. It would be nice if we could find a way to prevent factories from using Firebase.firestore when ran from a testing environment.

Also, the code coverage report shows low coverage on the LazyColumn and LazyRows, but this could be tested when mocking the AssociationViewModel and populating the column and rows with actual associations.

I will keep the approved status on this as it should be merged asap, but this issue needs to be addressed.

Actually I just corrected the view model issue by mocking it and wrote a test that checks that all category texts are displayed, all rows are displayed and all associations are displayed. Will add more mock associations to the list that it is tested on to validate these tests even more. Also, I just implemented the alphabetical ordering of the categories, and wrote a helper function for it: it returns sorted entries.

Zafouche commented 1 week ago

Interestingly enough, the associationsAreDisplayed test works locally but not on the CI. I will try a small fix and see what happens.

Zafouche commented 1 week ago

Still fails in the CI:

But not locally (ran a lot of times, in different ways):

For now I will go back to the previous mock association list as the merge is urgent for tomorrow. We can try to look into it later, as this is not really urgent. Note that maybe we should consider activating artifacts on GitHub actions, such that we could open the html file that contains the message error and can debug easily.

Zafouche commented 1 week ago

It passes with the two additional mock associations , perhaps the test is somehow not compatible with the CI's emulator. Anyway, since it works when we test by hand (everything is displayed, categories sorted, associations sorted), it is ok to merge and the issue is probably test specific. Definitely something we can ask the coaches if no one has the answer.

Zafouche commented 1 week ago

Also for some reason AssociationProfile doesn't display any text (error or the name of the association). I have however debugged with prints, and it successfully gets the associations data. The composable however doesn't seem to want to write it. Maybe some paddings issues, since they have been omitted.

Zafouche commented 1 week ago

Also for some reason AssociationProfile doesn't display any text (error or the name of the association). I have however debugged with prints, and it successfully gets the associations data. The composable however doesn't seem to want to write it. Maybe some paddings issues, since they have been omitted.

This was fixed by adding explicit padding in the scaffold and putting the text in a column. Unfortunately the small addition in MainActivity makes SonarCloud fail :( The changes in AssociationProfile also contribute, however they are temporary (in thw works in Romain’s PR), so shouldn’t worry about this. We should ask the coaches if it is relevant to test MainActivity at this stage.