Skip to content

refactor: disruption page workflow and dataflow#1238

Merged
Whoops merged 26 commits intomasterfrom
wh-refactor-disruption-pages
May 5, 2025
Merged

refactor: disruption page workflow and dataflow#1238
Whoops merged 26 commits intomasterfrom
wh-refactor-disruption-pages

Conversation

@Whoops
Copy link
Contributor

@Whoops Whoops commented Apr 24, 2025

Summary of changes

Asana Ticket: [stub] 🏹💅🏻🇵🇱 Create a separate "create" view for disruptions (so we have sensible CRUD)

This aims to refactor the disruption page to be easier to reason about. The primary goal is to ensure data flows in a single direction from the parent disruption page -> the child components, eliminating the need for send(self()) and reducing the situations. I'm also attempting to minimize the scope of the LiveComponents to ensure they are responsible for as little as possible, using functional components as much as possible.

The biggest changes made to accomplish these goals are:

  • Managing the sate of the page through the URL. When editing operations are complete, we no longer send a message to ourselves to signal the parent LiveView. Instead we simply save our work and navigate back to the view page for the disruption, and let the page load as normal.
    • There is a performance compromise here in that if we update/edit a limit for example, the entire disruption is reloaded from the database, instead of just editing/adding the modified entity. Given that in the wildest success of this application we'll have 10s of users, I'm not to worried about optimizing this. If we did feel the need, since push_patch is being used the old state actually is still available on the socket and we could do something more intelligent.
  • Similarly opening edit/create views are handled the same way, via navigation, with update_params/apply_action setting up the appropriate state, which gets passed to the child views.
  • All viewing state is being moved into functional components. Only the edit/create forms themselves remain LiveComponents.
    • This does have the side effect of meaning that the delete logic that was being handled by the child LiveComponent now gets handled by the parent LiveView.

Oh yeah and for funsies I also did the ticket and gave disruptions seperate view/edit pages.

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.

@Whoops Whoops force-pushed the wh-refactor-disruption-pages branch 2 times, most recently from 4688eaa to 7045309 Compare April 29, 2025 21:59
@Whoops Whoops requested a review from cmaddox5 April 30, 2025 19:03
@Whoops Whoops marked this pull request as ready for review April 30, 2025 19:03
@Whoops Whoops requested a review from a team as a code owner April 30, 2025 19:03
@Whoops
Copy link
Contributor Author

Whoops commented Apr 30, 2025

@cmaddox5 I picked on you here because this diff for this is pretty large, and you showed up on the blame for the pieces I changed the most. If you have suggestions on how I can make this easier for you to review, I'll happily spend time restructuring if you have suggestion on how I can make this easier to review. Because I reorganized the code, the diff makes it look like a larger change than it is. Attempting to summarize the actual changes vs copied code are:

  • Everything is driven by the URL, so lots of new routes
  • Pretty much all the buttons except the submit buttons are links now, and now use patch on the button instead of a handler
    • They are also removed instead of being disabled when editing, since you can't disable a link. I ran this passed @fsaid90.
  • The logic for removing/disabling buttons was moved to the view, based on whether there is something being edited or not, instead of a separate disabled assign
  • Handlers now call push_patch and put_flash directly instead of sending messages to the parent view.
  • Forms no longer have a separate "action" assign. Instead, they look to see if the entity being modified has an id to determine if they need to create a new one or update an existing one.
  • Almost all the logic from DisruptionV2ViewLive.mount was moved either to handle_params or to the local state for the edit form.
  • I turned mode_labels into a keyword list, allowing us to control the order, but also do lookups with Access
  • The "view" component for the disruption is entirely new
  • I got showy and added some animations

@Whoops
Copy link
Contributor Author

Whoops commented Apr 30, 2025

There were only minor changes to the unit tests, and the integration tests I only had to change to URL, so that helps give me confidence I didn't change any behavior too.

Copy link
Contributor

@cmaddox5 cmaddox5 left a comment

Choose a reason for hiding this comment

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

Did a first pass and it's looking good. Diff definitely seems to be mostly cut and pasted code but want to take one more pass to make sure I didn't miss anything.

<%= if Ecto.assoc_loaded?(@disruption.replacement_services) and Enum.any?(@disruption.replacement_services) do %>
<div
:for={replacement_service <- @disruption.replacement_services}
class="container border-2 border-dashed border-secondary border-mb-3 p-3 mb-3"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing some excess margins in all three edit components as well as the Replacement Service table. I think the issue is container.

Screenshot 2025-05-01 at 1 37 02 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes Firas asked me about expanding this page to match the table and I didn't retest afterwards. Everything should be using container-fluid now.

class="overflow-hidden"
style="display: none"
phx-mounted={
JS.show(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a big fan of these transitions!

<div class="col-lg-3">
<.button
type="button"
<.link
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you didn't edit the text on any of these, but could we go ahead and change button text to be sentence case as per zeroheight? Then we can just go with sentence case for buttons in the future and be confident we are being consistent.

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 think I've updated everything with sentence case now. Let me know if you spot something I missed.

@Whoops Whoops force-pushed the wh-refactor-disruption-pages branch from 53cb0e8 to b294347 Compare May 1, 2025 18:39
Copy link
Contributor

@cmaddox5 cmaddox5 left a comment

Choose a reason for hiding this comment

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

After another pass, I think this is good to go!

@Whoops Whoops merged commit c6ddb8b into master May 5, 2025
5 checks passed
@Whoops Whoops deleted the wh-refactor-disruption-pages branch May 5, 2025 14:12
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.

2 participants