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.

Sign up for GitHub

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

Jump to bottom

fix(shuttles): call Changeset.apply_action after uploading xlsx #1083

Merged
merged 9 commits into from
Jan 17, 2025
Merged

fix(shuttles): call Changeset.apply_action after uploading xlsx #1083

merged 9 commits into from
Jan 17, 2025
+157 −46

Conversation

meagharty
Copy link
Contributor

@meagharty meagharty commented Jan 9, 2025 •
edited
Loading

Summary of changes

Asana Ticket: [extra] 🏹🐛 [edit only] Upload Shuttle Definition XLSX then Retrieve Estimates - retrieve estimate does nothing (websocket crash)

  • This patches the issue when the uploaded XLS doesn't produce a valid route_stops changeset
  • We now error on upload if the changeset would be invalid (in the current case, if the stop doesn't exist), instead of allowing the invalid casted changeset to be applied to the form

image

Reviewer Checklist

  • Meets ticket's acceptance criteria
  • Any new or changed functions have typespecs
  • Tests were added for any new functionality (don't just rely on Codecov)
  • This branch was deployed to the staging environment and is currently running with no unexpected increase in warnings, and no errors or crashes.

@meagharty meagharty changed the base branch from master to meag/refactor-shuttles-form-to-nested-form January 9, 2025 22:59
@meagharty meagharty force-pushed the meag/apply-action-after-upload-xlsx branch from 0b854e8 to babb617 Compare January 10, 2025 14:11
@meagharty meagharty force-pushed the meag/refactor-shuttles-form-to-nested-form branch from 59aae6a to e36bba5 Compare January 10, 2025 16:37
Base automatically changed from meag/refactor-shuttles-form-to-nested-form to master January 10, 2025 16:44
@meagharty meagharty force-pushed the meag/apply-action-after-upload-xlsx branch 3 times, most recently from 2a0281c to 56834c5 Compare January 14, 2025 21:38
meagharty
meagharty commented Jan 14, 2025
View reviewed changes
lib/arrow_web/live/shuttle_live/shuttle_live.ex
)
stop_ids
|> elem(direction_id |> Atom.to_string() |> String.to_integer())
|> get_new_route_stops_changeset_with_uploaded_stops(direction_id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to utilize the route_stop's changeset validation within this function (for the stop_id validation etc). Things got very verbose with that. 🤔

@meagharty meagharty requested review from a team and jzimbel-mbta and removed request for a team January 14, 2025 21:59
@meagharty meagharty marked this pull request as ready for review January 14, 2025 22:04
jzimbel-mbta
jzimbel-mbta approved these changes Jan 17, 2025
View reviewed changes
Copy link
Member

@jzimbel-mbta jzimbel-mbta left a comment

Choose a reason for hiding this comment

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

1 suggestion re: spreadsheet parsing, and 2 small stylistic nits.
Good to go after you address them as you see fit 🙂

lib/arrow_web/live/shuttle_live/shuttle_live.ex Outdated Show resolved Hide resolved
lib/arrow_web/live/shuttle_live/shuttle_live.ex Outdated
%{
direction_id: direction_id,
stop_sequence: i,
display_stop_id: Integer.to_string(stop_id)
Copy link
Member

Choose a reason for hiding this comment

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

suggestion

Would it make sense to convert the stop IDs to strings right when we parse them from the uploaded spreadsheet over in DefinitionUpload?

I know we're only working with bus stop IDs right now, which are generally numeric in format, but unless we actually need to do arithmetic on them I think it would be safest to convert them to strings immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, done.

I kept the check that it's an integer in the upload since that is what is in the Shuttle Conventions doc for a shuttle stop replacing rail.

lib/arrow_web/live/shuttle_live/shuttle_live.ex Outdated
@@ -490,7 +492,7 @@ defmodule ArrowWeb.ShuttleViewLive do

changeset = Ecto.Changeset.put_assoc(changeset, :routes, new_routes)

socket = socket |> assign(:form, to_form(changeset)) |> update_map(changeset)
socket = socket |> assign(:form, to_form(changeset)) |> update_map()
Copy link
Member

Choose a reason for hiding this comment

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

nit

It doesn't look like there's a difference between assign/2 (takes a KW-list) and assign/3 (takes a key and a value) when only one assign is getting set or updated. Would you mind sticking with one or the other in this PR, for consistency's sake?

@meagharty meagharty force-pushed the meag/apply-action-after-upload-xlsx branch from fbc6ef6 to e5149cd Compare January 17, 2025 20:28
@meagharty meagharty force-pushed the meag/apply-action-after-upload-xlsx branch from e5149cd to 9f26918 Compare January 17, 2025 20:33
@meagharty meagharty merged commit 3698974 into master Jan 17, 2025
16 checks passed
@meagharty meagharty deleted the meag/apply-action-after-upload-xlsx branch January 17, 2025 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzimbel-mbta jzimbel-mbta jzimbel-mbta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@meagharty @jzimbel-mbta