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

1106-staff-deactivation-refactor #1142

Merged
merged 12 commits into from
Dec 16, 2024

Conversation

wandergithub
Copy link
Contributor

@wandergithub wandergithub commented Nov 13, 2024

🔗 Issue

#1106

✍️ Description

Refactor staff deactivation

📷 Screenshots/Demos

image

@wandergithub wandergithub marked this pull request as draft November 13, 2024 21:33
config/routes.rb Outdated Show resolved Hide resolved
@wandergithub wandergithub marked this pull request as ready for review November 15, 2024 02:37
@jmilljr24
Copy link
Collaborator

@wandergithub Awesome job!

The only thing I noticed on my end is that the flash is no longer working. Are you seeing the same thing?

This is on the main branch:
Screenshot from 2024-11-15 09-07-02

@wandergithub
Copy link
Contributor Author

Now that you point that out 🤔
I will take a look.

@wandergithub
Copy link
Contributor Author

Something is happening with the template, I think related to turbo stream and how the flash message is being handled but I don't fully understand how it works jet...

@jmilljr24
Copy link
Collaborator

Something is happening with the template, I think related to turbo stream and how the flash message is being handled but I don't fully understand how it works jet...

The update action in ActivationsController is implicitly looking for update.turbo_stream.erb in views/organizations/activations. You can explicitly render a file that is in a differently path but we probably should stick with convention unless there is a specific reason you can think of not to.

@kasugaijin
Copy link
Collaborator

Yeah what @jmilljr24 said. Check out app/views/organizations/staff/staff/update_activation.turbo_stream.erb. You need this for your new controller and action, so just move that turbo stream view file to the correct view directory for that new controller and rename it to update.turbo_stream.erb. You will note that this file does two things. It replaces the toggle because it should change after changing the activation, and it renders the flash.

@jmilljr24
Copy link
Collaborator

Yeah what @jmilljr24 said. Check out app/views/organizations/staff/staff/update_activation.turbo_stream.erb. You need this for your new controller and action, so just move that turbo stream view file to the correct view directory for that new controller and rename it to update.turbo_stream.erb. You will note that this file does two things. It replaces the toggle because it should change after changing the activation, and it renders the flash.

@kasugaijin I don't believe we need the turbo replace of the toggle any longer. I think that was used when the page was rendering two layouts that were user selected(table vs cards). If the user clicked the toggle in the table, the card wouldn't reflect the change until a page fresh.

@kasugaijin
Copy link
Collaborator

@jmilljr24 Ah yes we got rid of those cards...good point. We can remove the toggle bit in the turbostream response, then @wandergithub !

@wandergithub
Copy link
Contributor Author

It works now!
image

Thanks guys. Now I understand better how turbo_streams and the real-time update work.

Copy link
Collaborator

@kasugaijin kasugaijin left a comment

Choose a reason for hiding this comment

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

Looks nice and clean @wandergithub ! I like the refactor.
Just a couple more policy tests we could add now we are also mutating adopters and fosters. Should be a pretty quick addition. Let me know if you have any questions!

@kasugaijin
Copy link
Collaborator

@wandergithub just pining to see if you can finish those last few tests? If you're too busy, let me know and I can take care of it.

Copy link
Collaborator

@kasugaijin kasugaijin left a comment

Choose a reason for hiding this comment

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

@wandergithub I finished this one off! Thanks for getting it to where it was.

@kasugaijin kasugaijin merged commit e7169ff into rubyforgood:main Dec 16, 2024
5 checks passed
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.

3 participants