Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt new api architecture #475

Merged
merged 31 commits into from
Jun 2, 2023
Merged

Adopt new api architecture #475

merged 31 commits into from
Jun 2, 2023

Conversation

mltbnz
Copy link
Member

@mltbnz mltbnz commented Jun 1, 2023

📝 Docs

📲 What

Update api service to use new endpoints and architecture

🤔 Why

The api needed to be updated and is now using a new architecture.

note
Tests will break after merging this and since I was not happy with the app core test suite I decided there will be an updated one

mltbnz added 15 commits June 1, 2023 08:55
- use distanceFilter to reduce amount of requests that will be send to API
- lower precision a bit to reduce battery consumption
- use path components array
- add factory funcs for convenience
- remove unused request structs
- rename
- add post message
- add get messages
- add get locations
- add put location
- fetch messages and locations in parallel on view appearance
- on timer tick fetch messages and locations in parallel
- fetch nextRide and postlocation in parallel
- post location on locationmanager updateLocations event. Since it uses a distanceFilter now there should be less requests made
- update data type to reflect API arch changes
- display alert when sending a message fails
- remove network check before sending
temporary since I was not happy with them anyways and they grew too big
TODO for me to write new test cases that better reflect what needs to be tests and since now we can disable the exhaustivity of the tests we can have more tests with smaller scope
CriticalMapsKit/Sources/ApiClient/Endpoint.swift Outdated Show resolved Hide resolved
await send(.fetchLocations)
}
group.addTask {
await send(.fetchChatMessages)
Copy link
Member

Choose a reason for hiding this comment

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

just a question to understand how iOS is doing this: the desired outcome is that chat (post and get) and location (get) will only be run on demand, like only when the user opens that part of the app (map or chat window). the only thing that gets called on an interval (30sec) would be the locations put, is that right for that implementation as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok. the chat message get would be run on a timer tick (12 sec) as well as the getting the locations. The map is kind of open all the time due to the navigation structure we have but I could pause the timer when another view is openend of course.
regarding the chat feature: I can update the behaviour to only fetch messages when the view is open but then we would loose the "unread messages" feature I guess since it wouldn't make a lot sense anymore. But I am not sure if anyone was into that one anyhow 😅

The location put is currently triggered on location updates. If a user moves more then 40 metres the client will send that request.

Copy link
Member

Choose a reason for hiding this comment

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

pausing once the map view is out of sight sounds like a good idea. there can be another call right after switching back to the map though.

Copy link
Member

Choose a reason for hiding this comment

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

why would the "unread messages" feature not work anymore? wouldn't that just be a matter of comparing the last shown messages to the ones that we added since then? alternatively there could be a timestamp that saves the last shown message. when there is new content everything with an older timestamp would be considered "new". does that make sense or am i missing the point?

Copy link
Member Author

Choose a reason for hiding this comment

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

since we are not getting message updates anymore when the view is not showing there will only be data available the user has already seen. the comparison behaviour from before is very close to what you suggested btw

@mltbnz mltbnz marked this pull request as ready for review June 1, 2023 21:01
@mltbnz mltbnz requested a review from a team as a code owner June 1, 2023 21:01
@mltbnz mltbnz merged commit 5135607 into main Jun 2, 2023
1 check passed
@mltbnz mltbnz deleted the adopt-new-api-architecture branch June 2, 2023 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants