5906 add turbo with disabled turbo drive

[mononoken]

What github issue is this PR for, if any?

Resolves #5906 but with stipulations

What changed, and why?

• Adds turbo-rails gem
• Removes turbolinks package (does not look like it was being used anyway)
• Adds a turbo frame on the case_contacts/index view to prevent full page reloads when filtering and demonstrate turbo is working
• Disables Turbo Drive from applying globally by default
• Also does not bring redis in, which turbo:install does by default. This means turbo streams will not work unless we wire up an alternative to redis, but turbo frames and turbo drive should work fine without it. I left a note in the Gemfile calling attention to this incase anyone tries to add a stream and thinks this was a mistake.

I think this covers the criteria of the issue while making the most minimum changes.

It may be desired to not have Turbo Drive disabled globally. It is enabled by default with turbo-rails and thus with typical Rails 7 apps. It would be the predicted behavior for those familiar with turbo. It would also make performance better as a default, by avoiding full page reloads where they are unnecessary.

The main problem with Turbo drive enabled by default is that the JS in the app does not expect turbo events. This means event listeners may not behave or attach as expected, constants can potentially get re-declared, etc.

Some solutions that would allow Turbo drive to be enabled by default:

1. Disable turbo individually for all forms and links that currently would lose functionality with turbo enabled.
2. Refactor all JS to work with Turbo. More work. I initially recommended moving lots of JS over to Stimulus, but I think this could also be achieved by refactoring the current jQuery for turbo events.

Or we can stick with Turbo drive disabled as a default and just explicitly enable it on links and forms where it will not cause any issues or where it would greatly help performance. I am not sure how much it would help performance for this app specifically, so maybe doing the refactoring work is not worth doing now.

How is this tested? (please write tests!) 💖💪

I do not think we need to add new tests for this, but if you have suggestions please let me know.

Screenshots please :)

Without Turbo:

without.turbo.mov

With Turbo:

with.turbo.mov

[mononoken]

I did play around with turning Turbo drive on globally, running the spec suite, and then disabling Turbo for some forms to try to get all the system specs passing again. It does not seem like it will take many changes, so maybe option 1 I listed above is not that difficult to achieve. I worry about any cases that may not be covered in the spec suite though. If there is high confidence in the suite, and that option is desirable, I can make a draft for that branch too.

[thejonroberts]

This is great!

I would prefer Turbo Drive on by default (opt-out rather than opt-in), because I think a lot of people will be unaware of it. It's one of those automatic rails things most of the time. As long as the specs that break can be fixed with a quick opt-out - If it ends up being more work than that, this will work!

(Just a contributor here, no code ownership!)

[thejonroberts]

This makes it sound like Redis is a dependency of 'turbo-rails' gem.

Maybe something like

Suggested change

	# gem "redis", "~> 4.0" # Redis is by default included with turbo-rails install but excluded per https://github.com/rubyforgood/casa/issues/5906
	# gem "redis", "~> 4.0" # Redis is required for Turbo Streams but is not available in production yet.

Should we make an issue to add Redis to production to enable using turbo streams via ActionCable?

[elasticspoon]

We don't really have a use case for action cable atm. The closest thing is notifications but those are low enough in volume that them being delivered closer to real time would not have much value.

I am also not sure about the cost. We are doing all this on donated heroku credits.

[schoork]

I think with Redis in Heroku we will need another dyno

[mononoken]

I like your wording of the comment better. I will update it but with a further note to make it clear it isn't planned to be added until necessary. Thank you!

[thejonroberts]

not sure how to prevent removing this platform here, but as long as production/qa isn't using the platform, this is probably okay.

[mononoken]

Thanks for pointing this out! I have just been manually ignoring these platform removals from my PRs incase they caused any conflicts in any environments. I rebased to remove this change.

[thejonroberts]

I would prefer to remove this and have Drive on by default. It is unlikely that everyone adding a new view/feature will know that it needs to be enabled, or how to do so.

As long as broken specs can be opted-out of, as you've mentioned doing, I would prefer that approach.

[elasticspoon]

I did play around with turning Turbo drive on globally, running the spec suite, and then disabling Turbo for some forms to try to get all the system specs passing again. It does not seem like it will take many changes, so maybe option 1 I listed above is not that difficult to achieve. I worry about any cases that may not be covered in the spec suite though. If there is high confidence in the suite, and that option is desirable, I can make a draft for that branch too.

@mononoken I'm good with this PR being just these changes and then enabling it by default in another PR with whatever test fixes are needed.

[elasticspoon]

Also some dependabot stuff caused a conflict.

[mononoken]

@elasticspoon I rebased onto main to fix the merge conflicts and also remove the platform removal from the Gemfile that @thejonroberts pointed out.

I also added two more new commits. One changed the redis comment to what @thejonroberts suggested. The final one disables Turbo on some more links, so that they continue to work in the new turbo frame on the case_contact index.

I made this last commit after noticing two specs failing and also some other links not working locally after rebasing onto main. Not sure why they weren't failing before... The specs were ./spec/system/notifications/index_spec.rb:22 ./spec/system/case_contacts/followups/resolve_spec.rb:71 if that sounds alarming and needs investigating but otherwise they work on this branch now 🙂

[elasticspoon]

thank you @mononoken