WIP: fix "destroy" links

[pablobm]

Fixes #1643

When #1618 was merged, some JS broke. One important piece of functionality that stopped working was "destroy" links.

We were using the JS-driven destroy links that are common in Rails. These work on links with the attribute data-method="delete", and show a confirm() box with text extracted from the attribute data-confirm.

You might be wondering why the specs didn't break... Well, it turns out that Capybara tries to be clever about these special attributes, even when using Rack::Test. As a result, tests passed even though the JS to drive the functionality wasn't working. To avoid this happening again, I have told Capybara to respect_data_method: false.

OK, so how can we fix this? We can fix the JS, but there are two problems:

1. Remove jquery ujs #1618 wasn't merged entirely arbitrarily. It was the product of our trying to figure out how to best integrate with both the asset pipeline and webpacker, two competing alternatives for handling frontend assets. We are still trying to figure this one out, and I don't think there's going to be a simple answer.
2. I don't like JS-driven delete buttons anyway! I'm ok with progressive enhancement, but having them depend on JS leads to issues... like this one we had.

So I'm proposing that we change the way that destroy links work, so that they use a "traditional" approach, driven by the backend:

1. Click on "destroy" link.
2. View separate confirmation page.
3. Click "confirm" button.
4. Redirect back to index page.

This is a first draft, which perhaps should be styled a bit. The confirmation page looks like this at the moment:

Pending:

• Discuss: how do we feel generally about this approach?
• Discuss: this introduces new i18n strings. How do we feel about this? I suspect they should be more more, as the reuse I do here may not work well across languages.
• Style the new destroy confirmation page.
• Provide i18n strings for all supported languages.

[nickcharlton]

Ahh this is an interesting way to approach this problem!

Is it just deletions that have this problem? We don't do anything else which was dependent on ujs?

My initial thought would be that this will make deleting items really slow, and having something else to make mass deletions faster isn't something I think we should worry about right now.

[pablobm]

I agree: the UX of this solution is a step backwards compared to the pre-bug solution.

Now, I have these two thoughts:

1. I don't like relying on ujs. Perhaps I'm a bit old-fashioned, but I do appreciate progressive enhancement.
2. I think the JS should still be fixed, precisely to provide progressive enhancement and fix the UX, and even improve from what we had before.

Having said that, perhaps we should fix the JS issue first, and consider this PR an additional enhancement that should come later.

[nickcharlton]

Is the best option — in the interest of unblocking the release — to just revert the removal? I'd like to add the Capybara change from this too though.

[pablobm]

Yeah, I think you are right. I'd still like to provide a JS-independent solution, but let's solve the problem at hand first. So here's a PR that includes the breaking test and the commit reversal: #1691

[pablobm]

Oops, didn't notice you already had created a PR. I closed mine now.

[nickcharlton]

Sorry! I should've mentioned it!

Given other priorities, what do you think of closing this one for now and revising it later on?

[pablobm]

Yes. I'll come back to this in the future, but I'm ok with closing it for now.