From 45c164d5ae9997d15705e42d2a461d23ad168942 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Wed, 15 May 2024 16:47:37 +1000
Subject: [PATCH] Avoid submitting duplicate connected apps

Simple Rails forms prevent double-clicking on submit already. Converting
the StimulusReflex interaction to a simple form submit to a controller
solves the race condition.

The UX is slightly worse because the whole page is reloaded instead
rendering only the connected app panel. But we can solve that when we
add more apps and want to activate them independently. By then, we may
have good patterns for working with Turbo.

Technically, the new buttons are a form within a form which is invalid
HTML, but it works.
---
 .../admin/connected_apps_controller.rb        | 43 +++++++++++++++
 app/reflexes/admin/connected_app_reflex.rb    | 53 -------------------
 .../form/_connected_apps.html.haml            |  6 +--
 config/routes/admin.rb                        |  2 +
 4 files changed, 47 insertions(+), 57 deletions(-)
 create mode 100644 app/controllers/admin/connected_apps_controller.rb
 delete mode 100644 app/reflexes/admin/connected_app_reflex.rb

diff --git a/app/controllers/admin/connected_apps_controller.rb b/app/controllers/admin/connected_apps_controller.rb
new file mode 100644
index 0000000000..2cbaca7c9b
--- /dev/null
+++ b/app/controllers/admin/connected_apps_controller.rb
@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+module Admin
+  class ConnectedAppsController < ApplicationController
+    def create
+      authorize! :admin, enterprise
+
+      app = ConnectedApp.create!(enterprise_id: enterprise.id)
+
+      ConnectAppJob.perform_later(
+        app, spree_current_user.spree_api_key,
+        channel: SessionChannel.for_request(request),
+      )
+
+      render_panel
+    end
+
+    def destroy
+      authorize! :admin, enterprise
+
+      app = enterprise.connected_apps.first
+      app.destroy
+
+      WebhookDeliveryJob.perform_later(
+        app.data["destroy"],
+        "disconnect-app",
+        nil
+      )
+
+      render_panel
+    end
+
+    private
+
+    def enterprise
+      @enterprise ||= Enterprise.find(params.require(:enterprise_id))
+    end
+
+    def render_panel
+      redirect_to "#{edit_admin_enterprise_path(enterprise)}#/connected_apps_panel"
+    end
+  end
+end
diff --git a/app/reflexes/admin/connected_app_reflex.rb b/app/reflexes/admin/connected_app_reflex.rb
deleted file mode 100644
index ecfb974d96..0000000000
--- a/app/reflexes/admin/connected_app_reflex.rb
+++ /dev/null
@@ -1,53 +0,0 @@
-# frozen_string_literal: true
-
-module Admin
-  class ConnectedAppReflex < ApplicationReflex
-    def create
-      authorize! :admin, enterprise
-
-      app = ConnectedApp.create!(enterprise_id: enterprise.id)
-
-      # Avoid race condition by sending before enqueuing job:
-      broadcast_partial
-
-      ConnectAppJob.perform_later(
-        app, current_user.spree_api_key,
-        channel: SessionChannel.for_request(request),
-      )
-      morph :nothing
-    end
-
-    def destroy
-      authorize! :admin, enterprise
-
-      app = enterprise.connected_apps.first
-      app.destroy
-
-      broadcast_partial
-
-      WebhookDeliveryJob.perform_later(
-        app.data["destroy"],
-        "disconnect-app",
-        nil
-      )
-      morph :nothing
-    end
-
-    private
-
-    def enterprise
-      @enterprise ||= Enterprise.find(element.dataset.enterprise_id)
-    end
-
-    def broadcast_partial
-      selector = "#edit_enterprise_#{enterprise.id} #connected-app-discover-regen"
-      html = ApplicationController.render(
-        partial: "admin/enterprises/form/connected_apps",
-        locals: { enterprise: },
-      )
-
-      # Avoid race condition by sending before enqueuing job:
-      cable_ready.morph(selector:, html:).broadcast
-    end
-  end
-end
diff --git a/app/views/admin/enterprises/form/_connected_apps.html.haml b/app/views/admin/enterprises/form/_connected_apps.html.haml
index c60a7da1a1..b8e6be9fae 100644
--- a/app/views/admin/enterprises/form/_connected_apps.html.haml
+++ b/app/views/admin/enterprises/form/_connected_apps.html.haml
@@ -6,16 +6,14 @@
       %p= t ".tagline"
     %div
       - if enterprise.connected_apps.empty?
-        %button{ data: {reflex: "click->Admin::ConnectedApp#create", enterprise_id: enterprise.id} }
-          = t ".enable"
+        = button_to t(".enable"), admin_enterprise_connected_apps_path(enterprise.id), method: :post
       - elsif enterprise.connected_apps.connecting.present?
         %button{ disabled: true }
           %i.spinner.fa.fa-spin.fa-circle-o-notch
           &nbsp;
           = t ".loading"
       - else
-        %button{ data: {reflex: "click->Admin::ConnectedApp#destroy", enterprise_id: enterprise.id} }
-          = t ".disable"
+        = button_to t(".disable"), admin_enterprise_connected_app_path(0, enterprise_id: enterprise.id), method: :delete
 
   .connected-app__connection
     - if enterprise.connected_apps.ready.present?
diff --git a/config/routes/admin.rb b/config/routes/admin.rb
index 1a7ca8c3cc..c82fb18315 100644
--- a/config/routes/admin.rb
+++ b/config/routes/admin.rb
@@ -35,6 +35,8 @@ Openfoodnetwork::Application.routes.draw do
         patch :register
       end
 
+      resources :connected_apps, only: [:create, :destroy]
+
       resources :producer_properties do
         post :update_positions, on: :collection
       end