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

Refactor passenger data into own store #163

Open
andrewnoyes opened this issue Dec 11, 2020 · 0 comments
Open

Refactor passenger data into own store #163

andrewnoyes opened this issue Dec 11, 2020 · 0 comments
Labels
cleanup discussion needed Requires team discussion on how to proceed

Comments

@andrewnoyes
Copy link
Collaborator

andrewnoyes commented Dec 11, 2020

When you load a carpool, a list of drivers is retrieved that each contains passengerUserIds. We track this information so that we know if logged in users are already joined to a driver and to display the number of seats remaining for each given driver. We intentionally did not want to provide all passenger user information to every person who views a carpool, because that info is only relevant to drivers. But right now, we also attach a passengers array to the driver that's initially undefined. If the user viewing the carpool is the creator or is a driver, then that information is populated so that they have access to the user's phone number and address to pick them up.

This has made the driver store kind of bloated with duplicate logic for adding/removing passengers. I think a better approach would be to refactor passenger information into its own mobx store and remove the passengers property from the driver DTO (while still keeping the passengerUserIds). Then, if the user viewing a carpool is the creator or driver, we can then pull passenger information and house it in the passenger store. Adding/removing passenger logic would then only be concerned with the passengerUserIds, and passenger information accessed via the store could be a computed property or probably even just a function that's invoked when the user IDs change.

Some other stuff worth consideration:

  • Eventually we may want to handle passenger's updating their information. Right now, the only way for them to change their location/phone number is to remove themselves and then join as a passenger again.
  • Consolidating driver/carpool creator logic would be really nice - right now we've got these checks sprinkled around the stores and they handle various data fetches based on whether the current user is eligible to view that data.
  • Carpool DTO has metadata on number of drivers and the number of seats remaining. This too is somewhat duplicated logic, because we already compute this on the frontend for displaying remaining seats for each driver record. But the carpool details card displays it via the metadata property on the DTO, which is only populated and broadcast on the backend (when a driver/passenger is added or removed).
    • This also poses an issue which I've fixed (somewhat hackily) where when a carpool is updated, e.g. its description or name, a carpool:updated event is broadcast. But since it's not a metadata:updated event, that property just gets overwritten resulting in that field being displayed as all 0s in the UI.
    • We could just remove this from the DTO and carpool details component entirely (since we already display individually it on the driver items). Or we could keep it displayed and refactor it to maybe a computed/aggregated property that looks at all the drivers' capacities and the number of user IDs attached to each.
@andrewnoyes andrewnoyes added discussion needed Requires team discussion on how to proceed cleanup labels Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup discussion needed Requires team discussion on how to proceed
Projects
None yet
Development

No branches or pull requests

1 participant