Skip to content
This repository has been archived by the owner on Jan 10, 2024. It is now read-only.

Migration to Navigatum #1462

Merged
merged 31 commits into from
Aug 10, 2022
Merged

Conversation

PiotrKedra
Copy link
Contributor

@PiotrKedra PiotrKedra commented Jul 4, 2022

Issue

This fixes the following issue(s):

Depending on merging centralized search #1457, I decided to work on this PR rather than the master branch because I used some room finder logic there.

Description

Migrate to:

  • more information nav.tum.sexy/api/get/:id api-endpint
  • the search nav.tum.sexy/api/search?q=querry api-endpoint

@CommanderStorm CommanderStorm changed the title Navigatum migration Migration to Navigatum Jul 4, 2022
@PiotrKedra
Copy link
Contributor Author

PiotrKedra commented Jul 10, 2022

Migration more or less should be ready. I have migrated three roomfinder endpoints:

  • fetchAvailableMaps()
  • fetchRooms()
  • fetchCoordinates()
  • Into two NavigaTum endpoints (search navigation and navigation details)

What is still to be done?

Problems/questions

  1. I heard that NavigaTum is not stable yet, therefore can it be merged now?
  2. Old roomfinder API has an endpoint that fetches RoomFinderSchedule. However, I couldn't make this code run in the App, so I am not sure if this is still supported or it can be removed?
  3. Barrier-free API fetches rooms that have ID from the old roomfinder API and using NavigaTum with this ID does not work. Therefore, now when users click on the Barrier-free facility, the app redirects to the search screen with this facility address as a query input. Is this solution ok?

@CommanderStorm
Copy link
Member

CommanderStorm commented Jul 13, 2022

  1. I heard that NavigaTum is not stable yet, therefore can it be merged now?

If we comment on TUM-Dev/NavigaTUM#130 about the change timeline, it is stable ;)

  1. Old roomfinder API has an endpoint that fetches RoomFinderSchedule. However, I couldn't make this code run in the App, so I am not sure if this is still supported or it can be removed?

@joschahenningsen historically What is the status on this? Was this ever used in the app?

  1. Barrier-free API fetches rooms that have ID from the old roomfinder API and using NavigaTum with this ID does not work. Therefore, now when users click on the Barrier-free facility, the app redirects to the search screen with this facility address as a query input. Is this solution ok?

@joschahenningsen Can we migrate the backend (add a new field?)

@joschahenningsen
Copy link
Member

@joschahenningsen historically What is the status on this? Was this ever used in the app?

I think this might be used in the calendar somewhere. I'll need to do some more digging though.

@joschahenningsen Can we migrate the backend (add a new field?)

Sure, as long as it does not break old clients :) Do you have a way of mapping those ids?

On another note: Do y'all need some help with the merge conflicts? :D

@PiotrKedra
Copy link
Contributor Author

PiotrKedra commented Jul 18, 2022

I have implemented a new UI, so it has more information. Users can choose between all available site plans and zoom in and out. Besides that, users can open location in other apps that can handle geo-locations as input (#1456). Also, there is a button for a future interactive map, for now it just redirects to the NavigaTum website (idk if some1 is working on an interactive map right now #1437)

pic 1 pic 1
pic 1

@PiotrKedra
Copy link
Contributor Author

RoomFinderSchedule

I did some digging into this secret RoomFinderSchedule logic ;p

  1. Old roomfinder API has an endpoint that fetches RoomFinderSchedule. However, I couldn't make this code run in the App, so I am not sure if this is still supported or it can be removed?

I find out that this logic was not available to execute after these two commits:

In both these commits, it looks like this might be removed accidentally, but cannot be sure. However, it looks like, since April 2018 this RoomFinderSchedule feature does not work, so I think it would be best to deal with this problem in a separate issue. Also since it was not available for so long, there is a question if anybody needs it.

Barrier-free API

Another case is what shall we do with this Barrier-free API

  1. Barrier-free API fetches rooms that have ID from the old roomfinder API and using NavigaTum with this ID does not work. Therefore, now when users click on the Barrier-free facility, the app redirects to the search screen with this facility address as a query input. Is this solution ok?

Should we wait for TUM-Dev/Campus-Backend#80 to be merged and ready to use, so it can be implemented here as well? Or we just merge this PR now with my temporary solutions and take care of Barrier-free in another one? @CommanderStorm

@CommanderStorm
Copy link
Member

Should we wait for TUM-Dev/Campus-Backend#80 to be merged and ready to use, so it can be implemented here as well? Or we just merge this PR now with my temporary solutions and take care of Barrier-free in another one? @CommanderStorm

Actually, we don't need to wait for that PR, as the current backend does already provide us with the exact information we need:
image
When looking in the network tab, room_code is the ID, NavigaTUM uses, so probably we can simplify this UX a bit.
The study rooms also one place where we already have this field. (it is named code in the data class)
Thus, we can redirect directly to showing the room.

Doing so should cover all cases, since the fixes we added for invalid room_codes do not relate to study rooms or barierfree. Redirect to the details page for the search results sould be save here..

However, if you like, we can use your solution and migrate all occurrences of the room_id to room_code in a later PR…

…avigatum

� Conflicts:
�	app/src/main/AndroidManifest.xml
�	app/src/main/java/de/tum/in/tumcampusapp/api/app/TUMCabeClient.java
�	app/src/main/java/de/tum/in/tumcampusapp/component/other/general/RecentsDao.java
�	app/src/main/java/de/tum/in/tumcampusapp/component/other/locations/LocationManager.kt
�	app/src/main/java/de/tum/in/tumcampusapp/component/tumui/calendar/CalendarDetailsFragment.kt
�	app/src/main/java/de/tum/in/tumcampusapp/component/tumui/roomfinder/model/RoomFinderRoom.kt
�	app/src/main/java/de/tum/in/tumcampusapp/component/ui/barrierfree/BarrierFreeFacilitiesActivity.kt
�	app/src/main/java/de/tum/in/tumcampusapp/component/ui/search/SearchFragment.kt
�	app/src/main/java/de/tum/in/tumcampusapp/component/ui/search/SearchResult.kt
�	app/src/main/java/de/tum/in/tumcampusapp/component/ui/search/SearchResultState.kt
�	app/src/main/java/de/tum/in/tumcampusapp/component/ui/search/SearchResultType.kt
�	app/src/main/java/de/tum/in/tumcampusapp/component/ui/search/SearchViewModel.kt
�	app/src/main/java/de/tum/in/tumcampusapp/component/ui/search/adapter/RecentSearchesAdapter.kt
�	app/src/main/java/de/tum/in/tumcampusapp/component/ui/search/adapter/ResultTypesAdapter.kt
�	app/src/main/java/de/tum/in/tumcampusapp/component/ui/search/adapter/SearchResultsAdapter.kt
�	app/src/main/java/de/tum/in/tumcampusapp/component/ui/search/di/SearchComponent.kt
�	app/src/main/java/de/tum/in/tumcampusapp/di/AppComponent.kt
�	app/src/main/res/layout/activity_search.xml
�	app/src/main/res/layout/fragment_search.xml
�	app/src/main/res/layout/toolbar_search.xml
�	app/src/main/res/values-de/strings.xml
�	app/src/main/res/values/strings.xml
@CommanderStorm
Copy link
Member

Thanks for the extensive review :) WIth most of the comments I agree. I think it would be better If I aggregate these changes into one commit, rather than one by one by clicking here on GitHub (there will be long history ;d)? But I don't know what is better, I see these commit suggestions for the first time ^^

What works for you…
You don't have to worry about a long history, since all changes are squashed into one commit anyway 😉

When it comes to ordering since there are four different objects fetched in a centralized search. The "All results" sort alphabetically all of them, but when you choose a specific object type, then the ordering is as it was fetched from APIs. I added sorting because I thought it was slightly better to mix the results alphabetically than display groups of objects one by one (firstly all people, then all lectures, then all buildings and rooms). This was my reasoning, what do you think? :D

For rooms/buildings, this definitively does not make sense, as we have a priority between our items.
For people and lectures I dont know this for certain, but I assume they are also sorted somehow.

We should therefore just drop this sorting. Finding a total order between these items is not trivial and should be its PR

From an ordering, I would therefore prefer:

  • people
  • building/room (in the order we communicate if possible)
  • lectures

Assumptions being,

  • that there are few human names that map intersect with buildings/rooms and therefore this can be first without causing much of a bad impact.
  • Searching for your lectures is something you rarely do in a campus app

@PiotrKedra
Copy link
Contributor Author

PiotrKedra commented Jul 29, 2022

Ok, I will reimplement the ordering as u suggest and other fixes , but probably I will do it next week after my exams

@Bentipa
Copy link
Member

Bentipa commented Jul 31, 2022

I will take another look once @PiotrKedra has implemented the changes, just tag me again then please :)

@PiotrKedra
Copy link
Contributor Author

I have implemented changes, and also left comments on some conversations
@Bentipa

@PiotrKedra PiotrKedra marked this pull request as ready for review August 9, 2022 12:44
Copy link
Member

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo this can be merged. All my major comments have been adressed.
@Bentipa do you want to review it, or should I merge it?

@CommanderStorm
Copy link
Member

@PiotrKedra Thanks for the wonderful work you did ✨

@Bentipa
Copy link
Member

Bentipa commented Aug 10, 2022

@CommanderStorm Just gave it a quick look cause of time reasons, but looks good overall to me. So we can merge it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants