Feat/geocoder location nominatim

Zafouche commented 5 days ago

Description

This PR implements the integration of the Nominatim API into the Event creation screen. It allows users to search for and select a location. Location suggestions are fetched when the user types into the location input text field (with a 1 second debounce delay, to ensure no more than 1 request per second are made), and users can select a place from a dropdown list, which fills the location field with the place’s name and address, and allows us to get the exact coordinates of the address.

Motivation and Context

By integrating the Nominatim API, we enable users to search and select a location easily. This approach provides greater flexibility, as I have done my best to abstract this as much as possible, to be able to easily switch to another tool, like Search Places API, or Photon API.

NominatimLocationSearchViewModel:

Added logic for fetching location suggestions: Implemented functionality to query the Nominatim API as the user types in the search field, with a 1 second debounce delay.
Query: The user’s input is sent to the repository, which should return a list of location suggestions based on the query.

NominatimLocationRepository:

Implemented search functionality: Used Retrofit and OkHttp to make network requests to the Nominatim API for location suggestions.
Data Responses: Created NominatimLocationResponse data class, where the JSON results will be parsed to.

EventCreationScreen:

Dynamic location suggestions: Modified the location input field to display a list of suggestions in a drop down menu as the user types (with a second of delay between the typing and the display of suggestions).
ViewModel integration: Integrated the NominatimLocationSearchViewModel to handle the fetching of location suggestions.
User interaction: Enabled users to select a location from the suggestions, which automatically fills in entire location address.

What's next

Add Tests: Add unit tests (and perhaps a simple e2e test) to cover this new feature.

Zafouche commented 4 days ago

I will open this PR for review when the CI passes, such that my teammates can review my work: the feature works well, I just need to write more tests.

I am still working on improving the test coverage by:

Writing unit tests for the view model (which is already above 80% in terms of coverage, but that isn't from tests actually using the view model).
Writing an e2e test for the Event creation screen. This mostly goes out of the scope of the implementation of the feature. There is an existing task for writing an e2e test for the creation of an event, however it wasn't assigned to me, but I kind of have to have one to be able to test this screen with the new address input functionality. If I don't succeed tonight, I will either test the functionality in simple input field specific UI tests or just write simple display tests in the UI tests, and the functionality will be tested in the e2e which I will report to next sprint. I won't spend too much time on it since I already worked on it a little this afternoon, and mocking the web client which needs manipulations throughout the test while making work hilt injections is not an obvious process at all.

Other than that, the test coverage looks pretty good even if the e2e isn't added so I'm happy with it.

Zafouche commented 4 days ago

Good job implementing this! This nicely completes the event creation. Don't forget to comment your code, but besides that no complaints, LGTM (as long as you add your tests).

Thanks a lot for reminding me to add documentation! I've been deep in testing that I forgot about it, very good point. Just added documentation.

We should also be careful about merging our PRs as I refactored the EventCreation page a bit

And you're right, we should

Zafouche commented 4 days ago

Final verdict:

My additions to the EventCreationScreen have been tested through a new test I added in the screen's UI test. This effectively raises the coverage to above 80%!
Testing the view model: I haven't been able to successfully test the view model in isolation. The problem is, that the view model uses a ViewModel Scope in the collectQuery function, meaning that, in the tests, if nothing is explicitly defined, it only goes to execute the code inside of that Scope once, when the view model is initialized. After that, when the query changes, the inside code is simply skipped. I've done a lot of research and manipulating coroutines for tests isn't easy. The only potential solutions I've read look like the one in this stack overflow post. I've successfully made it such that Hilt injects the correct coroutine in production code, such as the feature still works when we run the app, however the solution does not work in the test, it still skips the code inside the scope block, so I still don't know how to fix this. Considering that the coverage is already over 80% in this PR, I will merge this and report this work for next sprint, in another PR. I will not push the solution of adding a Coroutine field to the view model, or the tests I've written, since the solution does not make the tests work.

However I will push the code of both the start of the e2e test and the attempt using the solution described above in the branch test/geocoder-e2e-and-viewmodel, that I will pick up or delete and start from scratch after #223 has been merged to main.