fix(trainsformer): fix adding / removing service dates, days of week#1351
fix(trainsformer): fix adding / removing service dates, days of week#1351
Conversation
…former service date
…s instead of using bespoke events
rudiejd
force-pushed
the
fix/polish-adding-service-dates
branch
from
6ac52bf to
7d2c1be
Compare
|
I'm having a really hard time creating an integration test to reproduce this behavior, but you can reproduce manually by adding and deleting a bunch of service dates at the same time for any trainsformer service |
jzimbel-mbta
left a comment
jzimbel-mbta
left a comment
There was a problem hiding this comment.
Looks good, and wow that's a lot of boilerplate magicked away by an ecto feature 😲
I have some suggestions, but nothing blocking.
lib/arrow/util/util.ex
Outdated
| end | ||
|
|
||
| @spec validate_start_date_before_end_date(Ecto.Changeset.t(any())) :: Ecto.Changeset.t(any()) | ||
| def validate_start_date_before_end_date(changeset) do |
There was a problem hiding this comment.
suggestion:
It makes total sense to consolidate this logic, but it feels a little "off" to put a function that makes assumptions about the field names of changeset, and the desired error message, into a very general catch-all module like Util.
Do you think it could make sense to put this in a separate Arrow.Validation module?
I could also see it being helpful to separate the logic that does the Date comparison from the logic that reads/writes to the changeset, but that's probably overkill.
There was a problem hiding this comment.
yep I can move this to a different module!
| patch={~p"/disruptions/#{@disruption.id}/trainsformer_export/#{export.id}/edit"} | ||
| > | ||
| <.icon name="hero-pencil-solid" class="bg-primary" /> | ||
| <.icon name="hero-pencil-solid" class="bg-primary hover:opacity-40" /> |
There was a problem hiding this comment.
question/suggestion:
This makes the icon's opacity decrease to 40% on hover, right?
Any reason for doing that instead of what seems to be the more standard behavior of (lower opacity while not hovered/focused) -> (higher opacity on hover/focus)?
For example, Tailwind's default styles have elements' opacity increase on hover.
There was a problem hiding this comment.
happy to do higher opacity! I just thought some sort of visual sign might be nice for the icons.
| class="btn-sm p-0" | ||
| type="button" | ||
| phx-click="delete_export" | ||
| phx-click="delete_trainsformer_export" |
There was a problem hiding this comment.
suggestion:
Good catch.
If it wouldn't be too much of a pain, it would also help to rename the delete_export event and its corresponding handle_event clause to delete_hastus_export.
3 participants
Summary of changes
Asana Ticket: 🏹Service date add / remove bug fix / polish
Problem(s):
Described in the above ticket, but briefly:
cannot replace related %Arrow.Trainsformer.ServiceDate... This typically happens when you are calling put_assoc/put_embed with the results of a previous put_assoc/put_embed/cast_assoc/cast_embed operation, which is not supported. You must call such operations only once per embed/assoc, in order for Ecto to track changes efficientlySolution:
After a lot of form reading and researching, I found a great blog from dockyard that describes an approach for handling an association without the need for any JavaScript. Basically, we now use the
sort_params/drop_paramsofEcto.Changeset.cast_assoc/3via hidden fields to handle adding / deleting service dates. This prevents us from callingput_changemultiple times, fixing (1). I also noticed that I cannot reproduce (2) after implementing this change.Additionally, I added:
Reviewer Checklist