Skip to content
GitLab
Projects Groups Snippets
  • /
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
  • A administrate
  • Project information
    • Project information
    • Activity
    • Labels
    • Members
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
  • Issues 96
    • Issues 96
    • List
    • Boards
    • Service Desk
    • Milestones
  • Merge requests 32
    • Merge requests 32
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Schedules
  • Deployments
    • Deployments
    • Environments
    • Releases
  • Packages and registries
    • Packages and registries
    • Package Registry
    • Infrastructure Registry
  • Monitor
    • Monitor
    • Incidents
  • Analytics
    • Analytics
    • Value stream
    • CI/CD
    • Repository
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
Collapse sidebar
  • thoughtbot, inc.
  • administrate
  • Merge requests
  • !1688

WIP: fix "destroy" links

  • Review changes

  • Download
  • Email patches
  • Plain diff
Closed Pablo Brasero requested to merge github/fork/pablobm/fix-js-actions into master Jun 25, 2020
  • Overview 7
  • Commits 3
  • Pipelines 0
  • Changes 10

Fixes https://github.com/thoughtbot/administrate/issues/1643

When https://github.com/thoughtbot/administrate/pull/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. #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:

Ask if user is sure, provide destroy button

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.
Assignee
Assign to
Reviewers
Request review from
Time tracking
Source branch: github/fork/pablobm/fix-js-actions