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

Group routes together #4

Closed
benmelz opened this issue Aug 16, 2023 · 5 comments · Fixed by #8
Closed

Group routes together #4

benmelz opened this issue Aug 16, 2023 · 5 comments · Fixed by #8
Assignees
Labels
enhancement New feature or request

Comments

@benmelz
Copy link
Member

benmelz commented Aug 16, 2023

As @frothedoatmilk noted on #1, duplicate message can occur.

I was on the fence but after talking to @sherson about the user stories for this app yeah it'll probably make sense to group them together.

@benmelz
Copy link
Member Author

benmelz commented Aug 17, 2023

Still pending green light (dont work on yet) but @frothedoatmilk called dibs on this

@benmelz benmelz added the enhancement New feature or request label Aug 17, 2023
@sherson
Copy link
Member

sherson commented Aug 18, 2023

Thanks, it's green lit.

There are pros and cons to each approach, but I think it's worth doing since our use case is for this to be rendered in a small iframe, and it's common for UMTS detours to impact multiple routes (so it's likely that detours listed for each route would not all fit in the iframe, which also doesn't have scroll bars).

Please sort by route within each detour, and also by route for all detours.

@benmelz
Copy link
Member Author

benmelz commented Aug 18, 2023

Please sort by route within each detour, and also by route for all detours.

On the second part, I guess we have no real choice but to use the "lowest" route in a grouping. The other uncertainty I have is what should happen if only part of a route grouping has been whitelisted. Should we remove them from the groupings, or still display labels for ones that have been filtered out?

@sherson
Copy link
Member

sherson commented Aug 18, 2023

I don't think we need to remove filtered-out-routes if they share a detour with routes that we included in our URL parameter (I could be wrong, but no situations come to mind where that would be an issue, should it ever come up).

However, now that I think about it, we ought to sort by Priority too. How about:

  • Group routes together (sort by route within each detour)
  • Sort detours by Priority, then ascending by route (where general messages [ones that aren't associated with any routes] are after route-specific messages at the same priority level, and we use the lowest route number when there are multiple routes associated with a single detour), then descending by FromDate, then descending by FromTime (so newest detour shows first if route and priority are the same)

Priority values appear to be:

  1. Emergency
  2. High
  3. Medium
  4. Low

No matter what we do, the nature of grouping by route means we may occasionally end up with detours for a specific route not being next to each other in the list (and sorting by priority will affect that as well).

I have a vague recollection of FromTime maybe not being used (or not being used in a way that make sense), and what there's now is weird:

<Message>
Due to staffing levels, "S" Trips will not operate between 05/27/2023 and 09/03/2023.
</Message>
...
<FromDate>2023-05-28T00:00:00</FromDate>
<FromTime>2023-05-30T00:00:00</FromTime>

But I can't think of harm being caused by sorting by FromDate and then FromTime.

Am open to suggestions.

@benmelz
Copy link
Member Author

benmelz commented Aug 18, 2023

I don't think we need to remove filtered-out-routes if they share a detour with routes that we included in our URL parameter (I could be wrong, but no situations come to mind where that would be an issue, should it ever come up).

However, now that I think about it, we ought to sort by Priority too. How about:

  • Group routes together (sort by route within each detour)
  • Sort detours by Priority, then ascending by route (where general messages [ones that aren't associated with any routes] are after route-specific messages at the same priority level, and we use the lowest route number when there are multiple routes associated with a single detour), then descending by FromDate, then descending by FromTime (so newest detour shows first if route and priority are the same)

Priority values appear to be:

  1. Emergency
  2. High
  3. Medium
  4. Low

No matter what we do, the nature of grouping by route means we may occasionally end up with detours for a specific route not being next to each other in the list (and sorting by priority will affect that as well).

I have a vague recollection of FromTime maybe not being used (or not being used in a way that make sense), and what there's now is weird:

<Message>
Due to staffing levels, "S" Trips will not operate between 05/27/2023 and 09/03/2023.
</Message>
...
<FromDate>2023-05-28T00:00:00</FromDate>
<FromTime>2023-05-30T00:00:00</FromTime>

But I can't think of harm being caused by sorting by FromDate and then FromTime.

Am open to suggestions.

This seems like something we can/should do, but I'd like it as a second ticket/PR (that I'm gonna make now). Just grouping the routes and reconciling that with the current sorting/filtering logic is already going to be a fairly dense PR.

So for this issue:

  • public messages should be grouped by route
  • all routes in group should be fully displayed, even if only one of the routes is a part of the filter
  • current ordering should be preserved, but using the "highest ranked" route within a group

@benmelz benmelz closed this as completed in #8 Sep 1, 2023
benmelz added a commit that referenced this issue Sep 1, 2023
As discussed in #1, some messages affect multiple routes. Displaying
them multiple times is undesired, so to fix #4, we'll group any routes
affected by one message under that message.

---------

Co-authored-by: benmelz <ben@melz.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants